[PATCH] net: Handle gso packets in dev_forward_skb

[PATCH] net: Handle gso packets in dev_forward_skb

[Eric W. Biederman]

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);
 
-- 
1.7.4

Re: [PATCH] net: Handle gso packets in dev_forward_skb

[Eric Dumazet]

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 ?

Re: [PATCH] net: Handle gso packets in dev_forward_skb

[Eric W. Biederman]

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

Re: [PATCH] net: Handle gso packets in dev_forward_skb

[David Miller]

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.

Re: [PATCH] net: Handle gso packets in dev_forward_skb

[Eric Dumazet]

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 ?

Re: [PATCH] net: Handle gso packets in dev_forward_skb

[Eric W. Biederman]

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

Re: [PATCH] net: Handle gso packets in dev_forward_skb

[Daniel Lezcano]

On 03/28/2011 09:20 PM, Eric Dumazet wrote:
> 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.

Yes, sorry for the delay.
I would like to fix it but I am not sure to understand the different 
solutions proposed.

On 03/28/2011 09:20 PM, Eric Dumazet wrote:
> 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.

Yes, sorry for the delay.
I would like to fix it, but my knowledge in limited on offloading.

I did the following patch, it fixes the problem. If it looks good I can 
resend it in patch format:

Index: net-2.6/net/core/dev.c
===================================================================
--- net-2.6.orig/net/core/dev.c 2011-03-29 14:50:00.047646000 +0200
+++ net-2.6/net/core/dev.c      2011-03-29 14:50:35.587646000 +0200
@@ -1454,6 +1454,27 @@ static inline void net_timestamp_check(s
                 __net_timestamp(skb);
  }

+static inline bool is_skb_forwardable(struct net_device *dev,
+                                     struct sk_buff *skb)
+{
+       unsigned int len;
+
+       if (!(dev->flags & IFF_UP))
+               return false;
+
+       len = dev->mtu + dev->hard_header_len + VLAN_HLEN;
+       if (skb->len <= len)
+               return true;
+
+       /* if TSO is enabled, we don't care about the length as the packet
+        * could be forwarded without being segmented before
+        */
+       if (skb_is_gso(skb))
+               return true;
+
+       return false;
+}
+
  /**
   * dev_forward_skb - loopback an skb to another netif
   *
@@ -1477,8 +1498,7 @@ int dev_forward_skb(struct net_device *d
         skb_orphan(skb);
         nf_reset(skb);

-       if (unlikely(!(dev->flags & IFF_UP) ||
-                    (skb->len > (dev->mtu + dev->hard_header_len + 
VLAN_HLEN)))) {
+       if (unlikely(!is_skb_forwardable(dev, skb))) {
                 atomic_long_inc(&dev->rx_dropped);
                 kfree_skb(skb);
                 return NET_RX_DROP;

Re: [PATCH] net: Handle gso packets in dev_forward_skb

[Eric W. Biederman]

Daniel Lezcano <daniel.lezcano@free.fr> writes:

> On 03/28/2011 09:20 PM, Eric Dumazet wrote:
>> 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.
>
> Yes, sorry for the delay.
> I would like to fix it but I am not sure to understand the different solutions
> proposed.
>
> On 03/28/2011 09:20 PM, Eric Dumazet wrote:
>> 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.
>
> Yes, sorry for the delay.
> I would like to fix it, but my knowledge in limited on offloading.
>
> I did the following patch, it fixes the problem. If it looks good I can resend
> it in patch format:

It looks fine to me.  I would resubmit it so that if anyone else has
complaints they will be jolted into looking at the patch again.

Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>


Eric



>
> Index: net-2.6/net/core/dev.c
> ===================================================================
> --- net-2.6.orig/net/core/dev.c 2011-03-29 14:50:00.047646000 +0200
> +++ net-2.6/net/core/dev.c      2011-03-29 14:50:35.587646000 +0200
> @@ -1454,6 +1454,27 @@ static inline void net_timestamp_check(s
>                 __net_timestamp(skb);
>  }
>
> +static inline bool is_skb_forwardable(struct net_device *dev,
> +                                     struct sk_buff *skb)
> +{
> +       unsigned int len;
> +
> +       if (!(dev->flags & IFF_UP))
> +               return false;
> +
> +       len = dev->mtu + dev->hard_header_len + VLAN_HLEN;
> +       if (skb->len <= len)
> +               return true;
> +
> +       /* if TSO is enabled, we don't care about the length as the packet
> +        * could be forwarded without being segmented before
> +        */
> +       if (skb_is_gso(skb))
> +               return true;
> +
> +       return false;
> +}
> +
>  /**
>   * dev_forward_skb - loopback an skb to another netif
>   *
> @@ -1477,8 +1498,7 @@ int dev_forward_skb(struct net_device *d
>         skb_orphan(skb);
>         nf_reset(skb);
>
> -       if (unlikely(!(dev->flags & IFF_UP) ||
> -                    (skb->len > (dev->mtu + dev->hard_header_len +
> VLAN_HLEN)))) {
> +       if (unlikely(!is_skb_forwardable(dev, skb))) {
>                 atomic_long_inc(&dev->rx_dropped);
>                 kfree_skb(skb);
>                 return NET_RX_DROP;

Re: [PATCH] net: Handle gso packets in dev_forward_skb

[David Miller]

From: ebiederm@xmission.com (Eric W. Biederman)
Date: Tue, 29 Mar 2011 14:02:58 -0700

> It looks fine to me.  I would resubmit it so that if anyone else has
> complaints they will be jolted into looking at the patch again.
> 
> Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>

I'm waiting patiently for this resubmission.