Re: Re: [PATCH bpf 1/2] bpf: Keep fastcall spills for helper stack reads

[Nuoqi Gui]

> -----Original Messages-----
> From: "Eduard Zingerman" <eddyz87@xxxxxxxxx>
> Send time:Friday, 26/06/2026 02:32:32
> To: "Nuoqi Gui" <gnq25@xxxxxxxxxxxxxxxxxxxxx>, "Alexei Starovoitov" <ast@xxxxxxxxxx>, "Daniel Borkmann" <daniel@xxxxxxxxxxxxx>, "Andrii Nakryiko" <andrii@xxxxxxxxxx>, "Kumar Kartikeya Dwivedi" <memxor@xxxxxxxxx>
> Cc: "John Fastabend" <john.fastabend@xxxxxxxxx>, "Martin KaFai Lau" <martin.lau@xxxxxxxxx>, "Shuah Khan" <shuah@xxxxxxxxxx>, bpf@xxxxxxxxxxxxxxx, linux-kselftest@xxxxxxxxxxxxxxx, linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH bpf 1/2] bpf: Keep fastcall spills for helper stack reads
> 
> On Wed, 2026-06-24 at 16:39 +0800, Nuoqi Gui wrote:
> > The fastcall spill/fill rewrite is only sound while the stack slots used by
> > the pattern are not accessed outside the pattern. Direct stack loads and
> > stores already call check_fastcall_stack_contract() to enforce this.
> > 
> > Helper and kfunc memory-argument checks can validate PTR_TO_STACK reads
> > through check_stack_range_initialized() without applying the same contract.
> > When such a read overlaps a fastcall spill slot,
> > bpf_remove_fastcall_spills_fills() can still remove the spill/fill pair.
> > It can then shrink the subprogram stack depth even though a helper or kfunc
> > reads that stack address.
> > 
> > Apply check_fastcall_stack_contract() from check_stack_range_initialized()
> > after the concrete stack range is known. Zero-sized accesses do not read or
> > write memory, so leave the fastcall optimization unchanged for those.
> > 
> > Fixes: 5b5f51bff1b66 ("bpf: no_caller_saved_registers attribute for helper calls")
> > Signed-off-by: Nuoqi Gui <gnq25@xxxxxxxxxxxxxxxxxxxxx>
> > ---
> >  kernel/bpf/verifier.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index ff9b1f68ceca4..d77332eeab359 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -6873,6 +6873,10 @@ static int check_stack_range_initialized(
> >  		max_off = reg->smax_value + off;
> >  	}
> >  
> > +	if (access_size)
> > +		check_fastcall_stack_contract(env, state, env->insn_idx,
> > +					      min_off);
> > +
> 
> Thank you for finding this bug.
> I think that placing the check_fastcall_stack_contract() call here
> makes the same call unnecessary in the check_stack_read_var_off(),
> as it calls to check_stack_range_initialized() with zero_size_allowed==false.
> 
> >  	if (meta && meta->raw_mode) {
> >  		/* Ensure we won't be overwriting dynptrs when simulating byte
> >  		 * by byte access in check_helper_call using meta.access_size.

Thanks. I'll remove the redundant call from check_stack_read_var_off() in v2.