[PATCH net-next 0/5] net: fix a couple of problems with XDP and bonding

[PATCH net-next 0/5] net: fix a couple of problems with XDP and bonding

[Jakub Kicinski]

I started looking at untangling XDP vs HW-GRO story because of mlx5.
mlx5 requires HW-GRO for HDS/memory providers. But generic XDP clears
HW-GRO enablement completely. This is annoying in NIPA but also,
from past experience with XDP changing ring geometry and IRQ mapping,
it will also be annoying in production.

My plan / hope is to make generic XDP attachment leave HW-GRO in wanted
features, that way it comes back after XDP is removed, making NIPA happy.
This also simplifies the drivers slightly, as they no longer have to
check XDP vs HW-GRO locally.

This is the most important part, I think - I would like to make
the claim that HW-GRO and XDP are incompatible (today). Most drivers
seem to agree, but IDPF allows XDP and HW-GRO to coexist. XDP can't
carry the GRO/GSO state so changing the packet or trying to send it
out would probably end badly. We should explicitly clear HW-GRO when
XDP is attached in the core, until we have an understanding of how
they would work together, and appropriate tests.

Please comment if you have opinion on the HW-GRO+XDP in general,
that said, this series is just prep around bonding. What I described
above will come next.

This series tries to shore up the gaps in XDP propagation.
bonding bypasses the XDP program accounting in net_device so all
the checks on control path trying to avoid enabling features
incompatible with XDP are moot (e.g. we can have XDP+memory providers).
First patch fixes the accounting (next 3 patches add tests).

Last patch makes us drop GSO packets if they reach XDP. I can't come
up with a clean way of stopping GRO from working on lower when upper
has generic XDP, so let's just drop the packets. I don't think
generic XDP is worth the effort, users can disable GRO themselves
if they really care.

All the issues here were discovered while working another series,
but they were reproduced and tested with the selftests included.

Jakub Kicinski (5):
  net: record XDP programs propagated to lower devices
  netdevsim: add ndo_xdp_xmit
  selftests: drv-net: check tcp-data-split against an already attached
    XDP
  selftests/bpf: check XDP attach on a nested bond slave
  net: drop GSO skbs instead of handing them to XDP

 include/linux/netdevice.h                     |   7 ++
 include/net/xdp.h                             |  16 +++
 drivers/net/bonding/bond_main.c               |   4 +-
 drivers/net/netdevsim/netdev.c                |  20 ++++
 drivers/net/veth.c                            |   3 +
 net/core/dev.c                                | 105 +++++++++++++-----
 .../selftests/bpf/prog_tests/xdp_bonding.c    |  18 ++-
 tools/testing/selftests/drivers/net/config    |   1 +
 tools/testing/selftests/drivers/net/hds.py    |  65 +++++++++++
 9 files changed, 206 insertions(+), 33 deletions(-)

-- 
2.55.0

[PATCH net-next 1/5] net: record XDP programs propagated to lower devices

[Jakub Kicinski]

An upper device installs its XDP program on each lower with
dev_xdp_propagate(), which calls ndo_bpf() directly and records nothing
in the lower's xdp_state[]. netif_xdp_propagate() open-codes two of the
checks dev_xdp_install() makes, but the lower still looks program-free
to everything else, so the checks made in the opposite direction miss it
entirely:

  ethtool -G $slave tcp-data-split on

is refused by dev_xdp_sb_prog_count() for a device running a
single-buffer XDP program, but not for a bond slave running the bond's,
even though netif_xdp_propagate() would have refused to install that
same program had header-data split been on already. Binding a memory
provider has the same asymmetry. The lower does not report the program
over rtnetlink either, so it is invisible to userspace as well.

Install it into xdp_state[] like any other program and mark it with
xdp_from_upper, rather than tracking it separately. Every existing user
of dev_xdp_prog_count() and dev_xdp_sb_prog_count() then gets the right
answer with no changes. The flag is only needed where the owner matters:
bonding reads it to tell a slave's own program apart from the one it
pushed down, both to refuse enslaving such a device and to let the
bond-wide program be replaced, and dev_xdp_attach() reads it to keep the
program from being replaced or removed behind the upper's back.

The lower now holds its own reference, as it does for a program of its
own; the upper's per-lower reference for the driver is unchanged.

While here, refuse to propagate onto a device which has a program of its
own, matching dev_xdp_install(). Bonding checks this before enslaving
and before installing, netvsc does not and would silently replace the
VF's program.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 include/linux/netdevice.h       |   7 +++
 drivers/net/bonding/bond_main.c |   4 +-
 net/core/dev.c                  | 101 +++++++++++++++++++++++---------
 3 files changed, 81 insertions(+), 31 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index d037faff7c44..512e3a21d0bd 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2517,6 +2517,12 @@ struct net_device {
 	unsigned long		change_proto_down:1;
 	unsigned long		netns_immutable:1;
 	unsigned long		fcoe_mtu:1;
+	/**
+	 * @xdp_from_upper: the program in @xdp_state was installed by an
+	 *	upper device with netif_xdp_propagate(); it belongs to the
+	 *	upper and can't be replaced or removed through this device.
+	 */
+	unsigned long		xdp_from_upper:1;
 
 	struct list_head	net_notifier_list;
 
@@ -4418,6 +4424,7 @@ struct sk_buff *dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
 
 int bpf_xdp_link_attach(const union bpf_attr *attr, struct bpf_prog *prog);
 u8 dev_xdp_prog_count(struct net_device *dev);
+bool dev_xdp_has_own_prog(struct net_device *dev);
 int netif_xdp_propagate(struct net_device *dev, struct netdev_bpf *bpf);
 int dev_xdp_propagate(struct net_device *dev, struct netdev_bpf *bpf);
 u8 dev_xdp_sb_prog_count(struct net_device *dev);
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index de2489c3d9bf..a62fff94fea3 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2315,7 +2315,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
 			.extack  = extack,
 		};
 
-		if (dev_xdp_prog_count(slave_dev) > 0) {
+		if (dev_xdp_has_own_prog(slave_dev)) {
 			SLAVE_NL_ERR(bond_dev, slave_dev, extack,
 				     "Slave has XDP program loaded, please unload before enslaving");
 			res = -EOPNOTSUPP;
@@ -5715,7 +5715,7 @@ static int bond_xdp_set(struct net_device *dev, struct bpf_prog *prog,
 			goto err;
 		}
 
-		if (dev_xdp_prog_count(slave_dev) > 0) {
+		if (dev_xdp_has_own_prog(slave_dev)) {
 			SLAVE_NL_ERR(dev, slave_dev, extack,
 				     "Slave has XDP program loaded, please unload before enslaving");
 			err = -EOPNOTSUPP;
diff --git a/net/core/dev.c b/net/core/dev.c
index f660fccfc0db..096d1dedebfd 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -10330,6 +10330,15 @@ u8 dev_xdp_prog_count(struct net_device *dev)
 }
 EXPORT_SYMBOL_GPL(dev_xdp_prog_count);
 
+/* Does the device have an XDP program, and was it installed directly on the
+ * device rather than propagated down by an upper with netif_xdp_propagate()?
+ */
+bool dev_xdp_has_own_prog(struct net_device *dev)
+{
+	return dev_xdp_prog_count(dev) && !dev->xdp_from_upper;
+}
+EXPORT_SYMBOL_GPL(dev_xdp_has_own_prog);
+
 u8 dev_xdp_sb_prog_count(struct net_device *dev)
 {
 	u8 count = 0;
@@ -10342,35 +10351,6 @@ u8 dev_xdp_sb_prog_count(struct net_device *dev)
 	return count;
 }
 
-int netif_xdp_propagate(struct net_device *dev, struct netdev_bpf *bpf)
-{
-	if (!dev->netdev_ops->ndo_bpf)
-		return -EOPNOTSUPP;
-
-	if (dev->cfg->hds_config == ETHTOOL_TCP_DATA_SPLIT_ENABLED &&
-	    bpf->command == XDP_SETUP_PROG &&
-	    bpf->prog && !bpf->prog->aux->xdp_has_frags) {
-		NL_SET_ERR_MSG(bpf->extack,
-			       "unable to propagate XDP to device using tcp-data-split");
-		return -EBUSY;
-	}
-
-	if (dev_get_min_mp_channel_count(dev)) {
-		NL_SET_ERR_MSG(bpf->extack, "unable to propagate XDP to device using memory provider");
-		return -EBUSY;
-	}
-
-	return dev->netdev_ops->ndo_bpf(dev, bpf);
-}
-EXPORT_SYMBOL_GPL(netif_xdp_propagate);
-
-u32 dev_xdp_prog_id(struct net_device *dev, enum bpf_xdp_mode mode)
-{
-	struct bpf_prog *prog = dev_xdp_prog(dev, mode);
-
-	return prog ? prog->aux->id : 0;
-}
-
 static void dev_xdp_set_link(struct net_device *dev, enum bpf_xdp_mode mode,
 			     struct bpf_xdp_link *link)
 {
@@ -10385,6 +10365,63 @@ static void dev_xdp_set_prog(struct net_device *dev, enum bpf_xdp_mode mode,
 	dev->xdp_state[mode].prog = prog;
 }
 
+int netif_xdp_propagate(struct net_device *dev, struct netdev_bpf *bpf)
+{
+	struct bpf_prog *old_prog;
+	int err;
+
+	if (!dev->netdev_ops->ndo_bpf)
+		return -EOPNOTSUPP;
+
+	/* we have more work to do for setup, bypass for other commands */
+	if (bpf->command != XDP_SETUP_PROG)
+		return dev->netdev_ops->ndo_bpf(dev, bpf);
+
+	if (bpf->prog && dev_xdp_has_own_prog(dev)) {
+		NL_SET_ERR_MSG(bpf->extack,
+			       "unable to propagate XDP to device with an XDP program of its own");
+		return -EBUSY;
+	}
+
+	if (dev->cfg->hds_config == ETHTOOL_TCP_DATA_SPLIT_ENABLED &&
+	    bpf->prog && !bpf->prog->aux->xdp_has_frags) {
+		NL_SET_ERR_MSG(bpf->extack,
+			       "unable to propagate XDP to device using tcp-data-split");
+		return -EBUSY;
+	}
+
+	if (dev_get_min_mp_channel_count(dev)) {
+		NL_SET_ERR_MSG(bpf->extack, "unable to propagate XDP to device using memory provider");
+		return -EBUSY;
+	}
+
+	err = dev->netdev_ops->ndo_bpf(dev, bpf);
+	if (err)
+		return err;
+
+	/* Record it like a program of our own, so that everything which asks
+	 * whether XDP is running here gets the right answer. @xdp_from_upper
+	 * keeps the two apart where it matters.
+	 */
+	old_prog = dev_xdp_prog(dev, XDP_MODE_DRV);
+	if (bpf->prog)
+		bpf_prog_inc(bpf->prog);
+	dev_xdp_set_prog(dev, XDP_MODE_DRV, bpf->prog);
+	dev->xdp_from_upper = !!bpf->prog;
+	if (old_prog)
+		bpf_prog_put(old_prog);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(netif_xdp_propagate);
+
+u32 dev_xdp_prog_id(struct net_device *dev, enum bpf_xdp_mode mode)
+{
+	struct bpf_prog *prog = dev_xdp_prog(dev, mode);
+
+	return prog ? prog->aux->id : 0;
+}
+
 static int dev_xdp_install(struct net_device *dev, enum bpf_xdp_mode mode,
 			   bpf_op_t bpf_op, struct netlink_ext_ack *extack,
 			   u32 flags, struct bpf_prog *prog)
@@ -10492,6 +10529,7 @@ static void dev_xdp_uninstall(struct net_device *dev)
 
 		dev_xdp_set_link(dev, mode, NULL);
 	}
+	dev->xdp_from_upper = 0;
 }
 
 static int dev_xdp_attach(struct net_device *dev, struct netlink_ext_ack *extack,
@@ -10532,6 +10570,11 @@ static int dev_xdp_attach(struct net_device *dev, struct netlink_ext_ack *extack
 		NL_SET_ERR_MSG(extack, "XDP_FLAGS_REPLACE is not specified");
 		return -EINVAL;
 	}
+	/* the program belongs to an upper device */
+	if (dev->xdp_from_upper) {
+		NL_SET_ERR_MSG(extack, "Can't replace an XDP program installed by an upper device");
+		return -EBUSY;
+	}
 
 	mode = dev_xdp_mode(dev, flags);
 	/* can't replace attached link */
-- 
2.55.0

[PATCH net-next 2/5] netdevsim: add ndo_xdp_xmit

[Jakub Kicinski]

Bonding refuses a slave which has no ndo_xdp_xmit once the bond runs an
XDP program, so netdevsim can't currently be enslaved into an XDP bond
and the propagation of XDP down to lower devices has no test coverage
at all.

There is no data path behind it. netdevsim does not advertise
NETDEV_XDP_ACT_NDO_XMIT, so nothing should be redirecting frames here;
anything which does anyway is counted and freed rather than dropped
silently.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 drivers/net/netdevsim/netdev.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
index eebc02ccc4a9..3790b6eb4083 100644
--- a/drivers/net/netdevsim/netdev.c
+++ b/drivers/net/netdevsim/netdev.c
@@ -185,6 +185,25 @@ static netdev_tx_t nsim_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	return NETDEV_TX_OK;
 }
 
+/* Do not advertise NETDEV_XDP_ACT_NDO_XMIT, no real datapath.
+ * This is a "stub" which allows netdevsim to be put under a bond with XDP.
+ */
+static int nsim_xdp_xmit(struct net_device *dev, int n,
+			 struct xdp_frame **frames, u32 flags)
+{
+	int i;
+
+	if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK))
+		return -EINVAL;
+
+	for (i = 0; i < n; i++) {
+		dev_dstats_tx_dropped(dev);
+		xdp_return_frame(frames[i]);
+	}
+
+	return n;
+}
+
 static netdev_tx_t nsim_start_xmit_vf(struct sk_buff *skb,
 				      struct net_device *dev)
 {
@@ -651,6 +670,7 @@ static const struct net_device_ops nsim_netdev_ops = {
 	.ndo_set_features	= nsim_set_features,
 	.ndo_get_iflink		= nsim_get_iflink,
 	.ndo_bpf		= nsim_bpf,
+	.ndo_xdp_xmit		= nsim_xdp_xmit,
 	.ndo_open		= nsim_open,
 	.ndo_stop		= nsim_stop,
 	.ndo_vlan_rx_add_vid	= nsim_vlan_rx_add_vid,
-- 
2.55.0

[PATCH net-next 3/5] selftests: drv-net: check tcp-data-split against an already attached XDP

[Jakub Kicinski]

hds.py only ever tested one direction of the rule: with header-data
split on, installing a single-buffer XDP program must fail. The other
direction was not covered at all, and that is the one which was broken
for a program an upper device pushed down - the device was running XDP
but nothing in the core knew, so enabling tcp-data-split underneath it
succeeded.

xdp_set_hds() covers the direct case, xdp_bond_set_hds() the propagated
one. The latter fails without the preceding fix.

The bond case is disruptive, it takes the device down to enslave it, and
skips on devices bonding will not take as an XDP slave.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/testing/selftests/drivers/net/config |  1 +
 tools/testing/selftests/drivers/net/hds.py | 65 ++++++++++++++++++++++
 2 files changed, 66 insertions(+)

diff --git a/tools/testing/selftests/drivers/net/config b/tools/testing/selftests/drivers/net/config
index 4838adf27fa1..29293e41c3fa 100644
--- a/tools/testing/selftests/drivers/net/config
+++ b/tools/testing/selftests/drivers/net/config
@@ -1,3 +1,4 @@
+CONFIG_BONDING=m
 CONFIG_CONFIGFS_FS=y
 CONFIG_DEBUG_INFO_BTF=y
 CONFIG_DEBUG_INFO_BTF_MODULES=n
diff --git a/tools/testing/selftests/drivers/net/hds.py b/tools/testing/selftests/drivers/net/hds.py
index 606e26d75951..5fe82b98c1fb 100755
--- a/tools/testing/selftests/drivers/net/hds.py
+++ b/tools/testing/selftests/drivers/net/hds.py
@@ -6,6 +6,7 @@ import os
 import random
 from typing import Union
 from lib.py import ksft_run, ksft_exit, ksft_eq, ksft_raises, KsftSkipEx
+from lib.py import ksft_disruptive
 from lib.py import CmdExitFailure, EthtoolFamily, NlError
 from lib.py import NetDrvEnv
 from lib.py import defer, ethtool, ip
@@ -28,6 +29,21 @@ from lib.py import defer, ethtool, ip
     ip("link set dev %s xdp off" % cfg.ifname)
 
 
+def _xdp_attach(cfg, ifname):
+    """Attach a single-buffer XDP program, detach it when the test ends."""
+    prog = cfg.net_lib_dir / "xdp_dummy.bpf.o"
+    ip(f"link set dev {ifname} xdp obj {prog} sec xdp")
+    defer(ip, f"link set dev {ifname} xdp off")
+
+
+def _hds_enable_expect_fail(cfg, netnl):
+    """Enabling HDS must be refused while single-buffer XDP is attached."""
+    with ksft_raises(NlError) as e:
+        netnl.rings_set({'header': {'dev-index': cfg.ifindex},
+                         'tcp-data-split': 'enabled'})
+    ksft_eq(e.exception.nl_msg.error, -errno.EINVAL)
+
+
 def _ioctl_ringparam_modify(cfg, netnl) -> None:
     """
     Helper for performing a hopefully unimportant IOCTL SET.
@@ -240,6 +256,53 @@ from lib.py import defer, ethtool, ip
         _xdp_onoff(cfg)
 
 
+def xdp_set_hds(cfg, netnl) -> None:
+    """
+    Enable single-buffer XDP on the device, then try to enable HDS.
+    The mirror of enabled_set_xdp(): HDS must be refused.
+    """
+    mode = _get_hds_mode(cfg, netnl)
+    _defer_reset_hds(cfg, netnl)
+    if mode == 'enabled':
+        netnl.rings_set({'header': {'dev-index': cfg.ifindex},
+                         'tcp-data-split': 'unknown'})
+
+    _xdp_attach(cfg, cfg.ifname)
+
+    _hds_enable_expect_fail(cfg, netnl)
+
+
+@ksft_disruptive
+def xdp_bond_set_hds(cfg, netnl) -> None:
+    """
+    Like xdp_set_hds(), but the program is installed on a bond and pushed
+    down to the device rather than attached to it directly. The device is
+    running it either way, so HDS must be refused all the same.
+    """
+    mode = _get_hds_mode(cfg, netnl)
+    _defer_reset_hds(cfg, netnl)
+    if mode == 'enabled':
+        netnl.rings_set({'header': {'dev-index': cfg.ifindex},
+                         'tcp-data-split': 'unknown'})
+
+    ip("link add hds-bond type bond mode active-backup")
+    defer(ip, "link del hds-bond")
+
+    # bonding refuses to enslave a device which is up
+    ip(f"link set dev {cfg.ifname} down")
+    defer(ip, f"link set dev {cfg.ifname} up")
+    ip(f"link set dev {cfg.ifname} master hds-bond")
+    defer(ip, f"link set dev {cfg.ifname} nomaster")
+    ip("link set dev hds-bond up")
+
+    try:
+        _xdp_attach(cfg, "hds-bond")
+    except CmdExitFailure:
+        raise KsftSkipEx("device can't be an XDP bond slave")
+
+    _hds_enable_expect_fail(cfg, netnl)
+
+
 def ioctl(cfg, netnl) -> None:
     mode1 = _get_hds_mode(cfg, netnl)
     _ioctl_ringparam_modify(cfg, netnl)
@@ -290,6 +353,8 @@ from lib.py import defer, ethtool, ip
                   set_hds_thresh_gt,
                   set_xdp,
                   enabled_set_xdp,
+                  xdp_set_hds,
+                  xdp_bond_set_hds,
                   ioctl,
                   ioctl_set_xdp,
                   ioctl_enabled_set_xdp],
-- 
2.55.0

[PATCH net-next 4/5] selftests/bpf: check XDP attach on a nested bond slave

[Jakub Kicinski]

test_xdp_bonding_attach() already covers a flat bond both ways round:
the master is refused while a slave has a program, and a slave is
refused while the master has one. The nested test only ever checked
that attaching to the outermost master succeeds.

That left the more interesting half of the nesting untested. Only the
direct upper is consulted when deciding whether a device is already
running XDP, so with bond -> bond_nest1 -> bond_nest2 the check on
bond_nest2 looks at bond_nest1, and it only sees a program there if
bond_nest1 recorded the one it was handed. Before the preceding fix it
did not, and a second program could be attached to bond_nest2 while the
outer bond's program was already running on it.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 .../selftests/bpf/prog_tests/xdp_bonding.c     | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_bonding.c b/tools/testing/selftests/bpf/prog_tests/xdp_bonding.c
index c42488e445c2..486e0f9a2ada 100644
--- a/tools/testing/selftests/bpf/prog_tests/xdp_bonding.c
+++ b/tools/testing/selftests/bpf/prog_tests/xdp_bonding.c
@@ -464,8 +464,9 @@ static void test_xdp_bonding_attach(struct skeletons *skeletons)
 /* Test with nested bonding devices to catch issue with negative jump label count */
 static void test_xdp_bonding_nested(struct skeletons *skeletons)
 {
+	struct bpf_link *link2 = NULL;
 	struct bpf_link *link = NULL;
-	int bond, err;
+	int bond, nest2, err;
 
 	if (!ASSERT_OK(system("ip link add bond type bond"), "add bond"))
 		goto out;
@@ -488,10 +489,23 @@ static void test_xdp_bonding_nested(struct skeletons *skeletons)
 	if (!ASSERT_OK(err, "set bond_nest2 master"))
 		goto out;
 
+	nest2 = if_nametoindex("bond_nest2");
+	if (!ASSERT_GE(nest2, 0, "if_nametoindex bond_nest2"))
+		goto out;
+
 	link = bpf_program__attach_xdp(skeletons->xdp_dummy->progs.xdp_dummy_prog, bond);
-	ASSERT_OK_PTR(link, "attach program to master");
+	if (!ASSERT_OK_PTR(link, "attach program to master"))
+		goto out;
+
+	/* Attaching to a nested slave is not allowed either. Only the direct
+	 * upper is consulted, so this only holds if every device the program
+	 * was propagated to records it.
+	 */
+	link2 = bpf_program__attach_xdp(skeletons->xdp_dummy->progs.xdp_dummy_prog, nest2);
+	ASSERT_ERR_PTR(link2, "attach program to nested slave when master has program");
 
 out:
+	bpf_link__destroy(link2);
 	bpf_link__destroy(link);
 	system("ip link del bond");
 	system("ip link del bond_nest1");
-- 
2.55.0

[PATCH net-next 5/5] net: drop GSO skbs instead of handing them to XDP

[Jakub Kicinski]

skb BPF paths have a number of helpers to fix up the GSO
state after packet modifications. XDP has no such support.

"Real" / driver XDP runs before the SW GRO, and drivers generally
disable HW-GRO when XDP is attached (with the exception of IDPF,
story for another time), so this is not an issue. We also try
to disable HW-GRO and elide SW GRO when generic XDP prog is
attached. These precautions are not 100% today, and IMHO fixing
that is impossible. Let's explicitly drop GSO skbs on input to XDP.

First example how things can go sideways today - bonding.
We only disable & elide GRO on the device to which program is
attached. Nothing reaches the devices below it, and for a stacked
device that is where the packets come from:

  ip link set dev bond0 xdpgeneric obj prog.o sec xdp

leaves every slave with GRO on and no xdp_prog of its own, so
netif_elide_gro() lets them coalesce as usual. bond_handle_frame()
then returns RX_HANDLER_ANOTHER, __netif_receive_skb_core() loops back
round to another_round with skb->dev switched to the bond, and the
program gets handed a packet which was never on the wire. HW-GRO is
the same story - while netif_disable_lro() walks the lower devices,
its HW-GRO counterpart never did (this is largely the point of
distinguishing between LRO and HW-GRO).

Example two - veth. Generic XDP on a veth gets there by a different route.
veth_xdp_set() takes NETIF_F_GSO_SOFTWARE away from the peer so that
no GSO skb is ever built for a device running XDP, but generic XDP does
not go through ndo_bpf, so none of that happens.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 include/net/xdp.h  | 16 ++++++++++++++++
 drivers/net/veth.c |  3 +++
 net/core/dev.c     |  4 ++++
 3 files changed, 23 insertions(+)

diff --git a/include/net/xdp.h b/include/net/xdp.h
index aa742f413c35..5a234aab1e1c 100644
--- a/include/net/xdp.h
+++ b/include/net/xdp.h
@@ -291,6 +291,22 @@ static inline bool xdp_buff_add_frag(struct xdp_buff *xdp, netmem_ref netmem,
 	return true;
 }
 
+/**
+ * xdp_skb_feature_check() - can this skb be handed to an XDP program?
+ * @skb: skb about to be turned into an xdp_buff
+ *
+ * Return: true if the skb must not reach the program and has to be dropped.
+ */
+static inline bool xdp_skb_feature_check(const struct sk_buff *skb)
+{
+	if (likely(!skb_is_gso(skb)))
+		return false;
+
+	net_warn_ratelimited("%s: dropping coalesced packet before XDP, receive offloads are still on\n",
+			     skb->dev->name);
+	return true;
+}
+
 struct xdp_frame {
 	void *data;
 	u32 len;
diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index 71227d0389aa..5635037e3b60 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -756,6 +756,9 @@ static int veth_convert_skb_to_xdp_buff(struct veth_rq *rq,
 	struct sk_buff *skb = *pskb;
 	u32 frame_sz;
 
+	if (unlikely(xdp_skb_feature_check(skb)))
+		goto drop;
+
 	if (skb_shared(skb) || skb_head_is_locked(skb) ||
 	    skb_is_nonlinear(skb) ||
 	    skb_headroom(skb) < XDP_PACKET_HEADROOM) {
diff --git a/net/core/dev.c b/net/core/dev.c
index 096d1dedebfd..fbeb3df4c11f 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -158,6 +158,7 @@
 #include <linux/once_lite.h>
 #include <net/netdev_lock.h>
 #include <net/netdev_rx_queue.h>
+#include <net/xdp.h>
 #include <net/page_pool/types.h>
 #include <net/page_pool/helpers.h>
 #include <net/page_pool/memory_provider.h>
@@ -5516,6 +5517,9 @@ u32 bpf_prog_run_generic_xdp(struct sk_buff *skb, struct xdp_buff *xdp,
 	u32 metalen, act;
 	int off;
 
+	if (unlikely(xdp_skb_feature_check(skb)))
+		return XDP_DROP;
+
 	/* The XDP program wants to see the packet starting at the MAC
 	 * header.
 	 */
-- 
2.55.0

Re: [PATCH net-next 1/5] net: record XDP programs propagated to lower devices

[Stanislav Fomichev]

On 09/28, Jakub Kicinski wrote:
> An upper device installs its XDP program on each lower with
> dev_xdp_propagate(), which calls ndo_bpf() directly and records nothing
> in the lower's xdp_state[]. netif_xdp_propagate() open-codes two of the
> checks dev_xdp_install() makes, but the lower still looks program-free
> to everything else, so the checks made in the opposite direction miss it
> entirely:
> 
>   ethtool -G $slave tcp-data-split on
> 
> is refused by dev_xdp_sb_prog_count() for a device running a
> single-buffer XDP program, but not for a bond slave running the bond's,
> even though netif_xdp_propagate() would have refused to install that
> same program had header-data split been on already. Binding a memory
> provider has the same asymmetry. The lower does not report the program
> over rtnetlink either, so it is invisible to userspace as well.
> 
> Install it into xdp_state[] like any other program and mark it with
> xdp_from_upper, rather than tracking it separately. Every existing user
> of dev_xdp_prog_count() and dev_xdp_sb_prog_count() then gets the right
> answer with no changes. The flag is only needed where the owner matters:
> bonding reads it to tell a slave's own program apart from the one it
> pushed down, both to refuse enslaving such a device and to let the
> bond-wide program be replaced, and dev_xdp_attach() reads it to keep the
> program from being replaced or removed behind the upper's back.
> 
> The lower now holds its own reference, as it does for a program of its
> own; the upper's per-lower reference for the driver is unchanged.
> 
> While here, refuse to propagate onto a device which has a program of its
> own, matching dev_xdp_install(). Bonding checks this before enslaving
> and before installing, netvsc does not and would silently replace the
> VF's program.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Acked-by: Stanislav Fomichev <sdf@fomichev.me>

Re: [PATCH net-next 2/5] netdevsim: add ndo_xdp_xmit

[Stanislav Fomichev]

On 09/28, Jakub Kicinski wrote:
> Bonding refuses a slave which has no ndo_xdp_xmit once the bond runs an
> XDP program, so netdevsim can't currently be enslaved into an XDP bond
> and the propagation of XDP down to lower devices has no test coverage
> at all.
> 
> There is no data path behind it. netdevsim does not advertise
> NETDEV_XDP_ACT_NDO_XMIT, so nothing should be redirecting frames here;
> anything which does anyway is counted and freed rather than dropped
> silently.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Acked-by: Stanislav Fomichev <sdf@fomichev.me>

Re: [PATCH net-next 3/5] selftests: drv-net: check tcp-data-split against an already attached XDP

[Stanislav Fomichev]

On 09/28, Jakub Kicinski wrote:
> hds.py only ever tested one direction of the rule: with header-data
> split on, installing a single-buffer XDP program must fail. The other
> direction was not covered at all, and that is the one which was broken
> for a program an upper device pushed down - the device was running XDP
> but nothing in the core knew, so enabling tcp-data-split underneath it
> succeeded.
> 
> xdp_set_hds() covers the direct case, xdp_bond_set_hds() the propagated
> one. The latter fails without the preceding fix.
> 
> The bond case is disruptive, it takes the device down to enslave it, and
> skips on devices bonding will not take as an XDP slave.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Acked-by: Stanislav Fomichev <sdf@fomichev.me>

Re: [PATCH net-next 4/5] selftests/bpf: check XDP attach on a nested bond slave

[Stanislav Fomichev]

On 09/28, Jakub Kicinski wrote:
> test_xdp_bonding_attach() already covers a flat bond both ways round:
> the master is refused while a slave has a program, and a slave is
> refused while the master has one. The nested test only ever checked
> that attaching to the outermost master succeeds.
> 
> That left the more interesting half of the nesting untested. Only the
> direct upper is consulted when deciding whether a device is already
> running XDP, so with bond -> bond_nest1 -> bond_nest2 the check on
> bond_nest2 looks at bond_nest1, and it only sees a program there if
> bond_nest1 recorded the one it was handed. Before the preceding fix it
> did not, and a second program could be attached to bond_nest2 while the
> outer bond's program was already running on it.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Acked-by: Stanislav Fomichev <sdf@fomichev.me>

Re: [PATCH net-next 5/5] net: drop GSO skbs instead of handing them to XDP

[Stanislav Fomichev]

On 09/28, Jakub Kicinski wrote:
> skb BPF paths have a number of helpers to fix up the GSO
> state after packet modifications. XDP has no such support.
> 
> "Real" / driver XDP runs before the SW GRO, and drivers generally
> disable HW-GRO when XDP is attached (with the exception of IDPF,
> story for another time), so this is not an issue. We also try
> to disable HW-GRO and elide SW GRO when generic XDP prog is
> attached. These precautions are not 100% today, and IMHO fixing
> that is impossible. Let's explicitly drop GSO skbs on input to XDP.
> 
> First example how things can go sideways today - bonding.
> We only disable & elide GRO on the device to which program is
> attached. Nothing reaches the devices below it, and for a stacked
> device that is where the packets come from:
> 
>   ip link set dev bond0 xdpgeneric obj prog.o sec xdp
> 
> leaves every slave with GRO on and no xdp_prog of its own, so
> netif_elide_gro() lets them coalesce as usual. bond_handle_frame()
> then returns RX_HANDLER_ANOTHER, __netif_receive_skb_core() loops back
> round to another_round with skb->dev switched to the bond, and the
> program gets handed a packet which was never on the wire. HW-GRO is
> the same story - while netif_disable_lro() walks the lower devices,
> its HW-GRO counterpart never did (this is largely the point of
> distinguishing between LRO and HW-GRO).
> 
> Example two - veth. Generic XDP on a veth gets there by a different route.
> veth_xdp_set() takes NETIF_F_GSO_SOFTWARE away from the peer so that
> no GSO skb is ever built for a device running XDP, but generic XDP does
> not go through ndo_bpf, so none of that happens.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Acked-by: Stanislav Fomichev <sdf@fomichev.me>

Re: [PATCH net-next 1/5] net: record XDP programs propagated to lower devices

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 8 potential
issue(s) to consider.

Critical: 2 · High: 0 · Medium: 3 · Low: 3

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Critical] A new -EBUSY refusal in netif_xdp_propagate()
  (net/core/dev.c:10415) causes a double bpf_prog_put through netvsc's…
- [Critical] netif_xdp_propagate() only refuses a device's own program
  when bpf->prog is non-NULL (net/core/dev.c:10415).
- [Medium] Only two things clear xdp_from_upper: a later
  netif_xdp_propagate() call and dev_xdp_uninstall() at unregister.
- [Medium] dev_xdp_install() treats any non-HW program in xdp_state[mode]
  as a registered XDP dispatcher user: `if (mode != XDP_MODE_HW)…
- [Medium] On netvsc's ndo_bpf path, netvsc_bpf() → netvsc_vf_setxdp() →
  netif_xdp_propagate(vf_netdev) runs with only RTNL held…
- [Low] The commit fixes user-visible bugs but has no Fixes: tag.
- [Low] The patch adds the bitfield `unsigned long xdp_from_upper:1` to
  struct net_device but does not add it to…
- [Low] netvsc_register_vf() ignores the return value of
  netvsc_vf_setxdp(vf_netdev, prog) and returns NOTIFY_OK.

--

net: record XDP programs propagated to lower devices

When an upper device pushes its XDP program down with
netif_xdp_propagate(), this commit records the program in the lower's
xdp_state[] and tags it with a new xdp_from_upper bit. Bonding and
dev_xdp_attach() use the bit to tell a lower's own program apart from the
upper's. netif_xdp_propagate() now also refuses to propagate onto a device
that has a program of its own.

> is refused by dev_xdp_sb_prog_count() for a device running a
> single-buffer XDP program, but not for a bond slave running the bond's,
> even though netif_xdp_propagate() would have refused to install that
> same program had header-data split been on already. Binding a memory
> provider has the same asymmetry.

[Severity: Low]

Should this have a Fixes: tag?

The one-directional HDS check on the propagate side came from commit
2d46e481a9af ("net: disallow setup single buffer XDP when tcp-data-split
is enabled."). The memory provider side dates to the devmem work, for
example commit 170aafe35cb9 ("netdev: support binding dma-buf to
netdevice").

> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index d037faff7c44b..512e3a21d0bda 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -2517,6 +2517,12 @@ struct net_device {
>  	unsigned long		change_proto_down:1;
>  	unsigned long		netns_immutable:1;
>  	unsigned long		fcoe_mtu:1;
> +	/**
> +	 * @xdp_from_upper: the program in @xdp_state was installed by an
> +	 *	upper device with netif_xdp_propagate(); it belongs to the
> +	 *	upper and can't be replaced or removed through this device.
> +	 */
> +	unsigned long		xdp_from_upper:1;

[Severity: Low]

This isn't a bug, but should xdp_from_upper also be added to
Documentation/networking/net_cachelines/net_device.rst?

The comment at the top of struct net_device asks for that document to be
updated when fields are added. It already lists the neighbouring
change_proto_down, netns_immutable and fcoe_mtu bitfields.

[ ... ]

> diff --git a/net/core/dev.c b/net/core/dev.c
> index a8eb382f40caf..5c67e555f82d6 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c

[ ... ]

> @@ -10377,13 +10386,39 @@ u8 dev_xdp_sb_prog_count(struct net_device *dev)

[ ... ]

>  int netif_xdp_propagate(struct net_device *dev, struct netdev_bpf *bpf)
>  {
> +	struct bpf_prog *old_prog;
> +	int err;
> +
>  	if (!dev->netdev_ops->ndo_bpf)
>  		return -EOPNOTSUPP;
>  
> +	/* we have more work to do for setup, bypass for other commands */
> +	if (bpf->command != XDP_SETUP_PROG)
> +		return dev->netdev_ops->ndo_bpf(dev, bpf);
> +
> +	if (bpf->prog && dev_xdp_has_own_prog(dev)) {
> +		NL_SET_ERR_MSG(bpf->extack,
> +			       "unable to propagate XDP to device with an XDP program of its own");
> +		return -EBUSY;
> +	}

[Severity: Critical]

Can this new -EBUSY cause a double bpf_prog_put() through netvsc's error
path?

The VF can easily have a program of its own. dev_xdp_attach() on the VF
only refuses when the upper already has one, and netvsc has none yet:

  ip link set dev $VF xdp obj q.o
  ip link set dev $NETVSC xdp obj p.o

The second command then goes roughly like this:

dev_change_xdp_fd(netvsc)            bpf_prog_get()
  dev_xdp_install()                  bpf_prog_inc()
    netvsc_bpf()
      netvsc_xdp_set(dev, prog)      bpf_prog_add(num_chn - 1)
      netvsc_vf_setxdp()             inc, then put on error
        netif_xdp_propagate()        returns -EBUSY
      netvsc_xdp_set(dev, NULL)      bpf_prog_put() num_chn times
  dev_xdp_install()                  bpf_prog_put() on error
dev_change_xdp_fd()                  bpf_prog_put() on error

The rollback in netvsc_bpf() drops all num_chn chan_table references. That
includes the one dev_xdp_install() moved into the driver:

drivers/net/hyperv/netvsc_bpf.c:netvsc_xdp_set() {
    ...
	if (old_prog)
		for (i = 0; i < nvdev->num_chn; i++)
			bpf_prog_put(old_prog);
    ...
}

Then dev_xdp_install() puts that same reference again:

	err = bpf_op(dev, &xdp);
	if (err) {
		if (prog)
			bpf_prog_put(prog);
		return err;
	}

Wouldn't this free the program while the user's fd still points at it?

The imbalance in netvsc's error path is older. Before this patch, though,
only the HDS, memory provider or driver failure refusals could reach it.
This refusal makes it reachable with two ordinary commands.

[Severity: Low]

Related to the above, netvsc_register_vf() ignores the return value of
netvsc_vf_setxdp():

drivers/net/hyperv/netvsc_drv.c:netvsc_register_vf() {
    ...
	prog = netvsc_xdp_get(netvsc_dev);
	netvsc_vf_setxdp(vf_netdev, prog);

	return NOTIFY_OK;
}

Suppose a VF with its own program joins netvsc while netvsc has a
different one. One way this happens is moving the VF into netvsc's netns,
which keeps XDP state. The new -EBUSY is then dropped without any message.
The VF keeps running its own program, and netvsc's program never applies
to VF traffic.

Should this failure at least be logged?

>  	if (dev->cfg->hds_config == ETHTOOL_TCP_DATA_SPLIT_ENABLED &&
> -	    bpf->command == XDP_SETUP_PROG &&
>  	    bpf->prog && !bpf->prog->aux->xdp_has_frags) {

[ ... ]

> @@ -10395,7 +10430,23 @@ int netif_xdp_propagate(struct net_device *dev, struct netdev_bpf *bpf)
>  		return -EBUSY;
>  	}
>  
> -	return dev->netdev_ops->ndo_bpf(dev, bpf);
> +	err = dev->netdev_ops->ndo_bpf(dev, bpf);
> +	if (err)
> +		return err;
> +
> +	/* Record it like a program of our own, so that everything which asks
> +	 * whether XDP is running here gets the right answer. @xdp_from_upper
> +	 * keeps the two apart where it matters.
> +	 */
> +	old_prog = dev_xdp_prog(dev, XDP_MODE_DRV);
> +	if (bpf->prog)
> +		bpf_prog_inc(bpf->prog);
> +	dev_xdp_set_prog(dev, XDP_MODE_DRV, bpf->prog);

[Severity: Medium]

Does recording the upper's program here unbalance the XDP dispatcher
accounting?

dev_xdp_install() assumes every non-HW program in xdp_state[] was
registered with the dispatcher by this device:

	if (mode != XDP_MODE_HW)
		bpf_prog_change_xdp(dev_xdp_prog(dev, mode), prog);

Here the program goes into the lower's xdp_state[XDP_MODE_DRV] without a
matching bpf_prog_change_xdp(). Only the upper's own dev_xdp_install()
registered it.

In unregister_netdevice_many_notify(), dev_xdp_uninstall() runs before the
NETDEV_UNREGISTER notifier that makes bonding or netvsc release the lower.
So deleting an enslaved lower, for example with ip link del on a veth bond
slave, goes:

dev_xdp_uninstall(lower)
  dev_xdp_install(lower, XDP_MODE_DRV, ..., NULL)
    bpf_prog_change_xdp(upper_prog, NULL)
      bpf_dispatcher_remove_prog()

Won't this drop the dispatcher user that the bond added? The program would
lose its direct-call entry while the bond is still running it.

> +	dev->xdp_from_upper = !!bpf->prog;
> +	if (old_prog)
> +		bpf_prog_put(old_prog);

[Severity: Critical]

What happens here when bpf->prog is NULL and the lower has its own program
attached through a bpf_link?

The own-program check above is skipped for NULL requests:

	if (bpf->prog && dev_xdp_has_own_prog(dev)) {

In that case dev_xdp_prog() returns link->link.prog, and this block puts
it. That reference belongs to the link. dev_xdp_uninstall() follows this
rule and never puts a link's prog:

	link = dev_xdp_link(dev, mode);
	if (link)
		link->dev = NULL;
	else
		bpf_prog_put(prog);

dev_xdp_set_prog() also clears xdp_state[XDP_MODE_DRV].link here without
setting link->dev = NULL.

netvsc_register_vf() calls netvsc_vf_setxdp() unconditionally, with a NULL
program whenever netvsc has none. It runs from netvsc_probe() for existing
VFs (hv_netvsc reload) and from NETDEV_REGISTER after a netns move.
__dev_change_net_namespace() keeps XDP state. So a VF with a user-created
XDP link can get here. ndo_bpf() drops the driver's reference and this
bpf_prog_put() drops the link's.

Closing the link fd afterwards runs bpf_xdp_link_release().
dev_xdp_detach_link() returns -EINVAL and trips the WARN_ON, and
bpf_link_free() puts the already freed prog.

If the VF is unregistered first, dev_xdp_uninstall() never sees the link.
link->dev then points at a freed net_device, which later
bpf_xdp_link_release(), fill_link_info, show_fdinfo and
bpf_xdp_link_update() calls would use. bpf_xdp_link_update() would also
bypass the new xdp_from_upper check.

For a plain non-link program the refcounts balance, but the user's program
is still removed without any error. Doesn't that go against refusing to
override a device's own program?

[Severity: Medium]

Is anything serializing these new xdp_state[] and xdp_from_upper writes
against the memory provider bind path on netvsc's VF?

netvsc_bpf()->netvsc_vf_setxdp()->netif_xdp_propagate(vf_netdev) runs with
only RTNL held. netvsc_vf_setxdp() only does ASSERT_RTNL() and never takes
the VF's instance lock.

netif_xdp_propagate() otherwise expects the caller to hold that lock.
dev_xdp_propagate() takes netdev_lock_ops(), and
dev_get_min_mp_channel_count() asserts it.

The bind side, netdev_nl_bind_rx_doit()->__netif_mp_open_rxq(), holds only
the VF instance lock and reads the state this patch writes:

	if (dev_xdp_prog_count(dev)) {
		NL_SET_ERR_MSG(extack, "unable to custom memory provider to device with XDP program attached");
		return -EEXIST;
	}

With no common lock, a concurrent bind and netvsc XDP set could each pass
the other's check. Could that leave an ops-locked VF with both an XDP
program and a memory provider? The commit message says this patch now
prevents that combination.

The unlocked VF ndo_bpf() call from netvsc predates this patch.

> +
> +	return 0;
>  }
>  EXPORT_SYMBOL_GPL(netif_xdp_propagate);

[ ... ]

> @@ -10567,6 +10605,11 @@ static int dev_xdp_attach(struct net_device *dev, struct netlink_ext_ack *extack
>  		NL_SET_ERR_MSG(extack, "XDP_FLAGS_REPLACE is not specified");
>  		return -EINVAL;
>  	}
> +	/* the program belongs to an upper device */
> +	if (dev->xdp_from_upper) {
> +		NL_SET_ERR_MSG(extack, "Can't replace an XDP program installed by an upper device");
> +		return -EBUSY;
> +	}

[Severity: Medium]

Can xdp_from_upper stay set after the upper is gone? If so, this check
would refuse every attach, replace and detach on the lower.

Only a later netif_xdp_propagate() or dev_xdp_uninstall() clears the bit.
netvsc_remove() clears XDP only on the synthetic device:

	netvsc_xdp_set(net, NULL, NULL, nvdev);

Then netvsc_unregister_vf() unlinks the VF without propagating NULL:

	netdev_rx_handler_unregister(vf_netdev);
	netdev_upper_dev_unlink(vf_netdev, ndev);
	RCU_INIT_POINTER(net_device_ctx->vf_netdev, NULL);

After rmmod hv_netvsc, a hot remove or an unbind, the VF still has
xdp_state[XDP_MODE_DRV] set with xdp_from_upper=1. It has no upper and its
driver keeps running the program.

This check then returns -EBUSY for every request, including prog=NULL,
until the VF is unregistered. Before this patch the user could attach a new
program and then detach it to clear the stale one.

Bonding can end up in the same state. In __bond_release_one(), a failed
dev_xdp_propagate(slave_dev, NULL) only warns, and the unlink still
happens:

	if (dev_xdp_propagate(slave_dev, &xdp))
		slave_warn(bond_dev, slave_dev, "failed to unload XDP program\n");

dev_xdp_has_own_prog() also returns false for such a device, so a future
upper would propagate over the orphaned program.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928223648.2739371-1-kuba%40kernel.org

Re: [PATCH net-next 2/5] netdevsim: add ndo_xdp_xmit

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message overstates the testing gap.
- [Low] nsim_xdp_xmit() in drivers/net/netdevsim/netdev.c counts every
  frame as tx_dropped, frees it with xdp_return_frame(), and then…

--

netdevsim: add ndo_xdp_xmit

This commit adds a stub ndo_xdp_xmit callback to netdevsim. With it,
netdevsim can be enslaved to a bond that has an XDP program attached. The
stub counts every frame as tx_dropped and frees it with xdp_return_frame(),
and netdevsim still does not advertise NETDEV_XDP_ACT_NDO_XMIT.

> Bonding refuses a slave which has no ndo_xdp_xmit once the bond runs an
> XDP program, so netdevsim can't currently be enslaved into an XDP bond
> and the propagation of XDP down to lower devices has no test coverage
> at all.

[Severity: Low]
Is "no test coverage at all" accurate here?

tools/testing/selftests/bpf/prog_tests/xdp_bonding.c already enslaves veth
devices to bonds and exercises bond_xdp_set()->dev_xdp_propagate(). veth
implements both ndo_bpf and ndo_xdp_xmit. For example,
test_xdp_bonding_attach() checks that attaching to a slave fails once the
master has a program:

	link2 = bpf_program__attach_xdp(skeletons->xdp_dummy->progs.xdp_dummy_prog, veth);
	if (!ASSERT_ERR_PTR(link2, "attach program to slave when master has program"))

test_xdp_bonding_with_mode(), test_xdp_bonding_nested(),
test_xdp_bonding_features() and test_xdp_bonding_xmit_policy_compat() also
rely on propagation.

The actual gap looks narrower. The drv-net tests can't use their default
device, netdevsim, as an XDP bond slave. The follow-up xdp_bond_set_hds()
test in tools/testing/selftests/drivers/net/hds.py in this series depends
on that.

Could the commit message describe the gap more precisely?

> diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
> index eebc02ccc4a92..3790b6eb4083b 100644
> --- a/drivers/net/netdevsim/netdev.c
> +++ b/drivers/net/netdevsim/netdev.c
> @@ -185,6 +185,25 @@ static netdev_tx_t nsim_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  	return NETDEV_TX_OK;
>  }
>  
> +/* Do not advertise NETDEV_XDP_ACT_NDO_XMIT, no real datapath.
> + * This is a "stub" which allows netdevsim to be put under a bond with XDP.
> + */
> +static int nsim_xdp_xmit(struct net_device *dev, int n,
> +			 struct xdp_frame **frames, u32 flags)
> +{
> +	int i;
> +
> +	if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK))
> +		return -EINVAL;
> +
> +	for (i = 0; i < n; i++) {
> +		dev_dstats_tx_dropped(dev);
> +		xdp_return_frame(frames[i]);
> +	}
> +
> +	return n;

[Severity: Low]
Should nsim_xdp_xmit() return n here? The ndo_xdp_xmit kernel-doc in
include/linux/netdevice.h says:

 *	netdevice. Returns number of frames successfully transmitted, frames
 *	that got dropped are freed/returned via xdp_return_frame().
 *	Returns negative number, means general error invoking ndo, meaning
 *	no frames were xmit'ed and core-caller will free all frames.

Every frame is counted as tx_dropped, but returning n tells the caller that
all of them were sent. bq_xmit_all() in kernel/bpf/devmap.c would then
report sent == n and zero drops:

	trace_xdp_devmap_xmit(bq->dev_rx, dev, sent, cnt - sent, err);

bond_xdp_xmit() would also count each frame into nxmit as transmitted:

		err = slave_dev->netdev_ops->ndo_xdp_xmit(slave_dev, 1, frames1, flags);
		if (err < 1)
			break;

As a result, netdevsim's tx_dropped counter disagrees with the tracepoint
and the bond about the same frames. There is no double free, because
sent == n.

Normally the NETDEV_XDP_ACT_NDO_XMIT checks in __xdp_enqueue() and
is_valid_dst() block this path, and bond_xdp_set_features() clears the flag
when a slave lacks it. So it is only reachable in the "anything which does
anyway" case from the commit message. One example is frames already sitting
in a bond's devmap bulk queue while a netdevsim slave is being enslaved.

Would it be more consistent to count the drops and return 0 or a negative
errno such as -ENETDOWN, without freeing the frames, so the core frees and
accounts for them?

> +}
> +

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928223648.2739371-1-kuba%40kernel.org

Re: [PATCH net-next 3/5] selftests: drv-net: check tcp-data-split against an already attached XDP

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] In tools/testing/selftests/drivers/net/hds.py, xdp_bond_set_hds()
  (lines 298-301) wraps `_xdp_attach(cfg, "hds-bond")` in a bare `except…

--

selftests: drv-net: check tcp-data-split against an already attached XDP

This adds two hds.py cases covering the reverse direction of the
HDS/XDP rule: enabling tcp-data-split must fail once a single-buffer XDP
program is running on the device. xdp_set_hds() attaches the program
directly. xdp_bond_set_hds() installs it on an active-backup bond, which
pushes it down to the device.

> diff --git a/tools/testing/selftests/drivers/net/hds.py b/tools/testing/selftests/drivers/net/hds.py
> index 606e26d75951f..5fe82b98c1fbf 100755
> --- a/tools/testing/selftests/drivers/net/hds.py
> +++ b/tools/testing/selftests/drivers/net/hds.py

[ ... ]

> +@ksft_disruptive
> +def xdp_bond_set_hds(cfg, netnl) -> None:
> +    """
> +    Like xdp_set_hds(), but the program is installed on a bond and pushed
> +    down to the device rather than attached to it directly. The device is
> +    running it either way, so HDS must be refused all the same.
> +    """

[ ... ]

> +    ip("link set dev hds-bond up")
> +
> +    try:
> +        _xdp_attach(cfg, "hds-bond")
> +    except CmdExitFailure:
> +        raise KsftSkipEx("device can't be an XDP bond slave")

[Severity: Low]
Is this skip too broad? Any failure of "ip link set dev hds-bond xdp obj
... sec xdp" is reported as a SKIP here, and that includes failures in the
propagation path this test is meant to cover.

The skip message only matches one refusal in bond_xdp_set():

drivers/net/bonding/bond_main.c:bond_xdp_set() {
    ...
	if (!slave_dev->netdev_ops->ndo_bpf ||
	    !slave_dev->netdev_ops->ndo_xdp_xmit) {
		SLAVE_NL_ERR(dev, slave_dev, extack,
			     "Slave device does not support XDP");
		err = -EOPNOTSUPP;
    ...
}

The same command also fails if netif_xdp_propagate(), which the previous
patch rewrote, wrongly refuses the program:

net/core/dev.c:netif_xdp_propagate() {
    ...
	if (bpf->prog && dev_xdp_has_own_prog(dev)) {
		NL_SET_ERR_MSG(bpf->extack,
			       "unable to propagate XDP to device with an XDP program of its own");
		return -EBUSY;
	}
    ...
}

It also fails if the lower driver's ndo_bpf fails, or if the
hds_config or memory provider check rejects the program by mistake.

Now that netdevsim has ndo_xdp_xmit, the attach should always succeed
there. A failure on netdevsim would be a kernel regression, but this
test would report it as SKIP rather than FAIL.

Attach failures are also handled differently in the two new tests.
xdp_set_hds() calls _xdp_attach() without a guard, so a failure there is
reported as FAIL.

The regression named in the commit message (HDS accepted under a
propagated program) is still caught, because in that case the attach
succeeds and _hds_enable_expect_fail() fails. The selftests/bpf
xdp_bonding tests, which a later patch in the series extends with a
nested-bond case, cover some of the attach path too.

Could the skip be limited to the "Slave device does not support XDP"
case, for example by matching that extack text in the CmdExitFailure
output, or by checking the lower device's capabilities first? Other
failures would then be reported as FAIL.

> +
> +    _hds_enable_expect_fail(cfg, netnl)

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928223648.2739371-1-kuba%40kernel.org

Re: [PATCH net-next 4/5] selftests/bpf: check XDP attach on a nested bond slave

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message and the new in-code comment in
  test_xdp_bonding_nested()…
- [Low] In test_xdp_bonding_nested()
  (tools/testing/selftests/bpf/prog_tests/xdp_bonding.c:492-494) the new…

--

selftests/bpf: check XDP attach on a nested bond slave

This extends test_xdp_bonding_nested() so that it also attaches a
program to bond_nest2 after the outer bond already has one. The new
attach is expected to be refused. The commit message says the refusal
works because the direct upper (bond_nest1) records the program it was
handed.

> diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_bonding.c b/tools/testing/selftests/bpf/prog_tests/xdp_bonding.c
> index c42488e445c28..486e0f9a2adac 100644
> --- a/tools/testing/selftests/bpf/prog_tests/xdp_bonding.c
> +++ b/tools/testing/selftests/bpf/prog_tests/xdp_bonding.c

[ ... ]

> @@ -488,10 +489,23 @@ static void test_xdp_bonding_nested(struct skeletons *skeletons)
>  	if (!ASSERT_OK(err, "set bond_nest2 master"))
>  		goto out;
>  
> +	nest2 = if_nametoindex("bond_nest2");
> +	if (!ASSERT_GE(nest2, 0, "if_nametoindex bond_nest2"))
> +		goto out;

[Severity: Low]
Can this check ever fail? if_nametoindex() returns 0 on failure, and
ASSERT_GE(nest2, 0, ...) then evaluates 0 >= 0, which is true. So the
goto out is never taken.

If the lookup did fail, nest2 would be 0, and the attach below would
fail in bpf_xdp_link_attach() before any XDP attach restrictions are
checked:

net/core/dev.c:bpf_xdp_link_attach() {
    ...
	dev = dev_get_by_index(net, attr->link_create.target_ifindex);
	if (!dev) {
		rtnl_unlock();
		return -EINVAL;
	}
    ...
}

The later ASSERT_ERR_PTR(link2, ...) accepts any error. That -EINVAL
would therefore be reported as the nested slave being correctly
refused.

The same pattern already exists for bond. There, though, a bad ifindex
makes the positive ASSERT_OK_PTR() fail. Here it feeds a negative
assertion, so the result is a false pass.

Should this be ASSERT_GT(nest2, 0, ...) instead?

> +
>  	link = bpf_program__attach_xdp(skeletons->xdp_dummy->progs.xdp_dummy_prog, bond);
> -	ASSERT_OK_PTR(link, "attach program to master");
> +	if (!ASSERT_OK_PTR(link, "attach program to master"))
> +		goto out;
> +
> +	/* Attaching to a nested slave is not allowed either. Only the direct
> +	 * upper is consulted, so this only holds if every device the program
> +	 * was propagated to records it.
> +	 */
> +	link2 = bpf_program__attach_xdp(skeletons->xdp_dummy->progs.xdp_dummy_prog, nest2);
> +	ASSERT_ERR_PTR(link2, "attach program to nested slave when master has program");

[Severity: Low]
Do this comment and the commit message describe the check that
actually refuses this attach?

The commit message says:

  "Only the direct upper is consulted when deciding whether a device is
  already running XDP, so with bond -> bond_nest1 -> bond_nest2 the
  check on bond_nest2 looks at bond_nest1, and it only sees a program
  there if bond_nest1 recorded the one it was handed."

Attaching to bond propagates all the way down the chain:

bond_xdp_set(bond)
  dev_xdp_propagate(bond_nest1)
    bond_xdp_set(bond_nest1)
      dev_xdp_propagate(bond_nest2)
        netif_xdp_propagate()
          dev->xdp_from_upper = !!bpf->prog;

This sets xdp_from_upper on bond_nest2 itself. The nest2 attach then
stops in dev_xdp_attach() before the upper device walk:

	/* the program belongs to an upper device */
	if (dev->xdp_from_upper) {
		NL_SET_ERR_MSG(extack, "Can't replace an XDP program installed by an upper device");
		return -EBUSY;
	}

The netdev_for_each_upper_dev_rcu() loop, which would return -EEXIST
after looking at bond_nest1, is never reached. So bond_nest1 is not
consulted for this attach.

ASSERT_ERR_PTR() also checks only that the attach failed, not which
errno was returned. -EBUSY, -EEXIST and unrelated failures all pass the
same way. The test does still catch the pre-fix behaviour, where the
attach succeeded.

Could the comment and commit message describe the xdp_from_upper
refusal instead? And could the test check the expected errno, so the
two rejection paths can be told apart?

>  
>  out:
> +	bpf_link__destroy(link2);
>  	bpf_link__destroy(link);
>  	system("ip link del bond");
>  	system("ip link del bond_nest1");

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928223648.2739371-1-kuba%40kernel.org

Re: [PATCH net-next 5/5] net: drop GSO skbs instead of handing them to XDP

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 2 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] Regression in tun/tap with native XDP attached: guest and
  vnet_hdr TSO/USO traffic is now silently dropped.
- [Medium] netdevsim with XDP attached now drops TCP TSO traffic from its
  peer.
- [Medium] veth: explicitly dropped GSO packets bypass kfree_skb
  tracepoints
- [Low] The patch describes and fixes a user-visible bug: generic XDP is
  handed GRO-coalesced/GSO skbs, e.g. generic XDP on bond0 over GRO-…
- [Low] In the generic XDP path, the new GSO check sits in
  bpf_prog_run_generic_xdp(), which runs after…

--

net: drop GSO skbs instead of handing them to XDP

This commit adds xdp_skb_feature_check(). It drops GSO skbs, with a
ratelimited warning, before they can reach an XDP program. The check is
called from bpf_prog_run_generic_xdp() and veth_convert_skb_to_xdp_buff().
The motivation is stacked devices such as bonding, and generic XDP on veth,
where GRO-coalesced packets can still get to the program.

> These precautions are not 100% today, and IMHO fixing
> that is impossible. Let's explicitly drop GSO skbs on input to XDP.

[ ... ]

> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

[Severity: Low]
The commit message describes a bug that exists today: generic XDP on bond0
over GRO-enabled slaves, and generic XDP on veth. There is no Fixes: tag,
though. Should one be added if this is meant for net?

The change also affects callers that send GSO skbs to generic XDP on
purpose, such as tun and netdevsim. An origin reference would help stable
backporters decide where the change applies.

> diff --git a/net/core/dev.c b/net/core/dev.c
> index 5c67e555f82d6..8c9dcb4126bd3 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c

[ ... ]

> @@ -5551,6 +5552,9 @@ u32 bpf_prog_run_generic_xdp(struct sk_buff *skb, struct xdp_buff *xdp,
>  	u32 metalen, act;
>  	int off;
>  
> +	if (unlikely(xdp_skb_feature_check(skb)))
> +		return XDP_DROP;
> +

[Severity: Medium]
Will anyone chasing lost packets be able to see this drop?

The new branch jumps to the existing drop label. That label frees the
skb with consume_skb() and sets *pskb to NULL. consume_skb() fires the
consume_skb tracepoint, not kfree_skb, so dropwatch, perf on
skb:kfree_skb and similar tools never see these packets.

The caller, veth_xdp_rcv_skb(), bumps stats->rx_drops and then calls
kfree_skb(skb) on its drop path. By then skb is already NULL, so that
call is a no-op and the drop is still reported as a normal consume.

The generic XDP call site behaves differently. There the XDP_DROP
returned by bpf_prog_run_generic_xdp() reaches the do_drop label in
netif_receive_generic_xdp(), which uses kfree_skb(). So the two callers
of xdp_skb_feature_check() report the same policy drop in two different
ways. Returning -ENOMEM is also misleading, since nothing failed to
allocate here.

The consume_skb() at the drop label predates this patch; it covers the
skb_pp_cow_data() failure. That is arguably an error path as well.
Would it make sense to free with kfree_skb_reason() for the GSO case?
A dedicated drop reason for GSO skbs reaching XDP could then be used by
both veth and the generic XDP path.

[Severity: High]
Does this break tun/tap when a native XDP program is attached?

tun sends packets whose virtio_net_hdr has a gso_type to the generic XDP
path on purpose, because its native XDP can't handle GSO:

drivers/net/tun.c:tun_build_skb() {
    ...
	if (hdr->gso_type || !xdp_prog) {
		*skb_xdp = 1;
		return __tun_build_skb(tfile, alloc_frag, buf, buflen, len,
				       pad, metasize);
    ...
}

The comment in tun_get_user() documents this:

		/* For the packet that is not easy to be processed
		 * (e.g gso or jumbo packet), we will do it at after
		 * skb was created with generic XDP routine.
		 */

Once tun_vnet_hdr_tnl_to_skb() has set gso_size, the path becomes:

tun_get_user()
  do_xdp_generic()
    netif_receive_generic_xdp()
      bpf_prog_run_generic_xdp()
        xdp_skb_feature_check() -> XDP_DROP

The vhost-net batched path in tun_xdp_one() does the same:

	if (xdp_prog) {
		if (gso->gso_type) {
			skb_xdp = true;
			goto build;
		}
    ...
	if (skb_xdp) {
		ret = do_xdp_generic(xdp_prog, &skb);
		if (ret != XDP_PASS) {
			ret = 0;
			goto out;

tun treats any result other than XDP_PASS as consumed. tun_get_user()
returns total_len and tun_xdp_one() returns 0. QEMU or vhost therefore sees
success, but every guest TSO/USO frame is dropped. This happens even when
the program only returns XDP_PASS.

Unlike veth_xdp_set(), tun_xdp_set() does nothing to stop the writer from
producing GSO frames. Guest TX TSO is negotiated between the guest and the
VMM, so the admin has no receive offload to turn off. That makes the
"receive offloads are still on" warning misleading here.

The commit message says driver XDP "is not an issue", but tun's native XDP
depends on generic XDP for GSO frames. Would tun need its own handling,
such as segmenting or exempting these skbs?

[Severity: Medium]
Could this also blackhole TCP bulk traffic on netdevsim when XDP is
attached?

nsim_setup() advertises NETIF_F_TSO. nsim_start_xmit() only calls
skb_linearize() before forwarding the skb to the peer, so gso_size is
kept. The peer then runs its program through do_xdp_generic():

drivers/net/netdevsim/netdev.c:nsim_rcv() {
    ...
			if (skb->ip_summed == CHECKSUM_PARTIAL)
				skb_checksum_help(skb);
			ret = do_xdp_generic(xdp_prog, &skb);
			if (ret != XDP_PASS) {
				dev_dstats_rx_add(dev, skblen);
				continue;
    ...
}

netdevsim never clears TSO on the peer when XDP is installed. Every TSO
skb would now get XDP_DROP here and still be counted as received.

For CHECKSUM_PARTIAL GSO skbs, this combination already triggered
skb_warn_bad_offload() via skb_checksum_help() before this patch. The
existing ping.py TCP subtests only send a short string, so they are
unlikely to catch this.

[Severity: Low]
This isn't a bug, but should the check move to the top of
netif_receive_generic_xdp()?

GRO/GSO skbs are almost always nonlinear. By the time
bpf_prog_run_generic_xdp() runs, netif_receive_generic_xdp() has already
done this:

	if (skb_cloned(skb) || skb_is_nonlinear(skb) ||
	    skb_headroom(skb) < XDP_PACKET_HEADROOM) {
		if (netif_skb_check_for_xdp(pskb, xdp_prog))
			goto do_drop;
	}

If the program lacks xdp_has_frags, or the page pool copy would be too
large, netif_skb_check_for_xdp() falls back to
pskb_expand_head(..., GFP_ATOMIC) plus skb_linearize(). That means a
high-order atomic allocation and a full copy of a 64KB (or BIG TCP sized)
aggregate, and the skb is then dropped right away.

The veth hunk already places the check before skb_pp_cow_data().

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928223648.2739371-1-kuba%40kernel.org