netdev - Re: [PATCH net-next v5 0/5] 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: <CAKgT0UexohK4Mup928VqLDfzuafB-geqhbHA_Y9dJL3Yo4goKw@mail.gmail.com>
Date:   Fri, 22 Dec 2017 10:14:32 -0800
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     Sabrina Dubroca <sd@...asysnail.net>
Cc:     Michael Chan <michael.chan@...adcom.com>,
        David Miller <davem@...emloft.net>,
        Netdev <netdev@...r.kernel.org>,
        Andrew Gospodarek <andrew.gospodarek@...adcom.com>
Subject: Re: [PATCH net-next v5 0/5] Introduce NETIF_F_GRO_HW

On Fri, Dec 22, 2017 at 6:57 AM, Sabrina Dubroca <sd@...asysnail.net> wrote:
> Hello,
>
> Sorry for commenting late.
>
> 2017-12-16, 03:09:39 -0500, Michael Chan wrote:
>> Introduce NETIF_F_GRO_HW feature flag and convert drivers that support
>> hardware GRO to use the new flag.
>>
>> v5:
>> - Documentation changes requested by Alexander Duyck.
>> - bnx2x changes requested by Manish Chopra to enable LRO by default, and
>> disable GRO_HW if disable_tpa module parameter is set.
>>
>> v4:
>> - more changes requested by Alexander Duyck:
>> - check GRO_HW/GRO dependency in drivers's ndo_fix_features().
>> - Reverse the order of RXCSUM and GRO_HW dependency check in
>> netdev_fix_features().
>> - No propagation in netdev_disable_gro_hw().
>
> IIUC, with the patches that were applied, each driver can define
> whether GRO_HW depends on GRO? From a user's perspective, this
> inconsistent behavior is going to be quite confusing.
>
> Worse than inconsistent behavior, it looks like a driver deciding that
> GRO_HW doesn't depend on GRO is going to introduce a change of
> behavior.  Previously, when GRO was disabled, there wouldn't be any
> packet over MTU handed to the network stack.  Now, even if GRO is
> disabled, GRO_HW might still be enabled, so we might get over-MTU
> packets because of hardware GRO.

This isn't actually true. LRO was still handling packets larger than
MTU over even when GRO was disabled.

> I don't think drivers should be allowed to say "GRO_HW doesn't depend
> on GRO".

Why not, it doesn't. In my mind GRO_HW is closer to LRO than it is to
GRO. The only ugly bit as I see it is that these devices were exposing
the feature via the GRO flag in the first place. So for the sake of
legacy they might want to carry around the dependency.

> I think it's reasonable to be able to disable software GRO even if
> hardware GRO is enabled. Thus, I would propose:
> - keep the current GRO flag
> - add a GRO_HW flag, depending on GRO, enforced by the core as in
>   earlier versions of these patches
> - add a GRO_SW flag, also depending on GRO

This seems like a bunch of extra overhead for not much gain. Do we
really need to fork GRO into 3 bits? I would argue that GRO_HW really
should have been branded something like FORWARDABLE_LRO, but nobody
wanted to touch the name LRO since it apparently has some negative
stigma to it. If we had used a name like that we probably wouldn't be
going through all these extra hoops. The only real reason why this is
even being associated with GRO in the first place is that is how this
feature was hidden by the drivers so they got around having to deal
with the LRO being disabled for routing/forwarding issue. Those are
the parts that want to keep it associated with GRO since that is how
they exposed it in their devices originally.

- Alex