[PATCH 0/5] skbuff: fix skb_segment with zero copy skbs

[PATCH 0/5] skbuff: fix skb_segment with zero copy skbs

[Michael S. Tsirkin]

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(-)

-- 
MST

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

[Michael S. Tsirkin]

frag points at nskb, so name it appropriately

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 net/core/skbuff.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 5976ef0..3bc6d75 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -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:
-- 
MST

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

[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 <mst@redhat.com>
---
 net/core/skbuff.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 3bc6d75..917cb60 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -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));
-- 
MST

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

[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 <mst@redhat.com>
---
 net/core/skbuff.c | 46 ++++++++++++++++++++++++----------------------
 1 file changed, 24 insertions(+), 22 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 917cb60..af386e1 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -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;
 
-- 
MST

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

[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 <mst@redhat.com>
---
 net/core/skbuff.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index af386e1..64ee4e6 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -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 >=
-- 
MST

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

[David Miller]

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.

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

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Mon, 10 Mar 2014 13:40:54 -0400 (EDT)

> 
> 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.

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

[Michael S. Tsirkin]

On Mon, Mar 10, 2014 at 01:43:59PM -0400, David Miller wrote:
> From: David Miller <davem@davemloft.net>
> Date: Mon, 10 Mar 2014 13:40:54 -0400 (EDT)
> 
> > 
> > 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?

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).

> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

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

[David Miller]

From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Mon, 10 Mar 2014 19:56:06 +0200

> On Mon, Mar 10, 2014 at 01:43:59PM -0400, David Miller wrote:
>> From: David Miller <davem@davemloft.net>
>> Date: Mon, 10 Mar 2014 13:40:54 -0400 (EDT)
>> 
>> > 
>> > 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?
> 
> 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.

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

[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 <mst@redhat.com>
---
 net/core/skbuff.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 64ee4e6..ae11f78 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -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);
-- 
MST

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

[Herbert Xu]

On Mon, Mar 10, 2014 at 06:29:36PM +0200, Michael S. Tsirkin wrote:
> 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 <mst@redhat.com>

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

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

[Michael S. Tsirkin]

On Tue, Mar 11, 2014 at 05:34:05AM +0800, Herbert Xu wrote:
> On Mon, Mar 10, 2014 at 06:29:36PM +0200, Michael S. Tsirkin wrote:
> > 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 <mst@redhat.com>
> 
> 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.


> 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.


> -- 
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Herbert Xu]

On Mon, Mar 10, 2014 at 11:52:36PM +0200, Michael S. Tsirkin wrote:
>
> The cover letter has more detail:

For some reason I didn't receive this cover letter.
> 	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,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Michael S. Tsirkin]

On Tue, Mar 11, 2014 at 05:56:40AM +0800, Herbert Xu wrote:
> On Mon, Mar 10, 2014 at 11:52:36PM +0200, Michael S. Tsirkin wrote:
> >
> > The cover letter has more detail:
> 
> For some reason I didn't receive this cover letter.
> > 	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?
> 
> 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.


> 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.


> -- 
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Herbert Xu]

On Tue, Mar 11, 2014 at 12:07:17AM +0200, Michael S. Tsirkin wrote:
> 
> 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?

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

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

[Michael S. Tsirkin]

On Tue, Mar 11, 2014 at 06:12:23AM +0800, Herbert Xu wrote:
> On Tue, Mar 11, 2014 at 12:07:17AM +0200, Michael S. Tsirkin wrote:
> > 
> > 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.

> > > 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?

> -- 
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Herbert Xu]

On Tue, Mar 11, 2014 at 12:30:01AM +0200, Michael S. Tsirkin wrote:
>
> 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,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Michael S. Tsirkin]

On Tue, Mar 11, 2014 at 08:39:41AM +0800, Herbert Xu wrote:
> On Tue, Mar 11, 2014 at 12:30:01AM +0200, Michael S. Tsirkin wrote:
> >
> > 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?

> -- 
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Herbert Xu]

On Tue, Mar 11, 2014 at 08:41:50AM +0200, Michael S. Tsirkin wrote:
> On Tue, Mar 11, 2014 at 08:39:41AM +0800, Herbert Xu wrote:
> > On Tue, Mar 11, 2014 at 12:30:01AM +0200, Michael S. Tsirkin wrote:
> > >
> > > 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 <herbert@gondor.apana.org.au>
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 0/5] skbuff: fix skb_segment with zero copy skbs

[David Miller]

From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Mon, 10 Mar 2014 18:29:01 +0200

> 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.

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