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

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