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] [day] [month] [year] [list]

Date:   Mon, 16 Dec 2019 07:38:34 +0000
From:   Manish Chopra <manishc@...vell.com>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>,
        Michael Chan <michael.chan@...adcom.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [EXT] Re: LRO/HW_GRO is not disabled when native xdp is installed

> -----Original Message-----
> From: Jakub Kicinski <jakub.kicinski@...ronome.com>
> Sent: Sunday, December 15, 2019 1:12 AM
> To: Michael Chan <michael.chan@...adcom.com>
> Cc: Manish Chopra <manishc@...vell.com>; 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.

Thanks Michael and Jakub,

I will rather opt to fix this in qede driver -  qede will implicitly disable the HW gro and
allow the xdp prog installation instead of failing it (just the way bridging disables LRO
implicitly on underlined NIC). I could also choose to fail xdp installation if HW gro is enabled,
but that will require some user intervention to disable HW gro explicitly by user and I am not
sure if such failure from qede can lead to unexpected issues in some deployment environment.

Regards,
Manish