Re: [PATCH v3 net-next 06/15] selftests: xsk: move the AF_XDP test suite to selftests/net

[netdev-bot+sashiko@kernel.org]

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

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

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] Undisclosed relicensing.
- [Low] In tools/testing/selftests/net/lib/Makefile, the xskxceiver
  binary is only removed by `make clean` when bpftool happens to work at…

--

selftests: xsk: move the AF_XDP test suite to selftests/net

This commit moves the xskxceiver engine, the xsk userspace library, the
test definitions and the XDP program from selftests/bpf to
selftests/net/lib/xsk. It also moves test_xsk.sh and xsk_prereqs.sh to
selftests/net, and net/lib now builds xskxceiver only when a working
bpftool is found. The two ethtool ring helpers move from
bpf/network_helpers.c into xsk.c.

> diff --git a/tools/testing/selftests/net/lib/Makefile b/tools/testing/selftests/net/lib/Makefile
> index ff83603397d06..c26088da04892 100644
> --- a/tools/testing/selftests/net/lib/Makefile
> +++ b/tools/testing/selftests/net/lib/Makefile
> @@ -5,6 +5,15 @@ CFLAGS += -I../../../../../usr/include/ $(KHDR_INCLUDES)
>  # Additional include paths needed by kselftest.h
>  CFLAGS += -I../../
>  
> +# xskxceiver needs a working bpftool to generate its skeleton
> +BPFTOOL ?= bpftool
> +HAS_BPFTOOL := $(shell $(BPFTOOL) version >/dev/null 2>&1 && echo y)
> +ifeq ($(HAS_BPFTOOL),y)
> +COND_GEN_FILES += xskxceiver
> +else
> +$(warning excluding xskxceiver, bpftool not installed or not working)
> +endif
> +

[ ... ]

> @@ -13,6 +22,7 @@ TEST_FILES := \
>  
>  TEST_GEN_FILES := \
>  	$(patsubst %.c,%.o,$(wildcard *.bpf.c)) \
> +	$(COND_GEN_FILES) \
>  	csum \
>  	gro \
>  	xdp_helper \
> @@ -23,3 +33,28 @@ TEST_INCLUDES := $(wildcard py/*.py sh/*.sh)

[ ... ]

> +$(OUTPUT)/xskxceiver: $(XSK_SOURCES) $(XSK_HEADERS) $(XSK_SKEL) $(BPFOBJ)
> +	$(call msg,CC,,$@)
> +	$(Q)$(CC) -I$(top_srcdir)/tools/include $(CFLAGS) -I$(OUTPUT) \
> +		-I$(SCRATCH_DIR)/include \
> +		$(XSK_SOURCES) $(BPFOBJ) $(LDLIBS) $(XSK_LDLIBS) -o $@
> +
> +EXTRA_CLEAN += $(XSK_BPF_OBJ) $(XSK_SKEL)

[Severity: Low]
Will make clean leave lib/xskxceiver behind if bpftool can't be used
when clean runs?

HAS_BPFTOOL is evaluated when the Makefile is parsed. xskxceiver only
ends up in TEST_GEN_FILES if "$(BPFTOOL) version" succeeds during that
same make invocation.

The clean target in lib.mk removes:

	$(RM) -r $(TEST_GEN_PROGS) $(TEST_GEN_PROGS_EXTENDED) $(TEST_GEN_FILES) $(EXTRA_CLEAN)

Here EXTRA_CLEAN only lists the .bpf.o and the skeleton.

Say the binary was built with a working bpftool, and make clean later
runs where bpftool doesn't work. That can happen with a different PATH,
a BPFTOOL= override, or a cross environment. The intermediates get
removed, but $(OUTPUT)/xskxceiver stays.

test_xsk.sh only checks:

	if [ ! -x "./${XSKOBJ}" ]; then

So it would run the stale binary instead of skipping.

Would it work to add $(OUTPUT)/xskxceiver to EXTRA_CLEAN
unconditionally? The same Makefile logic is still there at the end of
the series.

> diff --git a/tools/testing/selftests/bpf/xsk.c b/tools/testing/selftests/net/lib/xsk/xsk.c
> similarity index 95%
> rename from tools/testing/selftests/bpf/xsk.c
> rename to tools/testing/selftests/net/lib/xsk/xsk.c
> index 25d568abf0f20..6bd32caf5d085 100644
> --- a/tools/testing/selftests/bpf/xsk.c
> +++ b/tools/testing/selftests/net/lib/xsk/xsk.c

[ ... ]

> @@ -779,3 +778,51 @@ void xsk_socket__delete(struct xsk_socket *xsk)
>  		close(xsk->fd);
>  	free(xsk);
>  }
> +
> +int get_hw_ring_size(char *ifname, struct ethtool_ringparam *ring_param)
> +{

[Severity: Medium]
Does this move change the licence of these helpers?

get_hw_ring_size() and set_hw_ring_size() come from
tools/testing/selftests/bpf/network_helpers.c, which has:

	// SPDX-License-Identifier: GPL-2.0-only

xsk.c starts with:

	// SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause)

Tushar Vyavahare added both helpers in commits 90a695c3d31e1c and
bee3a7b0762422 under GPL-2.0-only. After the move they are unchanged,
but they now sit in a file that offers its contents under LGPL-2.1 or
BSD-2-Clause, with no per-function licence note.

The commit message only says "Move the two ethtool ring helpers from
network_helpers into xsk.c". It doesn't mention the licence change, and
there is no Ack from the original author. This is still true at the end
of the series.

Could the helpers go into one of the GPL-2.0 files in the same
directory instead, such as test_xsk.c or xskxceiver.c? If they need to
stay in xsk.c, could the relicensing be stated explicitly, with an Ack
from the copyright holder?

> +	struct ifreq ifr = {0};
> +	int sockfd, err;

[ ... ]

> +int set_hw_ring_size(char *ifname, struct ethtool_ringparam *ring_param)
> +{

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261014184539.774708-1-maciej.fijalkowski%40intel.com