netdev - Re: [EXT] Re: LRO/HW_GRO is not disabled when native xdp is installed

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:   Sat, 14 Dec 2019 11:42:28 -0800
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Michael Chan <michael.chan@...adcom.com>
Cc:     Manish Chopra <manishc@...vell.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [EXT] Re: LRO/HW_GRO is not disabled when native xdp is
 installed

On Fri, 13 Dec 2019 13:09:31 -0800, Michael Chan wrote:
> On Fri, Dec 13, 2019 at 1:45 AM Manish Chopra <manishc@...vell.com> wrote:
> 
> > It used to be the case for our devices as well long back. but after the commit 18c602dee4726 ("qede: Use NETIF_F_GRO_HW")
> > that part was removed from our driver and commit 56f5aa77cdad1 ("net: Disable GRO_HW when generic XDP is installed on a device")
> > was added to achieve the same instead of individual driver doing it, but it wasn't caught that time why later commit only does for generic xdp,
> > not for native xdp. So today when native xdp is attached to our device, HW GRO aggregation remains enabled on our device.
> > Please let me know if that fix is good enough to be posted. Will test it out and post.
> >  
> 
> The driver knows when an xdp program is attached and can disable any
> features that are not compatible, so I think it is more efficient to
> keep it this way.  Generic XDP on the other hand, does not involve the
> driver and so we need to disable them for the driver.

The only question is should the driver refuse the XDP prog installation
(with a nice extack message) or should it silently disable the feature?
IIRC ixgbe opts for returning -EINVAL if RSC/LRO is enabled. Micheal
says that bnxt disables automatically. My preference is for the former,
because it's simpler - we all keep the MTU intact and don't disable
queues to make room for XDP, IOW don't automatically change user
controlled configuration is a simpler policy. But I don't feel too
strongly, we should just make sure we document what's the expected
behaviour (even better make netdevsim behave that way and write a test).

Manish, if you were to go ahead with your patch please make sure you
don't disable the features when program is installed in offload mode,
though.