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]
Date:   Thu, 7 Dec 2017 14:08:39 -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 1:35 PM, Alexander Duyck
<alexander.duyck@...il.com> wrote:
> 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.

Are you saying that if LRO is dropped on a upper device (by the new
code added in this patch), all lower devices will need to drop it too?

If that's what you are saying, it's already taken care of.  There is
netdev_sync_upper_features() and netdev_sync_lower_features() that
will automatically do that as part of __netdev_update_features().

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

On the bond, you can have LRO enabled and it is propagated to lower
devices so that lower devices will enable LRO if it is supported.  If
LRO is disabled on the bond (e.g. due to bridging), It will be
propagated so all lower devices will disable LRO if it is enabled.

GRO/GRO_HW is different and it is not propagated from upper devices to
lower devices like LRO.  Lower devices can have GRO/GRO_HW
enabled/disabled regardless of upper device.  This is no different
from say RXCSUM.  Lower devices can have RXCSUM on/off regardless.