Re: [PATCH bpf-next v4 06/18] bpf: Use argument index instead of register index in kfunc verifier logs

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

On 4/12/26 3:01 PM, Alexei Starovoitov wrote:
> On Sat, Apr 11, 2026 at 9:59 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>> For kfunc argument checking, use the argument index (arg#0, arg#1, ...)
>> instead of the register index (R1, R2, ...) in verifier log messages.
>> This is a preparation for future stack-based arguments where kfuncs can
>> accept more than 5 arguments. Stack arguments won't have a corresponding
>> register, so using argument index is more appropriate.
>>
>> Since some functions like check_mem_access(), check_stack_read_var_off(),
>> and check_stack_range_initialized() are shared between kfunc argument
>> checking (check_kfunc_args) and other paths (check_func_arg, do_check_insn, ...),
>> introduce a `reg_or_arg` encoding: a non-negative value represents a register
>> index, while a negative value encodes an argument index as -(argno + 1).
>> The helper reg_arg_name() decodes this to produce either "R%d" or
>> "arg#%d" for log messages.
>>
>> For check_func_arg() callers, in certain cases, the register index is
>> preserved so existing helper function logs remain unchanged (e.g., "R1", "R2").
>>
>> Update selftests to expect the new "arg#N" format in kfunc error
>> messages.
>>
>> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
>> ---
>>   include/linux/bpf_verifier.h                  |   1 +
>>   kernel/bpf/verifier.c                         | 466 +++++++++---------
>>   .../selftests/bpf/prog_tests/cb_refs.c        |   2 +-
>>   .../selftests/bpf/prog_tests/linked_list.c    |   4 +-
>>   .../selftests/bpf/progs/cpumask_failure.c     |   4 +-
>>   .../testing/selftests/bpf/progs/dynptr_fail.c |   6 +-
>>   .../selftests/bpf/progs/iters_testmod.c       |   6 +-
>>   .../bpf/progs/local_kptr_stash_fail.c         |   2 +-
>>   .../selftests/bpf/progs/map_kptr_fail.c       |   4 +-
>>   .../bpf/progs/mem_rdonly_untrusted.c          |   2 +-
>>   .../bpf/progs/nested_trust_failure.c          |   2 +-
>>   .../selftests/bpf/progs/res_spin_lock_fail.c  |   2 +-
>>   .../testing/selftests/bpf/progs/stream_fail.c |   2 +-
>>   .../selftests/bpf/progs/task_kfunc_failure.c  |   4 +-
>>   .../bpf/progs/verifier_cgroup_storage.c       |   4 +-
>>   .../selftests/bpf/progs/verifier_ctx.c        |   2 +-
>>   .../bpf/progs/verifier_ref_tracking.c         |   2 +-
>>   .../selftests/bpf/progs/verifier_sock.c       |   6 +-
>>   .../selftests/bpf/progs/verifier_unpriv.c     |   4 +-
>>   .../selftests/bpf/progs/verifier_vfs_reject.c |   8 +-
>>   .../testing/selftests/bpf/progs/wq_failures.c |   4 +-
>>   tools/testing/selftests/bpf/verifier/calls.c  |   6 +-
>>   .../testing/selftests/bpf/verifier/map_kptr.c |  10 +-
>>   23 files changed, 286 insertions(+), 267 deletions(-)
>>
>> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
>> index 05b9fe98b8f8..291f11ddd176 100644
>> --- a/include/linux/bpf_verifier.h
>> +++ b/include/linux/bpf_verifier.h
>> @@ -910,6 +910,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_reg_arg_name_buf[16];
>>          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 54296d818d35..01df990f841a 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -2179,6 +2179,18 @@ static struct bpf_verifier_state *push_stack(struct bpf_verifier_env *env,
>>          return &elem->st;
>>   }
>>
>> +static const char *reg_arg_name(struct bpf_verifier_env *env, int reg_or_arg)
>> +{
>> +       char *buf = env->tmp_reg_arg_name_buf;
>> +       int len = sizeof(env->tmp_reg_arg_name_buf);
>> +
>> +       if (reg_or_arg >= 0)
>> +               snprintf(buf, len, "R%d", reg_or_arg);
>> +       else
>> +               snprintf(buf, len, "arg#%d", -(reg_or_arg + 1));
>> +       return buf;
>> +}
> The patches 1-4 make sense, but 5, 6 are too hacky.
>
> -       { "incorrect_head_var_off1", "R1 doesn't have constant offset" },
> +       { "incorrect_head_var_off1", "arg#0 doesn't have constant offset" },
>
> This just sucks.
> It degrades output for no good reason.
>
> Instead of inband negative vs positive signalling rename all
> 'regno' to 'argno' and always pass whatever argno you need 1,2,..5,6, etc
>
> Pass ptr_reg and size_reg as bpf_reg_state the way patches 1-4 are doing.
> If argno <= 5 keep 'R%d' output, so all selftest don't change.
> For argno >= 6 print '*(R12-xx)' where xx is where that arg lives.
> Printing arg# is too cryptic. Humans/agents need to do mental
> gymnastics to understand what it means.
> The output must be easy to consume by agents.

Currently argno is indexed from 0 in verifier for parameters
while regno is from 1 to 5. That is why I am using reg_or_arg
to distinguish regno vs. argno.

What you suggested to use argno sounds good. I will change verifier
for argno to start from 1 (esp. for verifier logs).

>
> I was also thinking whether we can get rid of this 'argno' too.
> cur_regs - reg is that number for <= 5 and
> some spilled_ptr - reg for >= 6.
> Technically we can
>
> u32 argno = cur_regs - reg;
> if (argno <= 5) use it
> else
>     argno = spilled_ptr - reg.
>
> Feels a bit hacky. Need to sleep on it.
>
> pw-bot: cr