netdev - Re: [PATCH net-next 1/4] 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: <CACKFLinxDzE++S-ML-41D4+cxHjW0Opkjy1JwYdc1feJtyGQ=A@mail.gmail.com>
Date:   Mon, 4 Dec 2017 14:31:14 -0800
From:   Michael Chan <michael.chan@...adcom.com>
To:     Yuval Mintz <yuvalm@...lanox.com>
Cc:     "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Ariel Elior <Ariel.Elior@...ium.com>,
        "everest-linux-l2@...ium.com" <everest-linux-l2@...ium.com>
Subject: Re: [PATCH net-next 1/4] net: Introduce NETIF_F_GRO_HW

On Mon, Dec 4, 2017 at 2:15 PM, Yuval Mintz <yuvalm@...lanox.com> wrote:
>> @@ -96,6 +98,7 @@ enum {
>>  #define NETIF_F_FRAGLIST     __NETIF_F(FRAGLIST)
>>  #define NETIF_F_FSO          __NETIF_F(FSO)
>>  #define NETIF_F_GRO          __NETIF_F(GRO)
>> +#define NETIF_F_GRO_HW               __NETIF_F(GRO_HW)
>>  #define NETIF_F_GSO          __NETIF_F(GSO)
>>  #define NETIF_F_GSO_ROBUST   __NETIF_F(GSO_ROBUST)
>>  #define NETIF_F_HIGHDMA              __NETIF_F(HIGHDMA)
>> @@ -193,7 +196,7 @@ enum {
>>   * If upper/master device has these features disabled, they must be disabled
>>   * on all lower/slave devices as well.
>>   */
>> -#define NETIF_F_UPPER_DISABLES       NETIF_F_LRO
>> +#define NETIF_F_UPPER_DISABLES       (NETIF_F_LRO | NETIF_F_GRO_HW)
>>
>>  /* changeable features with no special hardware requirements */
>>  #define NETIF_F_SOFT_FEATURES        (NETIF_F_GSO | NETIF_F_GRO)
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 30b5fe3..09c2ad0 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -7392,6 +7392,19 @@ 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. */
>> +             if (!(features & NETIF_F_GRO)) {
>
> While at it, perhaps also make it dependent on NETIF_F_RXCSUM?

OK.  Makes sense.

>
>> +                     netdev_dbg(dev, "Dropping NETIF_F_GSO_HW since
>> no GRO 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;
>
> Isn't this considered to be breaking an existing API?
> After this, while NETIF_F_GRO_HW is published an application trying to
> set NETIF_F_LRO and then query its state would discover it failed
> [while previously it could have succeeded, such as for bnx2]
>
> While I understand the need to make sure core doesn't enable
> two competing aggregation offloads, why make GRO_HW > LRO?
> I understand it's probably the better one, but until LRO gets deprecated
> isn't it safer to do this limitation the opposite way?
> I.e., make sure NETIF_F_GRO_HW can't be set as long as NETIF_F_LRO is set?

I am just following precedents in the netdev_fix_features() logic to
drop incompatible features.  I can make LRO and GRO_HW have equal
standing by dropping the other when one is set.  So if I do that, user
will be able to always enable LRO.  The code will just drop GRO_HW if
it is set.