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

["Toke Høiland-Jørgensen" <toke@redhat.com>]

Jakub Kicinski <kuba@kernel.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@linutronix.de>
> Reported-by: syzbot+039399a9b96297ddedca@syzkaller.appspotmail.com
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Makes sense!

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