Re: [PATCH net 1/3] vlan: Fix tcp checksums offloads for Q-in-Q vlan.

[Alexander Duyck]

On Mon, May 22, 2017 at 4:59 PM, David Miller  wrote:
> From: Vladislav Yasevich 
> Date: Thu, 18 May 2017 09:31:03 -0400
>
>> It appears that since commit 8cb65d000, Q-in-Q vlans have been
>> broken.  The series that commit is part of enabled TSO and checksum
>> offloading on Q-in-Q vlans.  However, most HW we support can't handle
>> it.  To work around the issue, the above commit added a function that
>> turns off offloads on Q-in-Q devices, but it left the checksum offload.
>> That will cause issues with most older devices that supprort very basic
>> checksum offload capabilities as well as some newer devices (we've
>> reproduced te problem with both be2net and bnx).
>>
>> To solve this for everyone, turn off checksum offloading feature
>> by default when sending Q-in-Q traffic.  Devices that are proven to
>> work can provided a corrected ndo_features_check implemetation.
>>
>> Fixes: 8cb65d000 ("net: Move check for multiple vlans to drivers")
>> CC: Toshiaki Makita 
>> Signed-off-by: Vladislav Yasevich 
>
> This is a tough one.  I can certainly sympathize with your frustration
> trying to track this down.
>
> Clearing NETIF_F_HW_CSUM completely is the most conservative change.
>
> However, for all the (perhaps many) cards upon which the checksumming
> does work properly in Q-in-Q situations, this change could be
> introducing non-trivial performance regressions.
>
> So I think Toshiaki's suggestion to drop IP_CSUM and IPV6_CSUM is,
> on balance, the best way forward.
>
> Thanks.

I would have to agree. I think we can simplify all this since all we
are essentially looking at doing is dropping the
netdev_features_intersect call and instead just do an AND instead of
fuzzing for the other CSUM bits. That way we don't have to worry about
systems that might only be able to handle frames that are IPv4/v6
with one level of VLAN and should be able to support multiple levels
of mixed headers.

We probably should add a comment about HW_CSUM being the only one we
can support as well so nobody comes back later and tries to revert the
change.

- Alex

Re: [PATCH net 1/3] vlan: Fix tcp checksums offloads for Q-in-Q vlan.

[Vlad Yasevich]

On 05/22/2017 07:59 PM, David Miller wrote:
> From: Vladislav Yasevich 
> Date: Thu, 18 May 2017 09:31:03 -0400
> 
>> It appears that since commit 8cb65d000, Q-in-Q vlans have been
>> broken.  The series that commit is part of enabled TSO and checksum
>> offloading on Q-in-Q vlans.  However, most HW we support can't handle
>> it.  To work around the issue, the above commit added a function that
>> turns off offloads on Q-in-Q devices, but it left the checksum offload.
>> That will cause issues with most older devices that supprort very basic
>> checksum offload capabilities as well as some newer devices (we've
>> reproduced te problem with both be2net and bnx).
>>
>> To solve this for everyone, turn off checksum offloading feature
>> by default when sending Q-in-Q traffic.  Devices that are proven to
>> work can provided a corrected ndo_features_check implemetation.
>>
>> Fixes: 8cb65d000 ("net: Move check for multiple vlans to drivers")
>> CC: Toshiaki Makita 
>> Signed-off-by: Vladislav Yasevich 
> 
> This is a tough one.  I can certainly sympathize with your frustration
> trying to track this down.
> 
> Clearing NETIF_F_HW_CSUM completely is the most conservative change.
> 
> However, for all the (perhaps many) cards upon which the checksumming
> does work properly in Q-in-Q situations, this change could be
> introducing non-trivial performance regressions.
> 
> So I think Toshiaki's suggestion to drop IP_CSUM and IPV6_CSUM is,
> on balance, the best way forward.
> 

Thanks.  I'll update and re-submit.

-vlad

> Thanks.
> 

Re: [PATCH net 1/3] vlan: Fix tcp checksums offloads for Q-in-Q vlan.

[David Miller]

From: Vladislav Yasevich 
Date: Thu, 18 May 2017 09:31:03 -0400

> It appears that since commit 8cb65d000, Q-in-Q vlans have been
> broken.  The series that commit is part of enabled TSO and checksum
> offloading on Q-in-Q vlans.  However, most HW we support can't handle
> it.  To work around the issue, the above commit added a function that
> turns off offloads on Q-in-Q devices, but it left the checksum offload.
> That will cause issues with most older devices that supprort very basic
> checksum offload capabilities as well as some newer devices (we've
> reproduced te problem with both be2net and bnx).
> 
> To solve this for everyone, turn off checksum offloading feature
> by default when sending Q-in-Q traffic.  Devices that are proven to
> work can provided a corrected ndo_features_check implemetation.
> 
> Fixes: 8cb65d000 ("net: Move check for multiple vlans to drivers")
> CC: Toshiaki Makita 
> Signed-off-by: Vladislav Yasevich 

This is a tough one.  I can certainly sympathize with your frustration
trying to track this down.

Clearing NETIF_F_HW_CSUM completely is the most conservative change.

However, for all the (perhaps many) cards upon which the checksumming
does work properly in Q-in-Q situations, this change could be
introducing non-trivial performance regressions.

So I think Toshiaki's suggestion to drop IP_CSUM and IPV6_CSUM is,
on balance, the best way forward.

Thanks.

Re: [PATCH net 1/3] vlan: Fix tcp checksums offloads for Q-in-Q vlan.

[Toshiaki Makita]

On 17/05/19 (金) 18:53, Vlad Yasevich wrote:

On 05/19/2017 04:16 AM, Toshiaki Makita wrote:

On 2017/05/19 16:09, Vlad Yasevich wrote:

On 05/18/2017 10:13 PM, Toshiaki Makita wrote:

On 2017/05/18 22:31, Vladislav Yasevich wrote:

It appears that since commit 8cb65d000, Q-in-Q vlans have been
broken.  The series that commit is part of enabled TSO and checksum
offloading on Q-in-Q vlans.  However, most HW we support can't handle
it.  To work around the issue, the above commit added a function that
turns off offloads on Q-in-Q devices, but it left the checksum offload.
That will cause issues with most older devices that supprort very basic
checksum offload capabilities as well as some newer devices (we've
reproduced te problem with both be2net and bnx).

To solve this for everyone, turn off checksum offloading feature
by default when sending Q-in-Q traffic.  Devices that are proven to
work can provided a corrected ndo_features_check implemetation.

Fixes: 8cb65d000 ("net: Move check for multiple vlans to drivers")
CC: Toshiaki Makita 
Signed-off-by: Vladislav Yasevich 
---
 include/linux/if_vlan.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
index 8d5fcd6..ae537f0 100644
--- a/include/linux/if_vlan.h
+++ b/include/linux/if_vlan.h
@@ -619,7 +619,6 @@ static inline netdev_features_t vlan_features_check(const 
struct sk_buff *skb,
 NETIF_F_SG |
 NETIF_F_HIGHDMA |
 NETIF_F_FRAGLIST |
-NETIF_F_HW_CSUM |
 NETIF_F_HW_VLAN_CTAG_TX |
 NETIF_F_HW_VLAN_STAG_TX);



I guess HW_CSUM theoretically can handle Q-in-Q packets and the problem
is IP_CSUM and IPV6_CSUM.
So wouldn't it be better to leave HW_CSUM and drop IP_CSUM/IPV6_CSUM,
i.e. change intersection into bitwise AND?



It wasn't really a problem before accelerations got enabled on q-in-q
vlans.


Right for stacked vlan device.
But I think the check was there for packets from guests forwarded by
bridge to vlan device so it was a problem before 8cb65d000.


Not really, since stacked vlans in guests wouldn't have accelerations on.
Haven't really tried a new guest on old hosts.  It might be an issue there...


It's real. I'm now remembering that I came across a similar issue before 
introducing 8cb65d000. The situation was that bridge (vlan_filtering) 
adds a vlan tag to a frame which is already tagged by guests, or by a 
vlan device on the top of the bridge (Note that virtio and bridge have 
HW_CSUM in vlan_features). I addressed the problem in drivers side since 
all the IP/IPV6_CSUM drivers I encountered the issue on are able to 
notify devices of IP header offset. Now I checked be2net driver's code 
and realized it doesn't provide IP offset so it makes sense to drop 
IP/IPV6_CSUM by default. Anyway, kernels before 8cb65d000 have that 
problem, not only after 8cb65d000.


Toshiaki Makita

Re: [PATCH net 1/3] vlan: Fix tcp checksums offloads for Q-in-Q vlan.

[Vlad Yasevich]

On 05/19/2017 04:16 AM, Toshiaki Makita wrote:
> On 2017/05/19 16:09, Vlad Yasevich wrote:
>> On 05/18/2017 10:13 PM, Toshiaki Makita wrote:
>>> On 2017/05/18 22:31, Vladislav Yasevich wrote:
 It appears that since commit 8cb65d000, Q-in-Q vlans have been
 broken.  The series that commit is part of enabled TSO and checksum
 offloading on Q-in-Q vlans.  However, most HW we support can't handle
 it.  To work around the issue, the above commit added a function that
 turns off offloads on Q-in-Q devices, but it left the checksum offload.
 That will cause issues with most older devices that supprort very basic
 checksum offload capabilities as well as some newer devices (we've
 reproduced te problem with both be2net and bnx).

 To solve this for everyone, turn off checksum offloading feature
 by default when sending Q-in-Q traffic.  Devices that are proven to
 work can provided a corrected ndo_features_check implemetation.

 Fixes: 8cb65d000 ("net: Move check for multiple vlans to drivers")
 CC: Toshiaki Makita 
 Signed-off-by: Vladislav Yasevich 
 ---
  include/linux/if_vlan.h | 1 -
  1 file changed, 1 deletion(-)

 diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
 index 8d5fcd6..ae537f0 100644
 --- a/include/linux/if_vlan.h
 +++ b/include/linux/if_vlan.h
 @@ -619,7 +619,6 @@ static inline netdev_features_t 
 vlan_features_check(const struct sk_buff *skb,
 NETIF_F_SG |
 NETIF_F_HIGHDMA |
 NETIF_F_FRAGLIST |
 -   NETIF_F_HW_CSUM |
 NETIF_F_HW_VLAN_CTAG_TX |
 NETIF_F_HW_VLAN_STAG_TX);
  
>>>
>>> I guess HW_CSUM theoretically can handle Q-in-Q packets and the problem
>>> is IP_CSUM and IPV6_CSUM.
>>> So wouldn't it be better to leave HW_CSUM and drop IP_CSUM/IPV6_CSUM,
>>> i.e. change intersection into bitwise AND?
>>>
>>
>> It wasn't really a problem before accelerations got enabled on q-in-q
>> vlans.
> 
> Right for stacked vlan device.
> But I think the check was there for packets from guests forwarded by
> bridge to vlan device so it was a problem before 8cb65d000.

Not really, since stacked vlans in guests wouldn't have accelerations on.
Haven't really tried a new guest on old hosts.  It might be an issue there...

> 
>>> The intersection was introduced in db115037bb57 ("net: fix checksum
>>> features handling in netif_skb_features()"), but I guess for this
>>> particular check the intersection was not needed.
>>>
>>
>> So, to put it another way, leave the intersection with HW_CSUM in the mask,
>> and then do:
>>
>>   return features & ~(NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM);
>>
>> This might work, but it assumes that everyone who announce HW_CSUM can
>> do q-in-q vlans.  It's been a bit of a pain tracking this down and I'd rather
>> fix it for everyone and let individual driver authors verify that Q-in-Q 
>> works
>> correctly with HW checksum.  However, I am willing to do the above if
>> that's what people want.
> 
> At least HW_CSUM in the check was introduced intentionally.
> https://www.spinics.net/lists/netdev/msg152016.html
> 
> And I think HW_CSUM should work with any packets.
> You know, include/linux/skbuff.h says
>>  *   NETIF_F_HW_CSUM - The driver (or its device) is able to compute one
>>  * IP (one's complement) checksum for any combination
>>  * of protocols or protocol layering.
> 
> We should be able to safely enable it.
> 
> ...But you are so worried about layer2 protocol handling of HW_CSUM
> devices, I'm ok with disabling it for now.
> 

It's a concern after running across this issue.  Granted, the few devices
we've seen this bug on use IP/IPV6 checksum features.  I am hoping someone
else might weigh in here.

-vlad

Re: [PATCH net 1/3] vlan: Fix tcp checksums offloads for Q-in-Q vlan.

[Toshiaki Makita]

On 2017/05/19 16:09, Vlad Yasevich wrote:
> On 05/18/2017 10:13 PM, Toshiaki Makita wrote:
>> On 2017/05/18 22:31, Vladislav Yasevich wrote:
>>> It appears that since commit 8cb65d000, Q-in-Q vlans have been
>>> broken.  The series that commit is part of enabled TSO and checksum
>>> offloading on Q-in-Q vlans.  However, most HW we support can't handle
>>> it.  To work around the issue, the above commit added a function that
>>> turns off offloads on Q-in-Q devices, but it left the checksum offload.
>>> That will cause issues with most older devices that supprort very basic
>>> checksum offload capabilities as well as some newer devices (we've
>>> reproduced te problem with both be2net and bnx).
>>>
>>> To solve this for everyone, turn off checksum offloading feature
>>> by default when sending Q-in-Q traffic.  Devices that are proven to
>>> work can provided a corrected ndo_features_check implemetation.
>>>
>>> Fixes: 8cb65d000 ("net: Move check for multiple vlans to drivers")
>>> CC: Toshiaki Makita 
>>> Signed-off-by: Vladislav Yasevich 
>>> ---
>>>  include/linux/if_vlan.h | 1 -
>>>  1 file changed, 1 deletion(-)
>>>
>>> diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
>>> index 8d5fcd6..ae537f0 100644
>>> --- a/include/linux/if_vlan.h
>>> +++ b/include/linux/if_vlan.h
>>> @@ -619,7 +619,6 @@ static inline netdev_features_t 
>>> vlan_features_check(const struct sk_buff *skb,
>>>  NETIF_F_SG |
>>>  NETIF_F_HIGHDMA |
>>>  NETIF_F_FRAGLIST |
>>> -NETIF_F_HW_CSUM |
>>>  NETIF_F_HW_VLAN_CTAG_TX |
>>>  NETIF_F_HW_VLAN_STAG_TX);
>>>  
>>
>> I guess HW_CSUM theoretically can handle Q-in-Q packets and the problem
>> is IP_CSUM and IPV6_CSUM.
>> So wouldn't it be better to leave HW_CSUM and drop IP_CSUM/IPV6_CSUM,
>> i.e. change intersection into bitwise AND?
>>
> 
> It wasn't really a problem before accelerations got enabled on q-in-q
> vlans.

Right for stacked vlan device.
But I think the check was there for packets from guests forwarded by
bridge to vlan device so it was a problem before 8cb65d000.

>> The intersection was introduced in db115037bb57 ("net: fix checksum
>> features handling in netif_skb_features()"), but I guess for this
>> particular check the intersection was not needed.
>>
> 
> So, to put it another way, leave the intersection with HW_CSUM in the mask,
> and then do:
> 
>   return features & ~(NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM);
> 
> This might work, but it assumes that everyone who announce HW_CSUM can
> do q-in-q vlans.  It's been a bit of a pain tracking this down and I'd rather
> fix it for everyone and let individual driver authors verify that Q-in-Q works
> correctly with HW checksum.  However, I am willing to do the above if
> that's what people want.

At least HW_CSUM in the check was introduced intentionally.
https://www.spinics.net/lists/netdev/msg152016.html

And I think HW_CSUM should work with any packets.
You know, include/linux/skbuff.h says
>  *NETIF_F_HW_CSUM - The driver (or its device) is able to compute one
>  *  IP (one's complement) checksum for any combination
>  *  of protocols or protocol layering.

We should be able to safely enable it.

...But you are so worried about layer2 protocol handling of HW_CSUM
devices, I'm ok with disabling it for now.

-- 
Toshiaki Makita

Re: [PATCH net 1/3] vlan: Fix tcp checksums offloads for Q-in-Q vlan.

[Vlad Yasevich]

On 05/18/2017 10:13 PM, Toshiaki Makita wrote:
> On 2017/05/18 22:31, Vladislav Yasevich wrote:
>> It appears that since commit 8cb65d000, Q-in-Q vlans have been
>> broken.  The series that commit is part of enabled TSO and checksum
>> offloading on Q-in-Q vlans.  However, most HW we support can't handle
>> it.  To work around the issue, the above commit added a function that
>> turns off offloads on Q-in-Q devices, but it left the checksum offload.
>> That will cause issues with most older devices that supprort very basic
>> checksum offload capabilities as well as some newer devices (we've
>> reproduced te problem with both be2net and bnx).
>>
>> To solve this for everyone, turn off checksum offloading feature
>> by default when sending Q-in-Q traffic.  Devices that are proven to
>> work can provided a corrected ndo_features_check implemetation.
>>
>> Fixes: 8cb65d000 ("net: Move check for multiple vlans to drivers")
>> CC: Toshiaki Makita 
>> Signed-off-by: Vladislav Yasevich 
>> ---
>>  include/linux/if_vlan.h | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
>> index 8d5fcd6..ae537f0 100644
>> --- a/include/linux/if_vlan.h
>> +++ b/include/linux/if_vlan.h
>> @@ -619,7 +619,6 @@ static inline netdev_features_t 
>> vlan_features_check(const struct sk_buff *skb,
>>   NETIF_F_SG |
>>   NETIF_F_HIGHDMA |
>>   NETIF_F_FRAGLIST |
>> - NETIF_F_HW_CSUM |
>>   NETIF_F_HW_VLAN_CTAG_TX |
>>   NETIF_F_HW_VLAN_STAG_TX);
>>  
> 
> I guess HW_CSUM theoretically can handle Q-in-Q packets and the problem
> is IP_CSUM and IPV6_CSUM.
> So wouldn't it be better to leave HW_CSUM and drop IP_CSUM/IPV6_CSUM,
> i.e. change intersection into bitwise AND?
> 

It wasn't really a problem before accelerations got enabled on q-in-q
vlans.

> The intersection was introduced in db115037bb57 ("net: fix checksum
> features handling in netif_skb_features()"), but I guess for this
> particular check the intersection was not needed.
> 

So, to put it another way, leave the intersection with HW_CSUM in the mask,
and then do:

  return features & ~(NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM);

This might work, but it assumes that everyone who announce HW_CSUM can
do q-in-q vlans.  It's been a bit of a pain tracking this down and I'd rather
fix it for everyone and let individual driver authors verify that Q-in-Q works
correctly with HW checksum.  However, I am willing to do the above if
that's what people want.

-vlad

Re: [PATCH net 1/3] vlan: Fix tcp checksums offloads for Q-in-Q vlan.

[Toshiaki Makita]

On 2017/05/18 22:31, Vladislav Yasevich wrote:
> It appears that since commit 8cb65d000, Q-in-Q vlans have been
> broken.  The series that commit is part of enabled TSO and checksum
> offloading on Q-in-Q vlans.  However, most HW we support can't handle
> it.  To work around the issue, the above commit added a function that
> turns off offloads on Q-in-Q devices, but it left the checksum offload.
> That will cause issues with most older devices that supprort very basic
> checksum offload capabilities as well as some newer devices (we've
> reproduced te problem with both be2net and bnx).
> 
> To solve this for everyone, turn off checksum offloading feature
> by default when sending Q-in-Q traffic.  Devices that are proven to
> work can provided a corrected ndo_features_check implemetation.
> 
> Fixes: 8cb65d000 ("net: Move check for multiple vlans to drivers")
> CC: Toshiaki Makita 
> Signed-off-by: Vladislav Yasevich 
> ---
>  include/linux/if_vlan.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
> index 8d5fcd6..ae537f0 100644
> --- a/include/linux/if_vlan.h
> +++ b/include/linux/if_vlan.h
> @@ -619,7 +619,6 @@ static inline netdev_features_t vlan_features_check(const 
> struct sk_buff *skb,
>NETIF_F_SG |
>NETIF_F_HIGHDMA |
>NETIF_F_FRAGLIST |
> -  NETIF_F_HW_CSUM |
>NETIF_F_HW_VLAN_CTAG_TX |
>NETIF_F_HW_VLAN_STAG_TX);
>  

I guess HW_CSUM theoretically can handle Q-in-Q packets and the problem
is IP_CSUM and IPV6_CSUM.
So wouldn't it be better to leave HW_CSUM and drop IP_CSUM/IPV6_CSUM,
i.e. change intersection into bitwise AND?

The intersection was introduced in db115037bb57 ("net: fix checksum
features handling in netif_skb_features()"), but I guess for this
particular check the intersection was not needed.

-- 
Toshiaki Makita

Re: [PATCH net 1/3] vlan: Fix tcp checksums offloads for Q-in-Q vlan.

[Toshiaki Makita]

On 2017/05/18 22:31, Vladislav Yasevich wrote:
> It appears that since commit 8cb65d000, Q-in-Q vlans have been
> broken.  The series that commit is part of enabled TSO and checksum
> offloading on Q-in-Q vlans.  However, most HW we support can't handle
> it.  To work around the issue, the above commit added a function that
> turns off offloads on Q-in-Q devices, but it left the checksum offload.
> That will cause issues with most older devices that supprort very basic
> checksum offload capabilities as well as some newer devices (we've
> reproduced te problem with both be2net and bnx).
> 
> To solve this for everyone, turn off checksum offloading feature
> by default when sending Q-in-Q traffic.  Devices that are proven to
> work can provided a corrected ndo_features_check implemetation.
> 
> Fixes: 8cb65d000 ("net: Move check for multiple vlans to drivers")
> CC: Toshiaki Makita 
> Signed-off-by: Vladislav Yasevich 

The patch looks ok, but why do you think 8cb65d000 is wrong?
The same check was there before my patch set.

kernel v4.0:
> netdev_features_t netif_skb_features(struct sk_buff *skb)
...
>   if (protocol == htons(ETH_P_8021Q) || protocol == htons(ETH_P_8021AD))
>   features = netdev_intersect_features(features,
>NETIF_F_SG |
>NETIF_F_HIGHDMA |
>NETIF_F_FRAGLIST |
>NETIF_F_GEN_CSUM |
>NETIF_F_HW_VLAN_CTAG_TX |
>NETIF_F_HW_VLAN_STAG_TX);

The commit just moved the check into another function.


Toshiaki Makita

[PATCH net 1/3] vlan: Fix tcp checksums offloads for Q-in-Q vlan.

[Vladislav Yasevich]

It appears that since commit 8cb65d000, Q-in-Q vlans have been
broken.  The series that commit is part of enabled TSO and checksum
offloading on Q-in-Q vlans.  However, most HW we support can't handle
it.  To work around the issue, the above commit added a function that
turns off offloads on Q-in-Q devices, but it left the checksum offload.
That will cause issues with most older devices that supprort very basic
checksum offload capabilities as well as some newer devices (we've
reproduced te problem with both be2net and bnx).

To solve this for everyone, turn off checksum offloading feature
by default when sending Q-in-Q traffic.  Devices that are proven to
work can provided a corrected ndo_features_check implemetation.

Fixes: 8cb65d000 ("net: Move check for multiple vlans to drivers")
CC: Toshiaki Makita 
Signed-off-by: Vladislav Yasevich 
---
 include/linux/if_vlan.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
index 8d5fcd6..ae537f0 100644
--- a/include/linux/if_vlan.h
+++ b/include/linux/if_vlan.h
@@ -619,7 +619,6 @@ static inline netdev_features_t vlan_features_check(const 
struct sk_buff *skb,
 NETIF_F_SG |
 NETIF_F_HIGHDMA |
 NETIF_F_FRAGLIST |
-NETIF_F_HW_CSUM |
 NETIF_F_HW_VLAN_CTAG_TX |
 NETIF_F_HW_VLAN_STAG_TX);
 
-- 
2.7.4