Re: [PATCH bpf-next v5 08/16] bpf: Support stack arguments for bpf functions

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

On 4/16/26 9:43 PM, bot+bpf-ci@kernel.org wrote:
>> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
>> index 7bbd62c51bb5c..e0341fa0b0f7c 100644
>> --- a/include/linux/bpf_verifier.h
>> +++ b/include/linux/bpf_verifier.h
> [ ... ]
>
>> @@ -508,6 +513,17 @@ struct bpf_func_state {
>>   	u32 callback_depth;
>>   	u32 async_entry_cnt;
>>
>> +	/* Stack argument tracking for current function. */
>> +	struct bpf_reg_state *stack_arg_regs;	/* Stack arg register states */
>> +	u32 stack_arg_depth;			/* Depth of stack_arg_regs array */
>> +	u32 incoming_stack_arg_depth;		/* Incoming stack arg count (callee) */
>> +	u64 out_stack_arg_mask;			/* Outgoing stack arg mask (caller) */
>> +};
>> +
>> +#define bpf_get_spilled_stack_arg(slot, frame, mask)			\
>> +	(((slot < frame->stack_arg_depth / BPF_REG_SIZE) &&		\
>> +	  ((1 << frame->stack_arg_regs[slot].type) & (mask)))	\
>> +	 ? &frame->stack_arg_regs[slot] : NULL)
> Does this macro work correctly? The existing bpf_get_spilled_reg macro
> filters on frame->stack[slot].slot_type[], which is drawn from the
> STACK_* enum (STACK_SPILL, STACK_ITER, etc.) and callers pass masks
> like (1 << STACK_SPILL) | (1 << STACK_ITER).
>
> This new macro filters on bpf_reg_state.type which is from a completely
> different enum bpf_reg_type (SCALAR_VALUE, PTR_TO_BTF_ID, etc.) with
> type modifier bits (PTR_MAYBE_NULL = BIT(8), PTR_TRUSTED = BIT(20),
> etc.).
>
> For example, check_kfunc_call() in the rcu_unlock path uses:
>
>      u32 clear_mask = (1 << STACK_SPILL) | (1 << STACK_ITER);
>      ...
>      bpf_for_each_reg_in_vstate_mask(env->cur_state, state, reg,
>                                      clear_mask, ...);
>
> A stack-arg slot holding a MEM_RCU-tagged register would have
> reg->type including MEM_RCU (BIT(13)). Then (1 << 13) = 0x2000, and
> 0x2000 & clear_mask (0x22) = 0, so the slot is skipped and MEM_RCU is
> not cleared on rcu_unlock.
>
> Additionally, when n >= 32 (for modifier bits like PTR_TRUSTED at
> BIT(20)), the shift (1 << n) invokes undefined behavior in C.
>
> Should this use a different approach to filter stack arg slots, perhaps
> based on initialization status (type != NOT_INIT)?
>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 32cc59810e14d..30f2d1f06a988 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
> [ ... ]
>
>> @@ -1340,6 +1340,20 @@ static int copy_stack_state(struct bpf_func_state *dst,
>>   			goto out_free_stack;
>>   	}
>>
>> +	n = src->stack_arg_depth / BPF_REG_SIZE;
>> +	if (n) {
>> +		dst->stack_arg_regs = copy_array(dst->stack_arg_regs, src->stack_arg_regs, n,
>> +						 sizeof(struct bpf_reg_state),
>> +						 GFP_KERNEL_ACCOUNT);
>> +		if (!dst->stack_arg_regs)
>> +			return -ENOMEM;
>> +
>> +		dst->stack_arg_depth = src->stack_arg_depth;
>> +		dst->incoming_stack_arg_depth = src->incoming_stack_arg_depth;
>> +		dst->out_stack_arg_mask = src->out_stack_arg_mask;
>> +	}
>> +
>> +	return 0;
> What happens when src->stack_arg_depth == 0 but dst has non-zero
> values from prior work?
>
> The stack_arg_depth, incoming_stack_arg_depth, out_stack_arg_mask, and
> stack_arg_regs fields come after the 'stack' field in struct
> bpf_func_state, so the preceding memcpy(dst, src, offsetof(..., stack))
> in copy_func_state does not overwrite them.
>
> When n == 0, the assignments to dst->stack_arg_depth,
> dst->incoming_stack_arg_depth, and dst->out_stack_arg_mask are skipped,
> leaving stale values in dst.
>
> This can be triggered via:
>
> 1. Frame starts with stack_arg_depth = 0, out_stack_arg_mask = 0
> 2. check_cond_jmp_op calls push_stack which saves this state
> 3. Verifier continues on one branch; BPF_ST/BPF_STX writes to r11-8,
>     r11-16 cause check_stack_arg_write to call grow_stack_arg_slots,
>     setting stack_arg_depth = 16 and out_stack_arg_mask = BIT(0)|BIT(1)
> 4. Branch ends; pop_stack calls bpf_copy_verifier_state which calls
>     copy_stack_state with src->stack_arg_depth == 0
> 5. Because n == 0, the assignment block is skipped, and dst retains
>     stack_arg_depth = 16 and out_stack_arg_mask = BIT(0)|BIT(1)
> 6. The other branch never writes r11-8 or r11-16, but when it calls a
>     subprog, set_callee_state checks:
>     if ((caller->out_stack_arg_mask & req_mask) != req_mask)
>     The stale mask makes this check pass incorrectly
>
> Would the verifier accept a call with uninitialized stack args on that
> second branch? Should these fields be assigned unconditionally, similar
> to how allocated_stack is handled earlier in this function?

The same issue as in the previous comment. Will fix.

>
>
> ---
> AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
> See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
>
> CI run summary: https://github.com/kernel-patches/bpf/actions/runs/24546989054