[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

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

[Stanislav Fomichev]

On 11/12, Jakub Kicinski wrote:
> 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

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