Re: [PATCH bpf-next v3 05/24] bpf: Support stack arguments for bpf functions

["Alexei Starovoitov" <alexei.starovoitov@gmail.com>]

On Mon May 11, 2026 at 8:46 AM PDT, Yonghong Song wrote:
>
>
> On 5/11/26 8:19 AM, bot+bpf-ci@kernel.org wrote:
>>> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
>>> index 321b9d69cf9c..f9020a4ea005 100644
>>> --- a/include/linux/bpf_verifier.h
>>> +++ b/include/linux/bpf_verifier.h
>>> @@ -402,6 +402,7 @@ struct bpf_func_state {
>>>   	bool in_callback_fn;
>>>   	bool in_async_callback_fn;
>>>   	bool in_exception_callback_fn;
>>> +	bool no_stack_arg_load;
>> [ ... ]
>>
>>> diff --git a/kernel/bpf/states.c b/kernel/bpf/states.c
>>> index bd9c22945050..c249eb40c6d6 100644
>>> --- a/kernel/bpf/states.c
>>> +++ b/kernel/bpf/states.c
>>> @@ -833,6 +833,32 @@ static bool stacksafe(struct bpf_verifier_env *env, struct bpf_func_state *old,
>>>   	return true;
>>>   }
>>>
>>> +/*
>>> + * Compare stack arg slots between old and current states.
>>> + * Outgoing stack args are path-local state and must agree for pruning.
>>> + */
>>> +static bool stack_arg_safe(struct bpf_verifier_env *env, struct bpf_func_state *old,
>>> +			   struct bpf_func_state *cur, struct bpf_idmap *idmap,
>>> +			   enum exact_level exact)
>>> +{
>>> +	int i, nslots;
>>> +
>>> +	nslots = max(old->out_stack_arg_cnt, cur->out_stack_arg_cnt);
>>> +	for (i = 0; i < nslots; i++) {
>>> +		struct bpf_reg_state *old_arg, *cur_arg;
>>> +		struct bpf_reg_state not_init = { .type = NOT_INIT };
>>> +
>>> +		old_arg = i < old->out_stack_arg_cnt ?
>>> +			  &old->stack_arg_regs[i] : &not_init;
>>> +		cur_arg = i < cur->out_stack_arg_cnt ?
>>> +			  &cur->stack_arg_regs[i] : &not_init;
>>> +		if (!regsafe(env, old_arg, cur_arg, idmap, exact))
>>> +			return false;
>>> +	}
>>> +
>>> +	return true;
>>> +}
>>> +
>>>   static bool refsafe(struct bpf_verifier_state *old, struct bpf_verifier_state *cur,
>>>   		    struct bpf_idmap *idmap)
>>>   {
>>> @@ -924,6 +950,9 @@ static bool func_states_equal(struct bpf_verifier_env *env, struct bpf_func_stat
>>>   	if (!stacksafe(env, old, cur, &env->idmap_scratch, exact))
>>>   		return false;
>>>
>>> +	if (!stack_arg_safe(env, old, cur, &env->idmap_scratch, exact))
>>> +		return false;
>>> +
>>>   	return true;
>>>   }
>> In v2 (May 7), bot+bpf-ci@kernel.org raised a concern that
>> no_stack_arg_load is not compared during state pruning in
>> func_states_equal():
>>
>>      "The same issue applies to state pruning: func_states_equal/
>>      stack_arg_safe does not compare no_stack_arg_load either, so a
>>      state with no_stack_arg_load=true can be pruned to an old state
>>      with no_stack_arg_load=false incorrectly. Should no_stack_arg_load
>>      be placed before stack (to be memcpy'd) or should copy_stack_state()
>>      explicitly assign it?"
>>
>> The field placement was fixed (moving no_stack_arg_load before 'stack'
>> so it gets copied by memcpy), but func_states_equal() still does not
>> explicitly compare this field.
>>
>> Could this allow incorrect pruning? If a BPF program writes to stack
>> args and calls a function on one path (setting no_stack_arg_load=true),
>> then branches and tries to read from stack args on another path, the
>> verifier might incorrectly prune to the old state with
>> no_stack_arg_load=false, missing the enforcement that stack arg reads
>> must happen before writes/calls.
>>
>> Looking at func_states_equal():
>>
>> kernel/bpf/states.c:func_states_equal() {
>>      if (!stacksafe(env, old, cur, &env->idmap_scratch, exact))
>>          return false;
>>
>>      if (!stack_arg_safe(env, old, cur, &env->idmap_scratch, exact))
>>          return false;
>>
>>      return true;
>> }
>>
>> Should no_stack_arg_load be compared here, similar to how other boolean
>> flags in bpf_func_state are handled during state comparison?
>
> You are right. The following is an example:
>
>    /* subprog with incoming stack arg */
>    r1 = *(r11+8);         /* read incoming arg, no_stack_arg_load = false */
>                                                                                                                     
>    if cond goto path2;
>                                                                                                                     
>    path1:
>      call some_helper;    /* sets no_stack_arg_load = true */
>      goto join;
>                                                                                                                     
>    path2:
>      /* no call, no_stack_arg_load = false */
>      goto join;
>                                                                                                                     
>    join:
>      r2 = *(r11+8);       /* read incoming arg again */
>
> In the above case, at 'join' point, we have no_stack_arg_load = true and false
> respectively. In this case, we cannot do pruning.
>
> Will fix.

Hold on. Didn't we agree that any call should scratch all arg slots?
In the above example call some_helper will scratch it and last read shouldn't be allowed.