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

Yonghong Song
2026-04-13T14:45:34Z

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