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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 22 Dec 2017 15:57:58 +0100
From:   Sabrina Dubroca <sd@...asysnail.net>
To:     Michael Chan <michael.chan@...adcom.com>
Cc:     davem@...emloft.net, netdev@...r.kernel.org,
        andrew.gospodarek@...adcom.com, alexander.duyck@...il.com
Subject: Re: [PATCH net-next v5 0/5] Introduce NETIF_F_GRO_HW

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.

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

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

Thanks,

-- 
Sabrina