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

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