Re: [PATCH bpf-next 9/9] bpf: Disable preemption in __bpf_get_stack

[Jiri Olsa <olsajiri@gmail.com>]

On Fri, Jul 24, 2026 at 01:23:21PM -0700, Ihor Solodrai wrote:
> On 2026-07-20 1:53 a.m., Jiri Olsa wrote:
> > From: Daniel Borkmann <borkmann@iogearbox.net>
> > 
> > get_perf_callchain() returns a per-CPU perf_callchain_entry buffer and
> > releases its recursion slot via put_callchain_entry() before returning,
> > so nothing keeps the entry reserved while __bpf_get_stack() consumes
> > it below.
> > 
> > A preemptible BPF program (e.g. a non-sleepable raw tracepoint program
> > on a PREEMPT kernel, which runs under migrate_disable() but not
> > preempt_disable()) can be scheduled out between obtaining the entry
> > and the copy. Another task scheduled on the same CPU then reuses the
> > same per-CPU buffer and overwrites trace->nr with a larger value.
> > copy_len is then computed from the inflated trace->nr and can exceed
> > the caller's buffer, causing an out-of-bounds write in the memcpy()
> > and in the build_id path.
> > 
> > The rcu_read_lock() previously taken here does not prevent this. It is
> > only taken on the may_fault path, and under CONFIG_PREEMPT_RCU it does
> > not disable preemption; it merely keeps perf's callchain buffer array
> > alive (freed via call_rcu()) and does nothing to stop another task
> > from reusing the entry.
> > 
> > Disable preemption around obtaining the callchain entry and copying
> > it into the caller's buffer, so the entry cannot be reused underneath
> > us and trace->nr stays bounded by max_depth. Build ID resolution may
> > fault and is therefore deferred until after preemption is re-enabled;
> > by then the instruction pointers have already been copied into buf,
> > so it operates only on that private copy. Note, preempt_disable() also
> > subsumes the buffer-lifetime guarantee the rcu_read_lock() provided,
> > since a preempt-disabled section is an RCU read-side critical section
> > for the callchain buffers' call_rcu() reclaim.
> > 
> > Cc: stable@vger.kernel.org
> > Fixes: c195651e565a ("bpf: add bpf_get_stack helper")
> > Reported-by: Tao Chen <chen.dylane@linux.dev>
> > Closes: https://lore.kernel.org/bpf/20260206090653.1336687-1-chen.dylane@linux.dev/
> > Reported-by: STAR Labs SG <info@starlabs.sg>
> > Signed-off-by: Daniel Borkmann <borkmann@iogearbox.net>
> > [ changed Fixes: commit ]
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >   kernel/bpf/stackmap.c | 9 +++------
> >   1 file changed, 3 insertions(+), 6 deletions(-)
> > 
> > diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
> > index 57cd4c33403b..37f8e46319b3 100644
> > --- a/kernel/bpf/stackmap.c
> > +++ b/kernel/bpf/stackmap.c
> > @@ -819,8 +819,7 @@ static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task,
> >   	max_depth = stack_map_calculate_max_depth(size, elem_size, flags);
> > -	if (may_fault)
> > -		rcu_read_lock(); /* need RCU for perf's callchain below */
> > +	preempt_disable();
> 
> With the series applied on bpf-ci-like kconfig I get a "suspicious RCU
> usage" splat on BPF selftests, pasted at the bottom.
> 
> AFAIU, the rcu_read_lock() that's removed here was not only
> controlling the lifetime (which is now covered by preempt_disable),
> but also putting the RCU read-side annotation for the
> 
> 	entries = rcu_dereference(callchain_cpus_entries);
> 
> in get_callchain_entry() (callchain.c:163).
> 
> preempt_disable() never takes rcu_lock_map, and a sleepable program
> holds only rcu_read_lock_trace() (rcu_tasks_trace_srcu_struct), which
> is a different lockmap. So on PREEMPT_RCU + PROVE_RCU the
> rcu_dereference_check() there now fails (see the splat).
> 
> The non-sleepable path is fine because it enters with rcu_read_lock()
> held via rcu_read_lock_dont_migrate().
> 
> I think to fix this we have to keep if (may_fault) rcu_read_lock();
> alongside preempt_disable(). Something like this:
> 
> diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
> index 37f8e46319b3..6be413072ef6 100644
> --- a/kernel/bpf/stackmap.c
> +++ b/kernel/bpf/stackmap.c
> @@ -820,6 +820,8 @@ static long __bpf_get_stack(struct pt_regs *regs, struct
> task_struct *task,
>         max_depth = stack_map_calculate_max_depth(size, elem_size, flags);
> 
>         preempt_disable();
> +       if (may_fault)
> +               rcu_read_lock();
> 
>         if (kernel && task) {
>                 trace = get_callchain_entry_for_task(task, max_depth);
> @@ -829,6 +831,8 @@ static long __bpf_get_stack(struct pt_regs *regs, struct
> task_struct *task,
>         }
> 
>         if (unlikely(!trace) || trace->nr < skip) {
> +               if (may_fault)
> +                       rcu_read_unlock();
>                 preempt_enable();
>                 goto err_fault;
>         }
> @@ -836,6 +840,8 @@ static long __bpf_get_stack(struct pt_regs *regs, struct
> task_struct *task,
>         trace_nr = callchain_store(trace, buf, size, elem_size, flags);
> 
>         /* trace should not be dereferenced after this point */
> +       if (may_fault)
> +               rcu_read_unlock();
>         preempt_enable();
> 
>         return callchain_finalize(buf, size, trace_nr, elem_size,
> user_build_id, user, may_fault);
> 
> 
> I confirmed this diff fixes the splat below.

yes, I managed to reproduce it as well.. basically just to add
the preemption_disable/enable hunks and keey rcu locks in place

> 
> I guess teaching the perf side to accept rcu_read_lock_sched_held()
> would also work, but that's out of scope for a bpf fix and may be no
> less tricky.

will check, but you might be right it'd be tricky

thanks,
jirka