[net-next-2.6 PATCH v2] net: consolidate netif_needs_gso() checks

[net-next-2.6 PATCH v2] net: consolidate netif_needs_gso() checks

[Jeff Kirsher]

From: John Fastabend <john.r.fastabend@intel.com>

netif_needs_gso() is checked twice in the TX path once,
before submitting the skb to the qdisc and once after
it is dequeued from the qdisc just before calling
ndo_hard_start().  This opens a window for a user to
change the gso/tso or tx checksum settings that can
cause netif_needs_gso to be true in one check and false
in the other.

Specifically, changing TX checksum setting may cause
the warning in skb_gso_segment() to be triggered if
the checksum is calculated earlier.

This consolidates the netif_needs_gso() calls so that
the stack only checks if gso is needed in
dev_hard_start_xmit().

Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

 net/core/dev.c |   50 +++++++++++++++++++++++++++++---------------------
 1 files changed, 29 insertions(+), 21 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index eb7f1a4..626124d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1835,12 +1835,40 @@ int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
 {
 	const struct net_device_ops *ops = dev->netdev_ops;
 	int rc = NETDEV_TX_OK;
+	int need_gso = netif_needs_gso(dev, skb);
+
+	if (!need_gso) {
+		if (skb_has_frags(skb) &&
+		    !(dev->features & NETIF_F_FRAGLIST) &&
+		    __skb_linearize(skb))
+			goto out_kfree_skb;
+
+		/* Fragmented skb is linearized if device does not support SG,
+		 * or if at least one of fragments is in highmem and device
+		 * does not support DMA from it.
+		 */
+		if (skb_shinfo(skb)->nr_frags &&
+		    (!(dev->features & NETIF_F_SG) ||
+		      illegal_highdma(dev, skb)) &&
+		    __skb_linearize(skb))
+			goto out_kfree_skb;
+		/* If packet is not checksummed and device does not support
+		 * checksumming for this protocol, complete checksumming here.
+		 */
+		if (skb->ip_summed == CHECKSUM_PARTIAL) {
+			skb_set_transport_header(skb, skb->csum_start -
+				      skb_headroom(skb));
+			if (!dev_can_checksum(dev, skb) &&
+			     skb_checksum_help(skb))
+				goto out_kfree_skb;
+		}
+	}
 
 	if (likely(!skb->next)) {
 		if (!list_empty(&ptype_all))
 			dev_queue_xmit_nit(skb, dev);
 
-		if (netif_needs_gso(dev, skb)) {
+		if (need_gso) {
 			if (unlikely(dev_gso_segment(skb)))
 				goto out_kfree_skb;
 			if (skb->next)
@@ -2056,25 +2084,6 @@ int dev_queue_xmit(struct sk_buff *skb)
 	struct Qdisc *q;
 	int rc = -ENOMEM;
 
-	/* GSO will handle the following emulations directly. */
-	if (netif_needs_gso(dev, skb))
-		goto gso;
-
-	/* Convert a paged skb to linear, if required */
-	if (skb_needs_linearize(skb, dev) && __skb_linearize(skb))
-		goto out_kfree_skb;
-
-	/* If packet is not checksummed and device does not support
-	 * checksumming for this protocol, complete checksumming here.
-	 */
-	if (skb->ip_summed == CHECKSUM_PARTIAL) {
-		skb_set_transport_header(skb, skb->csum_start -
-					      skb_headroom(skb));
-		if (!dev_can_checksum(dev, skb) && skb_checksum_help(skb))
-			goto out_kfree_skb;
-	}
-
-gso:
 	/* Disable soft irqs for various locks below. Also
 	 * stops preemption for RCU.
 	 */
@@ -2133,7 +2142,6 @@ gso:
 	rc = -ENETDOWN;
 	rcu_read_unlock_bh();
 
-out_kfree_skb:
 	kfree_skb(skb);
 	return rc;
 out:

Re: [net-next-2.6 PATCH v2] net: consolidate netif_needs_gso() checks

[David Miller]

From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Fri, 26 Feb 2010 16:20:11 -0800

> From: John Fastabend <john.r.fastabend@intel.com>
> 
> netif_needs_gso() is checked twice in the TX path once,
> before submitting the skb to the qdisc and once after
> it is dequeued from the qdisc just before calling
> ndo_hard_start().  This opens a window for a user to
> change the gso/tso or tx checksum settings that can
> cause netif_needs_gso to be true in one check and false
> in the other.
> 
> Specifically, changing TX checksum setting may cause
> the warning in skb_gso_segment() to be triggered if
> the checksum is calculated earlier.
> 
> This consolidates the netif_needs_gso() calls so that
> the stack only checks if gso is needed in
> dev_hard_start_xmit().
> 
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

This looks mostly fine, but I have at least one doubt.

If we have ip_summed == CHECKSUM_PARTIAL might some classifier
or packet scheduler action module require that the
transport header is setup properly before the SKB gets into
there?

Arguably, that's happening already in the GSO case but this
change is bringing the issue more to light.

Herbert, could you also take a look at this patch?

Thanks!

> diff --git a/net/core/dev.c b/net/core/dev.c
> index eb7f1a4..626124d 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1835,12 +1835,40 @@ int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
>  {
>  	const struct net_device_ops *ops = dev->netdev_ops;
>  	int rc = NETDEV_TX_OK;
> +	int need_gso = netif_needs_gso(dev, skb);
> +
> +	if (!need_gso) {
> +		if (skb_has_frags(skb) &&
> +		    !(dev->features & NETIF_F_FRAGLIST) &&
> +		    __skb_linearize(skb))
> +			goto out_kfree_skb;
> +
> +		/* Fragmented skb is linearized if device does not support SG,
> +		 * or if at least one of fragments is in highmem and device
> +		 * does not support DMA from it.
> +		 */
> +		if (skb_shinfo(skb)->nr_frags &&
> +		    (!(dev->features & NETIF_F_SG) ||
> +		      illegal_highdma(dev, skb)) &&
> +		    __skb_linearize(skb))
> +			goto out_kfree_skb;
> +		/* If packet is not checksummed and device does not support
> +		 * checksumming for this protocol, complete checksumming here.
> +		 */
> +		if (skb->ip_summed == CHECKSUM_PARTIAL) {
> +			skb_set_transport_header(skb, skb->csum_start -
> +				      skb_headroom(skb));
> +			if (!dev_can_checksum(dev, skb) &&
> +			     skb_checksum_help(skb))
> +				goto out_kfree_skb;
> +		}
> +	}
>  
>  	if (likely(!skb->next)) {
>  		if (!list_empty(&ptype_all))
>  			dev_queue_xmit_nit(skb, dev);
>  
> -		if (netif_needs_gso(dev, skb)) {
> +		if (need_gso) {
>  			if (unlikely(dev_gso_segment(skb)))
>  				goto out_kfree_skb;
>  			if (skb->next)
> @@ -2056,25 +2084,6 @@ int dev_queue_xmit(struct sk_buff *skb)
>  	struct Qdisc *q;
>  	int rc = -ENOMEM;
>  
> -	/* GSO will handle the following emulations directly. */
> -	if (netif_needs_gso(dev, skb))
> -		goto gso;
> -
> -	/* Convert a paged skb to linear, if required */
> -	if (skb_needs_linearize(skb, dev) && __skb_linearize(skb))
> -		goto out_kfree_skb;
> -
> -	/* If packet is not checksummed and device does not support
> -	 * checksumming for this protocol, complete checksumming here.
> -	 */
> -	if (skb->ip_summed == CHECKSUM_PARTIAL) {
> -		skb_set_transport_header(skb, skb->csum_start -
> -					      skb_headroom(skb));
> -		if (!dev_can_checksum(dev, skb) && skb_checksum_help(skb))
> -			goto out_kfree_skb;
> -	}
> -
> -gso:
>  	/* Disable soft irqs for various locks below. Also
>  	 * stops preemption for RCU.
>  	 */
> @@ -2133,7 +2142,6 @@ gso:
>  	rc = -ENETDOWN;
>  	rcu_read_unlock_bh();
>  
> -out_kfree_skb:
>  	kfree_skb(skb);
>  	return rc;
>  out:
> 

Re: [net-next-2.6 PATCH v2] net: consolidate netif_needs_gso() checks

[Herbert Xu]

On Sat, Feb 27, 2010 at 03:27:25AM -0800, David Miller wrote:
>
> If we have ip_summed == CHECKSUM_PARTIAL might some classifier
> or packet scheduler action module require that the
> transport header is setup properly before the SKB gets into
> there?

I think this is OK as the transport header setting was only there
for backwards compatibility with certain drivers that relied on it.
Its use was very much isolated.

I just did a grep on net/sched and couldn't see anything obvious
that uses transport_header.

> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index eb7f1a4..626124d 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -1835,12 +1835,40 @@ int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
> >  {
> >  	const struct net_device_ops *ops = dev->netdev_ops;
> >  	int rc = NETDEV_TX_OK;
> > +	int need_gso = netif_needs_gso(dev, skb);
> > +
> > +	if (!need_gso) {
> > +		if (skb_has_frags(skb) &&
> > +		    !(dev->features & NETIF_F_FRAGLIST) &&
> > +		    __skb_linearize(skb))
> > +			goto out_kfree_skb;
> > +
> > +		/* Fragmented skb is linearized if device does not support SG,
> > +		 * or if at least one of fragments is in highmem and device
> > +		 * does not support DMA from it.
> > +		 */
> > +		if (skb_shinfo(skb)->nr_frags &&
> > +		    (!(dev->features & NETIF_F_SG) ||
> > +		      illegal_highdma(dev, skb)) &&
> > +		    __skb_linearize(skb))
> > +			goto out_kfree_skb;

Please use skb_needs_linearize.

> > +		/* If packet is not checksummed and device does not support
> > +		 * checksumming for this protocol, complete checksumming here.
> > +		 */
> > +		if (skb->ip_summed == CHECKSUM_PARTIAL) {
> > +			skb_set_transport_header(skb, skb->csum_start -
> > +				      skb_headroom(skb));
> > +			if (!dev_can_checksum(dev, skb) &&
> > +			     skb_checksum_help(skb))
> > +				goto out_kfree_skb;
> > +		}
> > +	}
> >  
> >  	if (likely(!skb->next)) {
> >  		if (!list_empty(&ptype_all))
> >  			dev_queue_xmit_nit(skb, dev);
> >  
> > -		if (netif_needs_gso(dev, skb)) {
> > +		if (need_gso) {
> >  			if (unlikely(dev_gso_segment(skb)))
> >  				goto out_kfree_skb;
> >  			if (skb->next)

That whole if block should be moved into an else clause here:

		if (netif_needs_gso(dev, skb)) {
			if (unlikely(dev_gso_segment(skb)))
				goto out_kfree_skb;
			if (skb->next)
				goto gso;
		} else {
			do your thing
		}

The reason is that the other paths only act on the fragments
generated by GSO, so logically with your change we shouldn't
apply any further software emulation to them, even if the device
setting changed.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [net-next-2.6 PATCH v2] net: consolidate netif_needs_gso() checks

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Sat, 27 Feb 2010 23:52:45 +0800

> I just did a grep on net/sched and couldn't see anything obvious
> that uses transport_header.

I think skb_checksum_help() would be such a use and I
see a reference in net/sched/sch_netem.c

Re: [net-next-2.6 PATCH v2] net: consolidate netif_needs_gso() checks

[Herbert Xu]

On Sat, Feb 27, 2010 at 08:17:09AM -0800, David Miller wrote:
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Date: Sat, 27 Feb 2010 23:52:45 +0800
> 
> > I just did a grep on net/sched and couldn't see anything obvious
> > that uses transport_header.
> 
> I think skb_checksum_help() would be such a use and I
> see a reference in net/sched/sch_netem.c

AFAICS skb_checksum_help uses csum_start and not transport_header.
Has this changed recently?

Once upon a time some drivers used transport_header instead of
csum_start, but even those seem to be gone mostly so one day we
could remove this setting completely.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [net-next-2.6 PATCH v2] net: consolidate netif_needs_gso() checks

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Sun, 28 Feb 2010 08:29:53 +0800

> On Sat, Feb 27, 2010 at 08:17:09AM -0800, David Miller wrote:
>> From: Herbert Xu <herbert@gondor.apana.org.au>
>> Date: Sat, 27 Feb 2010 23:52:45 +0800
>> 
>> > I just did a grep on net/sched and couldn't see anything obvious
>> > that uses transport_header.
>> 
>> I think skb_checksum_help() would be such a use and I
>> see a reference in net/sched/sch_netem.c
> 
> AFAICS skb_checksum_help uses csum_start and not transport_header.
> Has this changed recently?
> 
> Once upon a time some drivers used transport_header instead of
> csum_start, but even those seem to be gone mostly so one day we
> could remove this setting completely.

Ok, then this clears up all of my concerns.  Thanks Herbert.

Re: [net-next-2.6 PATCH v2] net: consolidate netif_needs_gso() checks

[David Miller]

Jeff and John, please address Herbert's feedback and I'll apply
this patch.

Thanks!

Re: [net-next-2.6 PATCH v2] net: consolidate netif_needs_gso() checks

[John Fastabend]

Herbert Xu wrote:
> On Sat, Feb 27, 2010 at 03:27:25AM -0800, David Miller wrote:
>   
>> If we have ip_summed == CHECKSUM_PARTIAL might some classifier
>> or packet scheduler action module require that the
>> transport header is setup properly before the SKB gets into
>> there?
>>     
>
> I think this is OK as the transport header setting was only there
> for backwards compatibility with certain drivers that relied on it.
> Its use was very much isolated.
>
> I just did a grep on net/sched and couldn't see anything obvious
> that uses transport_header.
>
>   
>>> diff --git a/net/core/dev.c b/net/core/dev.c
>>> index eb7f1a4..626124d 100644
>>> --- a/net/core/dev.c
>>> +++ b/net/core/dev.c
>>> @@ -1835,12 +1835,40 @@ int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
>>>  {
>>>  	const struct net_device_ops *ops = dev->netdev_ops;
>>>  	int rc = NETDEV_TX_OK;
>>> +	int need_gso = netif_needs_gso(dev, skb);
>>> +
>>> +	if (!need_gso) {
>>> +		if (skb_has_frags(skb) &&
>>> +		    !(dev->features & NETIF_F_FRAGLIST) &&
>>> +		    __skb_linearize(skb))
>>> +			goto out_kfree_skb;
>>> +
>>> +		/* Fragmented skb is linearized if device does not support SG,
>>> +		 * or if at least one of fragments is in highmem and device
>>> +		 * does not support DMA from it.
>>> +		 */
>>> +		if (skb_shinfo(skb)->nr_frags &&
>>> +		    (!(dev->features & NETIF_F_SG) ||
>>> +		      illegal_highdma(dev, skb)) &&
>>> +		    __skb_linearize(skb))
>>> +			goto out_kfree_skb;
>>>       
>
> Please use skb_needs_linearize.
>
>   
>>> +		/* If packet is not checksummed and device does not support
>>> +		 * checksumming for this protocol, complete checksumming here.
>>> +		 */
>>> +		if (skb->ip_summed == CHECKSUM_PARTIAL) {
>>> +			skb_set_transport_header(skb, skb->csum_start -
>>> +				      skb_headroom(skb));
>>> +			if (!dev_can_checksum(dev, skb) &&
>>> +			     skb_checksum_help(skb))
>>> +				goto out_kfree_skb;
>>> +		}
>>> +	}
>>>  
>>>  	if (likely(!skb->next)) {
>>>  		if (!list_empty(&ptype_all))
>>>  			dev_queue_xmit_nit(skb, dev);
>>>  
>>> -		if (netif_needs_gso(dev, skb)) {
>>> +		if (need_gso) {
>>>  			if (unlikely(dev_gso_segment(skb)))
>>>  				goto out_kfree_skb;
>>>  			if (skb->next)
>>>       
>
> That whole if block should be moved into an else clause here:
>
> 		if (netif_needs_gso(dev, skb)) {
> 			if (unlikely(dev_gso_segment(skb)))
> 				goto out_kfree_skb;
> 			if (skb->next)
> 				goto gso;
> 		} else {
> 			do your thing
> 		}
>
> The reason is that the other paths only act on the fragments
> generated by GSO, so logically with your change we shouldn't
> apply any further software emulation to them, even if the device
> setting changed.
>
> Cheers,
>   
Herbert,

It looks like dev_gso_segment() could be used to "Verify header 
integrity only" according to the comment?  If this is true I think the 
logic should probably be

		if (netif_needs_gso(dev, skb)) {
			if (unlikely(dev_gso_segment(skb)))
				goto out_kfree_skb;
			if (skb->next)
				goto gso;
		} 
		do your thing

		

That way we linearize the skb if necessary in the case were 
dev_gso_segment() only verifies the header and does not return a list of 
segments.

thanks,
John.

Re: [net-next-2.6 PATCH v2] net: consolidate netif_needs_gso() checks

[Herbert Xu]

On Sat, Mar 06, 2010 at 11:27:50AM -0800, John Fastabend wrote:
>
> It looks like dev_gso_segment() could be used to "Verify header  
> integrity only" according to the comment?  If this is true I think the  
> logic should probably be
>
> 		if (netif_needs_gso(dev, skb)) {
> 			if (unlikely(dev_gso_segment(skb)))
> 				goto out_kfree_skb;
> 			if (skb->next)
> 				goto gso;
> 		} 		do your thing
>
> 		
>
> That way we linearize the skb if necessary in the case were  
> dev_gso_segment() only verifies the header and does not return a list of  
> segments.

If we needed to linearise the skb then dev_gso_segment should
perform the segmentation.  Is there a case where it doesn't?

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [net-next-2.6 PATCH v2] net: consolidate netif_needs_gso() checks

[John Fastabend]

Herbert Xu wrote:
> On Sat, Mar 06, 2010 at 11:27:50AM -0800, John Fastabend wrote:
>   
>> It looks like dev_gso_segment() could be used to "Verify header  
>> integrity only" according to the comment?  If this is true I think the  
>> logic should probably be
>>
>> 		if (netif_needs_gso(dev, skb)) {
>> 			if (unlikely(dev_gso_segment(skb)))
>> 				goto out_kfree_skb;
>> 			if (skb->next)
>> 				goto gso;
>> 		} 		do your thing
>>
>> 		
>>
>> That way we linearize the skb if necessary in the case were  
>> dev_gso_segment() only verifies the header and does not return a list of  
>> segments.
>>     
>
> If we needed to linearise the skb then dev_gso_segment should
> perform the segmentation.  Is there a case where it doesn't?
>
> Cheers,
Nope as far as I can tell all cases are covered my concerns were 
unfounded.  Thanks for the review, I'll get this updated and resent.

-John.