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
