[PATCH] tracing: perf: Fix stale head for perf syscall tracing

[PATCH] tracing: perf: Fix stale head for perf syscall tracing

[Steven Rostedt]

From: Steven Rostedt <rostedt@goodmis.org>

The code that can read the user space parameters of a system call may
enable preemption and migrate. The head of the per CPU perf events list
may be pointing to the wrong CPU event if the code migrates.

Move the taking assignment of the head pointer to after the faulting code
is called.

Cc: stable@vger.kernel.org
Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://sashiko.dev/#/patchset/20260717173252.3431565-1-usama.arif%40linux.dev
Fixes: edca33a56297d ("tracing: Fix failure to read user space from system call trace events")
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/trace_syscalls.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/kernel/trace/trace_syscalls.c b/kernel/trace/trace_syscalls.c
index e98ee7e1e66f..e541e964d193 100644
--- a/kernel/trace/trace_syscalls.c
+++ b/kernel/trace/trace_syscalls.c
@@ -1440,19 +1440,20 @@ static void perf_syscall_enter(void *ignore, struct pt_regs *regs, long id)
 	 */
 	guard(preempt_notrace)();
 
-	head = this_cpu_ptr(sys_data->enter_event->perf_events);
-	if (hlist_empty(head))
-		return;
-
 	/* Check if this syscall event faults in user space memory */
 	mayfault = sys_data->user_mask != 0;
 
 	if (mayfault) {
+		/* Enables preemption and may migrate */
 		if (syscall_get_data(sys_data, args, &user_ptr,
 				     &size, user_sizes, &uargs, buf_size) < 0)
 			return;
 	}
 
+	head = this_cpu_ptr(sys_data->enter_event->perf_events);
+	if (hlist_empty(head))
+		return;
+
 	/* get the size after alignment with the u32 buffer size field */
 	size += sizeof(unsigned long) * sys_data->nb_args + sizeof(*rec);
 	size = ALIGN(size + sizeof(u32), sizeof(u64));
-- 
2.53.0

Re: [PATCH] tracing: perf: Fix stale head for perf syscall tracing

[Steven Rostedt]

From Sashiko: https://sashiko.dev/#/patchset/20260724184250.357e91f1%40gandalf.local.home

> --- Patch [1]: [PATCH] tracing: perf: Fix stale head for perf syscall tracing ---
> commit c99950733eb63d5661f5e4d25f82dff506833b49
> Author: Steven Rostedt <rostedt@goodmis.org>
> 
> tracing: perf: Fix stale head for perf syscall tracing
> 
> This commit delays capturing the head of the per CPU perf events list until
> after the faulting code completes. This prevents the head pointer from pointing
> to the wrong CPU event if the syscall data read enables preemption and migrates.
> 
> Link: https://sashiko.dev/#/patchset/20260717173252.3431565-1-usama.arif%40linux.dev
> 
> > diff --git a/kernel/trace/trace_syscalls.c b/kernel/trace/trace_syscalls.c
> > index e98ee7e1e66f4..e541e964d1930 100644
> > --- a/kernel/trace/trace_syscalls.c
> > +++ b/kernel/trace/trace_syscalls.c
> > @@ -1440,19 +1440,20 @@ static void perf_syscall_enter(void *ignore, struct pt_regs *regs, long id)
> 
> [ ... ]
> 
> >  	guard(preempt_notrace)();
> >  
> > -	head = this_cpu_ptr(sys_data->enter_event->perf_events);
> > -	if (hlist_empty(head))
> > -		return;
> > -
> >  	/* Check if this syscall event faults in user space memory */
> >  	mayfault = sys_data->user_mask != 0;
> >  
> >  	if (mayfault) {
> > +		/* Enables preemption and may migrate */
> >  		if (syscall_get_data(sys_data, args, &user_ptr,
> >  				     &size, user_sizes, &uargs, buf_size) < 0)
> >  			return;
> >  	}
> >  
> > +	head = this_cpu_ptr(sys_data->enter_event->perf_events);
> > +	if (hlist_empty(head))
> > +		return;
> 
> Will moving this early check below syscall_get_data() cause a severe
> performance regression for untraced CPUs?

I was thinking this might be the case. I originally thought it wouldn't be
too much of an issue, but I can see that the reading of user space when you
don't need to would be a regression in performance.

I'll change the above to be:

	guard(preempt_notrace)();

	head = this_cpu_ptr(sys_data->enter_event->perf_events);
	if (hlist_empty(head))
		return;

	/* Check if this syscall event faults in user space memory */
	mayfault = sys_data->user_mask != 0;

	if (mayfault) {
		if (syscall_get_data(sys_data, args, &user_ptr,
				     &size, user_sizes, &uargs, buf_size) < 0)
			return;

		/* The above may have caused a migration */
		head = this_cpu_ptr(sys_data->enter_event->perf_events);
		if (hlist_empty(head))
			return;
	}

-- Steve



> 
> Because the enabled_perf_enter_syscalls bitmap is global, enabling a perf
> syscall trace on one CPU causes perf_syscall_enter() to be invoked on all
> CPUs executing that syscall.
> 
> Previously, the early hlist_empty() check allowed CPUs not actively tracing
> the event to return immediately.
> 
> By moving this check until after syscall_get_data(), doesn't this force all
> untraced CPUs to unconditionally execute expensive user-space memory copies
> and preemption toggling before discarding the data?