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

[netdev-bot+sashiko@kernel.org]

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 of "selftests/bpf: check replacing and
  detaching XDP on a bond through a link" leaves out two things a reader…

--

selftests/bpf: check replacing and detaching XDP on a bond through a link

This adds an xdp_bonding_link_update subtest. The subtest attaches a
program to a bond through a link, replaces it with bpf_link_update(), and
detaches it with bpf_link_detach(). It checks that the veth slave follows
each change and that the replaced program is released.

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

[Severity: Low]
This isn't a bug, but could the message name the fix this test guards?
The leak appears to have been fixed by commit 184b5ddf82d4 ("bonding: fix
the program leak when XDP is replaced"). That commit's message says the
selftest would come in the net-next series.

The message also leaves out that the slave-side assertions depend on
"net: record XDP programs propagated to lower devices", which is earlier
in this series. The previous patch, "selftests/bpf: check XDP attach on a
nested bond slave", does name that dependency.

bpf_xdp_query_id(veth, XDP_FLAGS_DRV_MODE, &id) reads
dev->xdp_state[XDP_MODE_DRV] through
rtnl_xdp_prog_drv()->dev_xdp_prog_id().

Before that patch, netif_xdp_propagate() only called ndo_bpf() and never
recorded the program on the lower device. A bond slave therefore always
reported id 0.

In test_xdp_bonding_link_update() in
tools/testing/selftests/bpf/prog_tests/xdp_bonding.c, the first slave
check jumps to out before the leak check runs:

	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;
	...
	fd = bpf_prog_get_fd_by_id(old_id);
	if (!ASSERT_EQ(fd, -ENOENT, "old program released") && fd >= 0)
		close(fd);

On a kernel that has 184b5ddf82d4 but not the propagation change, the test
fails at the first veth query and the leak assertion never runs.

Could the commit message reference both of these changes?

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