netdev - Re: [PATCH net-next v2 1/5] net: Introduce NETIF_F_GRO

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACKFLimmyQEDs8Ti3SqVD2Tq2uEjQv7En+BORgQSj8tqh0mx3A@mail.gmail.com>
Date:   Thu, 7 Dec 2017 10:44:50 -0800
From:   Michael Chan <michael.chan@...adcom.com>
To:     Alexander Duyck <alexander.duyck@...il.com>
Cc:     David Miller <davem@...emloft.net>,
        Netdev <netdev@...r.kernel.org>,
        Andrew Gospodarek <andrew.gospodarek@...adcom.com>,
        Ariel Elior <Ariel.Elior@...ium.com>,
        everest-linux-l2@...ium.com
Subject: Re: [PATCH net-next v2 1/5] net: Introduce NETIF_F_GRO_HW.

On Thu, Dec 7, 2017 at 10:13 AM, Alexander Duyck
<alexander.duyck@...il.com> wrote:
> On Thu, Dec 7, 2017 at 12:03 AM, Michael Chan <michael.chan@...adcom.com> wrote:
>> @@ -7405,6 +7405,23 @@ static netdev_features_t netdev_fix_features(struct net_device *dev,
>>                 features &= ~dev->gso_partial_features;
>>         }
>>
>> +       if (features & NETIF_F_GRO_HW) {
>> +               /* Hardware GRO depends on GRO and RXCSUM. */
>> +               if (!(features & NETIF_F_GRO)) {
>> +                       netdev_dbg(dev, "Dropping NETIF_F_GSO_HW since no GRO feature.\n");
>> +                       features &= ~NETIF_F_GRO_HW;
>> +               }
>
> I'm not sure I agree with this part. The hardware offload should not
> be dependent on the software offload.

As I said before, GRO_HW is basically hardware accelerated GRO. To me,
it makes perfect sense that GRO_HW depends on GRO.

> If you are concerned with the
> XDP case and such you should probably just look at handling it more
> like how TSO is handled with a group of flags that aggregate GRO, LRO,
> and GRO_HW into a group of features that must be disabled to support
> XDP.
>
>> +               if (!(features & NETIF_F_RXCSUM)) {
>> +                       netdev_dbg(dev, "Dropping NETIF_F_GSO_HW since no RXCSUM feature.\n");
>> +                       features &= ~NETIF_F_GRO_HW;
>> +               }
>> +               /* Hardware GRO and LRO are mutually exclusive. */
>> +               if (features & NETIF_F_LRO) {
>> +                       netdev_dbg(dev, "Dropping NETIF_F_LRO since GRO_HW is set.\n");
>> +                       features &= ~NETIF_F_LRO;
>> +               }
>
> This is going to be problematic. What if you bond one interface that
> has LRO and one that has GRO_HW? It seems like this is going to cause
> the bond interface to lie and say LRO isn't there when it actually is,
> or worse yet it will force features off when they shouldn't be.

This is just dropping incompatible features similar to how other
incompatible feature flags are dropped in netdev_fix_features().
Hardware cannot aggregate in both LRO and GRO_HW modes at the same
time.  It's one or the other.

LRO and GRO_HW are different.  We may never agree on this but they are
different.  If a bond needs to disable LRO, it will be propagated to
all slaves.  But each slave can have GRO enabled.  If GRO is enabled,
GRO_HW, being a subset of GRO, can be enabled.