Re: [PATCH bpf-next v9 2/8] selftests/bpf: Add basic libarena scaffolding

[sashiko-bot@kernel.org]

Sashiko AI review found 4 potential issue(s):
- [Medium] Silent argument truncation in `arena_alloc_reserve` causing fewer pages to be reserved than requested.
- [Medium] Buffer overflow in userspace `mincore()` array sizing if `sysconf(_SC_PAGESIZE)` differs from kernel `PAGE_SIZE`.
- [Low] Build Dependency Tracking Failure for libarena headers.
- [Low] Makefile logic flaw leading to silent test omission on name collision (vpath anti-pattern).
--

commit 7876e5868c676854bd421cbb98140a84f94fa226
Author: Emil Tsalapatis <emil@etsalapatis.com>

selftests/bpf: Add basic libarena scaffolding

Add initial code and a Makefile for an arena-based BPF library. Modules
can be added just by including the source file in the library's src/
subdirectory.

> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 6ef6872adbc3..5855064e7f9c 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile

[ ... ]

> @@ -525,6 +526,7 @@ LINKED_BPF_OBJS := $(foreach skel,$(LINKED_SKELS),$($(skel)-deps))
>  LINKED_BPF_SRCS := $(patsubst %.bpf.o,%.c,$(LINKED_BPF_OBJS))
>  
>  HEADERS_FOR_BPF_OBJS := $(wildcard $(BPFDIR)/*.bpf.h)		\
> +			$(wildcard $(CURDIR)/libarena/include/*.[ch])	\
>  			$(addprefix $(BPFDIR)/,	bpf_core_read.h	\
>  			                        bpf_endian.h	\
>  						bpf_helpers.h	\

Does this code miss changes to the newly introduced headers?

The headers common.h and userspace.h are located in the nested
libarena/include/libarena/ subdirectory. Since GNU Make's wildcard function
does not perform recursive searches, this evaluates to empty. 

Modifications to the headers might silently fail to trigger a rebuild.

[ ... ]

> diff --git a/tools/testing/selftests/bpf/libarena/Makefile b/tools/testing/selftests/bpf/libarena/Makefile
> new file mode 100644
> index 000000000000..e85b3ad96890
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/libarena/Makefile

[ ... ]

> +# Scan src/ and selftests/ to generate the final binaries
> +LIBARENA_SOURCES = $(wildcard $(LIBARENA)/src/*.bpf.c) $(wildcard $(LIBARENA)/selftests/*.bpf.c)
> +LIBARENA_OBJECTS = $(notdir $(LIBARENA_SOURCES:.bpf.c=.bpf.o))

Could this cause test modules to be silently omitted on name collisions?

If a test module is added as selftests/foo.bpf.c when src/foo.bpf.c already
exists, notdir creates duplicate object names. Because of the vpath directive
used later in the Makefile, make will find the source file in src/ first and
compile it. 

The bpftool utility will link the identical object twice, completely ignoring
the test file in selftests/.

[ ... ]

> diff --git a/tools/testing/selftests/bpf/libarena/include/libarena/common.h b/tools/testing/selftests/bpf/libarena/include/libarena/common.h
> new file mode 100644
> index 000000000000..92b67b20ed15
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/libarena/include/libarena/common.h

[ ... ]

> +struct arena_alloc_reserve_args {
> +	u64 nr_pages;
> +};

Will this silently truncate the number of requested pages?

The userspace API structure exposes a 64-bit nr_pages argument, which is
passed directly to the bpf_arena_reserve_pages BPF kfunc. However, the
internal BPF kfunc signature strictly takes a 32-bit integer.

If userspace requests a reservation exceeding 32 bits, the value will be
truncated by the BPF calling convention.

[ ... ]

> diff --git a/tools/testing/selftests/bpf/libarena/include/libarena/userspace.h b/tools/testing/selftests/bpf/libarena/include/libarena/userspace.h
> new file mode 100644
> index 000000000000..0438a751d5fd
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/libarena/include/libarena/userspace.h

[ ... ]

> +static inline int libarena_get_globals_pages(int arena_get_globals_fd,
> +					     size_t arena_all_pages,
> +					     u64 *globals_pages)
> +{
> +	size_t pgsize = sysconf(_SC_PAGESIZE);
> +	void *arena_base;
> +	ssize_t i;
> +	u8 *vec;
> +	int ret;
> +
> +	ret = libarena_get_arena_base(arena_get_globals_fd, &arena_base);
> +	if (ret)
> +		return ret;
> +
> +	if (!arena_base)
> +		return -EINVAL;
> +
> +	vec = calloc(arena_all_pages, sizeof(*vec));
> +	if (!vec)
> +		return -ENOMEM;
> +
> +	if (mincore(arena_base, arena_all_pages * pgsize, vec) < 0) {

Can this cause a buffer overflow in the vec array?

The vec buffer is allocated to hold arena_all_pages bytes. The length passed
to the mincore syscall is calculated as arena_all_pages *
sysconf(_SC_PAGESIZE).

However, the kernel's mincore implementation determines the number of output
bytes by dividing the requested length by the kernel's internal page size. If
the userspace environment reports a page size larger than the kernel's, the
kernel could calculate a page count greater than arena_all_pages and write
past the end of the vec buffer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260426190338.4615-1-emil@etsalapatis.com?part=2