Re: [PATCHv2 bpf-next 06/11] bpf: Factor callchain_store function from __bpf_get_stack

On Wed, Jul 29, 2026 at 08:57:51AM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [Critical] Preemption can cause `trace->nr` to be modified concurrently, leading to an integer underflow in `trace_nr = trace->nr - skip` and a massive out-of-bounds `memcpy`.
> --
> 
> --- Patch [1]: [PATCHv2 bpf-next 06/11] bpf: Factor callchain_store function from __bpf_get_stack ---
> commit 41f5133836d848295ac407c006940d4f0641b50f
> Author: Jiri Olsa <[email protected]>
> 
> bpf: Factor callchain_store function from __bpf_get_stack
> 
> This commit factors out the callchain storage logic from __bpf_get_stack
> into a new callchain_store function. The new function handles both IP and
> build ID data copying.
> 
> > diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
> > index 41fe87d7302f2..18d1a10bf76e5 100644
> > --- a/kernel/bpf/stackmap.c
> > +++ b/kernel/bpf/stackmap.c
> > @@ -680,6 +680,29 @@ const struct bpf_func_proto bpf_get_stackid_proto_pe = {
> >  	.arg3_type	= ARG_ANYTHING,
> >  };
> >  
> > +static u32 callchain_store(struct perf_callchain_entry *trace, void *buf,
> > +			   u32 elem_size, u64 flags)
> > +{
> > +	bool user_build_id = flags & BPF_F_USER_BUILD_ID;
> > +	u32 skip = flags & BPF_F_SKIP_FIELD_MASK;
> > +	u32 trace_nr, copy_len;
> > +	u64 *ips;
> > +
> > +	trace_nr = trace->nr - skip;
> 
> [Severity: Critical]
> This is a pre-existing issue, but could this calculation underflow if
> trace->nr is modified concurrently?
> 
> Looking at __bpf_get_stack(), when called from a sleepable BPF program via
> bpf_get_stack_sleepable(), it calls get_perf_callchain(). This allocates
> a per-CPU recursion buffer but unpins it by calling put_callchain_entry()
> before returning.
> 
> __bpf_get_stack() then checks if trace->nr is less than skip:
> 
> 	if (unlikely(!trace) || trace->nr < skip) {
> 		if (may_fault)
> 			rcu_read_unlock();
> 		goto err_fault;
> 	}
> 
> If the task is preempted immediately after this check (which is possible for
> sleepable BPF programs under CONFIG_PREEMPT_RCU, as rcu_read_lock() does not
> prevent preemption), could a new task on the same CPU trigger a BPF program
> that reacquires the same level-0 recursion buffer and overwrites trace->nr
> to 0?
> 
> When the original task resumes and calls callchain_store(), it re-reads
> trace->nr. If it is now 0, wouldn't 0 - skip underflow to a massive unsigned
> value?
> 
> > +	copy_len = trace_nr * elem_size;
> > +
> > +	ips = trace->ip + skip;
> > +	if (user_build_id) {
> > +		struct bpf_stack_build_id *id_offs = buf;
> > +
> > +		for (u32 i = 0; i < trace_nr; i++)
> > +			id_offs[i].ip = ips[i];
> > +	} else {
> > +		memcpy(buf, ips, copy_len);
> 
> If the underflow occurs, would this result in a massive out-of-bounds memcpy
> that could overwrite the BPF stack buffer or map value and cause memory
> corruption?

the preemption fix comes later in the patchset

jirka