Re: [PATCH net-next v5 5/5] selftests/net: Move the XDP feature test to drivers/net/hw

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 4

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The XDP_NDO_XMIT subtest fails on many drivers even when they
  work correctly, because the advertised mask is read before the XDP…
- [Low] The moved BPF object xdp_features.bpf.o is built by the shared
  rule in tools/testing/selftests/net/bpf.mk.
- [Low] run_feature() builds the local commands with shlex.join() (the
  probe at line 45, and the DUT command from feature_command() at line…
- [Low] tools/testing/selftests/bpf/.gitignore still has an
  `xdp_features` entry at line 44.
- [Low] The new README section heading 'XDP feature test' (16 characters)
  at tools/testing/selftests/drivers/net/README.rst:314 has a…

--

selftests/net: Move the XDP feature test to drivers/net/hw

This moves the xdp_features helper and its BPF program from selftests/bpf
to drivers/net/hw. It replaces the private veth shell script with a
NetDrvEpEnv Python test that runs the DUT locally and the tester remotely.
The new test compares detected features against the advertised mask with
ksft_eq().

> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 883c92701b9f1..cb76b68c97dbb 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile

[ ... ]

> @@ -142,7 +142,6 @@ TEST_GEN_PROGS_EXTENDED = \
>  	test_cpp \
>  	test_lirc_mode2_user \
>  	veristat \
> -	xdp_features \
>  	xdp_hw_metadata \
>  	xdp_synproxy \
>  	xskxceiver

[Severity: Low]
This isn't a bug, but tools/testing/selftests/bpf/.gitignore still has an
xdp_features entry. This patch removes the binary and its build rule from
this Makefile, and the ignore entry is now in drivers/net/hw/.gitignore.
Should the old entry be removed as well?

[ ... ]

> diff --git a/tools/testing/selftests/drivers/net/README.rst b/tools/testing/selftests/drivers/net/README.rst
> index 3fe49bce4f3a5..0791f02ba44b4 100644
> --- a/tools/testing/selftests/drivers/net/README.rst
> +++ b/tools/testing/selftests/drivers/net/README.rst
> @@ -310,3 +310,21 @@ Running tests CI-style
>  
>  See https://github.com/linux-netdev/nipa/wiki for instructions on how
>  to easily run the tests using ``virtme-ng``.
> +
> +XDP feature test
> +===============

[Severity: Low]
This isn't a bug, but the underline is one character shorter than the
title. "XDP feature test" is 16 characters and the underline is 15.
docutils warns "Title underline too short" for this.

[ ... ]

> diff --git a/tools/testing/selftests/drivers/net/hw/Makefile b/tools/testing/selftests/drivers/net/hw/Makefile
> index 18ce235a77f84..bd818e8625c23 100644
> --- a/tools/testing/selftests/drivers/net/hw/Makefile
> +++ b/tools/testing/selftests/drivers/net/hw/Makefile

[ ... ]

> @@ -94,6 +95,18 @@ include ../../../net/ynl.mk
>  
>  include ../../../net/bpf.mk
>  
> +$(OUTPUT)/xdp_features.bpf.o: xdp_features.h
> +
> +$(OUTPUT)/xdp_features: CFLAGS += -I$(OUTPUT) -I$(SCRATCH_DIR)/include
> +$(OUTPUT)/xdp_features: CFLAGS += -I../../../net/lib
> +$(OUTPUT)/xdp_features: LDLIBS += -lelf -lz -lpthread
> +$(OUTPUT)/xdp_features: xdp_features.c xdp_features.h \
> +			   $(OUTPUT)/xdp_features.skel.h $(BPFOBJ)
> +	$(call msg,BINARY,,$@)
> +	$(Q)$(CC) $(CFLAGS) $(CPPFLAGS) $(KHDR_INCLUDES) \
> +		$(filter %.c %.o,$^) $(BPFOBJ) \
> +		$(LDFLAGS) $(LDLIBS) -o $@

[Severity: Low]
Only the userspace helper gets KHDR_INCLUDES here. Can xdp_features.bpf.o
fail to build in an O= build?

The BPF object is built by the shared rule in
tools/testing/selftests/net/bpf.mk:

CCINCLUDE += -I$(selfdir)/bpf
CCINCLUDE += -I$(top_srcdir)/usr/include/
CCINCLUDE += -I$(SCRATCH_DIR)/include
...
	$(Q)$(CLANG) -O2 -g --target=bpf $(CCINCLUDE) $(CLANG_SYS_INCLUDES) \
	-c $< -o $@

With make O=<objdir> headers, tools/testing/selftests/Makefile sets
KHDR_INCLUDES to ${abs_objtree}/usr/include. That leaves
$(top_srcdir)/usr/include empty in a clean source tree.

The #include <linux/netdev.h> in xdp_features.bpf.c then falls back to the
system headers. Those don't have linux/netdev.h on distros whose UAPI
headers predate 6.3, for example Ubuntu 22.04.

The old selftests/bpf build passed -I$(APIDIR), and that directory has
tools/include/uapi/linux/netdev.h. Should the BPF compile for this object
also get KHDR_INCLUDES or -I$(APIDIR)?

[ ... ]

> diff --git a/tools/testing/selftests/drivers/net/hw/xdp_features.py b/tools/testing/selftests/drivers/net/hw/xdp_features.py
> new file mode 100755
> index 0000000000000..1d59772b2a4d0
> --- /dev/null
> +++ b/tools/testing/selftests/drivers/net/hw/xdp_features.py

[ ... ]

> +def feature_command(binary, feature, dut_addr, tester_addr, ifname,
> +                    tester=False):
> +    """Build the helper command with the configured endpoint addresses."""
> +    args = [str(binary)]
> +    if tester:
> +        args.append("-t")
> +    args += ["-f", feature, "-D", dut_addr]
> +    if tester:
> +        args += ["-C", dut_addr]
> +    args += ["-T", tester_addr, ifname]
> +    return shlex.join(args)
> +
> +
> +def run_feature(cfg, feature, ipver):
> +    """Run one feature with bounded helper lifetimes and verify its result."""
> +    cfg.require_cmd("timeout", local=False, remote=True)
> +    probe = cmd(shlex.join([str(cfg.xdp_features), "-q", cfg.ifname]),
> +                fail=False)

[ ... ]

> +    dut_cmd = feature_command(cfg.xdp_features, feature, dut_addr,
> +                              tester_addr, cfg.ifname)
> +    tester_cmd = feature_command(cfg.remote_xdp_features, feature, dut_addr,
> +                                 tester_addr, cfg.remote_ifname, tester=True)
> +
> +    with bkg(dut_cmd, exit_wait=True, ksft_ready=True):

[Severity: Low]
Is shlex.join() right for the local probe and dut_cmd? The local path of
cmd() in tools/testing/selftests/net/lib/py/utils.py does not use a shell:

            if shell is None and isinstance(comm, str) and ' ' in comm:
                comm = comm.split()

Suppose the resolved path to xdp_features contains a character that
shlex.quote() escapes, such as a space, '~', '$', '(' or '!'. The added
quotes are then passed to execve() as literal characters.

Would Popen() then raise FileNotFoundError and crash every subtest?

Only tester_cmd goes through a shell, over ssh. Could the local commands
pass an argument list instead?

> +        # Bound the remote process itself, not just the local SSH wait.
> +        result = cmd(f"timeout --signal=TERM --kill-after=5 90 {tester_cmd}",
> +                     host=cfg.remote, timeout=100)
> +
> +    output = ANSI_ESCAPE.sub("", result.stdout)
> +    match = RESULT.search(output)
> +    if not match:
> +        raise KsftFailEx(f"Unable to parse xdp_features output: {output}")
> +
> +    detected = match.group(1) is None
> +    advertised = match.group(2) is None
> +    ksft_eq(detected, advertised,
> +            comment=f"{feature}: detected and advertised support")

[Severity: Medium]
Can this report a false failure for XDP_NDO_XMIT on drivers that only
advertise it after an XDP program is attached?

tester_run() in xdp_features.c reads the advertised mask before it asks
the DUT to attach its program:

	err = send_and_recv_msg(sockfd, CMD_GET_XDP_CAP, &advertised_feature,
				sizeof(advertised_feature));
	...
	err = send_and_recv_msg(sockfd, CMD_START, NULL, 0);

dut_run() answers CMD_GET_XDP_CAP with a live bpf_xdp_query(). It only
calls dut_attach_xdp_prog() when CMD_START arrives.

Several drivers set NETDEV_XDP_ACT_NDO_XMIT in their XDP attach path. One
example is virtnet_xdp_set() in drivers/net/virtio_net.c:

		if (!old_prog)
			xdp_features_set_redirect_target(dev, true);

ixgbe, i40e, ice, igb, igc, stmmac, funeth, libeth and bnxt do something
similar.

On those drivers the tester sees NOT ADVERTISED. Once the program is
attached, though, the redirected frames still come back through
ndo_xdp_xmit, so the output is:

Feature XDP_NDO_XMIT: [DETECTED][NOT ADVERTISED]

The removed shell script only checked the helper's exit code. Wouldn't
this ksft_eq() now fail on a driver that works correctly? Could the
capability query be moved after CMD_START?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261010-xdp-v5-send-20261008-v5-0-de687cb67c40%40gmail.com