Re: [PATCH bpf-next 01/18] bpf: Support stack arguments for bpf functions

[Eduard Zingerman <eddyz87@gmail.com>]

On Wed, 2026-04-29 at 23:52 +0100, Yonghong Song wrote:

[...]

> > But this is a very partial check, the max_out_stack_arg_depth is
> > computed per-subprogram, not per-call. As far as I understand the
> > design, it can't be computed per-call at all. Meaning that if there
> > are, say, two calls:
> > - foo(1,2,3,4,5,6,7)   // where foo expects only 6 parameters
> > - bar(1,2,3,4,5,6,7,8) // where bar expects only 7 parameters
> > 
> > In this case:
> > - Verifier won't know which of the two calls is bogus, so won't be
> >    able to point user to the instruction where error occurs.
> > - This is not a safety condition, meaning that kernel state is not
> >    broken if more arguments are pushed onto stack (and if it *is* a
> >    safety condition, then we need to figure out something two check
> >    both calls above).
> 
> You are right in the sense, we do not reject at any callsite
> since we do not know whether it is exclusively used or shared.
> For example,
>     *(u64 *)(r11 - 24) = ...;
>     if (...) {
>       *(u64 *)(r11 - 16) = ...;
>       *(u64 *)(r11 - 8) = ...;
>       subprog_7args;
>     } else {
>       ...
>     }
> 
> We cannot warn at subprog_7args() since it is not clear
> whether '*(u64 *)(r11 - 24) = ...' is exclusively used
> by subprog_7args or other, unless we keep track of
> stack arguments. but I think this is not needed.
> The above check is in function bpf_fixup_call_args()
> where all subprog's have been processed. So we know
> *maximum* stack arg count, e.g.,
>      subprog_7args()
>      subprog_9args()
> We now the maximum stack argument count is 4 and
> this maximum stack argument count will be used in jit.
> 
> If we see '*(u64 *)(r11 - 40)', we will know it will
> not be used for any kfunc or bpf functions.
> 
> This is a little bit tying to JIT implementation.
> In JIT, the stack args (excepting the first few based
> on arch ABI) will consume some stack slot based on
> native arch calling convention. for example, for x86_64,
> 
>        high address
>        +-------------------------+
>        | incoming stack arg N    |  [rbp + 16 + (N-7)*8]  (from caller)
>        | ...                     |
>        | incoming stack arg 7    |  [rbp + 16]
>        +-------------------------+
>        | return address          |  [rbp + 8]
>        | saved rbp               |  [rbp]
>        +-------------------------+
>        | BPF program stack       |  (round_up(stack_depth, 8) bytes)
>        +-------------------------+
>        | callee-saved regs       |  (r12, rbx, r13, r14, r15 as needed)
>        +-------------------------+
>        | outgoing arg M          |  [rsp + (M-7)*8]
>        | ...                     |
>        | outgoing arg 7          |  [rsp]
>        +-------------------------+  rsp
>        low address
> 
> The native off is:
> 	native_off = outgoing_arg_base - outgoing_rsp - bpf_off - 16
> 
> So
>    r11 - 8:   r9 = <...>
>    r11 - 16:  outgoing arg 7
>               native_off = outgoing_arg_base - outgoing_rsp
>    r11 - 24:  outgoing arg 8:
>               native_off = outgoing_arg_base - outgoing_rsp + 8
>    r11 - 32:  outgoing arg 9:
>               native_off = outgoing_arg_base - outgoing_rsp + 16
> 
> Then we have
>    r11 - 40: native_off = outgoing_arg_base - outgoing_rsp + 24
> 
> This will have a problem as it may overwrite callee-saved regs.
> So we need to reject it.

I see, thank you for explanation.

[...]

> > > > > +	caller = vstate->frame[vstate->curframe - 1];
> > > > > +	arg = &caller->stack_arg_regs[spi];
> > > > > +	cur = vstate->frame[vstate->curframe];
> > > > > +
> > > > > +	if (is_spillable_regtype(arg->type))
> > > > > +		copy_register_state(&cur->regs[dst_regno], arg);
> > > > > +	else
> > > > > +		mark_reg_unknown(env, cur->regs, dst_regno);
> > > > For stack writes we report error in such situations,
> > > > should the same be done here?
> > > We should be fine here.
> > This is not a bug, sure, but it would be nice to have consistent
> > behavior for similar situations.
> 
> Okay, I figured out a better solution like below:
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 2f412128d76a..5fa16287353c 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -4159,11 +4159,7 @@ static int check_stack_arg_read(struct bpf_verifier_env *env, struct bpf_func_st
>          caller = vstate->frame[vstate->curframe - 1];
>          arg = &caller->stack_arg_regs[spi];
>          cur = vstate->frame[vstate->curframe];
> -
> -       if (is_spillable_regtype(arg->type))
> -               copy_register_state(&cur->regs[dst_regno], arg);
> -       else
> -               mark_reg_unknown(env, cur->regs, dst_regno);
> +       copy_register_state(&cur->regs[dst_regno], arg);

Hm, makes sense.

>          return bpf_push_jmp_history(env, env->cur_state,
>                                      insn_stack_arg_access_flags(state->frameno, spi), 0);
>   }
> 
> and
> 
> @@ -952,7 +951,8 @@ 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))
> +       if (!stack_arg_safe(env, old, cur, &env->idmap_scratch,
> +                           exact == NOT_EXACT ? RANGE_WITHIN : exact))
>                  return false;
> 
> <btw, copy_register_state() will be replaced with simple assignment aftre refactoring>
> 
> In stack_arg_safe, with NOT_EXACT seems not enough for precision tracking, so
> using RANGE_WITHIN can ensure proper pruning.

Could you please elaborate a bit?