Yonghong Song <yonghong.song@xxxxxxxxx> writes: > On 8/27/25 8:37 AM, Puranjay Mohan wrote: >> Add selftests for testing the reporting of arena page faults through BPF >> streams. Two new bpf programs are added that read and write to an >> unmapped arena address and the fault reporting is verified in the >> userspace through streams. >> >> Signed-off-by: Puranjay Mohan <puranjay@xxxxxxxxxx> >> --- >> .../testing/selftests/bpf/prog_tests/stream.c | 33 +++++++++++++++- >> tools/testing/selftests/bpf/progs/stream.c | 39 +++++++++++++++++++ >> 2 files changed, 71 insertions(+), 1 deletion(-) >> >> diff --git a/tools/testing/selftests/bpf/prog_tests/stream.c b/tools/testing/selftests/bpf/prog_tests/stream.c >> index 36a1a1ebde692..8fdc83260ea14 100644 >> --- a/tools/testing/selftests/bpf/prog_tests/stream.c >> +++ b/tools/testing/selftests/bpf/prog_tests/stream.c >> @@ -41,6 +41,22 @@ struct { >> "([a-zA-Z_][a-zA-Z0-9_]*\\+0x[0-9a-fA-F]+/0x[0-9a-fA-F]+\n" >> "|[ \t]+[^\n]+\n)*", >> }, >> + { >> + offsetof(struct stream, progs.stream_arena_read_fault), >> + "ERROR: Arena READ access at unmapped address 0x.*\n" >> + "CPU: [0-9]+ UID: 0 PID: [0-9]+ Comm: .*\n" >> + "Call trace:\n" >> + "([a-zA-Z_][a-zA-Z0-9_]*\\+0x[0-9a-fA-F]+/0x[0-9a-fA-F]+\n" >> + "|[ \t]+[^\n]+\n)*", >> + }, >> + { >> + offsetof(struct stream, progs.stream_arena_write_fault), >> + "ERROR: Arena WRITE access at unmapped address 0x.*\n" >> + "CPU: [0-9]+ UID: 0 PID: [0-9]+ Comm: .*\n" >> + "Call trace:\n" >> + "([a-zA-Z_][a-zA-Z0-9_]*\\+0x[0-9a-fA-F]+/0x[0-9a-fA-F]+\n" >> + "|[ \t]+[^\n]+\n)*", >> + }, >> }; >> >> static int match_regex(const char *pattern, const char *string) >> @@ -63,6 +79,7 @@ void test_stream_errors(void) >> struct stream *skel; >> int ret, prog_fd; >> char buf[1024]; >> + char fault_addr[64] = {0}; > > You can replace '{0}' to '{}' so the whole array will be initialized. Ack! >> >> skel = stream__open_and_load(); >> if (!ASSERT_OK_PTR(skel, "stream__open_and_load")) >> @@ -85,6 +102,14 @@ void test_stream_errors(void) >> continue; >> } >> #endif >> +#if !defined(__x86_64__) && !defined(__aarch64__) >> + ASSERT_TRUE(1, "Arena fault reporting unsupported, skip."); >> + if (i == 2 || i == 3) { >> + ret = bpf_prog_stream_read(prog_fd, 2, buf, sizeof(buf), &ropts); >> + ASSERT_EQ(ret, 0, "stream read"); >> + continue; >> + } >> +#endif >> >> ret = bpf_prog_stream_read(prog_fd, BPF_STREAM_STDERR, buf, sizeof(buf), &ropts); >> ASSERT_GT(ret, 0, "stream read"); >> @@ -92,8 +117,14 @@ void test_stream_errors(void) >> buf[ret] = '\0'; >> >> ret = match_regex(stream_error_arr[i].errstr, buf); >> - if (!ASSERT_TRUE(ret == 1, "regex match")) >> + if (ret && (i == 2 || i == 3)) { >> + sprintf(fault_addr, "0x%lx", skel->bss->fault_addr); >> + ret = match_regex(fault_addr, buf); >> + } >> + if (!ASSERT_TRUE(ret == 1, "regex match")) { >> fprintf(stderr, "Output from stream:\n%s\n", buf); >> + fprintf(stderr, "Fault Addr: 0x%lx\n", skel->bss->fault_addr); > > This will fault addr even for i == 0 or i == 1 and those address may be confusing > for test 0/1. Will add a check before printing this. >> + } >> } >> >> stream__destroy(skel); >> diff --git a/tools/testing/selftests/bpf/progs/stream.c b/tools/testing/selftests/bpf/progs/stream.c >> index 35790897dc879..9de015ac3ced5 100644 >> --- a/tools/testing/selftests/bpf/progs/stream.c >> +++ b/tools/testing/selftests/bpf/progs/stream.c >> @@ -5,6 +5,7 @@ >> #include <bpf/bpf_helpers.h> >> #include "bpf_misc.h" >> #include "bpf_experimental.h" >> +#include "bpf_arena_common.h" >> >> struct arr_elem { >> struct bpf_res_spin_lock lock; >> @@ -17,10 +18,17 @@ struct { >> __type(value, struct arr_elem); >> } arrmap SEC(".maps"); >> >> +struct { >> + __uint(type, BPF_MAP_TYPE_ARENA); >> + __uint(map_flags, BPF_F_MMAPABLE); >> + __uint(max_entries, 1); /* number of pages */ >> +} arena SEC(".maps"); >> + >> #define ENOSPC 28 >> #define _STR "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" >> >> int size; >> +u64 fault_addr; >> >> SEC("syscall") >> __success __retval(0) >> @@ -76,4 +84,35 @@ int stream_syscall(void *ctx) >> return 0; >> } >> >> +SEC("syscall") >> +__success __retval(0) >> +int stream_arena_write_fault(void *ctx) >> +{ >> + struct bpf_arena *ptr = (void *)&arena; >> + u64 user_vm_start; >> + >> + barrier_var(ptr); > > Do we need this barrier_var()? I tried llvm20 and it works fine without the > above barrier_var(). As kumar explained in his reply, this is for making it build with GCC, without the barrier_var() GCC fails with: progs/stream.c: In function ‘stream_arena_write_fault’: progs/stream.c:91:76: error: array subscript ‘struct bpf_arena[0]’ is partly outside array bounds of ‘struct <anonymous>[1]’ [-Werror=array-bounds=] 91 | u64 user_vm_start = ((struct bpf_arena *)(uintptr_t)(void *)&arena)->user_vm_start; | ^~ progs/stream.c:25:3: note: object ‘arena’ of size 24 25 | } arena SEC(".maps"); | ^~~~~ GCC-BPF [test_progs-bpf_gcc] struct_ops_refcounted_fail__tail_call.bpf.o progs/stream.c: In function ‘stream_arena_read_fault’: progs/stream.c:103:85: error: array subscript ‘struct bpf_arena[0]’ is partly outside array bounds of ‘struct <anonymous>[1]’ [-Werror=array-bounds=] 103 | volatile u64 user_vm_start = ((struct bpf_arena *)(uintptr_t)(void *)&arena)->user_vm_start; | ^~ progs/stream.c:25:3: note: object ‘arena’ of size 24 25 | } arena SEC(".maps"); | ^~~~~ cc1: all warnings being treated as errors >> + user_vm_start = ptr->user_vm_start; >> + > > Remove this line. > >> + fault_addr = user_vm_start + 0xbeef; >> + *(u32 __arena *)(user_vm_start + 0xbeef) = 1; > > Simplify to *(u32 __arena *)fault = 1; I wanted it to compile to an instruction with *(u32 *)src + offset = 1, which was naive of me to think but now I will rewrite this in inline assembly to also test dst_reg == src_reg case which Kumar found out. Thanks, Puranjay