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

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

[Jakub Kicinski]

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>
---
 drivers/net/veth.c | 35 +++++++++++++++++------------------
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index 578e36ea1589..a786be805709 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -1208,14 +1208,6 @@ static int veth_enable_xdp(struct net_device *dev)
 				veth_disable_xdp_range(dev, 0, dev->real_num_rx_queues, true);
 				return err;
 			}
-
-			if (!veth_gro_requested(dev)) {
-				/* user-space did not require GRO, but adding XDP
-				 * is supposed to get GRO working
-				 */
-				dev->features |= NETIF_F_GRO;
-				netdev_features_change(dev);
-			}
 		}
 	}
 
@@ -1235,18 +1227,9 @@ static void veth_disable_xdp(struct net_device *dev)
 	for (i = 0; i < dev->real_num_rx_queues; i++)
 		rcu_assign_pointer(priv->rq[i].xdp_prog, NULL);
 
-	if (!netif_running(dev) || !veth_gro_requested(dev)) {
+	if (!netif_running(dev) || !veth_gro_requested(dev))
 		veth_napi_del(dev);
 
-		/* if user-space did not require GRO, since adding XDP
-		 * enabled it, clear it now
-		 */
-		if (!veth_gro_requested(dev) && netif_running(dev)) {
-			dev->features &= ~NETIF_F_GRO;
-			netdev_features_change(dev);
-		}
-	}
-
 	veth_disable_xdp_range(dev, 0, dev->real_num_rx_queues, false);
 }
 
@@ -1654,6 +1637,14 @@ static int veth_xdp_set(struct net_device *dev, struct bpf_prog *prog,
 		}
 
 		if (!old_prog) {
+			if (!veth_gro_requested(dev)) {
+				/* user-space did not require GRO, but adding
+				 * XDP is supposed to get GRO working
+				 */
+				dev->features |= NETIF_F_GRO;
+				netdev_features_change(dev);
+			}
+
 			peer->hw_features &= ~NETIF_F_GSO_SOFTWARE;
 			peer->max_mtu = max_mtu;
 		}
@@ -1669,6 +1660,14 @@ static int veth_xdp_set(struct net_device *dev, struct bpf_prog *prog,
 			if (dev->flags & IFF_UP)
 				veth_disable_xdp(dev);
 
+			/* if user-space did not require GRO, since adding XDP
+			 * enabled it, clear it now
+			 */
+			if (!veth_gro_requested(dev)) {
+				dev->features &= ~NETIF_F_GRO;
+				netdev_features_change(dev);
+			}
+
 			if (peer) {
 				peer->hw_features |= NETIF_F_GSO_SOFTWARE;
 				peer->max_mtu = ETH_MAX_MTU;
-- 
2.43.0

[PATCH net 2/2] selftests: net: veth: test syncing GRO and XDP state while device is down

[Jakub Kicinski]

Test that we keep GRO flag in sync when XDP is disabled while
the device is closed.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/testing/selftests/net/veth.sh | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/tools/testing/selftests/net/veth.sh b/tools/testing/selftests/net/veth.sh
index 27574bbf2d63..5ae85def0739 100755
--- a/tools/testing/selftests/net/veth.sh
+++ b/tools/testing/selftests/net/veth.sh
@@ -246,6 +246,20 @@ ip netns exec $NS_DST ethtool -K veth$DST rx-udp-gro-forwarding on
 chk_gro "        - aggregation with TSO off" 1
 cleanup
 
+create_ns
+ip -n $NS_DST link set dev veth$DST up
+ip -n $NS_DST link set dev veth$DST xdp object ${BPF_FILE} section xdp
+chk_gro_flag "gro vs xdp while down - gro flag on" $DST on
+ip -n $NS_DST link set dev veth$DST down
+chk_gro_flag "                      - after down" $DST on
+ip -n $NS_DST link set dev veth$DST xdp off
+chk_gro_flag "                      - after xdp off" $DST off
+ip -n $NS_DST link set dev veth$DST up
+chk_gro_flag "                      - after up" $DST off
+ip -n $NS_SRC link set dev veth$SRC xdp object ${BPF_FILE} section xdp
+chk_gro_flag "                      - after peer xdp" $DST off
+cleanup
+
 create_ns
 chk_channels "default channels" $DST 1 1
 
-- 
2.43.0

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

[Toke Høiland-Jørgensen]

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>

Re: [PATCH net 2/2] selftests: net: veth: test syncing GRO and XDP state while device is down

[Toke Høiland-Jørgensen]

Jakub Kicinski <kuba@kernel.org> writes:

> Test that we keep GRO flag in sync when XDP is disabled while
> the device is closed.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

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

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

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Wed, 21 Feb 2024 15:12:10 -0800 you wrote:
> 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.
> 
> [...]

Here is the summary with links:
  - [net,1/2] net: veth: clear GRO when clearing XDP even when down
    https://git.kernel.org/netdev/net/c/fe9f801355f0
  - [net,2/2] selftests: net: veth: test syncing GRO and XDP state while device is down
    https://git.kernel.org/netdev/net/c/1a825e4cdf45

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html