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

From: Toke Høiland-Jørgensen <[email-masked]>
Date: Thu, 22 Feb 2024 12:22:51 +0100

Jakub Kicinski <[email-masked]> 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 <[email-masked]>
> Reported-by: [email-masked]
> Signed-off-by: Jakub Kicinski <[email-masked]>

Makes sense!

Reviewed-by: Toke Høiland-Jørgensen <[email-masked]>