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: <CAKgT0Ue9h8Pdb4OkBiWdL5bSNHPUkbZfBosrtZ4gQOAnuGiwRQ@mail.gmail.com>
Date:   Thu, 7 Dec 2017 13:35:46 -0800
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     Michael Chan <michael.chan@...adcom.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:44 AM, Michael Chan <michael.chan@...adcom.com> wrote:
> 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.

I'm not saying this is on a single device. The problem is in the case
of bonding you have combined 2 devices. We don't want to have an upper
device saying LRO isn't there when it is. A bonding setup is supposed
to advertise LRO as being enabled if a lower device has it present.
You are now making this an either/or thing at the driver level
generically when that isn't the case due to things like bridging and
bonding. This is why I was arguing that it would be easier to just
make this a super-set of LRO with one form that is reversible and one
that isn't. As is you are introducing all sorts of regressions that
are going to be problematic in any sort of mixed environment.

> 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.

I get that they are very different. At a driver level it makes sense
to say you get one or the other at a physical driver level. The point
I was trying to make is that the bond is only able to set one or the
other when you might have a mix of devices. On the bonding device you
should be able to have both LRO and GRO_HW enabled.