Re: [PATCH bpf-next 7/9] bpf: Prepare verifier logs for upcoming kfunc stack arguments

[Yonghong Song <yonghong.song@linux.dev>]

On 4/21/26 5:37 PM, Alexei Starovoitov wrote:
> On Tue Apr 21, 2026 at 4:56 PM PDT, Yonghong Song wrote:
>>
>> On 4/21/26 3:07 PM, Alexei Starovoitov wrote:
>>> On Tue Apr 21, 2026 at 10:20 AM PDT, Yonghong Song wrote:
>>>> This change prepares verifier log reporting for upcoming kfunc stack
>>>> argument support.
>>>>
>>>> Today verifier log code mostly assumes that an argument can be described
>>>> directly by a register number. That works for arguments passed in `R1`
>>>> to `R5`, but it does not work once kfunc arguments can also be
>>>> passed on the stack.
>>>>
>>>> Introduce an internal `argno` representation such that register-passed
>>>> arguments keep using their real register numbers, while stack-passed
>>>> arguments use an encoded value above a dedicated base.
>>>> `reg_arg_name()` converts this representation into either `R%d` or
>>>> `*(R11-off)` when emitting verifier logs. If a particular `argno`
>>>> is corresponding to a stack argument, print `*(R11-off)`. Otherwise,
>>>> print `R%d`. Here R11 presents the base of stack arguments.
>>>>
>>>> This keeps existing logs readable for register arguments and allows the
>>>> same log sites to handle future stack arguments without open-coding
>>>> special cases.
>>>>
>>>> Update selftests accordingly.
>>>>
>>>> Acked-by: Puranjay Mohan <puranjay@kernel.org>
>>>> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
>>>> ---
>>>>    include/linux/bpf_verifier.h                  |   1 +
>>>>    kernel/bpf/verifier.c                         | 640 ++++++++++--------
>>>>    .../testing/selftests/bpf/prog_tests/bpf_nf.c |  22 +-
>>>>    .../selftests/bpf/prog_tests/cb_refs.c        |   2 +-
>>>>    .../selftests/bpf/prog_tests/kfunc_call.c     |   2 +-
>>>>    .../selftests/bpf/prog_tests/linked_list.c    |   4 +-
>>>>    .../selftests/bpf/progs/cgrp_kfunc_failure.c  |  14 +-
>>>>    .../selftests/bpf/progs/cpumask_failure.c     |  10 +-
>>>>    .../testing/selftests/bpf/progs/dynptr_fail.c |  22 +-
>>>>    .../selftests/bpf/progs/file_reader_fail.c    |   4 +-
>>>>    tools/testing/selftests/bpf/progs/irq.c       |   4 +-
>>>>    tools/testing/selftests/bpf/progs/iters.c     |   6 +-
>>>>    .../selftests/bpf/progs/iters_state_safety.c  |  14 +-
>>>>    .../selftests/bpf/progs/iters_testmod.c       |   4 +-
>>>>    .../selftests/bpf/progs/iters_testmod_seq.c   |   4 +-
>>>>    .../selftests/bpf/progs/map_kptr_fail.c       |   2 +-
>>>>    .../selftests/bpf/progs/percpu_alloc_fail.c   |   4 +-
>>>>    .../testing/selftests/bpf/progs/rbtree_fail.c |   6 +-
>>>>    .../bpf/progs/refcounted_kptr_fail.c          |   2 +-
>>>>    .../testing/selftests/bpf/progs/stream_fail.c |   2 +-
>>>>    .../selftests/bpf/progs/task_kfunc_failure.c  |  18 +-
>>>>    .../selftests/bpf/progs/task_work_fail.c      |   6 +-
>>>>    .../selftests/bpf/progs/test_bpf_nf_fail.c    |   8 +-
>>>>    .../bpf/progs/test_kfunc_dynptr_param.c       |   2 +-
>>>>    .../bpf/progs/test_kfunc_param_nullable.c     |   2 +-
>>>>    .../selftests/bpf/progs/verifier_bits_iter.c  |   4 +-
>>>>    .../bpf/progs/verifier_ref_tracking.c         |   6 +-
>>>>    .../selftests/bpf/progs/verifier_vfs_reject.c |   8 +-
>>>>    .../testing/selftests/bpf/progs/wq_failures.c |   2 +-
>>>>    tools/testing/selftests/bpf/verifier/calls.c  |  14 +-
>>>>    30 files changed, 464 insertions(+), 375 deletions(-)
>>>>
>>>> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
>>>> index b148f816f25b..d5b4303315dd 100644
>>>> --- a/include/linux/bpf_verifier.h
>>>> +++ b/include/linux/bpf_verifier.h
>>>> @@ -913,6 +913,7 @@ struct bpf_verifier_env {
>>>>    	 * e.g., in reg_type_str() to generate reg_type string
>>>>    	 */
>>>>    	char tmp_str_buf[TMP_STR_BUF_LEN];
>>>> +	char tmp_arg_name[32];
>>>>    	struct bpf_insn insn_buf[INSN_BUF_SIZE];
>>>>    	struct bpf_insn epilogue_buf[INSN_BUF_SIZE];
>>>>    	struct bpf_scc_callchain callchain_buf;
>>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>>>> index 18ab92581452..82568a427211 100644
>>>> --- a/kernel/bpf/verifier.c
>>>> +++ b/kernel/bpf/verifier.c
>>>> @@ -1742,6 +1742,44 @@ static struct bpf_verifier_state *push_stack(struct bpf_verifier_env *env,
>>>>    	return &elem->st;
>>>>    }
>>>>    
>>>> +#define STACK_ARGNO_BASE 100
>>>> +
>>>> +static bool is_stack_argno(int argno)
>>>> +{
>>>> +	return argno > STACK_ARGNO_BASE;
>>>> +}
>>>> +
>>>> +/* arg starts at 1 */
>>>> +static u32 make_argno(u32 arg)
>>>> +{
>>>> +	if (arg <= MAX_BPF_FUNC_REG_ARGS)
>>>> +		return arg;
>>>> +	return STACK_ARGNO_BASE + arg;
>>>> +}
>>> You can remove this and simplify everything further by
>>>
>>> static bool is_stack_argno(int argno)
>>> {
>>> 	return argno > MAX_BPF_FUNC_REG_ARGS;
>>> }
>>>
>>>> +
>>>> +static u32 arg_from_argno(int argno)
>>>> +{
>>>> +	if (is_stack_argno(argno))
>>>> +		return argno - STACK_ARGNO_BASE;
>>>> +	return argno;
>>>> +}
>>> remove as well.
>>>
>>> and a comment like:
>>>
>>> /*
>>>    * switch (argno) {
>>>    * case 1: R1
>>>    * case 5: R5
>>>    * case 6: *(u64 *)(R11 +- 8)
>>>    * case 7: *(u64 *)(R11 +- 16)
>>>    */
>> This doesn't work. Let us see the following example:
>>
>> check_kfunc_args
>>     process_dynptr_func (argno)
>>       check_mem_access (argno, 4th argument)
>>         check_packet_access (argno)
>>           check_mem_region_access (argno)
>>             __check_mem_access (argno)
>>               <== verbose log with argno
>>
>> do_check
>>     do_check_insn (env)
>>       check_load_mem (insn)
>>         check_mem_access (insn->src_reg, 4th argument)
>>           check_packet_access (...)
>>             check_mem_region_access (...)
>>               __check_mem_access (insn->src_reg or argno)
> Ohh. Silent conversion. That's quite error prone.
>
> let's do
> typedef struct argno {
>      int argno;
> } argno_t;
>
> and make sure this callchain passes arg_t unmodified:
>
>      process_dynptr_func (argno)
>        check_mem_access (argno, 4th argument)
>          check_packet_access (argno) ...
>
> while here:
>
>        check_load_mem (insn)
>          check_mem_access (argno_from_reg(insn->src_reg), 4th argument)
>
> static argno_t argno_from_reg(u32 regno)
> {
>      return (argno_t){ .argno = regno };
> }
>
> static argno_t argno_from_arg(u32 arg)
> {
>      return (argno_t){ .argno = -arg };
> }
>
> static const char *reg_arg_name(struct bpf_verifier_env *env, argno_t argno)
>
> When positive vs negative is an internal implemenation of argno_t
> it's fine. It's better than shift by 100, but when negative was
> used as a signal everywhere it leaked details to caller.

This approach is kind of similar to what I proposed earlier with
an int variable, non-negative for reg and negative for arg (value -1/-2/...).
    https://lore.kernel.org/bpf/20260412045857.256260-1-yonghong.song@linux.dev/

But it is not explicit except the name 'reg_or_arg'.
Here, argno_t type makes it more explicit and should better.

For printing, I guess we still want to print 'R#' whenever possible
including positive registers and negative argno (1-5), and print
'*(R11-off)' for negative argno (6->...)?