Re: [PATCH v6 05/10] tracing/osnoise: Per-cpu mutexes and fd detachment

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] tracing/osnoise: unreachable migration handling in timerlat_fd_read()
- [Medium] tracing/osnoise: stale tracing_thread after signal in timerlat_fd_read()
--

commit fc0df59115a148fd84654dadbdbb8ffa3b889971
Author: Crystal Wood <crwood@redhat.com>

tracing/osnoise: Per-cpu mutexes and fd detachment

This patch introduces per-CPU mutexes in osnoise_variables and
timerlat_variables to synchronize kthread lifecycle and file descriptor
open, close, and detachment, replacing the previous SIGKILL mechanism
for user-space timerlat threads.

> diff --git a/kernel/trace/trace_osnoise.c b/kernel/trace/trace_osnoise.c
> index 791a9103c2ec0..cd386c75926e4 100644
> --- a/kernel/trace/trace_osnoise.c
> +++ b/kernel/trace/trace_osnoise.c

[ ... ]

> @@ -2496,37 +2572,44 @@ static ssize_t
>  timerlat_fd_read(struct file *file, char __user *ubuf, size_t count,
>  		  loff_t *ppos)
>  {
> -	long cpu = (long) file->private_data;
> +	struct fd_data *fdd = file->private_data;
>  	struct osnoise_variables *osn_var;
>  	struct timerlat_variables *tlat;
>  	struct timerlat_sample s;
>  	s64 diff;
>  	u64 now;
> +	int ret = 0;
>  
>  	migrate_disable();
>  
>  	tlat = this_cpu_tmr_var();
> +	osn_var = this_cpu_osn_var();
> +
> +	/* In normal usage, this should always be uncontended. */
> +	mutex_lock(&tlat->detach_lock);
> +	if (fdd != osn_var->fdd || current != fdd->thread) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
>  
>  	/*
>  	 * While in user-space, the thread is migratable. There is nothing
>  	 * we can do about it.
>  	 * So, if the thread is running on another CPU, stop the machinery.
>  	 */
> -	if (cpu == smp_processor_id()) {
> +	if (fdd->cpu == smp_processor_id()) {
>  		if (tlat->uthread_migrate) {
> -			migrate_enable();
> -			return -EINVAL;
> +			ret = -EINVAL;
> +			goto out;
>  		}
>  	} else {
> -		cpu_tmr_var(cpu)->uthread_migrate = 1;
> +		cpu_tmr_var(fdd->cpu)->uthread_migrate = 1;
>  		osnoise_taint("timerlat user thread migrate\n");
>  		osnoise_stop_tracing();
> -		migrate_enable();
> -		return -EINVAL;
> +		ret = -EINVAL;
> +		goto out;
>  	}

[Severity: Medium]
In timerlat_fd_open(), fdd is stored only in the osnoise_variables for
fdd->cpu.

In timerlat_fd_read(), osn_var and tlat are fetched via this_cpu_osn_var()
and this_cpu_tmr_var() for smp_processor_id(). If the user-space thread
migrated to another CPU before calling read(), osn_var belongs to the new
CPU, where osn_var->fdd cannot match fdd.

Because fdd != osn_var->fdd is checked before fdd->cpu == smp_processor_id(),
won't a migrated thread always lock the new CPU's detach_lock and bail out
at the fdd != osn_var->fdd check, making the else branch that sets
cpu_tmr_var(fdd->cpu)->uthread_migrate, calls osnoise_taint(), and stops
tracing via osnoise_stop_tracing() unreachable?

> @@ -2561,9 +2644,6 @@ timerlat_fd_read(struct file *file, char __user *ubuf, size_t count,
>  			}
>  		}
>  	} else {
> -		tlat->tracing_thread = false;
> -		tlat->thread = current;
> -
>  		/* Annotate now to drift new period */
>  		tlat->abs_period = hrtimer_cb_get_time(&tlat->timer);
>  
> @@ -2571,7 +2651,20 @@ timerlat_fd_read(struct file *file, char __user *ubuf, size_t count,
>  	}
>  
>  	/* wait for the next period */
> -	wait_next_period(tlat);
> +	if (wait_next_period(tlat) < 0) {
> +		if (signal_pending(current))
> +			ret = -ERESTARTSYS;
> +		else
> +			ret = -EINVAL;
> +
> +		osn_var->sampling = false;
> +		goto out;
> +	}

[Severity: Medium]
If timerlat_irq() fires in hardirq context while the thread is in
wait_next_period(), it sets tlat->tracing_thread = true:

kernel/trace/trace_osnoise.c:timerlat_irq() {
	...
	/*
	 * Enable the osnoise: events for thread an softirq.
	 */
	tlat->tracing_thread = true;
	...
}

If a signal is also pending before wait_next_period() checks
signal_pending(current), wait_next_period() returns -1 and timerlat_fd_read()
clears osn_var->sampling = false, but leaves tlat->tracing_thread set to
true.

Since tlat->tracing_thread = false was removed from the !osn_var->sampling
branch above, if the read() is restarted or called again, it sets
osn_var->sampling = 1 with tlat->tracing_thread still true before entering
wait_next_period(). While sleeping for the next timer activation, won't
timerlat_thread_exit() and timerlat_softirq_exit() see both osn_var->sampling
and tlat->tracing_thread as true and emit spurious noise events before the
timer expires?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028220120.4005187-1-crwood@redhat.com?part=5