Re: [PATCH bpf-next v1 5/7] selftests/bpf: don't report arena tests as passing with an empty body

[bot+bpf-ci]

> selftests/bpf: don't report arena tests as passing with an empty body

TL;DR: Commit message only: it cites arena_types to justify running arena
kfunc tests without a cast, but the address args are __ign (ARG_IGNORE) and
never checked against it. Conclusion looks right; the explanation may need
rewording.

> selftests/bpf: don't report arena tests as passing with an empty body
>
> Some arena tests put the __BPF_FEATURE_ADDR_SPACE_CAST guard inside the
> function rather than around it, so without arena support the program
> still exists, its body is empty, and __success __retval(0) is met by
> doing nothing.  A reader sees
>
>   #584/1	 verifier_arena/basic_alloc1_nosleep:OK
>
> and concludes arena works.  That is worse than the tests which vanish
> entirely, since nothing hints anything is missing.
>
> The guard was never needed to compile those bodies: without the
> feature __arena is still btf_type_tag("arena"), which every compiler
> accepts.  What it avoided was the load failure that follows from no
> addr_space_cast being emitted — but only for a body that dereferences
> an arena pointer.  The arena kfuncs take one as a plain scalar:
>
>   static const struct bpf_reg_types arena_types = {
> 	  .types = { PTR_TO_ARENA, SCALAR_VALUE }
>
> so a test that only passes pointers to bpf_arena_{alloc,free,reserve}_pages()
> needs no cast and runs fine.
>
> test_loader checks skip_reason before it opens anything, so the body no
> longer has to be empty to stay loadable: drop all 19 in-body guards.
> They do not all need a tag in their place, which is why 19 guards go
> and 7 tags arrive:
>
>   6   dereference, so they get __skip_without_arena and report SKIP:
>       basic_alloc1{,_nosleep}, basic_alloc2{,_nosleep}, big_alloc1,
>       access_reserved
>   11  never dereference, so they get no tag and run for real instead
>       of reporting a vacuous OK
>   2   big_alloc3 and big_alloc4, already inside the file's outer
>       whole-program guard, so the in-body one was redundant
>
> The 7th tag goes on check_arena_arg_ret(), which has no guard at all
> and is the one verifier_arena subtest that really fails under a stock
> GCC.
>
> Measured on bpf-next with a stock bpf-gcc, test_progs before and after:
>
>   verifier_arena/basic_alloc1            OK   -> SKIP
>   verifier_arena/basic_alloc1_nosleep    OK   -> SKIP
>   verifier_arena/basic_alloc2            OK   -> SKIP
>   verifier_arena/basic_alloc2_nosleep    OK   -> SKIP
>   verifier_arena_large/big_alloc1        OK   -> SKIP
>   verifier_arena_large/access_reserved   OK   -> SKIP
>   verifier_arena/check_arena_arg_ret     FAIL -> SKIP
>
> the six that dereference stop claiming to pass, and check_arena_arg_ret
> stops failing.  The eleven that do not dereference stay OK, having gone
> from an empty body to actually running.
>
> Removing the guards exposes the arena_base() calls they were hiding.
> The previous patch made that macro opaque to GCC's -Warray-bounds, so
> nothing is needed here beyond the guards going away.
>
> Signed-off-by: Vineet Gupta <vineet.gupta@xxxxxxxxx>

The paragraph that says "The arena kfuncs take one as a plain scalar" and
quotes arena_types: is that the check that actually lets these tests run
without an addr_space_cast?

bpf_arena_alloc_pages(), bpf_arena_free_pages() and
bpf_arena_reserve_pages() declare their address argument with the __ign
suffix in kernel/bpf/arena.c:

  __bpf_kfunc void *bpf_arena_alloc_pages(void *p__map, void *addr__ign, ...)
  __bpf_kfunc void bpf_arena_free_pages(void *p__map, void *ptr__ign, ...)
  __bpf_kfunc int bpf_arena_reserve_pages(void *p__map, void *ptr__ign, ...)

get_kfunc_arg_type() tests the __ign suffix first and returns ARG_IGNORE:

  if (is_kfunc_arg_ignore(meta->btf, &args[arg]) || is_kfunc_arg_implicit(meta, arg))
          return ARG_IGNORE;

and check_func_arg() returns for that type before any register type check:

  if (arg_type == ARG_IGNORE)
          return 0;

arena_types is only reached through compatible_reg_types[ARG_PTR_TO_ARENA],
which applies to kfunc parameters named with __arena or __arena__nullable and
to arena-tagged global subprog arguments.  resolve_btfids adds the arena type
tag for KF_ARENA_ARG2 but keeps the parameter name, so the __ign rule still
applies.

So the conclusion that no cast is needed looks right, but the address
argument is not checked against { PTR_TO_ARENA, SCALAR_VALUE } at all.  The
same reasoning applies to the tests that no longer have a guard or a tag:
basic_reserve1, basic_reserve2, reserve_twice, reserve_invalid_region and
arena_kfuncs_under_bpf_lock in verifier_arena.c, and
request_partially_reserved and free_reserved in verifier_arena_large.c.

Could the paragraph cite the addr__ign/ptr__ign parameters
(is_kfunc_arg_ignore() -> ARG_IGNORE) instead of arena_types?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/40608355234