[PATCH 3.2-3.12] skbuff: skb_segment: orphan frags before copying

[PATCH 3.2-3.12] skbuff: skb_segment: orphan frags before copying

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1446 bytes --]

From: "Michael S. Tsirkin" <mst@redhat.com>

commit 1fd819ecb90cc9b822cd84d3056ddba315d3340f upstream.

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>
Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
Signed-off-by: David S. Miller <davem@davemloft.net>
[bwh: Backported to 3.2.  As skb_segment() only supports page-frags *or* a
 frag list, there is no need for the additional frag_skb pointer or the
 preparatory renaming.]
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
For branches older than 3.6, commit a353e0ce0fd4 ('skbuff: add an api to
orphan frags') is needed before this.  This is untested and I would
appreciate a review.

Ben.
---
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -2699,6 +2699,9 @@ struct sk_buff *skb_segment(struct sk_bu
 						 skb_put(nskb, hsize), hsize);
 
 		while (pos < offset + len && i < nfrags) {
+			if (unlikely(skb_orphan_frags(skb, GFP_ATOMIC)))
+				goto err;
+
 			*frag = skb_shinfo(skb)->frags[i];
 			__skb_frag_ref(frag);
 			size = skb_frag_size(frag);


-- 
Ben Hutchings
I haven't lost my mind; it's backed up on tape somewhere.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH 3.2-3.12] skbuff: skb_segment: orphan frags before copying

[David Miller]

From: Ben Hutchings <ben@decadent.org.uk>
Date: Sun, 13 Apr 2014 23:57:40 +0100

> From: "Michael S. Tsirkin" <mst@redhat.com>
> 
> commit 1fd819ecb90cc9b822cd84d3056ddba315d3340f upstream.
> 
> 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>
> Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> [bwh: Backported to 3.2.  As skb_segment() only supports page-frags *or* a
>  frag list, there is no need for the additional frag_skb pointer or the
>  preparatory renaming.]
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> ---
> For branches older than 3.6, commit a353e0ce0fd4 ('skbuff: add an api to
> orphan frags') is needed before this.  This is untested and I would
> appreciate a review.

I didn't do this backport because it seemed risky unless Michael
or someone else tested it thoroughly.

Re: [PATCH 3.2-3.12] skbuff: skb_segment: orphan frags before copying

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1483 bytes --]

On Sun, 2014-04-13 at 19:20 -0400, David Miller wrote:
> From: Ben Hutchings <ben@decadent.org.uk>
> Date: Sun, 13 Apr 2014 23:57:40 +0100
> 
> > From: "Michael S. Tsirkin" <mst@redhat.com>
> > 
> > commit 1fd819ecb90cc9b822cd84d3056ddba315d3340f upstream.
> > 
> > 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>
> > Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
> > Signed-off-by: David S. Miller <davem@davemloft.net>
> > [bwh: Backported to 3.2.  As skb_segment() only supports page-frags *or* a
> >  frag list, there is no need for the additional frag_skb pointer or the
> >  preparatory renaming.]
> > Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> > ---
> > For branches older than 3.6, commit a353e0ce0fd4 ('skbuff: add an api to
> > orphan frags') is needed before this.  This is untested and I would
> > appreciate a review.
> 
> I didn't do this backport because it seemed risky unless Michael
> or someone else tested it thoroughly.

Understood; I'll wait for further feedback.

Ben.

-- 
Ben Hutchings
I say we take off; nuke the site from orbit.  It's the only way to be sure.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

[PATCH 3.2] ipv6: don't set DST_NOCOUNT for remotely added routes

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1222 bytes --]

From: Sabrina Dubroca <sd@queasysnail.net>

commit c88507fbad8055297c1d1e21e599f46960cbee39 upstream.

DST_NOCOUNT should only be used if an authorized user adds routes
locally. In case of routes which are added on behalf of router
advertisments this flag must not get used as it allows an unlimited
number of routes getting added remotely.

Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
Signed-off-by: David S. Miller <davem@davemloft.net>
[bwh: Backported to 3.2: adjust context]
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
This is untested, but looks almost identical to what you sent for 3.4.
Please ack/nak.

Ben.

 net/ipv6/route.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -1250,7 +1250,7 @@ int ip6_route_add(struct fib6_config *cf
 		goto out;
 	}
 
-	rt = ip6_dst_alloc(&net->ipv6.ip6_dst_ops, NULL, DST_NOCOUNT);
+	rt = ip6_dst_alloc(&net->ipv6.ip6_dst_ops, NULL, (cfg->fc_flags & RTF_ADDRCONF) ? 0 : DST_NOCOUNT);
 
 	if (rt == NULL) {
 		err = -ENOMEM;


-- 
Ben Hutchings
I haven't lost my mind; it's backed up on tape somewhere.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH 3.2] ipv6: don't set DST_NOCOUNT for remotely added routes

[David Miller]

From: Ben Hutchings <ben@decadent.org.uk>
Date: Mon, 14 Apr 2014 00:02:22 +0100

> From: Sabrina Dubroca <sd@queasysnail.net>
> 
> commit c88507fbad8055297c1d1e21e599f46960cbee39 upstream.
> 
> DST_NOCOUNT should only be used if an authorized user adds routes
> locally. In case of routes which are added on behalf of router
> advertisments this flag must not get used as it allows an unlimited
> number of routes getting added remotely.
> 
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
> Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> [bwh: Backported to 3.2: adjust context]
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> ---
> This is untested, but looks almost identical to what you sent for 3.4.
> Please ack/nak.

This looks good to me:

Acked-by: David S. Miller <davem@davemloft.net>

Re: [PATCH 3.2] ipv6: don't set DST_NOCOUNT for remotely added routes

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1149 bytes --]

On Sun, 2014-04-13 at 19:19 -0400, David Miller wrote:
> From: Ben Hutchings <ben@decadent.org.uk>
> Date: Mon, 14 Apr 2014 00:02:22 +0100
> 
> > From: Sabrina Dubroca <sd@queasysnail.net>
> > 
> > commit c88507fbad8055297c1d1e21e599f46960cbee39 upstream.
> > 
> > DST_NOCOUNT should only be used if an authorized user adds routes
> > locally. In case of routes which are added on behalf of router
> > advertisments this flag must not get used as it allows an unlimited
> > number of routes getting added remotely.
> > 
> > Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
> > Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> > Signed-off-by: David S. Miller <davem@davemloft.net>
> > [bwh: Backported to 3.2: adjust context]
> > Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> > ---
> > This is untested, but looks almost identical to what you sent for 3.4.
> > Please ack/nak.
> 
> This looks good to me:
> 
> Acked-by: David S. Miller <davem@davemloft.net>

Thanks, I've queued this up now.

Ben.

-- 
Ben Hutchings
I say we take off; nuke the site from orbit.  It's the only way to be sure.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

[PATCH 3.2-3.12] skbuff: skb_segment: orphan frags before copying

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1691 bytes --]

From: "Michael S. Tsirkin" <mst@redhat.com>

commit 1fd819ecb90cc9b822cd84d3056ddba315d3340f upstream.

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>
Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
Signed-off-by: David S. Miller <davem@davemloft.net>
[bwh: Backported to 3.2.  As skb_segment() only supports page-frags *or* a
 frag list, there is no need for the additional frag_skb pointer or the
 preparatory renaming.]
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
This is what I used in Debian for 3.2, and I believe it applies to all
stable branches up to 3.12 inclusive.

For branches older than 3.6, this requires cherry-picking commit
a353e0ce0fd4 ('skbuff: add an api to orphan frags').  To avoid breaking
OOT builds of openvswitch, which will use skb_orphan_frags() if
available, it is also necessary to cherry-pick commit dcc0fb782b3a
('skbuff: export skb_copy_ubufs').

Ben.

--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -2701,6 +2701,9 @@ struct sk_buff *skb_segment(struct sk_bu
 						 skb_put(nskb, hsize), hsize);
 
 		while (pos < offset + len && i < nfrags) {
+			if (unlikely(skb_orphan_frags(skb, GFP_ATOMIC)))
+				goto err;
+
 			*frag = skb_shinfo(skb)->frags[i];
 			__skb_frag_ref(frag);
 			size = skb_frag_size(frag);

-- 
Ben Hutchings
You can't have everything.  Where would you put it?

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 3.2-3.12] skbuff: skb_segment: orphan frags before copying

[Luis Henriques]

On Fri, Jun 06, 2014 at 05:09:28PM +0100, Ben Hutchings wrote:
> From: "Michael S. Tsirkin" <mst@redhat.com>
> 
> commit 1fd819ecb90cc9b822cd84d3056ddba315d3340f upstream.
> 
> 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>
> Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> [bwh: Backported to 3.2.  As skb_segment() only supports page-frags *or* a
>  frag list, there is no need for the additional frag_skb pointer or the
>  preparatory renaming.]
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> ---
> This is what I used in Debian for 3.2, and I believe it applies to all
> stable branches up to 3.12 inclusive.
> 
> For branches older than 3.6, this requires cherry-picking commit
> a353e0ce0fd4 ('skbuff: add an api to orphan frags').  To avoid breaking
> OOT builds of openvswitch, which will use skb_orphan_frags() if
> available, it is also necessary to cherry-pick commit dcc0fb782b3a
> ('skbuff: export skb_copy_ubufs').
> 
> Ben.
> 

Thanks Ben, I'll queue it for the 3.11 kernel.

Cheers,
--
Luís

> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -2701,6 +2701,9 @@ struct sk_buff *skb_segment(struct sk_bu
>  						 skb_put(nskb, hsize), hsize);
>  
>  		while (pos < offset + len && i < nfrags) {
> +			if (unlikely(skb_orphan_frags(skb, GFP_ATOMIC)))
> +				goto err;
> +
>  			*frag = skb_shinfo(skb)->frags[i];
>  			__skb_frag_ref(frag);
>  			size = skb_frag_size(frag);
> 
> -- 
> Ben Hutchings
> You can't have everything.  Where would you put it?

Re: [PATCH 3.2-3.12] skbuff: skb_segment: orphan frags before copying

[Greg KH]

On Fri, Jun 06, 2014 at 05:09:28PM +0100, Ben Hutchings wrote:
> From: "Michael S. Tsirkin" <mst@redhat.com>
> 
> commit 1fd819ecb90cc9b822cd84d3056ddba315d3340f upstream.
> 
> 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>
> Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> [bwh: Backported to 3.2.  As skb_segment() only supports page-frags *or* a
>  frag list, there is no need for the additional frag_skb pointer or the
>  preparatory renaming.]
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> ---
> This is what I used in Debian for 3.2, and I believe it applies to all
> stable branches up to 3.12 inclusive.
> 
> For branches older than 3.6, this requires cherry-picking commit
> a353e0ce0fd4 ('skbuff: add an api to orphan frags').  To avoid breaking
> OOT builds of openvswitch, which will use skb_orphan_frags() if
> available, it is also necessary to cherry-pick commit dcc0fb782b3a
> ('skbuff: export skb_copy_ubufs').

Thanks, I've done this for 3.4 now.

greg k-h