Re: [PATCH bpf-next v4 3/3] selftests/bpf: Add tests for arena fault reporting

[Yonghong Song <yonghong.song@linux.dev>]

On 8/28/25 5:25 AM, Puranjay Mohan wrote:
> Yonghong Song <yonghong.song@linux.dev> 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@kernel.org>
>>> ---
>>>    .../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

It would be great if you can have the above in the commit message.

Please also put a small comments in the code such that
without barrier_var(), gcc will do some transformation like
   u64 user_vm_start = ((struct bpf_arena *)(uintptr_t)(void *)&arena)->user_vm_start;
and this will cause struct out-of-bound access due to the casting
from smaller map struct to larger "struct bpf_arena".
clang does not have this issue.

>
>>> +	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