Re: [PATCH bpf 2/2] selftests/bpf: add arena fault recovery tests

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] selftests/bpf: unguarded arena dereference in arena_fault breaks no_alu32/bpf_gcc
--

commit 35e23b7a99b662f29a7ebcdbbc98176bb46d1150
Author: Emil Tsalapatis <emil@etsalapatis.com>

selftests/bpf: add arena fault recovery tests

This commit adds two BPF selftests and a supporting test kernel module to
verify arena page fault recovery when accessing an unbacked arena page from
a program with no arena or a program with a different arena map.

> diff --git a/tools/testing/selftests/bpf/progs/arena_fault.c b/tools/testing/selftests/bpf/progs/arena_fault.c
> new file mode 100644
> index 0000000000000..1967b128fdf69
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/arena_fault.c
> @@ -0,0 +1,55 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#define BPF_NO_KFUNC_PROTOTYPES
> +#include <vmlinux.h>
> +#include <bpf/bpf_helpers.h>
> +#include "bpf_arena_common.h"
> +#include "../test_kmods/bpf_test_arena_fault.h"
> +
> +struct {
> +	__uint(type, BPF_MAP_TYPE_ARENA);
> +	__uint(map_flags, BPF_F_MMAPABLE);
> +	__uint(max_entries, 2);
> +} arena SEC(".maps");
> +
> +u64 __arena arena_pad;
> +volatile u64 observed;
> +
> +SEC("syscall")
> +int stash_and_free(void *ctx)
> +{
> +	u64 __arena *val;
> +
> +	val = bpf_arena_alloc_pages(&arena, NULL, 1, NUMA_NO_NODE, 0);
> +	if (!val)
> +		return 1;
> +
> +	*val = 42;
> +	bpf_test_arena_fault_stash((u64 *)val);
> +	bpf_arena_free_pages(&arena, val, 1);
> +	return 0;
> +}
> +
> +SEC("syscall")
> +int fault_stashed_wrong_arena(void *ctx)
> +{
> +	u64 __arena *val;
> +
> +	val = bpf_arena_alloc_pages(&arena, NULL, 1, NUMA_NO_NODE, 0);
> +	if (!val)
> +		return 1;
> +
> +	*val = 1;

[Severity: Medium]
Will stash_and_free() and fault_stashed_wrong_arena() fail BPF verification
under test_progs-no_alu32 and test_progs-bpf_gcc when __BPF_FEATURE_ADDR_SPACE_CAST
is not defined?

In tools/testing/selftests/bpf/libarena/include/bpf_arena_common.h, __arena
only includes __attribute__((address_space(1))) when
__BPF_FEATURE_ADDR_SPACE_CAST is defined:

#if defined(__BPF_FEATURE_ADDR_SPACE_CAST) && !defined(BPF_ARENA_FORCE_ASM)
#define __arena __attribute__((address_space(1))) __attribute__((btf_type_tag("arena")))
...
#else
#define __arena __attribute__((btf_type_tag("arena")))

When tools/testing/selftests/bpf/Makefile builds test_progs-no_alu32 (with
-mcpu=v2) or test_progs-bpf_gcc, __BPF_FEATURE_ADDR_SPACE_CAST is not
defined. Without __attribute__((address_space(1))), the compiler does not
emit a BPF_ADDR_SPACE_CAST instruction before dereferencing val in
*val = 42 and *val = 1.

Because bpf_arena_alloc_pages() returns void * in kernel BTF,
check_kfunc_call() in kernel/bpf/verifier.c leaves R0 typed as SCALAR_VALUE
until BPF_ADDR_SPACE_CAST converts it to PTR_TO_ARENA in check_alu_op().
Without that cast, check_mem_access() rejects the stores to *val as invalid
scalar memory accesses, causing arena_fault__open_and_load() in
test_wrong_arena() and test_no_arena() in
tools/testing/selftests/bpf/prog_tests/arena_fault.c to fail instead of
skipping.

Should the arena pointer dereferences in progs/arena_fault.c be guarded by
#if defined(__BPF_FEATURE_ADDR_SPACE_CAST) (with the runner skipping when
unsupported), similar to other arena selftests?

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