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

On 4/23/26 12:35 AM, [email protected] wrote:
> Sashiko AI review found 2 potential issue(s):
> - [High] Uninitialized kernel stack memory leaked to BPF programs via `param` structure.
> - [High] Moving BPF program execution before `syscall_get_data()` breaks `bpf_probe_read_user()` for non-sleepable programs by removing implicit page faulting.
> --
> 
> commit 6d42174eb2c9a4cc877bdf6b63e5b37b8017a109
> Author: Mykyta Yatsenko <[email protected]>
> 
> bpf: Add sleepable support for classic tracepoint programs
> 
> This commit adds support for sleepable BPF programs on syscall tracepoints
> by moving the BPF program execution out of the non-preemptible ring buffer
> allocation path. It introduces trace_call_bpf_faultable() and restructures
> perf_syscall_enter() and perf_syscall_exit().
> 
>> 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 = {};
> 
> Could param be zero-initialized here?
> 
> Because param is allocated on the kernel stack without zero-initialization,
> there is a 4-byte padding between syscall_nr and args. Additionally, only
> sys_data->nb_args elements of the args array are initialized, leaving the
> remaining elements uninitialized.
> 
> Since the BPF verifier allows tracing programs to read the full context size,
> this padding and uninitialized array elements can be read by BPF programs,
> leaking kernel stack memory. Zero-initializing param (e.g., param = {})
> would prevent this.
> 

This is pre-existing, I'm not sure what is our stance on this problem in 
general. If I had to guess, leaking kernel stack memory to BPF is not an 
issue, because we can do bpf_probe_read_kernel() if we need it anyway.

> [ ... ]
> 
>> @@ -1429,6 +1424,26 @@ static void perf_syscall_enter(void *ignore, struct pt_regs *regs, long id)
>>   
>>   	syscall_get_arguments(current, regs, args);
>>   
>> +	/*
>> +	 * Run BPF program in faultable context before per-cpu buffer
>> +	 * allocation, allowing sleepable BPF programs to execute.
>> +	 */
>> +	valid_prog_array = bpf_prog_array_valid(sys_data->enter_event);
>> +	if (valid_prog_array &&
>> +	    !perf_call_bpf_enter(sys_data->enter_event, sys_data,
>> +				 syscall_nr, args))
>> +		return;
>> +
> 
> Does moving BPF program execution here break bpf_probe_read_user() for
> existing non-sleepable BPF programs?
> 
> Previously, BPF programs ran after syscall_get_data() was called, which
> explicitly faulted in user memory for string arguments. Because the memory
> became resident, non-sleepable BPF programs could use bpf_probe_read_user()
> to reliably read those arguments without taking page faults.
> 
> Now that the BPF program runs before syscall_get_data(), if the user memory
> is paged out, bpf_probe_read_user() in non-sleepable programs might fail
> with -EFAULT.
> 

Not sure if previous behaviour was purposful. bpf_probe_read_user() is
best-effort no guarantee that the data is paged in. This patch set
unlocks sleepable tracepoints, which should be used in these scenarios 
when user data needs to be read reliably.

> [ ... ]
> 
>> -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 = {};
> 
> Similarly to perf_call_bpf_enter(), could param be zero-initialized here
> to avoid leaking kernel stack memory through the padding between syscall_nr
> and ret?
>