Re: [v6 09/10] selftests/hornet: Add a selftest for the Hornet LSM

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Medium] Makefile targets lack `$(OUTPUT)/` prefix, breaking out-of-tree builds.
- [Medium] Makefile hardcodes host architecture and kernel paths, breaking cross-compilation.
- [Medium] Makefile has an unconditional build dependency on kernel cryptographic signing keys.
- [Medium] BPF program uses `bpf_probe_read_str()` for a user-space pointer.
- [Medium] BPF program uses `BPF_CORE_READ()` to access tracepoint context fields.
--

commit 1e13c1e9492cd70a4a039a9763aae8236928bf0c
Author: Blaise Boscaccy <bboscaccy@linux.microsoft.com>
selftests/hornet: Add a selftest for the Hornet LSM

This selftest contains a testcase that utilizes light skeleton eBPF
loaders and exercises hornet's map validation.

> diff --git a/tools/testing/selftests/hornet/Makefile b/tools/testing/selftests/hornet/Makefile
> new file mode 100644
> index 000000000000..432bce59f54e
> --- /dev/null
> +++ b/tools/testing/selftests/hornet/Makefile
[ ... ]
> +BPF_CFLAGS := -target bpf \
> +	-D__TARGET_ARCH_$(ARCH) \
> +	-I/usr/include/$(shell uname -m)-linux-gnu \

Does this hardcoded include path break cross-compilation? If building for
arm64 on an x86 host, it looks like this will pull in x86 headers instead
of the target architecture headers.

> +	$(KHDR_INCLUDES)
> +
> +vmlinux.h:
> +	$(BPFTOOL) btf dump file /sys/kernel/btf/vmlinux format c > vmlinux.h

Does this hardcode the host kernel's BTF? This will fail in build
environments where the host kernel lacks BTF, or when cross-compiling
where the target kernel's built BTF ($(objtree)/vmlinux) should be used.

Also, shouldn't custom build rules like this use the $(OUTPUT)/ prefix?
Since this Makefile includes ../lib.mk, items in TEST_GEN_PROGS and
TEST_GEN_FILES are prefixed with $(OUTPUT)/. When building out-of-tree
(make O=dir), make might fail to match the expected $(OUTPUT)/ targets
for vmlinux.h, trivial.bpf.o, loader.h, and the other explicit rules.

> +
> +trivial.bpf.o: trivial.bpf.c vmlinux.h
> +	$(CLANG) $(CFLAGS) $(BPF_CFLAGS) -c $< -o $@
> +
> +loader.h: trivial.bpf.o
> +	$(BPFTOOL) gen skeleton -S -k $(CERTDIR)/signing_key.pem -i $(CERTDIR)/signing_key.x509 \

Does this create an unconditional dependency on kernel cryptographic signing
keys? If the target kernel configuration disables module signing or other
features that generate these specific keys, the files won't exist and the
build might fail unconditionally instead of gracefully skipping.

> +		-L $< name trivial > $@
[ ... ]
> diff --git a/tools/testing/selftests/hornet/trivial.bpf.c b/tools/testing/selftests/hornet/trivial.bpf.c
> new file mode 100644
> index 000000000000..d38c5b53ff93
> --- /dev/null
> +++ b/tools/testing/selftests/hornet/trivial.bpf.c
[ ... ]
> +SEC("tracepoint/syscalls/sys_enter_unlinkat")
> +int handle_enter_unlink(struct trace_event_raw_sys_enter *ctx)
> +{
> +	char filename[128] = { 0 };
> +	struct task_struct *task;
> +	unsigned long start_time = 0;
> +	int pid = bpf_get_current_pid_tgid() >> 32;
> +	char *pathname_ptr = (char *) BPF_CORE_READ(ctx, args[1]);

Should tracepoint arguments be accessed directly (e.g., ctx->args[1])
instead of using BPF_CORE_READ()? 

Passing the verifier-translated tracepoint context pointer (PTR_TO_CTX)
to memory-reading helpers (which BPF_CORE_READ expands to) is an
anti-pattern that defeats direct memory access optimizations and can
lead to verifier rejection.

> +
> +	bpf_probe_read_str(filename, sizeof(filename), pathname_ptr);

Will this fail to read the user-space pointer? On architectures with strict
user/kernel memory separation (like x86 with SMAP), bpf_probe_read_str()
cannot read user memory. Would bpf_probe_read_user_str() be more
appropriate here?

> +	task = (struct task_struct *)bpf_get_current_task();
> +	start_time = BPF_CORE_READ(task, start_time);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260429191431.2345448-1-bboscaccy@linux.microsoft.com?part=9