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

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

On 5/11/26 6:05 PM, Alexei Starovoitov wrote:
> 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.

You are right.

    join:
      r2 = *(r11+8);       /* read incoming arg again */

is not allowed for path1. We can avoid this no_stack_arg_load
comparison in stack_arg_safe() and later the verifier will reject
at above join point with path1.