packet fixes

                if (unlikely(err < 0))
                        return -EINVAL;

- } else if (dev->hard_header_len) {

net/packet/af_packet.c +41 -29

-               skb_push(skb, dev->hard_header_len);
-               err = skb_store_bits(skb, 0, data,
-                               dev->hard_header_len);
-               if (unlikely(err))
-                       return err;
+               /* Copy Ethernet header in case we need to deal with extra
+                * VLAN space as otherwise data could change underneath us.
+                * The caller already accomodated for enough linear room.
+                */
+               if (dev->hard_header_len || tp_len > dev->mtu + reserve) {
+                       int hdr_len = dev->hard_header_len;
+
+                       if (hdr_len < ETH_HLEN)
+                               hdr_len = ETH_HLEN;
+                       if (unlikely(ll_header_truncated(hdr_len, tp_len)))
+                               return -EINVAL;
+
+                       skb_push(skb, hdr_len);
+                       err = skb_store_bits(skb, 0, data, hdr_len);
+                       if (unlikely(err))
+                               return err;
 
-               data += dev->hard_header_len;
-               to_write -= dev->hard_header_len;
+                       data     += hdr_len;
+                       to_write -= hdr_len;
+               }
        }
 
        offset = offset_in_page(data);

@@ -2447,6 +2471,20 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
                len = ((to_write > len_max) ? len_max : to_write);
        }
 
+       /* Check for full frame with extra VLAN space. We can probe
+        * here on the linear header, if necessary. Earlier code
+        * assumed this would be a VLAN pkt, double-check this now
+        * that we have the actual packet and protocol at hand.
+        */
+       if (tp_len > dev->mtu + reserve) {
+               if (skb->protocol != htons(ETH_P_8021Q))
+                       return -EMSGSIZE;
+
+               skb_reset_mac_header(skb);
+               if (eth_hdr(skb)->h_proto != htons(ETH_P_8021Q))
+                       return -EMSGSIZE;
+       }
+
        skb_probe_transport_header(skb, 0);
 
        return tp_len;

@@ -2493,12 +2531,12 @@ 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;
-       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 (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 + VLAN_HLEN)
+               size_max = dev->mtu + reserve + VLAN_HLEN;
 
        do {
                ph = packet_current_frame(po, &po->tx_ring,

@@ -2523,20 +2561,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
                        goto out_status;
                }
                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.
-                        */
-
-                       skb_reset_mac_header(skb);
-                       ehdr = eth_hdr(skb);
-                       if (ehdr->h_proto != htons(ETH_P_8021Q))
-                               tp_len = -EMSGSIZE;
-               }
+                                         addr, hlen, reserve);
                if (unlikely(tp_len < 0)) {
                        if (po->tp_loss) {
                                __packet_set_status(po, ph,

@@ -2754,7 +2779,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
                if (unlikely(offset < 0))
                        goto out_free;
        } else {
-               if (ll_header_truncated(dev, len))
+               if (unlikely(ll_header_truncated(dev->hard_header_len, len)))
                        goto out_free;
        }
 
--

1.9.3

net/packet/af_packet.c static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb, · line 2410

Quoted 10 lines

         if (sock->type == SOCK_DGRAM) {

net/packet/af_packet.c +7 -2

-               err = dev_hard_header(skb, dev, ntohs(proto), addr,
-                               NULL, tp_len);
+               /* In DGRAM sockets, we expect struct sockaddr_ll was filled
+                * via struct msghdr, so we have dest mac and skb->protocol.
+                * Otherwise there's not too much useful things we can do in
+                * this flush run.
+                */
+               err = dev_hard_header(skb, dev, ntohs(skb->protocol), addr,
+                                     NULL, tp_len);

This change is not really necessary.

Quoted 3 lines

                 if (unlikely(err < 0))
                         return -EINVAL;

- } else if (dev->hard_header_len) {

Why remove the check on hard_header_len?

net/packet/af_packet.c static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb, · line 2416

Quoted 6 lines

net/packet/af_packet.c +4 -2

-               if (ll_header_truncated(dev, tp_len))
-                       return -EINVAL;
+       } else {
+               /* If skb->protocol is still 0, try to infer/guess it. Might
+                * not be fully reliable in the sense that a user could still
+                * change/race data afterwards, but on the other hand the proto

The race goes away when probing it after the copy in skb_store_bits. Then it is also certain that tp_len is long enough to hold the entire link layer header.

net/packet/af_packet.c static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb, · line 2421

Quoted 5 lines

net/packet/af_packet.c +5 -0

+                * can be set arbitrarily anyways. We only need to take care
+                * in case of extra large VLAN frames.
+                */
+               if (!skb->protocol && tp_len >= ETH_HLEN)
+                       skb->protocol = ((struct ethhdr *)data)->h_proto;

Packet sockets are not restricted to link layer of type Ethernet.

There are a few other points in this file that also cast mac header to eth_hdr(skb).

Quoted 17 lines · Willem de Bruijn

Quoted 13 lines

         if (sock->type == SOCK_DGRAM) {

net/packet/af_packet.c +1 -2

-               err = dev_hard_header(skb, dev, ntohs(proto), addr,
-                               NULL, tp_len);
+               /* In DGRAM sockets, we expect struct sockaddr_ll was

filled
+ * via struct msghdr, so we have dest mac and
skb->protocol.
+ * Otherwise there's not too much useful things we can do
in

net/packet/af_packet.c +3 -0

+                * this flush run.
+                */
+               err = dev_hard_header(skb, dev, ntohs(skb->protocol),

Quoted 2 lines

addr,
+ NULL, tp_len);

Quoted 1 line

This change is not really necessary.

Sure agreed, I found it helpful though. Don't mind removing it.

Quoted 6 lines

Quoted 3 lines

                 if (unlikely(err < 0))
                         return -EINVAL;

- } else if (dev->hard_header_len) {

Why remove the check on hard_header_len?

Hmm, the patch doesn't remove the check (it's moved further below).

Quoted 11 lines

Quoted 9 lines

net/packet/af_packet.c +2 -2

-               if (ll_header_truncated(dev, tp_len))
-                       return -EINVAL;
+       } else {
+               /* If skb->protocol is still 0, try to infer/guess it.

Might
+ * not be fully reliable in the sense that a user could
still
+ * change/race data afterwards, but on the other hand the
proto

Quoted 3 lines

The race goes away when probing it after the copy in skb_store_bits. Then it is also certain that tp_len is long enough to hold the entire link layer header.

The skb_store_bits() is only done in case we do have a dev->hard_header_len or in case where we run into a possible situation where we have the additional
4 bytes on a full frame. In that case we need to check them properly, which requires copying, otherwise we don't copy any header.

Quoted 9 lines

Quoted 7 lines

+ * can be set arbitrarily anyways. We only need to take
care

net/packet/af_packet.c +4 -0

+                * in case of extra large VLAN frames.
+                */
+               if (!skb->protocol && tp_len >= ETH_HLEN)
+                       skb->protocol = ((struct ethhdr *)data)->h_proto;

Quoted 4 lines

Packet sockets are not restricted to link layer of type Ethernet.

There are a few other points in this file that also cast mac header to eth_hdr(skb).

Ok, the set doesn't address this assumption which we have elsewhere, too. Do you suggest to also check on dev->type for these cases?

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

Quoted 49 lines · Daniel Borkmann

Quoted 17 lines · Willem de Bruijn

Quoted 13 lines

          if (sock->type == SOCK_DGRAM) {

net/packet/af_packet.c +1 -2

-               err = dev_hard_header(skb, dev, ntohs(proto), addr,
-                               NULL, tp_len);
+               /* In DGRAM sockets, we expect struct sockaddr_ll was

filled
+ * via struct msghdr, so we have dest mac and
skb->protocol.
+ * Otherwise there's not too much useful things we can do
in

net/packet/af_packet.c +3 -0

+                * this flush run.
+                */
+               err = dev_hard_header(skb, dev, ntohs(skb->protocol),

Quoted 2 lines

addr,
+ NULL, tp_len);

Quoted 1 line

This change is not really necessary.

Sure agreed, I found it helpful though. Don't mind removing it.

Quoted 6 lines

Quoted 3 lines

                  if (unlikely(err < 0))
                          return -EINVAL;

- } else if (dev->hard_header_len) {

Why remove the check on hard_header_len?

Hmm, the patch doesn't remove the check (it's moved further below).

Quoted 11 lines

Quoted 9 lines

net/packet/af_packet.c +2 -2

-               if (ll_header_truncated(dev, tp_len))
-                       return -EINVAL;
+       } else {
+               /* If skb->protocol is still 0, try to infer/guess it.

Might
+ * not be fully reliable in the sense that a user could
still
+ * change/race data afterwards, but on the other hand the
proto

Quoted 3 lines

The race goes away when probing it after the copy in skb_store_bits. Then it is also certain that tp_len is long enough to hold the entire link layer header.

Quoted 5 lines

The skb_store_bits() is only done in case we do have a dev->hard_header_len or in case where we run into a possible situation where we have the additional
4 bytes on a full frame. In that case we need to check them properly, which requires copying, otherwise we don't copy any header.

I assumed that hard_header_len has to be non-zero if there is a link layer header to probe. If we only intend to implement probing in the case of Ethernet, then it certainly holds.

Quoted 8 lines

Quoted 8 lines

Quoted 6 lines

+ * can be set arbitrarily anyways. We only need to take
care

net/packet/af_packet.c +4 -0

+                * in case of extra large VLAN frames.
+                */
+               if (!skb->protocol && tp_len >= ETH_HLEN)
+                       skb->protocol = ((struct ethhdr *)data)->h_proto;

Quoted 8 lines

Quoted 4 lines

Packet sockets are not restricted to link layer of type Ethernet.

There are a few other points in this file that also cast mac header to eth_hdr(skb).

Ok, the set doesn't address this assumption which we have elsewhere, too. Do you suggest to also check on dev->type for these cases?

Yes. If I'm right, then the other cases have to be fixed, too. One of the eth_hdr(skb) calls was introduced by patch 09effa67a18d, where the deleted code shows how it is safely restricted to ethernet:

- if (dev->type == ARPHRD_ETHER) {
- skb->protocol = eth_type_trans(skb, dev);
- if (skb->protocol == htons(ETH_P_8021Q))

In tpacket_fill_skb() commit c1aad275b029 ("packet: set transport header before doing xmit") and later on 40893fd0fd4e ("net: switch to use skb_probe_transport_header()") was probing for a transport header on the skb from a ring buffer slot, but at a time, where the skb has _not even_ been filled with data yet. So that call into the flow dissector is pretty useless. Lets do it after we've set up the skb frags.

Fixes c1aad275b029 ("packet: set transport header before doing xmit")

Reported-by Eric Dumazet <[email protected]>

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

Cc Jason Wang <[email protected]>

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

net/packet/af_packet.c +0 -2

index af399ca..80c36c0 100644
@@ -2368,8 +2368,6 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
 	skb_reserve(skb, hlen);
 	skb_reset_network_header(skb);
 
-	if (!packet_use_direct_xmit(po))
-		skb_probe_transport_header(skb, 0);
 	if (unlikely(po->tp_tx_has_off)) {
 		int off_min, off_max, off;
 		off_min = po->tp_hdrlen - sizeof(struct sockaddr_ll);

We concluded that the skb_probe_transport_header() should better be called unconditionally. Avoiding the call into the flow dissector has also not really much to do with the direct xmit mode.

While it seems that only virtio_net code makes use of GSO from non RX/TX ring packet socket paths, we should probe for a transport header nevertheless before they hit devices.

Reference: http://thread.gmane.org/gmane.linux.network/386173/

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

Cc Jason Wang <[email protected]>

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

net/packet/af_packet.c +1 -2

index 80c36c0..bdecf17 100644
@@ -2447,8 +2447,7 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
 		len = ((to_write > len_max) ? len_max : to_write);
 	}
 
-	if (!packet_use_direct_xmit(po))
-		skb_probe_transport_header(skb, 0);
+	skb_probe_transport_header(skb, 0);
 
 	return tp_len;
 }

Quoted 71 lines · Willem de Bruijn

Quoted 59 lines · Daniel Borkmann

Quoted 20 lines · Willem de Bruijn

Quoted 15 lines

          if (sock->type == SOCK_DGRAM) {

net/packet/af_packet.c +1 -2

-               err = dev_hard_header(skb, dev, ntohs(proto), addr,
-                               NULL, tp_len);
+               /* In DGRAM sockets, we expect struct sockaddr_ll was

filled
+ * via struct msghdr, so we have dest mac and
skb->protocol.
+ * Otherwise there's not too much useful things we can
do
in

net/packet/af_packet.c +3 -0

+                * this flush run.
+                */
+               err = dev_hard_header(skb, dev, ntohs(skb->protocol),

Quoted 2 lines

addr,
+ NULL, tp_len);

Quoted 1 line

This change is not really necessary.

Sure agreed, I found it helpful though. Don't mind removing it.

Quoted 7 lines

Quoted 3 lines

                  if (unlikely(err < 0))
                          return -EINVAL;

- } else if (dev->hard_header_len) {

Why remove the check on hard_header_len?

Hmm, the patch doesn't remove the check (it's moved further below).

Quoted 13 lines

Quoted 10 lines

net/packet/af_packet.c +2 -2

-               if (ll_header_truncated(dev, tp_len))
-                       return -EINVAL;
+       } else {
+               /* If skb->protocol is still 0, try to infer/guess it.

Might
+ * not be fully reliable in the sense that a user could
still
+ * change/race data afterwards, but on the other hand
the
proto

Quoted 3 lines

The race goes away when probing it after the copy in skb_store_bits. Then it is also certain that tp_len is long enough to hold the entire link layer header.

Quoted 7 lines

The skb_store_bits() is only done in case we do have a dev->hard_header_len
or in case where we run into a possible situation where we have the additional
4 bytes on a full frame. In that case we need to check them properly, which
requires copying, otherwise we don't copy any header.

Quoted 3 lines

I assumed that hard_header_len has to be non-zero if there is a link layer header to probe. If we only intend to implement probing in the case of Ethernet, then it certainly holds.

Yeah, I guess we only care about ether_setup() alike devices, so we'd have ETH_HLEN room as dev->hard_header_len. That will hold, yes.

Quoted 21 lines

Quoted 12 lines

Quoted 12 lines

Quoted 7 lines

+ * can be set arbitrarily anyways. We only need to take
care

net/packet/af_packet.c +4 -0

+                * in case of extra large VLAN frames.
+                */
+               if (!skb->protocol && tp_len >= ETH_HLEN)
+                       skb->protocol = ((struct ethhdr

*)data)->h_proto;

Packet sockets are not restricted to link layer of type Ethernet.

Quoted 7 lines

Quoted 2 lines

There are a few other points in this file that also cast mac header to eth_hdr(skb).

Ok, the set doesn't address this assumption which we have elsewhere, too. Do you suggest to also check on dev->type for these cases?

Quoted 7 lines

Yes. If I'm right, then the other cases have to be fixed, too. One of the eth_hdr(skb) calls was introduced by patch 09effa67a18d, where the deleted code shows how it is safely restricted to ethernet:

- if (dev->type == ARPHRD_ETHER) {
- skb->protocol = eth_type_trans(skb, dev);
- if (skb->protocol == htons(ETH_P_8021Q))

Saw that, so we need the check as one more fix for 57f89bfa2140 ("network:
Allow af_packet to transmit +4 bytes for VLAN packets.") as well.

I'll see to respin a v3 tomorrow.

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

On Tue, Nov 10, 2015 at 6:51 PM, Daniel Borkmann <[email protected]> wrote:

Quoted 4 lines

Saw that, so we need the check as one more fix for 57f89bfa2140 ("network:
Allow af_packet to transmit +4 bytes for VLAN packets.") as well.

I'll see to respin a v3 tomorrow.

Thanks, Daniel.

The three existing uses of eth_hdr were applied in 2.6.39, 3.12 and 3.15. We probably need a patch per occurrence to allow backporting to the relevant stable branches. I can create the fixes independent from your patch set, if you prefer.