Re: [PATCH bpf-next v2 7/8] selftests/bpf: write the mixed arena/scalar ALU tests in asm

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 2998 bytes --]

> selftests/bpf: write the mixed arena/scalar ALU tests in asm

TL;DR: Test comment only: the new verifier_arena.c comment claiming the r9
arena load is needed for addr_space_cast looks stale, since libbpf now binds
the arena via fd_array. Suggest dropping or rewording it.

> diff --git a/tools/testing/selftests/bpf/progs/verifier_arena.c b/tools/testing/selftests/bpf/progs/verifier_arena.c
> index 21b66d51c61e..b84d8f1e0b39 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_arena.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_arena.c
> @@ -539,53 +539,58 @@ int arena_ptr_add_arena_ptr(void *ctx)
>  	return 0;
>  }
>
> +/*
> + * The r9 load is never read; it is there so libbpf records the arena map
> + * as used, which is what lets the verifier accept addr_space_cast at all.
> + */
>  SEC("syscall")
>  __failure __msg("same insn cannot be used with and without arena pointer")

Is this comment still accurate?  Commit cd7a395838cb ("libbpf: Bind the arena
of an object to every program in it") is already in the base of this series.
With it, bpf_object_load_prog() calls bpf_object__bind_arena(), which puts
the object's arena into a continuous fd_array when the kernel supports
fd_array_cnt:

    obj->fd_array[0] = map->fd;
    load_attr->fd_array = obj->fd_array;
    load_attr->fd_array_cnt = obj->fd_array_used;

The kernel binds that array before it looks at any insn:

    process_fd_array_continuous()
      add_fd_from_fd_array()
        __add_used_map()
          check_map_prog_compatibility()
            env->prog->aux->arena = (void *)map;

This happens for every program in verifier_arena.o, including these two,
whether or not an insn refers to the map.  The addr_space_cast check in
check_alu_op() (if (!env->prog->aux->arena)) and the prevent_zext/needs_zext
logic in adjust_reg_min_max_vals() only look at prog->aux->arena, so
removing the r9 load would not change what either test does with a kernel and
libbpf from this tree.

The comment also describes the mechanism differently from what happens.
libbpf only relocates the ld_imm64 to the map fd, and the verifier records
the map as used in add_used_map() during resolve_pseudo_ldimm64().

The log of the libbpf commit describes referring to the arena artificially,
as in asm volatile("" :: "r"(&arena)), as something that change makes
unnecessary.  The r9 load would only matter on a kernel that rejects
fd_array_cnt, or with the light skeleton loader, which test_loader does not
use.

The same four comment lines are repeated word for word above
mixed_arena_scalar_alu64_arena_first().  Could the comment be dropped
together with the r9 load, or be reworded to say the load is a fallback for
kernels where libbpf cannot bind the arena through fd_array_cnt?


---
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/40768193895