Re: [PATCH net-next v6 4/5] selftests/net: Add a shared BPF skeleton rule

[netdev-bot+sashiko]

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] Under LLVM=1, the host bpftool bootstrap rule in
  tools/testing/selftests/net/bpf.mk probes one compiler and builds with…
- [Low] The patch makes two changes outside its subject.

--

selftests/net: Add a shared BPF skeleton rule

This commit adds a shared %.skel.h rule to selftests/net/bpf.mk. When
BPFTOOL is not supplied, it also builds a host bootstrap bpftool. The
BPF compiler now takes KHDR_INCLUDES instead of the source-tree
usr/include, and the libbpf sub-make gets an absolute srctree.

> diff --git a/tools/testing/selftests/net/bpf.mk b/tools/testing/selftests/net/bpf.mk
> index a4f6755dd8941..016e32960280a 100644
> --- a/tools/testing/selftests/net/bpf.mk
> +++ b/tools/testing/selftests/net/bpf.mk
> @@ -6,8 +6,8 @@ BUILD_DIR := $(SCRATCH_DIR)/build
>  BPFDIR := $(top_srcdir)/tools/lib/bpf
>  APIDIR := $(top_srcdir)/tools/include/uapi
>  
> +CCINCLUDE += $(KHDR_INCLUDES)
>  CCINCLUDE += -I$(selfdir)/bpf
> -CCINCLUDE += -I$(top_srcdir)/usr/include/
>  CCINCLUDE += -I$(SCRATCH_DIR)/include
>  
>  BPFOBJ := $(BUILD_DIR)/libbpf/libbpf.a
> @@ -46,8 +46,37 @@ $(BPFOBJ): $(wildcard $(BPFDIR)/*.[ch] $(BPFDIR)/Makefile)		       \
>  	   $(APIDIR)/linux/bpf.h					       \
>  	   | $(BUILD_DIR)/libbpf
>  	$(call msg,MAKE,,$@)
> -	$(Q)$(MAKE) $(submake_extras) -C $(BPFDIR) OUTPUT=$(BUILD_DIR)/libbpf/ \
> +	$(Q)$(MAKE) $(submake_extras) -C $(BPFDIR) srctree=$(abspath $(top_srcdir)) \
> +		    OUTPUT=$(BUILD_DIR)/libbpf/ \
>  		    EXTRA_CFLAGS='-g -O0'				       \
>  		    DESTDIR=$(SCRATCH_DIR) prefix= all install_headers

[Severity: Low]
This isn't a bug, but the patch also changes two existing parts of
bpf.mk. CCINCLUDE now uses $(KHDR_INCLUDES) in place of
-I$(top_srcdir)/usr/include/, and the libbpf sub-make now gets an
absolute srctree.

Both changes fix behaviour that dates back to commit 3f584c211d8c
("selftests: net: extract BPF building logic from the Makefile").

The commit message does describe both changes. Would it be worth moving
them into their own patch, or adding a Fixes: tag, so they can be picked
up separately from the new skeleton rule?

>  
>  EXTRA_CLEAN += $(SCRATCH_DIR)
> +
> +# Build a host bpftool only when a test needs a generated skeleton.
> +HOSTCC ?= gcc
> +HOSTLD ?= ld
> +BPFTOOL_BUILD := $(BUILD_DIR)/bpftool

[ ... ]

> +$(BPFTOOL_BUILD)/bootstrap/bpftool: \
> +	$(wildcard $(top_srcdir)/tools/bpf/bpftool/*.[ch]) \
> +	$(top_srcdir)/tools/bpf/bpftool/Makefile \
> +	$(wildcard $(BPFDIR)/*.[ch] $(BPFDIR)/Makefile)
> +	$(call msg,MAKE,,$@)
> +	$(Q)mkdir -p $(BPFTOOL_BUILD)
> +	$(Q)$(MAKE) $(submake_extras) -C $(top_srcdir)/tools/bpf/bpftool \
> +		OUTPUT=$(BPFTOOL_BUILD)/ srctree=$(abspath $(top_srcdir)) \
> +		ARCH= CROSS_COMPILE= CC="$(HOSTCC)" LD="$(HOSTLD)" bootstrap

[Severity: Medium]
Can this bootstrap build fail with LLVM=1?

bpf.mk sets HOSTCC to gcc and HOSTLD to ld whatever LLVM=, LLVM_PREFIX
and LLVM_SUFFIX are, and it only hands them to the bpftool sub-make as
CC and LD. HOSTCC itself is never passed or exported, so inside the
sub-make tools/scripts/Makefile.include sets it from LLVM:

  $(call allow-override,HOSTCC,$(LLVM_PREFIX)clang$(LLVM_SUFFIX))
  $(call allow-override,HOSTAR,$(LLVM_PREFIX)llvm-ar$(LLVM_SUFFIX))
  $(call allow-override,HOSTLD,$(LLVM_PREFIX)ld.lld$(LLVM_SUFFIX))

bpftool then compiles the bootstrap objects and the bootstrap libbpf with
that HOSTCC, which is clang:

tools/bpf/bpftool/Makefile:
	$(QUIET_CC)$(HOSTCC) $(HOST_CFLAGS) -c -MMD $< -o $@
	...
		ARCH= CROSS_COMPILE= CC="$(HOSTCC)" LD="$(HOSTLD)" AR="$(HOSTAR)" \
		CFLAGS="$(LIBBPF_BOOTSTRAP_CFLAGS)" EXTRA_CFLAGS= $@ install_headers

The CC_NO_CLANG probe in Makefile.include, however, runs against $(CC).
That is the gcc forced on the command line, so the probe adds the
GCC-only warning:

  ifeq ($(CC_NO_CLANG), 1)
  EXTRA_WARNINGS += -Wstrict-aliasing=3

bpftool copies EXTRA_WARNINGS into CFLAGS, HOST_CFLAGS and
LIBBPF_BOOTSTRAP_CFLAGS, and tools/lib/bpf/Makefile adds -Werror. Clang
would then stop with:

  error: unknown warning option '-Wstrict-aliasing=3' [-Werror,-Wunknown-warning-option]

Commit 093b75ef5995 ("tools: Suppress request for warning options not
existent in clang") added the CC_NO_CLANG guard to avoid exactly this
error.

On a clang-only host the result is the same, because a missing gcc
leaves CC_NO_CLANG at 1. The libelf-zstd feature probe would also run
with gcc, not with the clang that does the linking.

The following command should hit this once "selftests/net: Move the XDP
feature test to drivers/net/hw" makes xdp_features depend on
xdp_features.skel.h:

  make LLVM=1 -C tools/testing/selftests TARGETS=drivers/net/hw

selftests/bpf and selftests/hid include tools/scripts/Makefile.include,
so their HOSTCC follows LLVM=. Could bpf.mk set its HOSTCC/HOSTLD
defaults the same way, or pass HOSTCC, HOSTLD and HOSTAR to the bpftool
sub-make as well as CC and LD?

[ ... ]

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