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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 23 May 2017 09:29:29 -0700
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     David Miller <davem@...emloft.net>
Cc:     Vlad Yasevich <vyasevich@...il.com>,
        Netdev <netdev@...r.kernel.org>,
        "vyasevic@...hat.com" <vyasevic@...hat.com>,
        makita.toshiaki@....ntt.co.jp
Subject: Re: [PATCH net 1/3] vlan: Fix tcp checksums offloads for Q-in-Q vlan.

On Mon, May 22, 2017 at 4:59 PM, David Miller <davem@...emloft.net> wrote:
> From: Vladislav Yasevich <vyasevich@...il.com>
> 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 <makita.toshiaki@....ntt.co.jp>
>> Signed-off-by: Vladislav Yasevich <vyasevic@...hat.com>
>
> 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