[PATCH net-next 0/5] net: make xdp config checks effective vs netif_xdp_propagate()

[PATCH net-next 0/5] net: make xdp config checks effective vs netif_xdp_propagate()

[Jakub Kicinski]

Bonding (and netvsc) try to auto-propagate XDP programs down
to their lower devices. These programs were installed directly
with .ndo_bpf and not recorded in struct net_device of the lowers.
This makes all the config checks for features incompatible with XDP
ineffective - HDS, memory providers etc. use dev_xdp_prog_count()
and do not call down to the driver to ask it whether it has XDP
installed.

Record the state appropriately. dev_xdp_prog_count() will now
"see" the propagated programs.

The issue was discovered when reworking HW-GRO and XDP interactions.
I was trying to make sure HW-GRO is reliably disabled with XDP,
and discovered we have a way to bypass all checks with bonding.
Not treating this as a fix since it's admin-only self-inflicted
damage that nobody is reporting being bothered by in real life.

Jakub Kicinski (5):
  net: don't safety-check device features for XDP uninstall
  net: move dev_xdp_set_link() and dev_xdp_set_prog() up
  net: record XDP programs propagated to lower devices
  selftests/bpf: check XDP attach on a nested bond slave
  selftests/bpf: check replacing and detaching XDP on a bond through a
    link

 .../networking/net_cachelines/net_device.rst  |   1 +
 include/linux/netdevice.h                     |   7 +
 drivers/net/bonding/bond_main.c               |   4 +-
 drivers/net/hyperv/netvsc_bpf.c               |   2 +-
 drivers/net/hyperv/netvsc_drv.c               |   5 +-
 net/core/dev.c                                | 133 ++++++++++++------
 .../selftests/bpf/prog_tests/xdp_bonding.c    | 120 +++++++++++++++-
 7 files changed, 224 insertions(+), 48 deletions(-)

-- 
2.56.0

[PATCH net-next 1/5] net: don't safety-check device features for XDP uninstall

[Jakub Kicinski]

Commit ad27ed7d2309 ("bpf, xdp: move offload check into dev_xdp_install()")
lifted the safety checks from dev_xdp_attach() to dev_xdp_install()
to cover more paths. Since all these checks are prog
related they are under one big "if (prog)".

Lift the pre-existing checks under "if (prog)" as well,
and do the same thing in netif_xdp_propagate().
This way we don't have to worry about triggering WARN()s
on cleanup paths mistakenly trying to un-install a program
when there is none, and failing because device has a memory
provider.

This is a refactoring / consistency cleanup, later patches
don't depend on this change.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/core/dev.c | 48 +++++++++++++++++++++++++++---------------------
 1 file changed, 27 insertions(+), 21 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index a230ecce9327..a5120da879d1 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -10580,17 +10580,23 @@ 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;
-	}
+	/* setup requires extra handling, bypass those for other commands */
+	if (bpf->command != XDP_SETUP_PROG)
+		return dev->netdev_ops->ndo_bpf(dev, bpf);
 
-	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;
+	if (bpf->prog) {
+		if (dev->cfg->hds_config == ETHTOOL_TCP_DATA_SPLIT_ENABLED &&
+		    !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);
@@ -10656,17 +10662,17 @@ static int dev_xdp_install(struct net_device *dev, enum bpf_xdp_mode mode,
 			NL_SET_ERR_MSG(extack, "BPF_XDP_CPUMAP programs can not be attached to a device");
 			return -EINVAL;
 		}
-	}
-
-	if (dev->cfg->hds_config == ETHTOOL_TCP_DATA_SPLIT_ENABLED &&
-	    prog && !prog->aux->xdp_has_frags) {
-		NL_SET_ERR_MSG(extack, "unable to install XDP to device using tcp-data-split");
-		return -EBUSY;
-	}
-
-	if (dev_get_min_mp_channel_count(dev)) {
-		NL_SET_ERR_MSG(extack, "unable to install XDP to device using memory provider");
-		return -EBUSY;
+		if (dev->cfg->hds_config == ETHTOOL_TCP_DATA_SPLIT_ENABLED &&
+		    !prog->aux->xdp_has_frags) {
+			NL_SET_ERR_MSG(extack,
+				       "unable to install XDP to device using tcp-data-split");
+			return -EBUSY;
+		}
+		if (dev_get_min_mp_channel_count(dev)) {
+			NL_SET_ERR_MSG(extack,
+				       "unable to install XDP to device using memory provider");
+			return -EBUSY;
+		}
 	}
 
 	memset(&xdp, 0, sizeof(xdp));
-- 
2.56.0

[PATCH net-next 2/5] net: move dev_xdp_set_link() and dev_xdp_set_prog() up

[Jakub Kicinski]

netif_xdp_propagate() will need dev_xdp_set_prog() in the next patch.
Move both helpers above it, no functional change.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/core/dev.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index a5120da879d1..69ee4342f43e 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -10575,6 +10575,20 @@ u8 dev_xdp_sb_prog_count(struct net_device *dev)
 	return count;
 }
 
+static void dev_xdp_set_link(struct net_device *dev, enum bpf_xdp_mode mode,
+			     struct bpf_xdp_link *link)
+{
+	dev->xdp_state[mode].link = link;
+	dev->xdp_state[mode].prog = NULL;
+}
+
+static void dev_xdp_set_prog(struct net_device *dev, enum bpf_xdp_mode mode,
+			     struct bpf_prog *prog)
+{
+	dev->xdp_state[mode].link = NULL;
+	dev->xdp_state[mode].prog = prog;
+}
+
 int netif_xdp_propagate(struct net_device *dev, struct netdev_bpf *bpf)
 {
 	if (!dev->netdev_ops->ndo_bpf)
@@ -10610,20 +10624,6 @@ u32 dev_xdp_prog_id(struct net_device *dev, enum bpf_xdp_mode 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)
-{
-	dev->xdp_state[mode].link = link;
-	dev->xdp_state[mode].prog = NULL;
-}
-
-static void dev_xdp_set_prog(struct net_device *dev, enum bpf_xdp_mode mode,
-			     struct bpf_prog *prog)
-{
-	dev->xdp_state[mode].link = NULL;
-	dev->xdp_state[mode].prog = prog;
-}
-
 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)
-- 
2.56.0

[PATCH net-next 3/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 other 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 and netvsc read it to tell a lower's own program apart from the
one they pushed down, to refuse taking over such a device, and to let
their own program be replaced and taken back, 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 and its own registration with the
XDP dispatcher, as it does for a program of its own, so that tearing it
down through dev_xdp_uninstall() when the lower goes away first stays
balanced. The upper's per-lower reference for the driver is unchanged.

An upper is expected to propagate NULL before it unlinks, but the lower
may refuse it - bonding and netvsc then only warn, and the program keeps
running. So when the master unlinks, let the device own whatever is left
rather than leave it with nobody allowed to remove it.

While here, refuse to propagate onto a device which has a program of its
own, the counterpart of the new check in dev_xdp_attach(). Bonding and
netvsc already check for this before taking a device over and before
installing.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
v2:
 - register the lower's program with the XDP dispatcher as well, so
   that dev_xdp_uninstall() of the lower stays balanced
 - let the device own a program left behind when its master unlinks
 - refuse removing the device's own program too, not only replacing it
 - assert the lower's ops lock in netif_xdp_propagate()
 - convert netvsc's checks of its VF to dev_xdp_has_own_prog()
 - split out moving dev_xdp_set_link() and dev_xdp_set_prog() up
v1: https://lore.kernel.org/20260928223648.2739371-2-kuba@kernel.org
---
 .../networking/net_cachelines/net_device.rst  |  1 +
 include/linux/netdevice.h                     |  7 +++
 drivers/net/bonding/bond_main.c               |  4 +-
 drivers/net/hyperv/netvsc_bpf.c               |  2 +-
 drivers/net/hyperv/netvsc_drv.c               |  5 +-
 net/core/dev.c                                | 49 ++++++++++++++++++-
 6 files changed, 62 insertions(+), 6 deletions(-)

diff --git a/Documentation/networking/net_cachelines/net_device.rst b/Documentation/networking/net_cachelines/net_device.rst
index 8eceaa80b686..7620d3fcb8dc 100644
--- a/Documentation/networking/net_cachelines/net_device.rst
+++ b/Documentation/networking/net_cachelines/net_device.rst
@@ -169,6 +169,7 @@ unsigned_long:1                     see_all_hwtstamp_requests
 unsigned_long:1                     change_proto_down
 unsigned_long:1                     netns_immutable
 unsigned_long:1                     fcoe_mtu
+unsigned_long:1                     xdp_from_upper
 struct list_head                    net_notifier_list
 struct macsec_ops*                  macsec_ops
 struct udp_tunnel_nic_info*         udp_tunnel_nic_info
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index f18bafb4efe2..670fe869a875 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2531,6 +2531,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;
 
@@ -4445,6 +4451,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 9249a79a168e..167503d1fd15 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2327,7 +2327,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;
@@ -5736,7 +5736,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/drivers/net/hyperv/netvsc_bpf.c b/drivers/net/hyperv/netvsc_bpf.c
index 14bb520b44ed..fa6be9c71c81 100644
--- a/drivers/net/hyperv/netvsc_bpf.c
+++ b/drivers/net/hyperv/netvsc_bpf.c
@@ -228,7 +228,7 @@ int netvsc_bpf(struct net_device *dev, struct netdev_bpf *bpf)
 		/* The VF's own program is not ours: refuse to install over
 		 * it, as bonding does, and leave it be on removal.
 		 */
-		if (vf_netdev && dev_xdp_prog_count(vf_netdev)) {
+		if (vf_netdev && dev_xdp_has_own_prog(vf_netdev)) {
 			if (bpf->prog) {
 				NL_SET_ERR_MSG_MOD(extack,
 						   "VF has an XDP program of its own");
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 61109ff9927f..6fb5c21b88dc 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -2332,7 +2332,7 @@ static int netvsc_register_vf(struct net_device *vf_netdev, int context)
 		return NOTIFY_DONE;
 
 	prog = netvsc_xdp_get(netvsc_dev);
-	if (prog && dev_xdp_prog_count(vf_netdev)) {
+	if (prog && dev_xdp_has_own_prog(vf_netdev)) {
 		netdev_warn(ndev, "not using VF %s, it has an XDP program attached\n",
 			    vf_netdev->name);
 		return NOTIFY_DONE;
@@ -2485,7 +2485,8 @@ static int netvsc_unregister_vf(struct net_device *vf_netdev)
 	 * reach netvsc, but leave a program of the VF's own be.
 	 */
 	nvdev = rtnl_dereference(net_device_ctx->nvdev);
-	if (nvdev && netvsc_xdp_get(nvdev) && !dev_xdp_prog_count(vf_netdev)) {
+	if (nvdev && netvsc_xdp_get(nvdev) &&
+	    !dev_xdp_has_own_prog(vf_netdev)) {
 		netdev_lock_ops(vf_netdev);
 		ret = netvsc_vf_setxdp(vf_netdev, NULL);
 		netdev_unlock_ops(vf_netdev);
diff --git a/net/core/dev.c b/net/core/dev.c
index 69ee4342f43e..3a520c798d4a 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -9326,6 +9326,9 @@ static void __netdev_upper_dev_unlink(struct net_device *dev,
 
 	__netdev_adjacent_dev_unlink_neighbour(dev, upper_dev);
 
+	if (changeupper_info.master)
+		dev->xdp_from_upper = 0;
+
 	call_netdevice_notifiers_info(NETDEV_CHANGEUPPER,
 				      &changeupper_info.info);
 
@@ -10561,6 +10564,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)
 {
 	struct bpf_prog *prog;
@@ -10591,6 +10603,11 @@ static void dev_xdp_set_prog(struct net_device *dev, enum bpf_xdp_mode mode,
 
 int netif_xdp_propagate(struct net_device *dev, struct netdev_bpf *bpf)
 {
+	struct bpf_prog *old_prog;
+	int err;
+
+	netdev_assert_locked_ops_compat(dev);
+
 	if (!dev->netdev_ops->ndo_bpf)
 		return -EOPNOTSUPP;
 
@@ -10598,6 +10615,12 @@ int netif_xdp_propagate(struct net_device *dev, struct netdev_bpf *bpf)
 	if (bpf->command != XDP_SETUP_PROG)
 		return dev->netdev_ops->ndo_bpf(dev, bpf);
 
+	if (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 (bpf->prog) {
 		if (dev->cfg->hds_config == ETHTOOL_TCP_DATA_SPLIT_ENABLED &&
 		    !bpf->prog->aux->xdp_has_frags) {
@@ -10613,7 +10636,26 @@ int netif_xdp_propagate(struct net_device *dev, struct netdev_bpf *bpf)
 		}
 	}
 
-	return dev->netdev_ops->ndo_bpf(dev, bpf);
+	err = dev->netdev_ops->ndo_bpf(dev, bpf);
+	if (err)
+		return err;
+	if (bpf->prog)
+		bpf_prog_inc(bpf->prog);
+
+	/* 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. Register it with the
+	 * dispatcher too, dev_xdp_uninstall() will drop it from there if this
+	 * device goes away first.
+	 */
+	old_prog = dev_xdp_prog(dev, XDP_MODE_DRV);
+	bpf_prog_change_xdp(old_prog, 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);
 
@@ -10731,6 +10773,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,
@@ -10773,6 +10816,10 @@ static int dev_xdp_attach(struct net_device *dev, struct netlink_ext_ack *extack
 	}
 
 	mode = dev_xdp_mode(dev, flags);
+	if (dev->xdp_from_upper) {
+		NL_SET_ERR_MSG(extack, "Can't replace an XDP program installed by an upper device");
+		return -EBUSY;
+	}
 	/* can't replace attached link */
 	if (dev_xdp_link(dev, mode)) {
 		NL_SET_ERR_MSG(extack, "Can't replace active BPF XDP link");
-- 
2.56.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. With
bond -> bond_nest1 -> bond_nest2, bond_nest2 refuses a program of its
own only if the program the outer bond pushed down is recorded on it.
Before "net: record XDP programs propagated to lower devices" it was
recorded nowhere, and a second program could be attached to bond_nest2
while the outer bond's was already running on it. Check that the attach
is refused, and with -EBUSY, which is what the recorded program gives.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 .../selftests/bpf/prog_tests/xdp_bonding.c    | 24 ++++++++++++++++---
 1 file changed, 21 insertions(+), 3 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..ab0f01be4131 100644
--- a/tools/testing/selftests/bpf/prog_tests/xdp_bonding.c
+++ b/tools/testing/selftests/bpf/prog_tests/xdp_bonding.c
@@ -461,11 +461,15 @@ static void test_xdp_bonding_attach(struct skeletons *skeletons)
 	system("ip link del vxlan");
 }
 
-/* Test with nested bonding devices to catch issue with negative jump label count */
+/*
+ * Test with nested bonding devices to catch issue with negative jump label
+ * count, and check that a nested slave refuses a program of its own.
+ */
 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 +492,24 @@ 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_GT(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. The program the
+	 * outer bond pushed down is recorded there too.
+	 */
+	link2 = bpf_program__attach_xdp(skeletons->xdp_dummy->progs.xdp_dummy_prog, nest2);
+	ASSERT_EQ(libbpf_get_error(link2), -EBUSY,
+		  "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.56.0

[PATCH net-next 5/5] selftests/bpf: check replacing and detaching XDP on a bond through a link

[Jakub Kicinski]

The bond tests attach and destroy links, but never replace the program
behind one, or detach it without closing the link. Both only ever
operate on the device the link was created on, and rely on the bond to
pass the change down to its slaves.

Check that the slave follows the replacement and the detach, and that
the program which got replaced is released - the bond used to leak it.

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

diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_bonding.c b/tools/testing/selftests/bpf/prog_tests/xdp_bonding.c
index ab0f01be4131..05f2e8079858 100644
--- a/tools/testing/selftests/bpf/prog_tests/xdp_bonding.c
+++ b/tools/testing/selftests/bpf/prog_tests/xdp_bonding.c
@@ -516,6 +516,99 @@ static void test_xdp_bonding_nested(struct skeletons *skeletons)
 	system("ip link del bond_nest2");
 }
 
+/*
+ * Replace, then detach the program on a bond through its link. The link only
+ * knows about the bond, the slave has to follow it, and the replaced program
+ * has to be released.
+ */
+static void test_xdp_bonding_link_update(struct skeletons *skeletons)
+{
+	LIBBPF_OPTS(bpf_prog_load_opts, opts, .expected_attach_type = BPF_XDP);
+	struct bpf_program *new_prog = skeletons->xdp_dummy->progs.xdp_dummy_prog;
+	struct bpf_insn insns[] = {
+		BPF_MOV64_IMM(BPF_REG_0, XDP_PASS),
+		BPF_EXIT_INSN(),
+	};
+	int veth, bond, err, fd, old_fd = -1, link_fd = -1;
+	struct bpf_prog_info info = {};
+	__u32 old_id, new_id, id, len;
+
+	if (!ASSERT_OK(system("ip link add veth type veth"), "add veth"))
+		goto out;
+	if (!ASSERT_OK(system("ip link add bond type bond"), "add bond"))
+		goto out;
+
+	veth = if_nametoindex("veth");
+	if (!ASSERT_GT(veth, 0, "if_nametoindex veth"))
+		goto out;
+	bond = if_nametoindex("bond");
+	if (!ASSERT_GT(bond, 0, "if_nametoindex bond"))
+		goto out;
+
+	err = system("ip link set veth master bond");
+	if (!ASSERT_OK(err, "set veth master"))
+		goto out;
+
+	/* same attach type as the skeleton's, or the link won't take it */
+	old_fd = bpf_prog_load(BPF_PROG_TYPE_XDP, "xdp_old", "GPL", insns,
+			       ARRAY_SIZE(insns), &opts);
+	if (!ASSERT_GE(old_fd, 0, "load old program"))
+		goto out;
+	len = sizeof(info);
+	if (!ASSERT_OK(bpf_prog_get_info_by_fd(old_fd, &info, &len), "old program info"))
+		goto out;
+	old_id = info.id;
+
+	memset(&info, 0, sizeof(info));
+	len = sizeof(info);
+	err = bpf_prog_get_info_by_fd(bpf_program__fd(new_prog), &info, &len);
+	if (!ASSERT_OK(err, "new program info"))
+		goto out;
+	new_id = info.id;
+
+	link_fd = bpf_link_create(old_fd, bond, BPF_XDP, NULL);
+	if (!ASSERT_GE(link_fd, 0, "attach old program to bond"))
+		goto out;
+
+	err = bpf_xdp_query_id(veth, XDP_FLAGS_DRV_MODE, &id);
+	if (!ASSERT_OK(err, "query veth") ||
+	    !ASSERT_EQ(id, old_id, "veth runs the old program"))
+		goto out;
+
+	err = bpf_link_update(link_fd, bpf_program__fd(new_prog), NULL);
+	if (!ASSERT_OK(err, "replace the program through the link"))
+		goto out;
+
+	err = bpf_xdp_query_id(veth, XDP_FLAGS_DRV_MODE, &id);
+	if (!ASSERT_OK(err, "query veth") ||
+	    !ASSERT_EQ(id, new_id, "veth follows to the new program"))
+		goto out;
+
+	/* nothing but our fd should be holding on to the old program now */
+	close(old_fd);
+	old_fd = -1;
+	fd = bpf_prog_get_fd_by_id(old_id);
+	if (!ASSERT_EQ(fd, -ENOENT, "old program released") && fd >= 0)
+		close(fd);
+
+	err = bpf_link_detach(link_fd);
+	if (!ASSERT_OK(err, "detach the link"))
+		goto out;
+
+	err = bpf_xdp_query_id(veth, XDP_FLAGS_DRV_MODE, &id);
+	if (!ASSERT_OK(err, "query veth"))
+		goto out;
+	ASSERT_EQ(id, 0, "veth has no program left");
+
+out:
+	if (link_fd >= 0)
+		close(link_fd);
+	if (old_fd >= 0)
+		close(old_fd);
+	system("ip link del veth");
+	system("ip link del bond");
+}
+
 /*
  * Test that XDP redirect via xdp_master_redirect() does not crash when
  * the bond master device is not up. When bond is in round-robin mode but
@@ -826,6 +919,9 @@ void serial_test_xdp_bonding(void)
 	if (test__start_subtest("xdp_bonding_nested"))
 		test_xdp_bonding_nested(&skeletons);
 
+	if (test__start_subtest("xdp_bonding_link_update"))
+		test_xdp_bonding_link_update(&skeletons);
+
 	if (test__start_subtest("xdp_bonding_features"))
 		test_xdp_bonding_features(&skeletons);
 
-- 
2.56.0

[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 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

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 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