[PATCH][v2] dev : fix mtu check when TSO is enabled

[PATCH][v2] dev : fix mtu check when TSO is enabled

[Daniel Lezcano]

In case the device where is coming from the packet has TSO enabled,
we should not check the mtu size value as this one could be bigger
than the expected value.

This is the case for the macvlan driver when the lower device has
TSO enabled. The macvlan inherit this feature and forward the packets
without fragmenting them. Then the packets go through dev_forward_skb
and are dropped. This patch fix this by checking TSO is not enabled
when we want to check the mtu size.

Signed-off-by: Daniel Lezcano <daniel.lezcano@free.fr>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Patrick McHardy <kaber@trash.net>
Cc: Andrian Nord <nightnord@gmail.com>
---
 net/core/dev.c |   24 ++++++++++++++++++++++--
 1 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 6561021..f1607a0 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1503,6 +1503,27 @@ static inline void net_timestamp_check(struct sk_buff *skb)
 		__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;
+
+	/* we should not check the mtu size if TSO is enabled otherwise
+	 * we may have a packet with a length bigger than the expected
+	 * one as it was not segmented before */
+	if (skb->dev && skb->dev->features & NETIF_F_TSO)
+		return true;
+
+	len = dev->mtu + dev->hard_header_len + VLAN_HLEN;
+	if (skb->len > len)
+		return false;
+
+	return true;
+}
+
 /**
  * dev_forward_skb - loopback an skb to another netif
  *
@@ -1526,8 +1547,7 @@ 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)))) {
+	if (unlikely(!is_skb_forwardable(dev, skb))) {
 		atomic_long_inc(&dev->rx_dropped);
 		kfree_skb(skb);
 		return NET_RX_DROP;
-- 
1.7.1

Re: [PATCH][v2] dev : fix mtu check when TSO is enabled

[Eric Dumazet]

Le lundi 14 mars 2011 à 20:10 +0100, Daniel Lezcano a écrit :
> In case the device where is coming from the packet has TSO enabled,
> we should not check the mtu size value as this one could be bigger
> than the expected value.
> 
> This is the case for the macvlan driver when the lower device has
> TSO enabled. The macvlan inherit this feature and forward the packets
> without fragmenting them. Then the packets go through dev_forward_skb
> and are dropped. This patch fix this by checking TSO is not enabled
> when we want to check the mtu size.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@free.fr>
> Cc: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: Patrick McHardy <kaber@trash.net>
> Cc: Andrian Nord <nightnord@gmail.com>
> ---
>  net/core/dev.c |   24 ++++++++++++++++++++++--
>  1 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 6561021..f1607a0 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1503,6 +1503,27 @@ static inline void net_timestamp_check(struct sk_buff *skb)
>  		__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;

	if (!(dev->flags & IFF_UP))
		return false;

> +
> +	/* we should not check the mtu size if TSO is enabled otherwise
> +	 * we may have a packet with a length bigger than the expected
> +	 * one as it was not segmented before */
> +	if (skb->dev && skb->dev->features & NETIF_F_TSO)
> +		return true;
> +
> +	len = dev->mtu + dev->hard_header_len + VLAN_HLEN;
> +	if (skb->len > len)
> +		return false;
> +


I suggest you reorder tests so that the first one is discriminant most
of the time

	maxlen = dev->mtu + dev->hard_header_len + VLAN_HLEN;
	if (skb->len < maxlen)
		return true;
	
	if (skb->dev && skb->dev->features & NETIF_F_TSO)
		return true;

	return false;

> +	return true;
> +}
> +
>  /**
>   * dev_forward_skb - loopback an skb to another netif
>   *
> @@ -1526,8 +1547,7 @@ 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)))) {
> +	if (unlikely(!is_skb_forwardable(dev, skb))) {
>  		atomic_long_inc(&dev->rx_dropped);
>  		kfree_skb(skb);
>  		return NET_RX_DROP;

Re: [PATCH][v2] dev : fix mtu check when TSO is enabled

[Daniel Lezcano]

On 03/14/2011 09:02 PM, Eric Dumazet wrote:
> Le lundi 14 mars 2011 à 20:10 +0100, Daniel Lezcano a écrit :
>> In case the device where is coming from the packet has TSO enabled,
>> we should not check the mtu size value as this one could be bigger
>> than the expected value.
>>
>> This is the case for the macvlan driver when the lower device has
>> TSO enabled. The macvlan inherit this feature and forward the packets
>> without fragmenting them. Then the packets go through dev_forward_skb
>> and are dropped. This patch fix this by checking TSO is not enabled
>> when we want to check the mtu size.
>>
>> Signed-off-by: Daniel Lezcano<daniel.lezcano@free.fr>
>> Cc: Eric Dumazet<eric.dumazet@gmail.com>
>> Cc: Patrick McHardy<kaber@trash.net>
>> Cc: Andrian Nord<nightnord@gmail.com>
>> ---
>>   net/core/dev.c |   24 ++++++++++++++++++++++--
>>   1 files changed, 22 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 6561021..f1607a0 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -1503,6 +1503,27 @@ static inline void net_timestamp_check(struct sk_buff *skb)
>>   		__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;
> 	if (!(dev->flags&  IFF_UP))
> 		return false;

Argh ! Hopefully you saw it.

>> +
>> +	/* we should not check the mtu size if TSO is enabled otherwise
>> +	 * we may have a packet with a length bigger than the expected
>> +	 * one as it was not segmented before */
>> +	if (skb->dev&&  skb->dev->features&  NETIF_F_TSO)
>> +		return true;
>> +
>> +	len = dev->mtu + dev->hard_header_len + VLAN_HLEN;
>> +	if (skb->len>  len)
>> +		return false;
>> +
> I suggest you reorder tests so that the first one is discriminant most
> of the time
>
> 	maxlen = dev->mtu + dev->hard_header_len + VLAN_HLEN;
> 	if (skb->len<  maxlen)
> 		return true;
> 	
> 	if (skb->dev&&  skb->dev->features&  NETIF_F_TSO)
> 		return true;
>
> 	return false;

Ok. Thanks !

   -- Daniel