[PATCH bpf] bpf: Always hold RCU read lock around perf callchain in __bpf_get_stack()

[Songsen Yang]

__bpf_get_stack() only takes rcu_read_lock() around get_perf_callchain()
when may_fault is true, assuming that non-faultable callers already run
inside an RCU read-side critical section.

That assumption does not hold for BPF_PROG_TYPE_SYSCALL programs. They
are always sleepable and bpf_prog_test_run_syscall() only holds
rcu_read_lock_trace() while running them, yet through
tracing_prog_func_proto() they are given the non-sleepable
bpf_get_stack_raw_tp() helper, which calls __bpf_get_stack() with
may_fault == false. get_callchain_entry() then does
rcu_dereference(callchain_cpus_entries) without rcu_read_lock() held,
and lockdep complains:

  WARNING: suspicious RCU usage
  kernel/events/callchain.c:163 suspicious rcu_dereference_check() usage!
  1 lock held by repro:
   #0: (rcu_tasks_trace_srcu_struct){....}-{0:0}, at: bpf_prog_test_run_syscall
  Call Trace:
   lockdep_rcu_suspicious
   get_callchain_entry
   get_perf_callchain
   __bpf_get_stack
   bpf_get_stack_raw_tp
   bpf_prog_test_run_syscall
   __sys_bpf

The callchain buffers are freed with call_rcu(), and an RCU Tasks Trace
read-side section does not imply a normal RCU read-side section, so
holding rcu_read_lock_trace() alone is not what get_callchain_entry()
expects.

Take rcu_read_lock() unconditionally. Nesting it inside an existing RCU
read-side critical section is cheap and harmless, as suggested by Andrii
in an earlier discussion.

bpf_get_stackid() is not affected: sleepable programs cannot use
BPF_MAP_TYPE_STACK_TRACE maps, so syscall programs cannot reach it.

Testing: on top of bpf/master at commit c8a8a68d3ab4 ("bpf: Don't re-sort
subprog_info[] for every new subprogram"), x86_64 under QEMU TCG with
CONFIG_PROVE_RCU=y (syzbot config with KASAN/KCOV/modules disabled), the
syzbot C reproducer
triggers the splat above without this patch and does not with it. A
minimal BPF_PROG_TYPE_SYSCALL program calling bpf_get_stack() was also
run via BPF_PROG_TEST_RUN: it warns before, and returns 0 without a
warning after. BPF selftests were not run.

This issue was located, and the fix, changelog and tests were prepared,
with the help of an AI coding assistant (Claude); the submitter has
reviewed the change.

Fixes: 79a7f8bdb159 ("bpf: Introduce bpf_sys_bpf() helper and program type.")
Reported-by: syzbot+72a43cdb78469f7fbad1@xxxxxxxxxxxxxxxxxxxxxxxxx
Closes: https://syzkaller.appspot.com/bug?extid=72a43cdb78469f7fbad1
Link: https://lore.kernel.org/all/CAEf4BzYN33yQhO-tsF8pUWDMZoiLAXkV+Z1oP6fJkByocYcjpw@xxxxxxxxxxxxxx/
Signed-off-by: Songsen Yang <yangsongsen@xxxxxxxxxxxx>
---
 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 f7e8d766d128..e452ca7c859a 100644
--- a/kernel/bpf/stackmap.c
+++ b/kernel/bpf/stackmap.c
@@ -812,8 +812,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);
 
 	preempt_disable();
-	if (may_fault)
-		rcu_read_lock(); /* need RCU for perf's callchain below */
+	rcu_read_lock(); /* need RCU for perf's callchain below */
 
 	if (kernel && task) {
 		trace = get_callchain_entry_for_task(task, max_depth);
@@ -823,8 +822,7 @@ 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();
+		rcu_read_unlock();
 		preempt_enable();
 		goto err_fault;
 	}
@@ -832,8 +830,7 @@ static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task,
 	trace_nr = callchain_store(trace, trace->nr, buf, elem_size, flags);
 
 	/* trace should not be dereferenced after this point */
-	if (may_fault)
-		rcu_read_unlock();
+	rcu_read_unlock();
 	preempt_enable();
 
 	return callchain_finalize(buf, size, trace_nr, elem_size, flags, may_fault);
-- 
2.50.1