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

[PATCH][v3] 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..29329d3 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;
+
+	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->dev && skb->dev->features & NETIF_F_TSO)
+		return true;
+
+	return false;
+}
+
 /**
  * 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][v3] dev : fix mtu check when TSO is enabled

[David Miller]

From: Daniel Lezcano <daniel.lezcano@free.fr>
Date: Mon, 14 Mar 2011 21:39:50 +0100

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

This is not a correct translation of the original test:

> -		     (skb->len > (dev->mtu + dev->hard_header_len + VLAN_HLEN)))) {

You need to use "<=" in your version, which currently rejects all
full sized frames. :-)

> +
> +	/* if TSO is enabled, we don't care about the length as the packet
> +	 * could be forwarded without being segmented before
> +	 */
> +	if (skb->dev && skb->dev->features & NETIF_F_TSO)
> +		return true;

I am trying to understand why you aren't simply checking also if this
is a segmented frame?  Perhaps skb_is_gso() && device has NETIF_F_TSO
set?

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

[Daniel Lezcano]

On 03/15/2011 12:59 AM, David Miller wrote:
> From: Daniel Lezcano<daniel.lezcano@free.fr>
> Date: Mon, 14 Mar 2011 21:39:50 +0100
>
>> +	len = dev->mtu + dev->hard_header_len + VLAN_HLEN;
>> +	if (skb->len<  len)
>> +		return true;
> This is not a correct translation of the original test:
>
>> -		     (skb->len>  (dev->mtu + dev->hard_header_len + VLAN_HLEN)))) {
> You need to use "<=" in your version, which currently rejects all
> full sized frames. :-)

Right, thanks.

>> +
>> +	/* if TSO is enabled, we don't care about the length as the packet
>> +	 * could be forwarded without being segmented before
>> +	 */
>> +	if (skb->dev&&  skb->dev->features&  NETIF_F_TSO)
>> +		return true;
> I am trying to understand why you aren't simply checking also if this
> is a segmented frame?  Perhaps skb_is_gso()&&  device has NETIF_F_TSO
> set?

Maybe I am misunderstanding but the packet was forwarded by another device.
In our case from macvlan:

macvlan_start_xmit
     macvlan_queue_xmit
         dest->forward
             dev_skb_forward

When we reached dev_skb_forward, that means we passed through 
dev_hard_start_xmit where the packet was already segmented so we should 
exit at the first test (skb->len < len). I don't see the point of adding 
the skb_is_gso.
But maybe I am missing something, can you explain ?

Thanks
   -- Daniel

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

[Stephen Hemminger]

On Tue, 15 Mar 2011 14:57:40 +0100
Daniel Lezcano <daniel.lezcano@free.fr> wrote:

> On 03/15/2011 12:59 AM, David Miller wrote:
> > From: Daniel Lezcano<daniel.lezcano@free.fr>
> > Date: Mon, 14 Mar 2011 21:39:50 +0100
> >
> >> +	len = dev->mtu + dev->hard_header_len + VLAN_HLEN;
> >> +	if (skb->len<  len)
> >> +		return true;
> > This is not a correct translation of the original test:
> >
> >> -		     (skb->len>  (dev->mtu + dev->hard_header_len + VLAN_HLEN)))) {
> > You need to use "<=" in your version, which currently rejects all
> > full sized frames. :-)
> 
> Right, thanks.
> 
> >> +
> >> +	/* if TSO is enabled, we don't care about the length as the packet
> >> +	 * could be forwarded without being segmented before
> >> +	 */
> >> +	if (skb->dev&&  skb->dev->features&  NETIF_F_TSO)
> >> +		return true;
> > I am trying to understand why you aren't simply checking also if this
> > is a segmented frame?  Perhaps skb_is_gso()&&  device has NETIF_F_TSO
> > set?
> 
> Maybe I am misunderstanding but the packet was forwarded by another device.
> In our case from macvlan:
> 
> macvlan_start_xmit
>      macvlan_queue_xmit
>          dest->forward
>              dev_skb_forward
> 
> When we reached dev_skb_forward, that means we passed through 
> dev_hard_start_xmit where the packet was already segmented so we should 
> exit at the first test (skb->len < len). I don't see the point of adding 
> the skb_is_gso.
> But maybe I am missing something, can you explain ?

The macvlan device only has one downstream device (slave).
If kernel is working properly, macvlan device should have a subset
of the features of the underlying device and macvlan device should
have same MTU as underlying device. If the feature/MTU flags
were correct, then the path calling macvlan should be respecting
the MTU.



-- 

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

[Daniel Lezcano]

On 03/15/2011 07:17 PM, Stephen Hemminger wrote:
> On Tue, 15 Mar 2011 14:57:40 +0100
> Daniel Lezcano<daniel.lezcano@free.fr>  wrote:
>
>> On 03/15/2011 12:59 AM, David Miller wrote:
>>> From: Daniel Lezcano<daniel.lezcano@free.fr>
>>> Date: Mon, 14 Mar 2011 21:39:50 +0100
>>>
>>>> +	len = dev->mtu + dev->hard_header_len + VLAN_HLEN;
>>>> +	if (skb->len<   len)
>>>> +		return true;
>>> This is not a correct translation of the original test:
>>>
>>>> -		     (skb->len>   (dev->mtu + dev->hard_header_len + VLAN_HLEN)))) {
>>> You need to use "<=" in your version, which currently rejects all
>>> full sized frames. :-)
>> Right, thanks.
>>
>>>> +
>>>> +	/* if TSO is enabled, we don't care about the length as the packet
>>>> +	 * could be forwarded without being segmented before
>>>> +	 */
>>>> +	if (skb->dev&&   skb->dev->features&   NETIF_F_TSO)
>>>> +		return true;
>>> I am trying to understand why you aren't simply checking also if this
>>> is a segmented frame?  Perhaps skb_is_gso()&&   device has NETIF_F_TSO
>>> set?
>> Maybe I am misunderstanding but the packet was forwarded by another device.
>> In our case from macvlan:
>>
>> macvlan_start_xmit
>>       macvlan_queue_xmit
>>           dest->forward
>>               dev_skb_forward
>>
>> When we reached dev_skb_forward, that means we passed through
>> dev_hard_start_xmit where the packet was already segmented so we should
>> exit at the first test (skb->len<  len). I don't see the point of adding
>> the skb_is_gso.
>> But maybe I am missing something, can you explain ?
> The macvlan device only has one downstream device (slave).
> If kernel is working properly, macvlan device should have a subset
> of the features of the underlying device

Right, dev->features = lowerdev->features & MACVLAN_FEATURES

> and macvlan device should
> have same MTU as underlying device.

Right,

...

  if (!tb[IFLA_MTU])
         dev->mtu = lowerdev->mtu;

...
> If the feature/MTU flags
> were correct, then the path calling macvlan should be respecting
> the MTU.

But if the TSO is enabled on the macvlan (inherited from eg e1000), the 
packet won't be fragmented to the mtu size no ?

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

[Stephen Hemminger]

On Wed, 16 Mar 2011 14:56:09 +0100
Daniel Lezcano <daniel.lezcano@free.fr> wrote:

> On 03/15/2011 07:17 PM, Stephen Hemminger wrote:
> > On Tue, 15 Mar 2011 14:57:40 +0100
> > Daniel Lezcano<daniel.lezcano@free.fr>  wrote:
> >
> >> On 03/15/2011 12:59 AM, David Miller wrote:
> >>> From: Daniel Lezcano<daniel.lezcano@free.fr>
> >>> Date: Mon, 14 Mar 2011 21:39:50 +0100
> >>>
> >>>> +	len = dev->mtu + dev->hard_header_len + VLAN_HLEN;
> >>>> +	if (skb->len<   len)
> >>>> +		return true;
> >>> This is not a correct translation of the original test:
> >>>
> >>>> -		     (skb->len>   (dev->mtu + dev->hard_header_len + VLAN_HLEN)))) {
> >>> You need to use "<=" in your version, which currently rejects all
> >>> full sized frames. :-)
> >> Right, thanks.
> >>
> >>>> +
> >>>> +	/* if TSO is enabled, we don't care about the length as the packet
> >>>> +	 * could be forwarded without being segmented before
> >>>> +	 */
> >>>> +	if (skb->dev&&   skb->dev->features&   NETIF_F_TSO)
> >>>> +		return true;
> >>> I am trying to understand why you aren't simply checking also if this
> >>> is a segmented frame?  Perhaps skb_is_gso()&&   device has NETIF_F_TSO
> >>> set?
> >> Maybe I am misunderstanding but the packet was forwarded by another device.
> >> In our case from macvlan:
> >>
> >> macvlan_start_xmit
> >>       macvlan_queue_xmit
> >>           dest->forward
> >>               dev_skb_forward
> >>
> >> When we reached dev_skb_forward, that means we passed through
> >> dev_hard_start_xmit where the packet was already segmented so we should
> >> exit at the first test (skb->len<  len). I don't see the point of adding
> >> the skb_is_gso.
> >> But maybe I am missing something, can you explain ?
> > The macvlan device only has one downstream device (slave).
> > If kernel is working properly, macvlan device should have a subset
> > of the features of the underlying device
> 
> Right, dev->features = lowerdev->features & MACVLAN_FEATURES
> 
> > and macvlan device should
> > have same MTU as underlying device.
> 
> Right,
> 
> ...
> 
>   if (!tb[IFLA_MTU])
>          dev->mtu = lowerdev->mtu;
> 
> ...
> > If the feature/MTU flags
> > were correct, then the path calling macvlan should be respecting
> > the MTU.
> 
> But if the TSO is enabled on the macvlan (inherited from eg e1000), the 
> packet won't be fragmented to the mtu size no ?

That is the responsiblity of the hardware that receives the packet.
Macvlan should be passing it through to the lowerdev and since the hardware
supports TSO, it will fragment it.


-- 

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

[Daniel Lezcano]

On 03/16/2011 04:35 PM, Stephen Hemminger wrote:
> On Wed, 16 Mar 2011 14:56:09 +0100
> Daniel Lezcano<daniel.lezcano@free.fr>  wrote:
>
>> On 03/15/2011 07:17 PM, Stephen Hemminger wrote:
>>> On Tue, 15 Mar 2011 14:57:40 +0100
>>> Daniel Lezcano<daniel.lezcano@free.fr>   wrote:
>>>
>>>> On 03/15/2011 12:59 AM, David Miller wrote:
>>>>> From: Daniel Lezcano<daniel.lezcano@free.fr>
>>>>> Date: Mon, 14 Mar 2011 21:39:50 +0100
>>>>>
>>>>>> +	len = dev->mtu + dev->hard_header_len + VLAN_HLEN;
>>>>>> +	if (skb->len<    len)
>>>>>> +		return true;
>>>>> This is not a correct translation of the original test:
>>>>>
>>>>>> -		     (skb->len>    (dev->mtu + dev->hard_header_len + VLAN_HLEN)))) {
>>>>> You need to use "<=" in your version, which currently rejects all
>>>>> full sized frames. :-)
>>>> Right, thanks.
>>>>
>>>>>> +
>>>>>> +	/* if TSO is enabled, we don't care about the length as the packet
>>>>>> +	 * could be forwarded without being segmented before
>>>>>> +	 */
>>>>>> +	if (skb->dev&&    skb->dev->features&    NETIF_F_TSO)
>>>>>> +		return true;
>>>>> I am trying to understand why you aren't simply checking also if this
>>>>> is a segmented frame?  Perhaps skb_is_gso()&&    device has NETIF_F_TSO
>>>>> set?
>>>> Maybe I am misunderstanding but the packet was forwarded by another device.
>>>> In our case from macvlan:
>>>>
>>>> macvlan_start_xmit
>>>>        macvlan_queue_xmit
>>>>            dest->forward
>>>>                dev_skb_forward
>>>>
>>>> When we reached dev_skb_forward, that means we passed through
>>>> dev_hard_start_xmit where the packet was already segmented so we should
>>>> exit at the first test (skb->len<   len). I don't see the point of adding
>>>> the skb_is_gso.
>>>> But maybe I am missing something, can you explain ?
>>> The macvlan device only has one downstream device (slave).
>>> If kernel is working properly, macvlan device should have a subset
>>> of the features of the underlying device
>> Right, dev->features = lowerdev->features&  MACVLAN_FEATURES
>>
>>> and macvlan device should
>>> have same MTU as underlying device.
>> Right,
>>
>> ...
>>
>>    if (!tb[IFLA_MTU])
>>           dev->mtu = lowerdev->mtu;
>>
>> ...
>>> If the feature/MTU flags
>>> were correct, then the path calling macvlan should be respecting
>>> the MTU.
>> But if the TSO is enabled on the macvlan (inherited from eg e1000), the
>> packet won't be fragmented to the mtu size no ?
> That is the responsiblity of the hardware that receives the packet.
> Macvlan should be passing it through to the lowerdev and since the hardware
> supports TSO, it will fragment it.

Ok, but in the case the macvlan is in bridge mode, the dev_skb_forward 
function will forward the packet (which is not fragmented) to to another 
macvlan port without going through the hardware driver. In this 
function, the packet length is checked against the mtu size and of 
course the packet is dropped in case the lower device support the TSO 
(if the packet is larger than the mtu size). Dave suggested to check 
skb_is_gso and against the TSO feature of the macvlan but I don't 
understand why we should check skb_is_gso too.

	if (skb_is_gso(skb)&&  (skb->dev&&  skb->dev->features&  NETIF_F_TSO))
		return true;

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

[Stephen Hemminger]

On Wed, 16 Mar 2011 17:19:14 +0100
Daniel Lezcano <daniel.lezcano@free.fr> wrote:

> On 03/16/2011 04:35 PM, Stephen Hemminger wrote:
> > On Wed, 16 Mar 2011 14:56:09 +0100
> > Daniel Lezcano<daniel.lezcano@free.fr>  wrote:
> >
> >> On 03/15/2011 07:17 PM, Stephen Hemminger wrote:
> >>> On Tue, 15 Mar 2011 14:57:40 +0100
> >>> Daniel Lezcano<daniel.lezcano@free.fr>   wrote:
> >>>
> >>>> On 03/15/2011 12:59 AM, David Miller wrote:
> >>>>> From: Daniel Lezcano<daniel.lezcano@free.fr>
> >>>>> Date: Mon, 14 Mar 2011 21:39:50 +0100
> >>>>>
> >>>>>> +	len = dev->mtu + dev->hard_header_len + VLAN_HLEN;
> >>>>>> +	if (skb->len<    len)
> >>>>>> +		return true;
> >>>>> This is not a correct translation of the original test:
> >>>>>
> >>>>>> -		     (skb->len>    (dev->mtu + dev->hard_header_len + VLAN_HLEN)))) {
> >>>>> You need to use "<=" in your version, which currently rejects all
> >>>>> full sized frames. :-)
> >>>> Right, thanks.
> >>>>
> >>>>>> +
> >>>>>> +	/* if TSO is enabled, we don't care about the length as the packet
> >>>>>> +	 * could be forwarded without being segmented before
> >>>>>> +	 */
> >>>>>> +	if (skb->dev&&    skb->dev->features&    NETIF_F_TSO)
> >>>>>> +		return true;
> >>>>> I am trying to understand why you aren't simply checking also if this
> >>>>> is a segmented frame?  Perhaps skb_is_gso()&&    device has NETIF_F_TSO
> >>>>> set?
> >>>> Maybe I am misunderstanding but the packet was forwarded by another device.
> >>>> In our case from macvlan:
> >>>>
> >>>> macvlan_start_xmit
> >>>>        macvlan_queue_xmit
> >>>>            dest->forward
> >>>>                dev_skb_forward
> >>>>
> >>>> When we reached dev_skb_forward, that means we passed through
> >>>> dev_hard_start_xmit where the packet was already segmented so we should
> >>>> exit at the first test (skb->len<   len). I don't see the point of adding
> >>>> the skb_is_gso.
> >>>> But maybe I am missing something, can you explain ?
> >>> The macvlan device only has one downstream device (slave).
> >>> If kernel is working properly, macvlan device should have a subset
> >>> of the features of the underlying device
> >> Right, dev->features = lowerdev->features&  MACVLAN_FEATURES
> >>
> >>> and macvlan device should
> >>> have same MTU as underlying device.
> >> Right,
> >>
> >> ...
> >>
> >>    if (!tb[IFLA_MTU])
> >>           dev->mtu = lowerdev->mtu;
> >>
> >> ...
> >>> If the feature/MTU flags
> >>> were correct, then the path calling macvlan should be respecting
> >>> the MTU.
> >> But if the TSO is enabled on the macvlan (inherited from eg e1000), the
> >> packet won't be fragmented to the mtu size no ?
> > That is the responsiblity of the hardware that receives the packet.
> > Macvlan should be passing it through to the lowerdev and since the hardware
> > supports TSO, it will fragment it.
> 
> Ok, but in the case the macvlan is in bridge mode, the dev_skb_forward 
> function will forward the packet (which is not fragmented) to to another 
> macvlan port without going through the hardware driver. In this 
> function, the packet length is checked against the mtu size and of 
> course the packet is dropped in case the lower device support the TSO 
> (if the packet is larger than the mtu size). Dave suggested to check 
> skb_is_gso and against the TSO feature of the macvlan but I don't 
> understand why we should check skb_is_gso too.
> 
> 	if (skb_is_gso(skb)&&  (skb->dev&&  skb->dev->features&  NETIF_F_TSO))
> 		return true;
> 
> 

Then it is up to macvlan to do the same thing as bridge code.

static inline unsigned packet_length(const struct sk_buff *skb)
{
	return skb->len - (skb->protocol == htons(ETH_P_8021Q) ? VLAN_HLEN : 0);
}

int br_dev_queue_push_xmit(struct sk_buff *skb)
{
	/* ip_fragment doesn't copy the MAC header */
	if (nf_bridge_maybe_copy_header(skb) ||
	    (packet_length(skb) > skb->dev->mtu && !skb_is_gso(skb))) {
		kfree_skb(skb);
	} else {
		skb_push(skb, ETH_HLEN);
		dev_queue_xmit(skb);
	}

	return 0;
}

Ps: not sure adding macvlan bridge mode was a good idea, we already have
a working bridge code. And there are too many missing pieces in the macvlan
implementation of bridging



-- 

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

[Eric W. Biederman]

Stephen Hemminger <shemminger@vyatta.com> writes:

> On Wed, 16 Mar 2011 17:19:14 +0100
> Daniel Lezcano <daniel.lezcano@free.fr> wrote:
>
>> On 03/16/2011 04:35 PM, Stephen Hemminger wrote:
>> > On Wed, 16 Mar 2011 14:56:09 +0100
>> > Daniel Lezcano<daniel.lezcano@free.fr>  wrote:
>> >
>> >> On 03/15/2011 07:17 PM, Stephen Hemminger wrote:
>> >>> On Tue, 15 Mar 2011 14:57:40 +0100
>> >>> Daniel Lezcano<daniel.lezcano@free.fr>   wrote:
>> >>>
>> >>>> On 03/15/2011 12:59 AM, David Miller wrote:
>> >>>>> From: Daniel Lezcano<daniel.lezcano@free.fr>
>> >>>>> Date: Mon, 14 Mar 2011 21:39:50 +0100
>> >>>>>
>> >>>>>> +	len = dev->mtu + dev->hard_header_len + VLAN_HLEN;
>> >>>>>> +	if (skb->len<    len)
>> >>>>>> +		return true;
>> >>>>> This is not a correct translation of the original test:
>> >>>>>
>> >>>>>> -		     (skb->len>    (dev->mtu + dev->hard_header_len + VLAN_HLEN)))) {
>> >>>>> You need to use "<=" in your version, which currently rejects all
>> >>>>> full sized frames. :-)
>> >>>> Right, thanks.
>> >>>>
>> >>>>>> +
>> >>>>>> +	/* if TSO is enabled, we don't care about the length as the packet
>> >>>>>> +	 * could be forwarded without being segmented before
>> >>>>>> +	 */
>> >>>>>> +	if (skb->dev&&    skb->dev->features&    NETIF_F_TSO)
>> >>>>>> +		return true;
>> >>>>> I am trying to understand why you aren't simply checking also if this
>> >>>>> is a segmented frame?  Perhaps skb_is_gso()&&    device has NETIF_F_TSO
>> >>>>> set?
>> >>>> Maybe I am misunderstanding but the packet was forwarded by another device.
>> >>>> In our case from macvlan:
>> >>>>
>> >>>> macvlan_start_xmit
>> >>>>        macvlan_queue_xmit
>> >>>>            dest->forward
>> >>>>                dev_skb_forward
>> >>>>
>> >>>> When we reached dev_skb_forward, that means we passed through
>> >>>> dev_hard_start_xmit where the packet was already segmented so we should
>> >>>> exit at the first test (skb->len<   len). I don't see the point of adding
>> >>>> the skb_is_gso.
>> >>>> But maybe I am missing something, can you explain ?
>> >>> The macvlan device only has one downstream device (slave).
>> >>> If kernel is working properly, macvlan device should have a subset
>> >>> of the features of the underlying device
>> >> Right, dev->features = lowerdev->features&  MACVLAN_FEATURES
>> >>
>> >>> and macvlan device should
>> >>> have same MTU as underlying device.
>> >> Right,
>> >>
>> >> ...
>> >>
>> >>    if (!tb[IFLA_MTU])
>> >>           dev->mtu = lowerdev->mtu;
>> >>
>> >> ...
>> >>> If the feature/MTU flags
>> >>> were correct, then the path calling macvlan should be respecting
>> >>> the MTU.
>> >> But if the TSO is enabled on the macvlan (inherited from eg e1000), the
>> >> packet won't be fragmented to the mtu size no ?
>> > That is the responsiblity of the hardware that receives the packet.
>> > Macvlan should be passing it through to the lowerdev and since the hardware
>> > supports TSO, it will fragment it.
>> 
>> Ok, but in the case the macvlan is in bridge mode, the dev_skb_forward 
>> function will forward the packet (which is not fragmented) to to another 
>> macvlan port without going through the hardware driver. In this 
>> function, the packet length is checked against the mtu size and of 
>> course the packet is dropped in case the lower device support the TSO 
>> (if the packet is larger than the mtu size). Dave suggested to check 
>> skb_is_gso and against the TSO feature of the macvlan but I don't 
>> understand why we should check skb_is_gso too.
>> 
>> 	if (skb_is_gso(skb)&&  (skb->dev&&  skb->dev->features&  NETIF_F_TSO))
>> 		return true;
>> 
>> 
>
> Then it is up to macvlan to do the same thing as bridge code.
>
> static inline unsigned packet_length(const struct sk_buff *skb)
> {
> 	return skb->len - (skb->protocol == htons(ETH_P_8021Q) ? VLAN_HLEN : 0);
> }

Which is incorrect in this context.  skb->len at this point in the code
includes the ethernet header, as we are down below dev_queue_xmit.

The test really does need to be dev->mtu + dev->hard_header_len. 

As for conditionally include the VLAN_HLEN that is likely doable but I
think if we are being pedantic that case needs to deal with accelerated
vlan headers.

> int br_dev_queue_push_xmit(struct sk_buff *skb)
> {
> 	/* ip_fragment doesn't copy the MAC header */
> 	if (nf_bridge_maybe_copy_header(skb) ||
> 	    (packet_length(skb) > skb->dev->mtu && !skb_is_gso(skb))) {
> 		kfree_skb(skb);
> 	} else {
> 		skb_push(skb, ETH_HLEN);
> 		dev_queue_xmit(skb);
> 	}
e>
> 	return 0;
> }


> Ps: not sure adding macvlan bridge mode was a good idea, we already have
> a working bridge code. And there are too many missing pieces in the macvlan
> implementation of bridging

The macvlan is a trivial bridge that doesn't need to do learning as it
knows every mac address it can receive, so it doesn't need to implement
learning or stp.  Which makes it simple stupid and and fast.

Strictly speaking the problems we are running into are the problems of
hardware acceleration and software devices, not properly providing the
same interfaces as the hardware.  To not take a significant hit on
performance we do want hardware acceleration on the path to real
hardware from the transmitter.

To use the network bridge to cross network namespaces we would have to
add veth pairs and unless something has drastically changed we would
still need to emulate all of the hardware acceleration in so the
dev_forward_skb so that we don't provide a device to the bridge that
has lesser capabilities than the real hardware and downgrade the
capabilities of the entire bridge.

So I don't think there is much that would be gained by using bridges
instead of the macvlan driver.

Eric

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

[Michał Mirosław]

2011/3/15 David Miller <davem@davemloft.net>:
> From: Daniel Lezcano <daniel.lezcano@free.fr>
> Date: Mon, 14 Mar 2011 21:39:50 +0100
>> +
>> +     /* if TSO is enabled, we don't care about the length as the packet
>> +      * could be forwarded without being segmented before
>> +      */
>> +     if (skb->dev && skb->dev->features & NETIF_F_TSO)
>> +             return true;
> I am trying to understand why you aren't simply checking also if this
> is a segmented frame?  Perhaps skb_is_gso() && device has NETIF_F_TSO
> set?

Is the check of netdev features even necessary? Devices without TSO
enabled the skbs are segmented before ndo_start_xmit() anyway.

With TSO there's a question if the receiving end of macvlan can handle
GRO/LRO packets?

BTW, checking only NETIF_F_TSO will break at least for IPv6 TSO
(NETIF_F_TSO6) or whatever else can be segmented in hardware.

Best Regards,
Michał Mirosław

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

[Jesse Gross]

On Mon, Mar 21, 2011 at 3:58 PM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
> Stephen Hemminger <shemminger@vyatta.com> writes:
>
>> On Wed, 16 Mar 2011 17:19:14 +0100
>> Daniel Lezcano <daniel.lezcano@free.fr> wrote:
>>
>>> On 03/16/2011 04:35 PM, Stephen Hemminger wrote:
>>> > On Wed, 16 Mar 2011 14:56:09 +0100
>>> > Daniel Lezcano<daniel.lezcano@free.fr>  wrote:
>>> >
>>> >> On 03/15/2011 07:17 PM, Stephen Hemminger wrote:
>>> >>> On Tue, 15 Mar 2011 14:57:40 +0100
>>> >>> Daniel Lezcano<daniel.lezcano@free.fr>   wrote:
>>> >>>
>>> >>>> On 03/15/2011 12:59 AM, David Miller wrote:
>>> >>>>> From: Daniel Lezcano<daniel.lezcano@free.fr>
>>> >>>>> Date: Mon, 14 Mar 2011 21:39:50 +0100
>>> >>>>>
>>> >>>>>> +     len = dev->mtu + dev->hard_header_len + VLAN_HLEN;
>>> >>>>>> +     if (skb->len<    len)
>>> >>>>>> +             return true;
>>> >>>>> This is not a correct translation of the original test:
>>> >>>>>
>>> >>>>>> -                  (skb->len>    (dev->mtu + dev->hard_header_len + VLAN_HLEN)))) {
>>> >>>>> You need to use "<=" in your version, which currently rejects all
>>> >>>>> full sized frames. :-)
>>> >>>> Right, thanks.
>>> >>>>
>>> >>>>>> +
>>> >>>>>> +     /* if TSO is enabled, we don't care about the length as the packet
>>> >>>>>> +      * could be forwarded without being segmented before
>>> >>>>>> +      */
>>> >>>>>> +     if (skb->dev&&    skb->dev->features&    NETIF_F_TSO)
>>> >>>>>> +             return true;
>>> >>>>> I am trying to understand why you aren't simply checking also if this
>>> >>>>> is a segmented frame?  Perhaps skb_is_gso()&&    device has NETIF_F_TSO
>>> >>>>> set?
>>> >>>> Maybe I am misunderstanding but the packet was forwarded by another device.
>>> >>>> In our case from macvlan:
>>> >>>>
>>> >>>> macvlan_start_xmit
>>> >>>>        macvlan_queue_xmit
>>> >>>>            dest->forward
>>> >>>>                dev_skb_forward
>>> >>>>
>>> >>>> When we reached dev_skb_forward, that means we passed through
>>> >>>> dev_hard_start_xmit where the packet was already segmented so we should
>>> >>>> exit at the first test (skb->len<   len). I don't see the point of adding
>>> >>>> the skb_is_gso.
>>> >>>> But maybe I am missing something, can you explain ?
>>> >>> The macvlan device only has one downstream device (slave).
>>> >>> If kernel is working properly, macvlan device should have a subset
>>> >>> of the features of the underlying device
>>> >> Right, dev->features = lowerdev->features&  MACVLAN_FEATURES
>>> >>
>>> >>> and macvlan device should
>>> >>> have same MTU as underlying device.
>>> >> Right,
>>> >>
>>> >> ...
>>> >>
>>> >>    if (!tb[IFLA_MTU])
>>> >>           dev->mtu = lowerdev->mtu;
>>> >>
>>> >> ...
>>> >>> If the feature/MTU flags
>>> >>> were correct, then the path calling macvlan should be respecting
>>> >>> the MTU.
>>> >> But if the TSO is enabled on the macvlan (inherited from eg e1000), the
>>> >> packet won't be fragmented to the mtu size no ?
>>> > That is the responsiblity of the hardware that receives the packet.
>>> > Macvlan should be passing it through to the lowerdev and since the hardware
>>> > supports TSO, it will fragment it.
>>>
>>> Ok, but in the case the macvlan is in bridge mode, the dev_skb_forward
>>> function will forward the packet (which is not fragmented) to to another
>>> macvlan port without going through the hardware driver. In this
>>> function, the packet length is checked against the mtu size and of
>>> course the packet is dropped in case the lower device support the TSO
>>> (if the packet is larger than the mtu size). Dave suggested to check
>>> skb_is_gso and against the TSO feature of the macvlan but I don't
>>> understand why we should check skb_is_gso too.
>>>
>>>      if (skb_is_gso(skb)&&  (skb->dev&&  skb->dev->features&  NETIF_F_TSO))
>>>              return true;
>>>
>>>
>>
>> Then it is up to macvlan to do the same thing as bridge code.
>>
>> static inline unsigned packet_length(const struct sk_buff *skb)
>> {
>>       return skb->len - (skb->protocol == htons(ETH_P_8021Q) ? VLAN_HLEN : 0);
>> }
>
> Which is incorrect in this context.  skb->len at this point in the code
> includes the ethernet header, as we are down below dev_queue_xmit.
>
> The test really does need to be dev->mtu + dev->hard_header_len.
>
> As for conditionally include the VLAN_HLEN that is likely doable but I
> think if we are being pedantic that case needs to deal with accelerated
> vlan headers.

If vlan acceleration is in use then the protocol is not ETH_P_8021Q
and the tag is not included in skb->len.

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

[Eric W. Biederman]

Jesse Gross <jesse@nicira.com> writes:

> On Mon, Mar 21, 2011 at 3:58 PM, Eric W. Biederman
> <ebiederm@xmission.com> wrote:
>> Stephen Hemminger <shemminger@vyatta.com> writes:
>>
>>> On Wed, 16 Mar 2011 17:19:14 +0100
>>> Daniel Lezcano <daniel.lezcano@free.fr> wrote:
>>>
>>>> On 03/16/2011 04:35 PM, Stephen Hemminger wrote:
>>>> > On Wed, 16 Mar 2011 14:56:09 +0100
>>>> > Daniel Lezcano<daniel.lezcano@free.fr>  wrote:
>>>> >
>>>> >> On 03/15/2011 07:17 PM, Stephen Hemminger wrote:
>>>> >>> On Tue, 15 Mar 2011 14:57:40 +0100
>>>> >>> Daniel Lezcano<daniel.lezcano@free.fr>   wrote:
>>>> >>>
>>>> >>>> On 03/15/2011 12:59 AM, David Miller wrote:
>>>> >>>>> From: Daniel Lezcano<daniel.lezcano@free.fr>
>>>> >>>>> Date: Mon, 14 Mar 2011 21:39:50 +0100
>>>> >>>>>
>>>> >>>>>> +     len = dev->mtu + dev->hard_header_len + VLAN_HLEN;
>>>> >>>>>> +     if (skb->len<    len)
>>>> >>>>>> +             return true;
>>>> >>>>> This is not a correct translation of the original test:
>>>> >>>>>
>>>> >>>>>> -                  (skb->len>    (dev->mtu + dev->hard_header_len + VLAN_HLEN)))) {
>>>> >>>>> You need to use "<=" in your version, which currently rejects all
>>>> >>>>> full sized frames. :-)
>>>> >>>> Right, thanks.
>>>> >>>>
>>>> >>>>>> +
>>>> >>>>>> +     /* if TSO is enabled, we don't care about the length as the packet
>>>> >>>>>> +      * could be forwarded without being segmented before
>>>> >>>>>> +      */
>>>> >>>>>> +     if (skb->dev&&    skb->dev->features&    NETIF_F_TSO)
>>>> >>>>>> +             return true;
>>>> >>>>> I am trying to understand why you aren't simply checking also if this
>>>> >>>>> is a segmented frame?  Perhaps skb_is_gso()&&    device has NETIF_F_TSO
>>>> >>>>> set?
>>>> >>>> Maybe I am misunderstanding but the packet was forwarded by another device.
>>>> >>>> In our case from macvlan:
>>>> >>>>
>>>> >>>> macvlan_start_xmit
>>>> >>>>        macvlan_queue_xmit
>>>> >>>>            dest->forward
>>>> >>>>                dev_skb_forward
>>>> >>>>
>>>> >>>> When we reached dev_skb_forward, that means we passed through
>>>> >>>> dev_hard_start_xmit where the packet was already segmented so we should
>>>> >>>> exit at the first test (skb->len<   len). I don't see the point of adding
>>>> >>>> the skb_is_gso.
>>>> >>>> But maybe I am missing something, can you explain ?
>>>> >>> The macvlan device only has one downstream device (slave).
>>>> >>> If kernel is working properly, macvlan device should have a subset
>>>> >>> of the features of the underlying device
>>>> >> Right, dev->features = lowerdev->features&  MACVLAN_FEATURES
>>>> >>
>>>> >>> and macvlan device should
>>>> >>> have same MTU as underlying device.
>>>> >> Right,
>>>> >>
>>>> >> ...
>>>> >>
>>>> >>    if (!tb[IFLA_MTU])
>>>> >>           dev->mtu = lowerdev->mtu;
>>>> >>
>>>> >> ...
>>>> >>> If the feature/MTU flags
>>>> >>> were correct, then the path calling macvlan should be respecting
>>>> >>> the MTU.
>>>> >> But if the TSO is enabled on the macvlan (inherited from eg e1000), the
>>>> >> packet won't be fragmented to the mtu size no ?
>>>> > That is the responsiblity of the hardware that receives the packet.
>>>> > Macvlan should be passing it through to the lowerdev and since the hardware
>>>> > supports TSO, it will fragment it.
>>>>
>>>> Ok, but in the case the macvlan is in bridge mode, the dev_skb_forward
>>>> function will forward the packet (which is not fragmented) to to another
>>>> macvlan port without going through the hardware driver. In this
>>>> function, the packet length is checked against the mtu size and of
>>>> course the packet is dropped in case the lower device support the TSO
>>>> (if the packet is larger than the mtu size). Dave suggested to check
>>>> skb_is_gso and against the TSO feature of the macvlan but I don't
>>>> understand why we should check skb_is_gso too.
>>>>
>>>>      if (skb_is_gso(skb)&&  (skb->dev&&  skb->dev->features&  NETIF_F_TSO))
>>>>              return true;
>>>>
>>>>
>>>
>>> Then it is up to macvlan to do the same thing as bridge code.
>>>
>>> static inline unsigned packet_length(const struct sk_buff *skb)
>>> {
>>>       return skb->len - (skb->protocol == htons(ETH_P_8021Q) ? VLAN_HLEN : 0);
>>> }
>>
>> Which is incorrect in this context.  skb->len at this point in the code
>> includes the ethernet header, as we are down below dev_queue_xmit.
>>
>> The test really does need to be dev->mtu + dev->hard_header_len.
>>
>> As for conditionally include the VLAN_HLEN that is likely doable but I
>> think if we are being pedantic that case needs to deal with accelerated
>> vlan headers.
>
> If vlan acceleration is in use then the protocol is not ETH_P_8021Q
> and the tag is not included in skb->len.

Except for the case of double tagged packets.

Eric