packet fixes

Packet sockets can be used by various net devices and are not really restricted to ARPHRD_ETHER device types. However, when currently checking for the extra 4 bytes that can be transmitted in VLAN case, our assumption is that we generally probe on ARPHRD_ETHER devices. Therefore, before looking into Ethernet header, check the device type first.

This also fixes the issue where non-ARPHRD_ETHER devices could have no dev->hard_header_len in TX_RING SOCK_RAW case, and thus the check would test unfilled linear part of the skb (instead of non-linear).

Fixes 57f89bfa2140 ("network: Allow af_packet to transmit +4 bytes for VLAN packets.")

Fixes 52f1454f629f ("packet: allow to transmit +4 byte in TX_RING slot for VLAN case")

Signed-off-by Daniel Borkmann <[email protected]>

---
 net/packet/af_packet.c | 60 +++++++++++++++++++++-----------------------------
 1 file changed, 25 insertions(+), 35 deletions(-)

net/packet/af_packet.c +21 -29

index bdecf17..8795b0f 100644
@@ -1741,6 +1741,20 @@ static void fanout_release(struct sock *sk)
                kfree_rcu(po->rollover, rcu);
 }
 
+static bool packet_extra_vlan_len_allowed(const struct net_device *dev,
+                                         struct sk_buff *skb)
+{
+       /* Earlier code assumed this would be a VLAN pkt, double-check
+        * this now that we have the actual packet in hand. We can only
+        * do this check on Ethernet devices.
+        */
+       if (unlikely(dev->type != ARPHRD_ETHER))
+               return false;
+
+       skb_reset_mac_header(skb);
+       return likely(eth_hdr(skb)->h_proto == htons(ETH_P_8021Q));
+}
+
 static const struct proto_ops packet_ops;
 
 static const struct proto_ops packet_ops_spkt;

@@ -1902,18 +1916,10 @@ retry:
                goto retry;
        }
 
-       if (len > (dev->mtu + dev->hard_header_len + extra_len)) {
-               /* Earlier code assumed this would be a VLAN pkt,
-                * double-check this now that we have the actual
-                * packet in hand.
-                */
-               struct ethhdr *ehdr;
-               skb_reset_mac_header(skb);
-               ehdr = eth_hdr(skb);
-               if (ehdr->h_proto != htons(ETH_P_8021Q)) {
-                       err = -EMSGSIZE;
-                       goto out_unlock;
-               }
+       if (len > (dev->mtu + dev->hard_header_len + extra_len) &&
+           !packet_extra_vlan_len_allowed(dev, skb)) {
+               err = -EMSGSIZE;
+               goto out_unlock;
        }
 
        skb->protocol = proto;

@@ -2525,18 +2531,10 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
                tp_len = tpacket_fill_skb(po, skb, ph, dev, size_max, proto,
                                          addr, hlen);
                if (likely(tp_len >= 0) &&
-                   tp_len > dev->mtu + dev->hard_header_len) {
-                       struct ethhdr *ehdr;
-                       /* Earlier code assumed this would be a VLAN pkt,
-                        * double-check this now that we have the actual
-                        * packet in hand.
-                        */
+                   tp_len > dev->mtu + dev->hard_header_len &&
+                   !packet_extra_vlan_len_allowed(dev, skb))
+                       tp_len = -EMSGSIZE;
 
-                       skb_reset_mac_header(skb);
-                       ehdr = eth_hdr(skb);
-                       if (ehdr->h_proto != htons(ETH_P_8021Q))
-                               tp_len = -EMSGSIZE;
-               }
                if (unlikely(tp_len < 0)) {
                        if (po->tp_loss) {
                                __packet_set_status(po, ph,

@@ -2765,18 +2763,10 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
 
        sock_tx_timestamp(sk, &skb_shinfo(skb)->tx_flags);
 
-       if (!gso_type && (len > dev->mtu + reserve + extra_len)) {
-               /* Earlier code assumed this would be a VLAN pkt,
-                * double-check this now that we have the actual
-                * packet in hand.
-                */
-               struct ethhdr *ehdr;

net/packet/af_packet.c +0 -1

        }
 
        skb->protocol = proto;
--

1.9.3

Since it's introduction in commit 69e3c75f4d54 ("net: TX_RING and packet mmap"), TX_RING could be used from SOCK_DGRAM and SOCK_RAW side. When used with SOCK_DGRAM only, the size_max > dev->mtu + reserve check should have reserve as 0, but currently, this is unconditionally set (in it's original form as dev->hard_header_len).

I think this is not correct since tpacket_fill_skb() would then take dev->mtu and dev->hard_header_len into account for SOCK_DGRAM, the extra VLAN_HLEN could be possible in both cases. Presumably, the reserve code was copied from packet_snd(), but later on missed the check. Make it similar as we have it in packet_snd().

Fixes 69e3c75f4d54 ("net: TX_RING and packet mmap")

Signed-off-by Daniel Borkmann <[email protected]>

---
 net/packet/af_packet.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

net/packet/af_packet.c +5 -5

index 0066da2..242bce1 100644
@@ -2510,12 +2510,13 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
        if (unlikely(!(dev->flags & IFF_UP)))
                goto out_put;
 
-       reserve = dev->hard_header_len + VLAN_HLEN;
+       if (po->sk.sk_socket->type == SOCK_RAW)
+               reserve = dev->hard_header_len;
        size_max = po->tx_ring.frame_size
                - (po->tp_hdrlen - sizeof(struct sockaddr_ll));
 
-       if (size_max > dev->mtu + reserve)
-               size_max = dev->mtu + reserve;
+       if (size_max > dev->mtu + reserve + VLAN_HLEN)
+               size_max = dev->mtu + reserve + VLAN_HLEN;
 
        do {
                ph = packet_current_frame(po, &po->tx_ring,

@@ -2542,7 +2543,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
                tp_len = tpacket_fill_skb(po, skb, ph, dev, size_max, proto,
                                          addr, hlen);
                if (likely(tp_len >= 0) &&
-                   tp_len > dev->mtu + dev->hard_header_len &&
+                   tp_len > dev->mtu + reserve &&
                    !packet_extra_vlan_len_allowed(dev, skb))
                        tp_len = -EMSGSIZE;
 
--

1.9.3

On Wed, Nov 11, 2015 at 5:25 PM, Daniel Borkmann <[email protected]> wrote:

net/packet/af_packet.c static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len) · line 2769

Quoted 4 lines

net/packet/af_packet.c +4 -0

+       if (!gso_type && (len > dev->mtu + reserve + extra_len) &&
+           !packet_extra_vlan_len_allowed(dev, skb)) {
+               err = -EMSGSIZE;
+               goto out_free;

This nicely reuses the same code in three locations.

If you end up having to send a v4, it would be nice to also fold the repeated len check into the shared code. Variable reserve here is just dev->hard_header_len. No need to spin a patch just for that cleanup, though.

In case no struct sockaddr_ll has been passed to packet socket's sendmsg() when doing a TX_RING flush run, then skb->protocol is set to po->num instead, which is the protocol passed via socket(2)/bind(2).

Applications only xmitting can go the path of allocating the socket as socket(PF_PACKET, <mode>, 0) and do a bind(2) on the TX_RING with sll_protocol of 0. That way, register_prot_hook() is neither called on creation nor on bind time, which saves cycles when there's no interest in capturing anyway.

That leaves us however with po->num 0 instead and therefore the TX_RING flush run sets skb->protocol to 0 as well. Eric reported that this leads to problems when using tools like trafgen over bonding device.
I.e. the bonding's hash function
could invoke the kernel's flow dissector, which depends on skb->protocol being properly set. In the current situation, all the traffic is then directed to a single slave.

Fix it up by inferring skb->protocol from the Ethernet header when not set and we have ARPHRD_ETHER device type. This is only done in case of SOCK_RAW and where we have a dev->hard_header_len length. In case of ARPHRD_ETHER devices, this is guaranteed to cover ETH_HLEN, and therefore being accessed on the skb after the skb_store_bits().

Reported-by Eric Dumazet <[email protected]>

Signed-off-by Daniel Borkmann <[email protected]>

---
 net/packet/af_packet.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

net/packet/af_packet.c +11 -1

index 8795b0f..0066da2 100644
@@ -2338,6 +2338,15 @@ static bool ll_header_truncated(const struct net_device *dev, int len)
        return false;
 }
 
+static void tpacket_set_protocol(const struct net_device *dev,
+                                struct sk_buff *skb)
+{
+       if (dev->type == ARPHRD_ETHER) {
+               skb_reset_mac_header(skb);
+               skb->protocol = eth_hdr(skb)->h_proto;
+       }
+}
+
 static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
                void *frame, struct net_device *dev, int size_max,
                __be16 proto, unsigned char *addr, int hlen)

@@ -2419,6 +2428,8 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
                                dev->hard_header_len);
                if (unlikely(err))
                        return err;
+               if (!skb->protocol)
+                       tpacket_set_protocol(dev, skb);
 
                data += dev->hard_header_len;
                to_write -= dev->hard_header_len;
--

1.9.3

Quoted 3 lines · Willem de Bruijn

On Wed, Nov 11, 2015 at 5:25 PM, Daniel Borkmann <[email protected]> wrote:

...

Quoted 7 lines

net/packet/af_packet.c static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len) · line 2769

Quoted 5 lines

net/packet/af_packet.c +4 -0

+       if (!gso_type && (len > dev->mtu + reserve + extra_len) &&
+           !packet_extra_vlan_len_allowed(dev, skb)) {
+               err = -EMSGSIZE;
+               goto out_free;

Quoted 6 lines

This nicely reuses the same code in three locations.

If you end up having to send a v4, it would be nice to also fold the repeated len check into the shared code. Variable reserve here is just dev->hard_header_len. No need to spin a patch just for that cleanup, though.

Noted, I was also thinking for net-next to move the tpacket_snd() check into tpacket_fill_skb(), that would save us to do the likely(tp_len >= 0) check there. Can take care of both when net-next opens, I feared there would be critics that it's not ending up minimal enough otherwise. ;)

Thanks,
Daniel
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Fixes a couple of issues in packet sockets, i.e. on TX ring side. See individual patches for details.

v2 -> v3:
- First two patches unchanged, kept Jason's Ack
- Reworked 3rd patch and split into 3:
- check for dev type as discussed with Willem
- infer skb->protocol
- fix max len for dgram
v1 -> v2:
- Added patch 2 as suggested by Dave
- Rest is unchanged from previous submission