Re: [PATCH v5 4/8] 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: broken state on signal restart in timerlat_fd_read()
- [Medium] tracing/osnoise: migration check precedes fd validation in timerlat_fd_read()
--

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

tracing/osnoise: Per-cpu mutexes and fd detachment

This commit introduces per-CPU mutexes to synchronize kthread and timerlat fd
lifecycle operations and replaces the SIGKILL mechanism for detaching
user-space timerlat threads with a mutex-protected fd detachment mechanism.

> diff --git a/Documentation/trace/timerlat-tracer.rst b/Documentation/trace/timerlat-tracer.rst
> index 68d429d454a54..ea94627e821b5 100644
> --- a/Documentation/trace/timerlat-tracer.rst
> +++ b/Documentation/trace/timerlat-tracer.rst
> @@ -220,8 +220,9 @@ After reporting the total latency, timerlat will restart the cycle, arm
>  a timer, and go to sleep for the following activation.
>  
>  If at any time one of the conditions is broken, e.g., the thread migrates
> -while in user space, or the timerlat tracer is disabled, the SIG_KILL
> -signal will be sent to the user-space thread.
> +while in user space, or the timerlat tracer is disabled, the fd will be
> +detached from the tracer, and subsequent usage will fail with EINVAL.
> +Note that older kernels may deliver a SIGKILL instead.
>  
>  Here is an basic example of user-space code for timerlat::
>  
> diff --git a/kernel/trace/trace_osnoise.c b/kernel/trace/trace_osnoise.c
> index 40cbabb98a1ba..f383bc4df6b7b 100644
> --- a/kernel/trace/trace_osnoise.c
> +++ b/kernel/trace/trace_osnoise.c

[ ... ]

> @@ -2498,12 +2574,13 @@ 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();
>  
> @@ -2514,13 +2591,13 @@ timerlat_fd_read(struct file *file, char __user *ubuf, size_t count,
>  	 * 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;
>  		}
>  	} 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();
> @@ -2529,6 +2606,13 @@ timerlat_fd_read(struct file *file, char __user *ubuf, size_t count,
>  
>  	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;
> +	}

[Severity: Medium]
In timerlat_fd_read(), the CPU migration check runs before acquiring
tlat->detach_lock and before verifying that fdd == osn_var->fdd and
current == fdd->thread.

Because stop_kthread() now detaches the fd via timerlat_fd_detach() instead
of sending SIGKILL, the user-space thread stays alive with an open file
descriptor after detachment. If that thread later migrates to another CPU
and calls read() on the detached fd (or if another thread sharing the file
table calls read() on a CPU other than fdd->cpu), timerlat_fd_read() takes
the fdd->cpu != smp_processor_id() branch:

	cpu_tmr_var(fdd->cpu)->uthread_migrate = 1;
	osnoise_taint("timerlat user thread migrate\n");
	osnoise_stop_tracing();

This taints the tracer, stops tracing (or panics if OSN_PANIC_ON_STOP is
set), and leaves cpu_tmr_var(fdd->cpu)->uthread_migrate set to 1. Because
fdd is already detached (fdd != osn_var->fdd), a subsequent close() in
timerlat_fd_release():

	mutex_lock(&osn_var->lock);
	if (fdd == osn_var->fdd)
		timerlat_fd_detach(fdd);
	mutex_unlock(&osn_var->lock);

will not call timerlat_fd_detach() to zero tlat->zero, and
timerlat_fd_open() does not clear tlat->uthread_migrate either, leaving new
timerlat fd sessions on fdd->cpu broken until the tracer is restarted.

Also, Documentation/trace/timerlat-tracer.rst states that if the thread
migrates while in user space, the fd will be detached from the tracer, but
neither trace_sched_migrate_callback() nor timerlat_fd_read() calls
timerlat_fd_detach().

Should timerlat_fd_read() validate that fdd is still attached and owned by
current before acting on migration, and should migration detach the fd?

>  	/*
>  	 * The timerlat in user-space runs in a different order:
>  	 * the read() starts from the execution of the previous occurrence,
> @@ -2563,9 +2647,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);
>  
> @@ -2573,7 +2654,19 @@ 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;
> +
> +		goto out;
> +	}

[Severity: Medium]
What happens when wait_next_period() in timerlat_fd_read() is interrupted by
a signal and returns -ERESTARTSYS?

Before calling wait_next_period(), timerlat_fd_read() has either set
osn_var->sampling = 1 (on the first read) or already recorded the
THREAD_URET latency sample for the previous wakeup (on subsequent reads).
Inside wait_next_period(), tlat->abs_period is advanced to next_abs_period,
and when signal_pending(current) is true, the timer is cancelled via
hrtimer_cancel(&tlat->timer) and -1 is returned:

	if (tlat->detach || signal_pending(current)) {
		hrtimer_cancel(&tlat->timer);
		return -1;
	}

When the read() syscall is restarted (or retried by user space after EINTR),
osn_var->sampling is still 1, so timerlat_fd_read() enters the
if (likely(osn_var->sampling)) branch at the top of the function:

	if (likely(osn_var->sampling)) {
		now = ktime_to_ns(hrtimer_cb_get_time(&tlat->timer));
		diff = now - tlat->abs_period;

		/*
		 * it was not a timer firing, but some other signal?
		 */
		if (diff < 0)
			goto out;

		s.seqnum = tlat->count;
		s.timer_latency = diff;
		s.context = THREAD_URET;

		record_timerlat_sample(&s);

While now is still earlier than the future tlat->abs_period, diff < 0 is
true and timerlat_fd_read() jumps to out and returns 0 (EOF) without arming
the timer or sleeping. Once now reaches tlat->abs_period, diff >= 0 and
timerlat_fd_read() records a bogus or duplicate THREAD_URET sample and
checks stop_tracing_total for a timer activation that was cancelled and
never fired.

Does timerlat_fd_read() need to avoid treating the next read() after a
signal interruption as a wakeup from a completed timer cycle?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261024015614.3095251-2-crwood@redhat.com?part=4