netdev - Re: [PATCH net 1/2] net: veth: clear GRO when clearing XDP even when down

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: <87y1bchhf8.fsf@toke.dk>
Date: Thu, 22 Feb 2024 12:22:51 +0100
From: Toke Høiland-Jørgensen <toke@...hat.com>
To: Jakub Kicinski <kuba@...nel.org>, davem@...emloft.net
Cc: netdev@...r.kernel.org, edumazet@...gle.com, pabeni@...hat.com,
 lorenzo@...nel.org, Jakub Kicinski <kuba@...nel.org>, Thomas Gleixner
 <tglx@...utronix.de>,
 syzbot+039399a9b96297ddedca@...kaller.appspotmail.com
Subject: Re: [PATCH net 1/2] net: veth: clear GRO when clearing XDP even
 when down

Jakub Kicinski <kuba@...nel.org> writes:

> veth sets NETIF_F_GRO automatically when XDP is enabled,
> because both features use the same NAPI machinery.
>
> The logic to clear NETIF_F_GRO sits in veth_disable_xdp() which
> is called both on ndo_stop and when XDP is turned off.
> To avoid the flag from being cleared when the device is brought
> down, the clearing is skipped when IFF_UP is not set.
> Bringing the device down should indeed not modify its features.
>
> Unfortunately, this means that clearing is also skipped when
> XDP is disabled _while_ the device is down. And there's nothing
> on the open path to bring the device features back into sync.
> IOW if user enables XDP, disables it and then brings the device
> up we'll end up with a stray GRO flag set but no NAPI instances.
>
> We don't depend on the GRO flag on the datapath, so the datapath
> won't crash. We will crash (or hang), however, next time features
> are sync'ed (either by user via ethtool or peer changing its config).
> The GRO flag will go away, and veth will try to disable the NAPIs.
> But the open path never created them since XDP was off, the GRO flag
> was a stray. If NAPI was initialized before we'll hang in napi_disable().
> If it never was we'll crash trying to stop uninitialized hrtimer.
>
> Move the GRO flag updates to the XDP enable / disable paths,
> instead of mixing them with the ndo_open / ndo_close paths.
>
> Fixes: d3256efd8e8b ("veth: allow enabling NAPI even without XDP")
> Reported-by: Thomas Gleixner <tglx@...utronix.de>
> Reported-by: syzbot+039399a9b96297ddedca@...kaller.appspotmail.com
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>

Makes sense!

Reviewed-by: Toke Høiland-Jørgensen <toke@...hat.com>