Re: [PATCH bpf-next v12 3/6] bpf: Add sleepable support for classic tracepoint programs

[sashiko-bot@kernel.org]

Sashiko AI review found 2 potential issue(s):
- [Critical] Inverted RCU lock logic in `bpf_prog_run_array_sleepable()` causes sleepable programs to run under `rcu_read_lock()` and non-sleepable programs to run without it.
- [High] Uninitialized kernel stack memory is leaked to BPF programs via the `param` struct in `perf_call_bpf_enter()` and `perf_call_bpf_exit()`.
--

commit 37b318a3f61844cb76121bebc8014d8031e06996
Author: Mykyta Yatsenko <yatsenko@meta.com>

bpf: Add sleepable support for classic tracepoint programs

This commit adds trace_call_bpf_faultable() for faultable tracepoints,
supporting sleepable bpf programs. It also restructures perf syscall hooks
to run bpf before preempt_disable.

> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 4e763dd2aa2bb..bbc0710615014 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c

[ ... ]

> +unsigned int trace_call_bpf_faultable(struct trace_event_call *call, void *ctx)
> +{
> +	struct bpf_prog_array *prog_array;
> +
> +	might_fault();
> +	guard(rcu_tasks_trace)();
> +
> +	prog_array = rcu_dereference_check(call->prog_array,
> +					   rcu_read_lock_trace_held());
> +	return bpf_prog_run_array_sleepable(prog_array, ctx, bpf_prog_run);
> +}

Does bpf_prog_run_array_sleepable() properly handle rcu locks for sleepable
programs?

A preceding patch introduced bpf_prog_run_array_sleepable() which appears
to have an inverted logic check:

if (prog->sleepable) {
    guard(rcu)();
    ...
}

This causes sleepable programs to execute with the rcu read lock held,
preventing them from sleeping. Conversely, non-sleepable bpf programs run
without the rcu read lock, which could lead to regressions when accessing
rcu-protected data.

> diff --git a/kernel/trace/trace_syscalls.c b/kernel/trace/trace_syscalls.c
> index 8ad72e17d8eb9..e98ee7e1e66f4 100644
> --- a/kernel/trace/trace_syscalls.c
> +++ b/kernel/trace/trace_syscalls.c
> @@ -1371,33 +1371,33 @@ static DECLARE_BITMAP(enabled_perf_exit_syscalls, NR_syscalls);
>  static int sys_perf_refcount_enter;
>  static int sys_perf_refcount_exit;
>  
> -static int perf_call_bpf_enter(struct trace_event_call *call, struct pt_regs *regs,
> +static int perf_call_bpf_enter(struct trace_event_call *call,
>  			       struct syscall_metadata *sys_data,
> -			       struct syscall_trace_enter *rec)
> +			       int syscall_nr, unsigned long *args)
>  {
>  	struct syscall_tp_t {
>  		struct trace_entry ent;
>  		int syscall_nr;
>  		unsigned long args[SYSCALL_DEFINE_MAXARGS];
>  	} __aligned(8) param;
> +	struct pt_regs regs = {};

Is it possible for this code to leak uninitialized kernel stack memory to
bpf programs?

The param struct is allocated on the stack but not fully initialized. On
32-bit systems, overwriting the first few bytes only partially covers ent,
leaving ent.pid uninitialized. 

Additionally, if the system call has fewer than 6 arguments, the trailing
elements of param.args remain uninitialized and could be read by bpf
tracepoint programs.

[ ... ]

> -static int perf_call_bpf_exit(struct trace_event_call *call, struct pt_regs *regs,
> -			      struct syscall_trace_exit *rec)
> +static int perf_call_bpf_exit(struct trace_event_call *call,
> +			      int syscall_nr, long ret_val)
>  {
>  	struct syscall_tp_t {
>  		struct trace_entry ent;
>  		int syscall_nr;
>  		unsigned long ret;
>  	} __aligned(8) param;
> -
> -	/* bpf prog requires 'regs' to be the first member in the ctx (a.k.a. &param) */
> -	perf_fetch_caller_regs(regs);
> -	*(struct pt_regs **)&param = regs;
> -	param.syscall_nr = rec->nr;
> -	param.ret = rec->ret;
> -	return trace_call_bpf(call, &param);
> +	struct pt_regs regs = {};

Could this also leak uninitialized padding to bpf programs?

Similar to perf_call_bpf_enter(), param is not zero-initialized here.
On 64-bit systems, compiler-inserted padding between syscall_nr (4 bytes)
and ret (8 bytes) is left uninitialized and could also be read.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260422-sleepable_tracepoints-v12-0-744bf0e3b311@meta.com?part=3