> diff --git a/tools/testing/selftests/bpf/progs/verifier_arena.c 
> b/tools/testing/selftests/bpf/progs/verifier_arena.c
> index b241bbcf54a8..b22bab33301a 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_arena.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_arena.c
> @@ -637,6 +637,45 @@ int non_arena_ptr_add_to_arena_ptr(void *ctx)
>
>  #endif
>
> +static const struct bpf_insn addr_space_cast_insn = {
> +     .code = 0xbf,
> +     .dst_reg = BPF_REG_7,
> +     .src_reg = BPF_REG_7,
> +     .off = 1,
> +     .imm = 1,
> +};

This isn't a bug, but could the struct carry the same kind of decoding
comments as the raw insns in stream.c?  The register fields are written
symbolically (BPF_REG_7) while the three fields that actually need decoding
are bare numbers: .code = 0xbf is BPF_ALU64 | BPF_MOV | BPF_X, .off = 1 is
BPF_ADDR_SPACE_CAST, and .imm = 1 is the (dst_as << 16) | src_as encoding
for as(1) -> as(0).

Could it be annotated like stream.c does (e.g. /* BPF_ALU64 | BPF_MOV |
BPF_X */, /* BPF_ADDR_SPACE_CAST */, /* (dst_as << 16) | src_as */)?  And
could the .8byte line below note that it is r7 = addr_space_cast(r7, 0, 1)?

> +
> +SEC("socket")
> +__description("arena and stack atomic at the same instruction")
> +__failure __msg("same insn cannot be used with different pointers")
> +__arch_x86_64
> +__load_if_JITed()
> +__naked void mixed_arena_stack_atomic(void)
> +{
> +     asm volatile ("                                 \
> +     r1 = %[arena] ll;                               \
> +     r6 = r10;                                       \
> +     r6 += -8;                                       \
> +     r9 = 0;                                 \
> +     *(u64 *)(r6 + 0) = r9;                  \
> +     r7 = 8192;                                      \
> +     .8byte %[addr_space_cast];                      \
> +     call %[bpf_get_prandom_u32];                    \
> +     if w0 != 0 goto 1f;                             \
> +     r8 = r6;                                        \
> +     goto 2f;                                        \
> +1:   r8 = r7;                                        \
> +2:   r9 = 1;                                 \
> +     lock *(u64 *)(r8 + 0) += r9;                    \
> +     r0 = 0;                                 \
> +     exit;                                           \

This isn't a bug, but would it be worth adding one more tab on the r9 = 0;,
*(u64 *)(r6 + 0) = r9;, 2:\tr9 = 1; and r0 = 0; lines so the line
continuations line up like the rest of the block?  Four of the seventeen
continuation lines put the trailing backslash at column 48 instead of the
column 56 used by the other thirteen lines.

> +"    :
> +     : __imm_addr(arena),
> +       __imm_insn(addr_space_cast, addr_space_cast_insn),
> +       __imm(bpf_get_prandom_u32)
> +     : __clobber_all);
> +}
> +
>  static __noinline
>  u32 __arena *check_arena_arg_nonglobal(u32 __arena *arg)
>  {


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

Reply via email to