Re: [PATCH bpf v3 2/2] selftests/bpf: Test missing and callback stack arguments

[bot+bpf-ci]

> selftests/bpf: Test missing and callback stack arguments

TL;DR: Commit message only: the stated reason for the r11 stores before helper
calls in the callback selftests looks inaccurate; the tests themselves look
correct. Asks for the real reason.

>     selftests/bpf: Test missing and callback stack arguments
>
>     Exercise the path where a static seven-argument subprogram has unreliable
>     BTF and is called through a bridge with only one initialized outgoing stack
>     argument. The callee expects two stack arguments, but the unreliable-BTF
>     path skips check_outgoing_stack_args(). Expect the verifier to reject the
>     second r11 load at offset 16.
>
>     Also exercise synchronous and asynchronous callbacks whose BTF prototypes
>     declare a sixth argument. Initialize the outgoing slot at each helper call
>     site so the programs reach the callback-specific rejection. Keep a valid
>     six-argument direct call in each program so bpf_fixup_call_args() records
>     the stack-argument requirement and cannot become an unrelated rejection
>     fallback.
>
>     The fix also rejects the existing uninitialized-stack-argument test before
>     the later unreadable-register check. Update that test's expected diagnostic
>     accordingly.
>
>     Suggested-by: Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx>
>     Assisted-by: LLM
>     Signed-off-by: Weiming Shi <bestswngs@xxxxxxxxx>

This isn't a bug, but is the stated reason for the r11 stores before the
helper calls accurate?  The commit message says:

    Initialize the outgoing slot at each helper call site so the programs
    reach the callback-specific rejection.

push_callback_call() calls btf_check_subprog_call() ->
btf_check_func_arg_match() before the new "callback subprog cannot have stack
args" check.  callback_6args() is static and its cached arg_slot_cnt is 6, so
btf_check_func_arg_match() first runs:

    for (i = 0; i < min_t(u32, sub->arg_slot_cnt, MAX_BPF_FUNC_REG_ARGS); i++)
        if (caller->regs[BPF_REG_1 + i].type == NOT_INIT)
            return -EINVAL;

and returns -EINVAL before check_func_args() -> check_outgoing_stack_args()
gets a chance to return -EFAULT.

In stack_arg_sync_callback(), r5 is never written before "call %[bpf_loop]".
In stack_arg_async_callback(), r3-r5 are cleared by the earlier
bpf_map_lookup_elem() call.

push_callback_call() ignores that -EINVAL (only -EFAULT is fatal) and goes on
to reject on bpf_in_stack_arg_cnt(), with or without the stores.

The stores only change what an unfixed kernel does.  In the sync case they
keep caller->stack_arg_regs non-NULL, so the old check_stack_arg_read() reads
a real slot.  In the async case they change nothing, because the old code
reads frame[curframe - 1] with curframe == 0.

The tests themselves look correct, and the expected messages match the fixed
verifier on every explored path.  Could the changelog give the real reason for
the stores, or are they needed on some path not covered here?


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