Patchwork net: Handle gso packets in dev_forward_skb

login
register
mail settings
Submitter Eric W. Biederman
Date March 21, 2011, 9:25 p.m.
Message ID <m1wrjsi2tf.fsf@fess.ebiederm.org>
Download mbox | patch
Permalink /patch/87829/
State Deferred
Delegated to: David Miller
Headers show

Comments

Eric W. Biederman - March 21, 2011, 9:25 p.m.
Today when gso packets reach dev_forward_skb through the macvlan driver
we drop them on the floor because they exceed the device mtu.  Ouch!

I don't undersand the subtelties of gso but I think it is sufficient to
simply relax the checks and let gso packets through without an mtu
check, and it works in my test case.

If needed we can split the gso packets into multiple packets here but
that just seems like a wast of memory and time.

Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
---
 net/core/dev.c |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)
Eric Dumazet - March 21, 2011, 9:42 p.m.
Le lundi 21 mars 2011 à 14:25 -0700, Eric W. Biederman a écrit :
> Today when gso packets reach dev_forward_skb through the macvlan driver
> we drop them on the floor because they exceed the device mtu.  Ouch!
> 
> I don't undersand the subtelties of gso but I think it is sufficient to
> simply relax the checks and let gso packets through without an mtu
> check, and it works in my test case.
> 
> If needed we can split the gso packets into multiple packets here but
> that just seems like a wast of memory and time.
> 
> Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
> ---
>  net/core/dev.c |   16 ++++++++++------
>  1 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 0b88eba..2e26606 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1527,17 +1527,21 @@ int dev_forward_skb(struct net_device *dev, struct sk_buff *skb)
>  	skb_orphan(skb);
>  	nf_reset(skb);
>  
> -	if (unlikely(!(dev->flags & IFF_UP) ||
> -		     (skb->len > (dev->mtu + dev->hard_header_len + VLAN_HLEN)))) {
> -		atomic_long_inc(&dev->rx_dropped);
> -		kfree_skb(skb);
> -		return NET_RX_DROP;
> -	}
> +	if (unlikely(!(dev->flags & IFF_UP)))
> +		goto kfree_skb;
> +	/* Don't check mtu on gso packets... */
> +	if (unlikely(!skb_is_gso(skb) &&
> +		     (skb->len > (dev->mtu + dev->hard_header_len + VLAN_HLEN))))
> +		goto kfree_skb;
>  	skb_set_dev(skb, dev);
>  	skb->tstamp.tv64 = 0;
>  	skb->pkt_type = PACKET_HOST;
>  	skb->protocol = eth_type_trans(skb, dev);
>  	return netif_rx(skb);
> +kfree_skb:
> +	atomic_long_inc(&dev->rx_dropped);
> +	kfree_skb(skb);
> +	return NET_RX_DROP;
>  }
>  EXPORT_SYMBOL_GPL(dev_forward_skb);
>  


Hmm, did you follow http://patchwork.ozlabs.org/patch/86815/
discussion ?



--
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
Eric W. Biederman - March 21, 2011, 10 p.m.
Eric Dumazet <eric.dumazet@gmail.com> writes:

> Hmm, did you follow http://patchwork.ozlabs.org/patch/86815/
> discussion ?

No.  It seems I missed it, but I do have the same problem, and
essentially the same fix. 

Eric
--
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
David Miller - March 28, 2011, 1:09 a.m.
From: ebiederm@xmission.com (Eric W. Biederman)
Date: Mon, 21 Mar 2011 15:00:56 -0700

> Eric Dumazet <eric.dumazet@gmail.com> writes:
> 
>> Hmm, did you follow http://patchwork.ozlabs.org/patch/86815/
>> discussion ?
> 
> No.  It seems I missed it, but I do have the same problem, and
> essentially the same fix. 

Can someone work on getting this straightened out and resubmit the
final patch so I can apply something?

Thanks.
--
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
Eric Dumazet - March 28, 2011, 7:20 p.m.
Le dimanche 27 mars 2011 à 18:09 -0700, David Miller a écrit :
> From: ebiederm@xmission.com (Eric W. Biederman)
> Date: Mon, 21 Mar 2011 15:00:56 -0700
> 
> > Eric Dumazet <eric.dumazet@gmail.com> writes:
> > 
> >> Hmm, did you follow http://patchwork.ozlabs.org/patch/86815/
> >> discussion ?
> > 
> > No.  It seems I missed it, but I do have the same problem, and
> > essentially the same fix. 
> 
> Can someone work on getting this straightened out and resubmit the
> final patch so I can apply something?
> 

Apparently Daniel is busy.

I guess we can use Eric B. patch, then ?



--
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
Eric W. Biederman - March 28, 2011, 7:36 p.m.
Eric Dumazet <eric.dumazet@gmail.com> writes:

> Le dimanche 27 mars 2011 à 18:09 -0700, David Miller a écrit :
>> From: ebiederm@xmission.com (Eric W. Biederman)
>> Date: Mon, 21 Mar 2011 15:00:56 -0700
>> 
>> > Eric Dumazet <eric.dumazet@gmail.com> writes:
>> > 
>> >> Hmm, did you follow http://patchwork.ozlabs.org/patch/86815/
>> >> discussion ?
>> > 
>> > No.  It seems I missed it, but I do have the same problem, and
>> > essentially the same fix. 
>> 
>> Can someone work on getting this straightened out and resubmit the
>> final patch so I can apply something?
>> 
>
> Apparently Daniel is busy.
>
> I guess we can use Eric B. patch, then ?

That sounds good to me.

After reviewing the other thread to the best of my knowledge there are
no issues with the patch I submitted.  At worst I am 4 bytes extra
permissive about the mtu because of the way that path handles vlan
headers and that isn't something my patch changed.

Eric


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

Patch

diff --git a/net/core/dev.c b/net/core/dev.c
index 0b88eba..2e26606 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1527,17 +1527,21 @@  int dev_forward_skb(struct net_device *dev, struct sk_buff *skb)
 	skb_orphan(skb);
 	nf_reset(skb);
 
-	if (unlikely(!(dev->flags & IFF_UP) ||
-		     (skb->len > (dev->mtu + dev->hard_header_len + VLAN_HLEN)))) {
-		atomic_long_inc(&dev->rx_dropped);
-		kfree_skb(skb);
-		return NET_RX_DROP;
-	}
+	if (unlikely(!(dev->flags & IFF_UP)))
+		goto kfree_skb;
+	/* Don't check mtu on gso packets... */
+	if (unlikely(!skb_is_gso(skb) &&
+		     (skb->len > (dev->mtu + dev->hard_header_len + VLAN_HLEN))))
+		goto kfree_skb;
 	skb_set_dev(skb, dev);
 	skb->tstamp.tv64 = 0;
 	skb->pkt_type = PACKET_HOST;
 	skb->protocol = eth_type_trans(skb, dev);
 	return netif_rx(skb);
+kfree_skb:
+	atomic_long_inc(&dev->rx_dropped);
+	kfree_skb(skb);
+	return NET_RX_DROP;
 }
 EXPORT_SYMBOL_GPL(dev_forward_skb);