On Mon, 11 Aug 2025 at 12:35, Puranjay Mohan <puranjay@xxxxxxxxxx> wrote:
>
> Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx> writes:
>
> > On Thu, 7 Aug 2025 at 15:25, <puranjay@xxxxxxxxxx> wrote:
> >>
> >> Yonghong Song <yonghong.song@xxxxxxxxx> writes:
> >>
> >> > On 8/6/25 1:58 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 | 24 ++++++++++++
> >> >> tools/testing/selftests/bpf/progs/stream.c | 37 +++++++++++++++++++
> >> >> 2 files changed, 61 insertions(+)
> >> >>
> >> >> diff --git a/tools/testing/selftests/bpf/prog_tests/stream.c b/tools/testing/selftests/bpf/prog_tests/stream.c
> >> >> index d9f0185dca61b..4bdde56de35b1 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)
> >> >> @@ -85,6 +101,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");
> >> >> diff --git a/tools/testing/selftests/bpf/progs/stream.c b/tools/testing/selftests/bpf/progs/stream.c
> >> >> index 35790897dc879..58ebff60cd96a 100644
> >> >> --- a/tools/testing/selftests/bpf/progs/stream.c
> >> >> +++ b/tools/testing/selftests/bpf/progs/stream.c
> >> >> @@ -1,10 +1,15 @@
> >> >> // SPDX-License-Identifier: GPL-2.0
> >> >> /* Copyright (c) 2025 Meta Platforms, Inc. and affiliates. */
> >> >> +#define BPF_NO_KFUNC_PROTOTYPES
> >> >
> >> > Do we have to defineBPF_NO_KFUNC_PROTOTYPES in the above? Without the above, we do not need
> >> > below extern bpf_res_spin_lock and bpf_res_spin_unlock.
> >> >
> >>
> >> If we don't define BPF_NO_KFUNC_PROTOTYPES then there are build failures
> >> for bpf_arena_alloc/free_pages() because the prototypes in vmlinux.h
> >> lack __arena attribute.
> >
> > I would address this by dropping the alloc/free.
> > Instead to work around "addr_space_cast insn in program without arena error",
> > insert a dummy store "ptr = &arena" in the program, where ptr is a
> > global void *.
> >
>
> I want to use alloc/free and not use a dummy address because because
> arena pointers are special as they are returned by alloc() with
> arena->user_vm_start added to them, and the
> bpf_prog_report_arena_violation() also adds back arena->user_vm_start to
> the 32 bit address received by the fault handler. If I use a random
> address in the bpf program, bpf_prog_report_arena_violation() will print
> a bogus address.
That is easy to address, you can simply cast &arena to struct
bpf_arena * to get the user_vm_start.
Then just deref user_vm_start + 0xdeadbeef.
Then we also have a stable address we can match in the regex.
It will also fix your problem of needing the alloc/free pair in the
first place, i.e. the lack of an arena map reference in the program.
Then we can drop this BPF_NO_KFUNC_PROTOTYPES kludge.
As Eduard pointed out, newer pahole already emits address_space tags in
kfuncs, so the vmlinux.h suppression shouldn't be needed either way.
>
> So, I think we should keep using alloc/free for this test because we
> want to test this arena->user_vm_start addition as well.
>
> Thanks,
> Puranjay
