[PATCH net 0/2] packet fix

[PATCH net 0/2] packet fix

[Daniel Borkmann]

Hi Eric,

does this resolve the issues you're having on the TX_RING when dealing
with trafgen [1]?

Thanks a lot,
Daniel

  [1] http://thread.gmane.org/gmane.linux.network/385988

Daniel Borkmann (2):
  packet: do skb_probe_transport_header when we actually have data
  packet: fix tpacket_snd max frame and vlan handling

 net/packet/af_packet.c | 106 ++++++++++++++++++++++++++++++-------------------
 1 file changed, 66 insertions(+), 40 deletions(-)

-- 
1.9.3

[PATCH net 1/2] packet: do skb_probe_transport_header when we actually have data

[Daniel Borkmann]

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 <edumazet@google.com>
Cc: Jason Wang <jasowang@redhat.com>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
 net/packet/af_packet.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index af399ca..80c36c0 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -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);
@@ -2449,6 +2447,9 @@ 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);
+
 	return tp_len;
 }
 
-- 
1.9.3

[PATCH net 2/2] packet: fix tpacket_snd max frame and vlan handling

[Daniel Borkmann]

There seem to be a couple of issues in tpacket_snd() path. 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.

Moreover, the check for ETH_P_8021Q seems buggy as we potentially
operate on non-linear header data. In sock_alloc_send_skb() we seem
to allocate enough linear head-room in most cases, so the test could
just yield false results. The sock_alloc_send_skb() has an extra room
of (for some reason) sizeof(struct sockaddr_ll), so we have enough
header room for at least ethernet header in case the device doesn't
ask for it. Note that in TX_RING's tpacket_fill_skb() we fetch the
frame data after the slot header (or at some provided offset), thus
from TX side (as opposed to RX_RING), it doesn't contain a sockaddr_ll
structure in between.

Anyway, the ETH_P_8021Q check would be better fixed in tpacket_fill_skb()
by making sure that in SOCK_RAW cases, where we deal with tp_len >
dev->mtu + dev->hard_header_len, to place at least the ethernet header
into the linear section (which is the case in SOCK_DGRAM already).
Doing this test on ring buffer data (either from set up skb or on data
directly) could race underneath us. The test is done at the end so we
can take both socket types into consideration. We check that
skb->protocol and also h_proto are correct in these cases. For packet
sockets that have proto of 0, guess the skb->protocol from the user
payload as suggested.

Fixes: 69e3c75f4d54 ("net: TX_RING and packet mmap")
Fixes: 52f1454f629f ("packet: allow to transmit +4 byte in TX_RING slot for VLAN case")
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Willem de Bruijn <willemb@google.com>
Cc: Tobias Klauser <tklauser@distanz.ch>
---
 net/packet/af_packet.c | 101 ++++++++++++++++++++++++++++++-------------------
 1 file changed, 63 insertions(+), 38 deletions(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 80c36c0..e9a7bb4 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -2320,12 +2320,12 @@ static void tpacket_destruct_skb(struct sk_buff *skb)
 	sock_wfree(skb);
 }
 
-static bool ll_header_truncated(const struct net_device *dev, int len)
+static bool ll_header_truncated(int hard_header_len, int len)
 {
 	/* net device doesn't like empty head */
-	if (unlikely(len <= dev->hard_header_len)) {
+	if (unlikely(len <= hard_header_len)) {
 		net_warn_ratelimited("%s: packet size is too short (%d <= %d)\n",
-				     current->comm, len, dev->hard_header_len);
+				     current->comm, len, hard_header_len);
 		return true;
 	}
 
@@ -2333,8 +2333,9 @@ static bool ll_header_truncated(const struct net_device *dev, int len)
 }
 
 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)
+			    void *frame, struct net_device *dev, int size_max,
+			    __be16 proto, unsigned char *addr, int hlen,
+			    int reserve)
 {
 	union tpacket_uhdr ph;
 	int to_write, offset, len, tp_len, nr_frags, len_max;
@@ -2400,22 +2401,45 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
 	to_write = tp_len;
 
 	if (sock->type == SOCK_DGRAM) {
-		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);
 		if (unlikely(err < 0))
 			return -EINVAL;
-	} else if (dev->hard_header_len) {
-		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
+		 * 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;
 
-		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;
+	}
+
 	if (!packet_use_direct_xmit(po))
 		skb_probe_transport_header(skb, 0);
 
@@ -2494,12 +2532,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,
@@ -2524,20 +2562,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,
@@ -2755,7 +2780,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

Re: [PATCH net 1/2] packet: do skb_probe_transport_header when we actually have data

[Eric Dumazet]

On Fri, 2015-11-06 at 22:02 +0100, Daniel Borkmann wrote:
> 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 <edumazet@google.com>
> Cc: Jason Wang <jasowang@redhat.com>


> +	if (!packet_use_direct_xmit(po))
> +		skb_probe_transport_header(skb, 0);
> +

Thanks Daniel for working on this.

The if (!packet_use_direct_xmit(po)) test looks dubious.

Setting transport header has nothing to do with bypassing qdisc ?

This might lead to hard to debug problems, for drivers expecting
transport header being set ?

Maybe this needs a special socket flag, but this does not seem worth the pain.

(This would be different of course if trafgen was not defaulting to qdisc bypass)

Thanks.

Re: [PATCH net 1/2] packet: do skb_probe_transport_header when we actually have data

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Sat, 07 Nov 2015 04:42:56 -0800

> The if (!packet_use_direct_xmit(po)) test looks dubious.
> 
> Setting transport header has nothing to do with bypassing qdisc ?
> 
> This might lead to hard to debug problems, for drivers expecting
> transport header being set ?

Do we have any such drivers that need it in this scenerio?

Re: [PATCH net 1/2] packet: do skb_probe_transport_header when we actually have data

[Eric Dumazet]

On Sat, 2015-11-07 at 13:35 -0500, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Sat, 07 Nov 2015 04:42:56 -0800
> 
> > The if (!packet_use_direct_xmit(po)) test looks dubious.
> > 
> > Setting transport header has nothing to do with bypassing qdisc ?
> > 
> > This might lead to hard to debug problems, for drivers expecting
> > transport header being set ?
> 
> Do we have any such drivers that need it in this scenerio?

Well, imagine following scenario (a real one, as I use it all of time,
thus how I discovered all trafgen traffic ends up on one slave only)

Even if qdisc is bypassed on the bond0, the current handling does not
prevent going to the slave qdiscs.


So it is not clear to me why we do a selective probe depending on the
bypass of first qdisc.

Re: [PATCH net 1/2] packet: do skb_probe_transport_header when we actually have data

[Eric Dumazet]

On Sat, 2015-11-07 at 10:53 -0800, Eric Dumazet wrote:


> Well, imagine following scenario (a real one, as I use it all of time,
> thus how I discovered all trafgen traffic ends up on one slave only)
> 
> Even if qdisc is bypassed on the bond0, the current handling does not
> prevent going to the slave qdiscs.
> 
> 
> So it is not clear to me why we do a selective probe depending on the
> bypass of first qdisc.

Presumably the transport_header only needs to be set for gso packets in
some drivers (look at igbvf_tso() for example)

It looks like we might need an audit and/or some guidelines/fixes.

Re: [PATCH net 1/2] packet: do skb_probe_transport_header when we actually have data

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Sat, 07 Nov 2015 10:53:50 -0800

> Well, imagine following scenario (a real one, as I use it all of
> time, thus how I discovered all trafgen traffic ends up on one slave
> only)
> 
> Even if qdisc is bypassed on the bond0, the current handling does
> not prevent going to the slave qdiscs.

Ok, depending upon the semantics Daniel intended, we may have to add
a qdisc bypass boolean bit to SKBs.

Re: [PATCH net 1/2] packet: do skb_probe_transport_header when we actually have data

[Daniel Borkmann]

On 11/07/2015 08:06 PM, Eric Dumazet wrote:
> On Sat, 2015-11-07 at 10:53 -0800, Eric Dumazet wrote:
>
>> Well, imagine following scenario (a real one, as I use it all of time,
>> thus how I discovered all trafgen traffic ends up on one slave only)
>>
>> Even if qdisc is bypassed on the bond0, the current handling does not
>> prevent going to the slave qdiscs.
>>
>> So it is not clear to me why we do a selective probe depending on the
>> bypass of first qdisc.
>
> Presumably the transport_header only needs to be set for gso packets in
> some drivers (look at igbvf_tso() for example)
>
> It looks like we might need an audit and/or some guidelines/fixes.

Hmm, yeah, on a (only quick) look, it seems this is mostly needed for the
virtio_net related code in packet_snd() / packet_recvmsg(), not handled in
RX/TX ring paths actually.

$ git grep -n gso_size net/packet/
net/packet/af_packet.c:2748:                    if (vnet_hdr.gso_size == 0)
net/packet/af_packet.c:2825:            skb_shinfo(skb)->gso_size =
net/packet/af_packet.c:2826:                    __virtio16_to_cpu(vio_le(), vnet_hdr.gso_size);
net/packet/af_packet.c:3219:                    vnet_hdr.gso_size =
net/packet/af_packet.c:3220:                            __cpu_to_virtio16(vio_le(), sinfo->gso_size);

Need to take a closer look on Monday.

Re: [PATCH net 1/2] packet: do skb_probe_transport_header when we actually have data

[David Miller]

From: Daniel Borkmann <daniel@iogearbox.net>
Date: Sun, 08 Nov 2015 01:33:56 +0100

> Hmm, yeah, on a (only quick) look, it seems this is mostly needed for
> the
> virtio_net related code in packet_snd() / packet_recvmsg(), not
> handled in
> RX/TX ring paths actually.
> 
> $ git grep -n gso_size net/packet/
> net/packet/af_packet.c:2748: if (vnet_hdr.gso_size == 0)
> net/packet/af_packet.c:2825:            skb_shinfo(skb)->gso_size =
> net/packet/af_packet.c:2826: __virtio16_to_cpu(vio_le(),
> vnet_hdr.gso_size);
> net/packet/af_packet.c:3219:                    vnet_hdr.gso_size =
> net/packet/af_packet.c:3220: __cpu_to_virtio16(vio_le(),
> sinfo->gso_size);
> 
> Need to take a closer look on Monday.

I think for complete safety, we need the transport header set for
all SKBs once they hit the device.

I know this is separate from the bugs you are trying to fix, but
let's take care of this in this series ok?

Re: [PATCH net 1/2] packet: do skb_probe_transport_header when we actually have data

[Daniel Borkmann]

On 11/09/2015 04:11 AM, David Miller wrote:
> From: Daniel Borkmann <daniel@iogearbox.net>
> Date: Sun, 08 Nov 2015 01:33:56 +0100
>
>> Hmm, yeah, on a (only quick) look, it seems this is mostly needed for
>> the
>> virtio_net related code in packet_snd() / packet_recvmsg(), not
>> handled in
>> RX/TX ring paths actually.
>>
>> $ git grep -n gso_size net/packet/
>> net/packet/af_packet.c:2748: if (vnet_hdr.gso_size == 0)
>> net/packet/af_packet.c:2825:            skb_shinfo(skb)->gso_size =
>> net/packet/af_packet.c:2826: __virtio16_to_cpu(vio_le(),
>> vnet_hdr.gso_size);
>> net/packet/af_packet.c:3219:                    vnet_hdr.gso_size =
>> net/packet/af_packet.c:3220: __cpu_to_virtio16(vio_le(),
>> sinfo->gso_size);
>>
>> Need to take a closer look on Monday.
>
> I think for complete safety, we need the transport header set for
> all SKBs once they hit the device.
>
> I know this is separate from the bugs you are trying to fix, but
> let's take care of this in this series ok?

Ok, sure. I can add an extra one into this series removing the
conditional.

Thanks,
Daniel

Re: [PATCH net 1/2] packet: do skb_probe_transport_header when we actually have data

[Daniel Borkmann]

On 11/07/2015 11:29 PM, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Sat, 07 Nov 2015 10:53:50 -0800
>
>> Well, imagine following scenario (a real one, as I use it all of
>> time, thus how I discovered all trafgen traffic ends up on one slave
>> only)
>>
>> Even if qdisc is bypassed on the bond0, the current handling does
>> not prevent going to the slave qdiscs.
>
> Ok, depending upon the semantics Daniel intended, we may have to add
> a qdisc bypass boolean bit to SKBs.

It was resembling pktgen to some extend, only to be more flexible in
terms of defining packet payload (trafgen, I mean). Yes, pktgen has
the same issue on that regard, hmm I'm not yet sure, though, if it's
worth burning an extra skb bit for both cases.