skbuff: fix skb_segment with zero copy skbs

PATCH In review Michael S. Tsirkin 3 participants 20 messages Mar 10, 2014 – Mar 11, 2014

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Michael S. Tsirkin

author

2014-03-10 16:29 UTC #0 lore raw

cover skbuff: fix skb_segment with zero copy skbs

This fixes a bug in skb_segment where it moves frags between skbs without orphaning them.
This causes userspace to assume it's safe to reuse the buffer, and receiver gets corrupted data. This further might leak information from the transmitter on the wire.

To fix track which skb does a copied frag belong to, and orphan frags when copying them.

As we are tracking multiple skbs here, using
short names (skb,nskb,fskb,skb_frag,frag) becomes confusing. So before adding another one, I refactor these names slightly.

Patch is split out to make it easier to verify that all trasformations are trivially correct.

The problem was observed in the field,
so I think that the patch is necessary on stable as well.

Michael S. Tsirkin (5):
skbuff: skb_segment: s/frag/nskb_frag/
skbuff: skb_segment: s/skb_frag/frag/
skbuff: skb_segment: s/skb/head_skb/
skbuff: skb_segment: s/fskb/list_skb/
skbuff: skb_segment: orphan frags before copying

 net/core/skbuff.c | 100 +++++++++++++++++++++++++++++-------------------------
 1 file changed, 54 insertions(+), 46 deletions(-)

Signature · 1 line

MST

Michael S. Tsirkin

author

2014-03-10 16:29 UTC #1 lore raw

PATCH 1/5 skbuff: skb_segment: s/frag/nskb_frag/

frag points at nskb, so name it appropriately

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 net/core/skbuff.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

net/core/skbuff.c +9 -9

index 5976ef0..3bc6d75 100644
@@ -2879,7 +2879,7 @@ struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features)
 
 	do {
 		struct sk_buff *nskb;
-		skb_frag_t *frag;
+		skb_frag_t *nskb_frag;
 		int hsize;
 		int size;

@@ -2972,7 +2972,7 @@ struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features)
 			continue;
 		}
 
-		frag = skb_shinfo(nskb)->frags;
+		nskb_frag = skb_shinfo(nskb)->frags;
 
 		skb_copy_from_linear_data_offset(skb, offset,
 						 skb_put(nskb, hsize), hsize);

@@ -3000,13 +3000,13 @@ struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features)
 				goto err;
 			}
 
-			*frag = *skb_frag;
-			__skb_frag_ref(frag);
-			size = skb_frag_size(frag);
+			*nskb_frag = *skb_frag;
+			__skb_frag_ref(nskb_frag);
+			size = skb_frag_size(nskb_frag);
 
 			if (pos < offset) {
-				frag->page_offset += offset - pos;
-				skb_frag_size_sub(frag, offset - pos);
+				nskb_frag->page_offset += offset - pos;
+				skb_frag_size_sub(nskb_frag, offset - pos);
 			}
 
 			skb_shinfo(nskb)->nr_frags++;

@@ -3016,11 +3016,11 @@ struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features)
 				skb_frag++;
 				pos += size;
 			} else {
-				skb_frag_size_sub(frag, pos + size - (offset + len));
+				skb_frag_size_sub(nskb_frag, pos + size - (offset + len));
 				goto skip_fraglist;
 			}
 
-			frag++;
+			nskb_frag++;
 		}
 
 skip_fraglist:

Signature · 1 line

MST

Michael S. Tsirkin

author

2014-03-10 16:29 UTC #2 lore raw

PATCH 2/5 skbuff: skb_segment: s/skb_frag/frag/

In reply to Michael S. Tsirkin

skb_frag can in fact point at either skb
or fskb so rename it generally "frag".

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 net/core/skbuff.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

net/core/skbuff.c +7 -7

index 3bc6d75..917cb60 100644
@@ -2853,7 +2853,7 @@ struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features)
 	struct sk_buff *segs = NULL;
 	struct sk_buff *tail = NULL;
 	struct sk_buff *fskb = skb_shinfo(skb)->frag_list;
-	skb_frag_t *skb_frag = skb_shinfo(skb)->frags;
+	skb_frag_t *frag = skb_shinfo(skb)->frags;
 	unsigned int mss = skb_shinfo(skb)->gso_size;
 	unsigned int doffset = skb->data - skb_mac_header(skb);
 	unsigned int offset = doffset;

@@ -2899,19 +2899,19 @@ struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features)
 
 			i = 0;
 			nfrags = skb_shinfo(fskb)->nr_frags;
-			skb_frag = skb_shinfo(fskb)->frags;
+			frag = skb_shinfo(fskb)->frags;
 			pos += skb_headlen(fskb);
 
 			while (pos < offset + len) {
 				BUG_ON(i >= nfrags);
 
-				size = skb_frag_size(skb_frag);
+				size = skb_frag_size(frag);
 				if (pos + size > offset + len)
 					break;
 
 				i++;
 				pos += size;
-				skb_frag++;
+				frag++;
 			}
 
 			nskb = skb_clone(fskb, GFP_ATOMIC);

@@ -2985,7 +2985,7 @@ struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features)
 
 				i = 0;
 				nfrags = skb_shinfo(fskb)->nr_frags;
-				skb_frag = skb_shinfo(fskb)->frags;
+				frag = skb_shinfo(fskb)->frags;
 
 				BUG_ON(!nfrags);

@@ -3000,7 +3000,7 @@ struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features)
 				goto err;
 			}
 
-			*nskb_frag = *skb_frag;
+			*nskb_frag = *frag;
 			__skb_frag_ref(nskb_frag);
 			size = skb_frag_size(nskb_frag);

@@ -3013,7 +3013,7 @@ struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features)
 
 			if (pos + size <= offset + len) {
 				i++;
-				skb_frag++;
+				frag++;
 				pos += size;
 			} else {
 				skb_frag_size_sub(nskb_frag, pos + size - (offset + len));

Signature · 1 line

MST

Michael S. Tsirkin

author

2014-03-10 16:29 UTC #3 lore raw

PATCH 3/5 skbuff: skb_segment: s/skb/head_skb/

In reply to Michael S. Tsirkin

rename local variable to make it easier to tell at a glance that we are dealing with a head skb.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 net/core/skbuff.c | 46 ++++++++++++++++++++++++----------------------
 1 file changed, 24 insertions(+), 22 deletions(-)

net/core/skbuff.c +24 ~~-22~~

index 917cb60..af386e1 100644
@@ -2841,41 +2841,42 @@ EXPORT_SYMBOL_GPL(skb_pull_rcsum);
 
 /**
  *	skb_segment - Perform protocol segmentation on skb.
- *	@skb: buffer to segment
+ *	@head_skb: buffer to segment
  *	@features: features for the output path (see dev->features)
  *
  *	This function performs segmentation on the given skb.  It returns
  *	a pointer to the first in a list of new skbs for the segments.
  *	In case of error it returns ERR_PTR(err).
  */
-struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features)
+struct sk_buff *skb_segment(struct sk_buff *head_skb,
+			    netdev_features_t features)
 {
 	struct sk_buff *segs = NULL;
 	struct sk_buff *tail = NULL;
-	struct sk_buff *fskb = skb_shinfo(skb)->frag_list;
-	skb_frag_t *frag = skb_shinfo(skb)->frags;
-	unsigned int mss = skb_shinfo(skb)->gso_size;
-	unsigned int doffset = skb->data - skb_mac_header(skb);
+	struct sk_buff *fskb = skb_shinfo(head_skb)->frag_list;
+	skb_frag_t *frag = skb_shinfo(head_skb)->frags;
+	unsigned int mss = skb_shinfo(head_skb)->gso_size;
+	unsigned int doffset = head_skb->data - skb_mac_header(head_skb);
 	unsigned int offset = doffset;
-	unsigned int tnl_hlen = skb_tnl_header_len(skb);
+	unsigned int tnl_hlen = skb_tnl_header_len(head_skb);
 	unsigned int headroom;
 	unsigned int len;
 	__be16 proto;
 	bool csum;
 	int sg = !!(features & NETIF_F_SG);
-	int nfrags = skb_shinfo(skb)->nr_frags;
+	int nfrags = skb_shinfo(head_skb)->nr_frags;
 	int err = -ENOMEM;
 	int i = 0;
 	int pos;
 
-	proto = skb_network_protocol(skb);
+	proto = skb_network_protocol(head_skb);
 	if (unlikely(!proto))
 		return ERR_PTR(-EINVAL);
 
 	csum = !!can_checksum_protocol(features, proto);
-	__skb_push(skb, doffset);
-	headroom = skb_headroom(skb);
-	pos = skb_headlen(skb);
+	__skb_push(head_skb, doffset);
+	headroom = skb_headroom(head_skb);
+	pos = skb_headlen(head_skb);
 
 	do {
 		struct sk_buff *nskb;

@@ -2883,11 +2884,11 @@ struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features)
 		int hsize;
 		int size;
 
-		len = skb->len - offset;
+		len = head_skb->len - offset;
 		if (len > mss)
 			len = mss;
 
-		hsize = skb_headlen(skb) - offset;
+		hsize = skb_headlen(head_skb) - offset;
 		if (hsize < 0)
 			hsize = 0;
 		if (hsize > len || !sg)

@@ -2936,7 +2937,7 @@ struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features)
 			__skb_push(nskb, doffset);
 		} else {
 			nskb = __alloc_skb(hsize + doffset + headroom,
-					   GFP_ATOMIC, skb_alloc_rx_flag(skb),
+					   GFP_ATOMIC, skb_alloc_rx_flag(head_skb),
 					   NUMA_NO_NODE);
 
 			if (unlikely(!nskb))

@@ -2952,12 +2953,12 @@ struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features)
 			segs = nskb;
 		tail = nskb;
 
-		__copy_skb_header(nskb, skb);
-		nskb->mac_len = skb->mac_len;
+		__copy_skb_header(nskb, head_skb);
+		nskb->mac_len = head_skb->mac_len;
 
 		skb_headers_offset_update(nskb, skb_headroom(nskb) - headroom);
 
-		skb_copy_from_linear_data_offset(skb, -tnl_hlen,
+		skb_copy_from_linear_data_offset(head_skb, -tnl_hlen,
 						 nskb->data - tnl_hlen,
 						 doffset + tnl_hlen);

@@ -2966,7 +2967,7 @@ struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features)
 
 		if (!sg) {
 			nskb->ip_summed = CHECKSUM_NONE;
-			nskb->csum = skb_copy_and_csum_bits(skb, offset,
+			nskb->csum = skb_copy_and_csum_bits(head_skb, offset,
 							    skb_put(nskb, len),
 							    len, 0);
 			continue;

@@ -2974,10 +2975,11 @@ struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features)
 
 		nskb_frag = skb_shinfo(nskb)->frags;
 
-		skb_copy_from_linear_data_offset(skb, offset,
+		skb_copy_from_linear_data_offset(head_skb, offset,
 						 skb_put(nskb, hsize), hsize);
 
-		skb_shinfo(nskb)->tx_flags = skb_shinfo(skb)->tx_flags & SKBTX_SHARED_FRAG;
+		skb_shinfo(nskb)->tx_flags = skb_shinfo(head_skb)->tx_flags &
+			SKBTX_SHARED_FRAG;
 
 		while (pos < offset + len) {
 			if (i >= nfrags) {

@@ -3034,7 +3036,7 @@ perform_csum_check:
 						  nskb->len - doffset, 0);
 			nskb->ip_summed = CHECKSUM_NONE;
 		}
-	} while ((offset += len) < skb->len);
+	} while ((offset += len) < head_skb->len);
 
 	return segs;

Signature · 1 line

MST

Michael S. Tsirkin

author

2014-03-10 17:27 UTC #4 lore raw

PATCH 4/5 skbuff: skb_segment: s/fskb/list_skb/

In reply to Michael S. Tsirkin

fskb is unrelated to frag: it's coming from
frag_list. Rename it list_skb to avoid confusion.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 net/core/skbuff.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

net/core/skbuff.c +13 ~~-13~~

index af386e1..64ee4e6 100644
@@ -2853,7 +2853,7 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
 {
 	struct sk_buff *segs = NULL;
 	struct sk_buff *tail = NULL;
-	struct sk_buff *fskb = skb_shinfo(head_skb)->frag_list;
+	struct sk_buff *list_skb = skb_shinfo(head_skb)->frag_list;
 	skb_frag_t *frag = skb_shinfo(head_skb)->frags;
 	unsigned int mss = skb_shinfo(head_skb)->gso_size;
 	unsigned int doffset = head_skb->data - skb_mac_header(head_skb);

@@ -2894,14 +2894,14 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
 		if (hsize > len || !sg)
 			hsize = len;
 
-		if (!hsize && i >= nfrags && skb_headlen(fskb) &&
-		    (skb_headlen(fskb) == len || sg)) {
-			BUG_ON(skb_headlen(fskb) > len);
+		if (!hsize && i >= nfrags && skb_headlen(list_skb) &&
+		    (skb_headlen(list_skb) == len || sg)) {
+			BUG_ON(skb_headlen(list_skb) > len);
 
 			i = 0;
-			nfrags = skb_shinfo(fskb)->nr_frags;
-			frag = skb_shinfo(fskb)->frags;
-			pos += skb_headlen(fskb);
+			nfrags = skb_shinfo(list_skb)->nr_frags;
+			frag = skb_shinfo(list_skb)->frags;
+			pos += skb_headlen(list_skb);
 
 			while (pos < offset + len) {
 				BUG_ON(i >= nfrags);

@@ -2915,8 +2915,8 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
 				frag++;
 			}
 
-			nskb = skb_clone(fskb, GFP_ATOMIC);
-			fskb = fskb->next;
+			nskb = skb_clone(list_skb, GFP_ATOMIC);
+			list_skb = list_skb->next;
 
 			if (unlikely(!nskb))
 				goto err;

@@ -2983,15 +2983,15 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
 
 		while (pos < offset + len) {
 			if (i >= nfrags) {
-				BUG_ON(skb_headlen(fskb));
+				BUG_ON(skb_headlen(list_skb));
 
 				i = 0;
-				nfrags = skb_shinfo(fskb)->nr_frags;
-				frag = skb_shinfo(fskb)->frags;
+				nfrags = skb_shinfo(list_skb)->nr_frags;
+				frag = skb_shinfo(list_skb)->frags;
 
 				BUG_ON(!nfrags);
 
-				fskb = fskb->next;
+				list_skb = list_skb->next;
 			}
 
 			if (unlikely(skb_shinfo(nskb)->nr_frags >=

Signature · 1 line

MST

David Miller

maintainer

2014-03-10 17:40 UTC #5 lore raw

Re: [PATCH 4/5] skbuff: skb_segment: s/fskb/list_skb/

As a netdev reader, I don't see patches 1-3, did you post them somewhere else and not CC: netdev?

If so, please don't do that without at least making a mention of what's going on in the 0/N posting.

David Miller

maintainer

2014-03-10 17:43 UTC #6 lore raw

Re: [PATCH 4/5] skbuff: skb_segment: s/fskb/list_skb/

From: David Miller <[email protected]>
Date: Mon, 10 Mar 2014 13:40:54 -0400 (EDT)

Quoted 6 lines

As a netdev reader, I don't see patches 1-3, did you post them somewhere else and not CC: netdev?
If so, please don't do that without at least making a mention of what's going on in the 0/N posting.

Nevermind, I see them as older postings in my queue.

Are you using the commit date as the date for your outgoing emails?

Don't do that, it causes a patch series get chopped up and intertwined with other ones in patchwork and therefore makes more work for me.

Michael S. Tsirkin

author

2014-03-10 17:56 UTC #7 lore raw

Re: [PATCH 4/5] skbuff: skb_segment: s/fskb/list_skb/

Quoted 7 lines · David Miller

From: David Miller <[email protected]>
Date: Mon, 10 Mar 2014 13:40:54 -0400 (EDT)
Quoted 4 lines
As a netdev reader, I don't see patches 1-3, did you post them somewhere else and not CC: netdev?

Quoted 6 lines

Quoted 2 lines
If so, please don't do that without at least making a mention of what's going on in the 0/N posting.
Nevermind, I see them as older postings in my queue.
Are you using the commit date as the date for your outgoing emails?

I don't really know, I just used git send-email to prepare them.

I saw a large delay after I sent them all until patches 4/5 appeared on list. Could just be some hiccup in the smtp server (using ssmtp).

Quoted 7 lines

Don't do that, it causes a patch series get chopped up and intertwined with other ones in patchwork and therefore makes more work for me. --
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

David Miller

maintainer

2014-03-10 20:45 UTC #8 lore raw

Re: [PATCH 4/5] skbuff: skb_segment: s/fskb/list_skb/

From: "Michael S. Tsirkin" <[email protected]>
Date: Mon, 10 Mar 2014 19:56:06 +0200

Quoted 13 lines · David Miller

Quoted 12 lines
From: David Miller <[email protected]>
Date: Mon, 10 Mar 2014 13:40:54 -0400 (EDT)
Quoted 6 lines
As a netdev reader, I don't see patches 1-3, did you post them somewhere else and not CC: netdev?
If so, please don't do that without at least making a mention of what's going on in the 0/N posting.
Nevermind, I see them as older postings in my queue.

Quoted 7 lines

Quoted 1 line
Are you using the commit date as the date for your outgoing emails?
I don't really know, I just used git send-email to prepare them.
I saw a large delay after I sent them all until patches 4/5 appeared on list. Could just be some hiccup in the smtp server (using ssmtp).

Yeah that sounds like a better explanation then.

Michael S. Tsirkin

author

2014-03-10 17:28 UTC #9 lore raw

PATCH 5/5 skbuff: skb_segment: orphan frags before copying

In reply to Michael S. Tsirkin

skb_segment copies frags around, so we need to copy them carefully to avoid accessing user memory after reporting completion to userspace through a callback.

skb_segment doesn't normally happen on datapath:
TSO needs to be disabled - so disabling zero copy in this case does not look like a big deal.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 net/core/skbuff.c | 6 ++++++
 1 file changed, 6 insertions(+)

net/core/skbuff.c +6 -0

index 64ee4e6..ae11f78 100644
@@ -2857,6 +2857,7 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
 	skb_frag_t *frag = skb_shinfo(head_skb)->frags;
 	unsigned int mss = skb_shinfo(head_skb)->gso_size;
 	unsigned int doffset = head_skb->data - skb_mac_header(head_skb);
+	struct sk_buff *frag_skb = head_skb;
 	unsigned int offset = doffset;
 	unsigned int tnl_hlen = skb_tnl_header_len(head_skb);
 	unsigned int headroom;

@@ -2901,6 +2902,7 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
 			i = 0;
 			nfrags = skb_shinfo(list_skb)->nr_frags;
 			frag = skb_shinfo(list_skb)->frags;
+			frag_skb = list_skb;
 			pos += skb_headlen(list_skb);
 
 			while (pos < offset + len) {

@@ -2988,6 +2990,7 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
 				i = 0;
 				nfrags = skb_shinfo(list_skb)->nr_frags;
 				frag = skb_shinfo(list_skb)->frags;
+				frag_skb = list_skb;
 
 				BUG_ON(!nfrags);

@@ -3002,6 +3005,9 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
 				goto err;
 			}
 
+			if (unlikely(skb_orphan_frags(frag_skb, GFP_ATOMIC)))
+				goto err;
+
 			*nskb_frag = *frag;
 			__skb_frag_ref(nskb_frag);
 			size = skb_frag_size(nskb_frag);

Signature · 1 line

MST

Herbert Xu

2014-03-10 21:34 UTC #10 lore raw

Re: [PATCH 5/5] skbuff: skb_segment: orphan frags before copying

Quoted 10 lines · Michael S. Tsirkin

skb_segment copies frags around, so we need to copy them carefully to avoid accessing user memory after reporting completion to userspace through a callback.
skb_segment doesn't normally happen on datapath:
TSO needs to be disabled - so disabling zero copy in this case does not look like a big deal.
Signed-off-by Michael S. Tsirkin <[email protected]>

Why do we need this patch at all? Contrary to what you said, this path is definitely performance-critical since the basis of being able to enable GSO by default is that it causes no regressions in non-TSO cases.

Cheers,

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Michael S. Tsirkin

author

2014-03-10 21:52 UTC #11 lore raw

Re: [PATCH 5/5] skbuff: skb_segment: orphan frags before copying

Quoted 6 lines · Herbert Xu

Quoted 5 lines · Michael S. Tsirkin
skb_segment copies frags around, so we need to copy them carefully to avoid accessing user memory after reporting completion to userspace through a callback.

Quoted 7 lines

Quoted 5 lines
skb_segment doesn't normally happen on datapath:
TSO needs to be disabled - so disabling zero copy in this case does not look like a big deal.
Signed-off-by Michael S. Tsirkin <[email protected]>
Why do we need this patch at all?

The cover letter has more detail:
skb_segment ... moves frags
between skbs without orphaning them.
This causes userspace to assume it's safe to reuse the buffer, and receiver gets corrupted data. This further might leak information from the transmitter on the wire.

if still unclear, pls let me know.

Quoted 6 lines

Contrary to what you said,
this path is definitely performance-critical since the basis of being able to enable GSO by default is that it causes no regressions in non-TSO cases.
Cheers,

Well it's a single non taken branch.
If the branch is taken it's more expensive, but once we take it once callback is invoked and zero copy is disabled for future users, so it will be non taken from here on.

Quoted 8 lines

Signature · 7 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
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

Herbert Xu

2014-03-10 21:56 UTC #12 lore raw

Re: [PATCH 5/5] skbuff: skb_segment: orphan frags before copying

Quoted 2 lines · Michael S. Tsirkin

The cover letter has more detail:

For some reason I didn't receive this cover letter.

Quoted 8 lines

skb_segment ... moves frags
between skbs without orphaning them.
This causes userspace to assume it's safe to reuse the buffer, and receiver gets corrupted data. This further might leak information from the transmitter on the wire.
if still unclear, pls let me know.

Why can't we deal with this by simply postponing the copy until later? IOW if we pass along SKBTX_SHARED_FRAG will it work?

Thanks,

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Michael S. Tsirkin

author

2014-03-10 22:07 UTC #13 lore raw

Re: [PATCH 5/5] skbuff: skb_segment: orphan frags before copying

Quoted 12 lines · Herbert Xu

Quoted 2 lines · Michael S. Tsirkin
The cover letter has more detail:
For some reason I didn't receive this cover letter.
Quoted 7 lines
skb_segment ... moves frags
between skbs without orphaning them.
This causes userspace to assume it's safe to reuse the buffer, and receiver gets corrupted data. This further might leak information from the transmitter on the wire.

Quoted 6 lines

Quoted 1 line
if still unclear, pls let me know.
Why can't we deal with this by simply postponing the copy until later?
Thanks,

Once one skb completes the callback is invoked and userspace reuses this buffer for something else.
At that point it's too late to do the copy.

Quoted 1 line

IOW if we pass along SKBTX_SHARED_FRAG will it work?

I don't see how would SKBTX_SHARED_FRAG help with this at all. That only works for pages gifted to kernel by e.g. vmsplice that aren't reused by userspace.

Quoted 4 lines

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Herbert Xu

2014-03-10 22:12 UTC #14 lore raw

Re: [PATCH 5/5] skbuff: skb_segment: orphan frags before copying

Quoted 4 lines · Michael S. Tsirkin

Once one skb completes the callback is invoked and userspace reuses this buffer for something else.
At that point it's too late to do the copy.

OK, would it be possible to delay the callback until all generated skbs are completed?

Quoted 5 lines

Quoted 1 line
IOW if we pass along SKBTX_SHARED_FRAG will it work?
I don't see how would SKBTX_SHARED_FRAG help with this at all. That only works for pages gifted to kernel by e.g. vmsplice that aren't reused by userspace.

Sorry cut-n-paste error, I meant SKBTX_DEV_ZEROCOPY.

Cheers,

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Michael S. Tsirkin

author

2014-03-10 22:30 UTC #15 lore raw

Re: [PATCH 5/5] skbuff: skb_segment: orphan frags before copying

Quoted 8 lines · Herbert Xu

Quoted 4 lines · Michael S. Tsirkin
Once one skb completes the callback is invoked and userspace reuses this buffer for something else.
At that point it's too late to do the copy.
OK, would it be possible to delay the callback until all generated skbs are completed?

Hmm this won't be easy since devices can complete them in any order. See below for some ideas.

Quoted 9 lines

Quoted 5 lines
Quoted 1 line
IOW if we pass along SKBTX_SHARED_FRAG will it work?
I don't see how would SKBTX_SHARED_FRAG help with this at all. That only works for pages gifted to kernel by e.g. vmsplice that aren't reused by userspace.
Sorry cut-n-paste error, I meant SKBTX_DEV_ZEROCOPY.
Cheers,

Ah ok. Well we'd need to reference-count them somehow. Maybe skb_get and stick the pointer in ubuf callback for the new skbs, make ubuf callback kfree_skb?
Not sure if that's too much memory ...

I also worry a bit that we'll lose the success flag value which is used to disable zero copy on OOM errors more aggressively.

How about we do the simple thing as the first step and try to optimize as a separate patch on top?

Quoted 4 lines

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Herbert Xu

2014-03-11 00:39 UTC #16 lore raw

Re: [PATCH 5/5] skbuff: skb_segment: orphan frags before copying

Quoted 3 lines · Michael S. Tsirkin

How about we do the simple thing as the first step and try to optimize as a separate patch on top?

Sure let's do that.

Cheers,

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Michael S. Tsirkin

author

2014-03-11 06:41 UTC #17 lore raw

Re: [PATCH 5/5] skbuff: skb_segment: orphan frags before copying

Quoted 8 lines · Herbert Xu

Quoted 3 lines · Michael S. Tsirkin
How about we do the simple thing as the first step and try to optimize as a separate patch on top?
Sure let's do that.
Cheers,

Could you send an ack to my patchset then?
Or do you have something else in mind?

Quoted 4 lines

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Herbert Xu

Acked-by

2014-03-11 07:21 UTC #18 lore raw

Re: [PATCH 5/5] skbuff: skb_segment: orphan frags before copying

Quoted 11 lines · Michael S. Tsirkin

Quoted 8 lines · Herbert Xu
Quoted 3 lines · Michael S. Tsirkin
How about we do the simple thing as the first step and try to optimize as a separate patch on top?
Sure let's do that.
Cheers,
Could you send an ack to my patchset then?

Sure,

Acked-by Herbert Xu <[email protected]>

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

David Miller

maintainer

2014-03-11 20:27 UTC #19 lore raw

In reply to Michael S. Tsirkin

From: "Michael S. Tsirkin" <[email protected]>
Date: Mon, 10 Mar 2014 18:29:01 +0200

Quoted 15 lines

This fixes a bug in skb_segment where it moves frags between skbs without orphaning them.
This causes userspace to assume it's safe to reuse the buffer, and receiver gets corrupted data. This further might leak information from the transmitter on the wire.
To fix track which skb does a copied frag belong to, and orphan frags when copying them.
As we are tracking multiple skbs here, using
short names (skb,nskb,fskb,skb_frag,frag) becomes confusing. So before adding another one, I refactor these names slightly.

Quoted 6 lines

Patch is split out to make it easier to verify that all trasformations are trivially correct.
The problem was observed in the field,
so I think that the patch is necessary on stable as well.

Series applied and queued up for -stable, thanks for fixing this.