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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Replacing rcu_read_lock() with preempt_disable() triggers a lockdep warning ('suspicious rcu_dereference_check() usage') when __bpf_get_stack() is called from sleepable BPF programs.

Pre-existing issues:
- [Critical] Synchronous re-entrancy from BPF trampolines (fentry/fexit) enables a Time-Of-Check to Time-Of-Use (TOCTOU) vulnerability leading to an out-of-bounds memory write.
- [High] bpf_get_stack_pe permanently truncates the shared perf callchain length for user stacks.
--

--- Patch [9]: [PATCH bpf-next 9/9] bpf: Disable preemption in __bpf_get_stack ---
commit d1da66040bb1a7fc97bf16d7cb97a0712702bc93
Author: Daniel Borkmann <borkmann@iogearbox.net>

bpf: Disable preemption in __bpf_get_stack

This patch replaces rcu_read_lock() with preempt_disable() in
__bpf_get_stack() to prevent another task from reusing the per-CPU
perf_callchain_entry buffer. It ensures the callchain entry cannot be
reused and overwritten underneath the copy operation. Build ID resolution
is deferred until after preemption is re-enabled.

Closes: https://lore.kernel.org/bpf/20260206090653.1336687-1-chen.dylane@linux.dev/

> diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
> index 57cd4c33403bd..37f8e46319b34 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();

[Severity: High]
Does replacing rcu_read_lock() with preempt_disable() trigger a lockdep
warning when __bpf_get_stack() is called from sleepable BPF programs?

Sleepable programs run under rcu_read_lock_trace(), not rcu_read_lock().
With preempt_disable() replacing rcu_read_lock(), the lockdep rcu_lock_map
is not acquired.

When get_perf_callchain() calls get_callchain_entry(), it accesses
callchain_cpus_entries using rcu_dereference(). Since rcu_dereference()
explicitly checks for rcu_read_lock_held(), will this evaluate to false
and trigger a suspicious rcu_dereference_check() warning?

>  
>  	if (kernel && task) {
>  		trace = get_callchain_entry_for_task(task, max_depth);
> @@ -830,16 +829,14 @@ static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task,
>  	}

[Severity: Critical]
This is a pre-existing issue, but can synchronous re-entrancy from BPF
trampolines enable a Time-Of-Check to Time-Of-Use (TOCTOU) vulnerability?

get_perf_callchain() acquires the shared per-CPU perf_callchain_entry
buffer but drops the recursion lock via put_callchain_entry() right
before returning.

If a BPF fexit program is attached to get_perf_callchain(), it will run
synchronously before __bpf_get_stack() resumes. The fexit program can
recursively call bpf_get_stack() with a much larger size. Since the lock
was dropped, it re-acquires the identical buffer and inflates trace->nr.

Upon returning, __bpf_get_stack() passes the inflated trace to
callchain_store(). Because callchain_store() blindly trusts trace->nr
without re-validating it against max_depth, will this result in an
out-of-bounds memcpy()?

>  
>  	if (unlikely(!trace) || trace->nr < skip) {
> -		if (may_fault)
> -			rcu_read_unlock();
> +		preempt_enable();
>  		goto err_fault;
>  	}
>  
>  	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);

[Severity: High]
This is a pre-existing issue, but does bpf_get_stack_pe() permanently
truncate the shared perf callchain length for user stacks?

When bpf_get_stack_pe() is called with BPF_F_USER_STACK, it routes to
__bpf_get_stack_pe(), passing the shared perf event buffer:

kernel/bpf/stackmap.c:__bpf_get_stack_pe() {
    ...
    trace->nr = min_t(u32, trace->nr, max_depth);
    ...
}

Unlike the kernel branch which explicitly saves and restores trace->nr,
the user branch fails to restore the original length before returning in
bpf_get_stack_pe().

Can this inadvertently or maliciously corrupt the event record for any
subsequent users of the perf subsystem?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720085351.655075-1-jolsa@kernel.org?part=9