Re: [PATCH 1/4] tracing/osnoise: Per-cpu mutex and fd detachment

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Deadlock / Priority Inversion in `timerlat_fd_read()` due to holding a mutex across an indefinite sleep.
- [High] Uninitialized Mutex for offline CPUs leads to crash during CPU hotplug.
- [High] AB-BA deadlock between `stop_kthread()` and `osnoise_migration_pending()`.
- [Medium] Spurious `WARN_ON` due to a contradictory double clear of `kthread_cpumask`.
--

--- Patch [1]: [PATCH 1/4] tracing/osnoise: Per-cpu mutex and fd detachment ---
commit 3d6fdd195012eb0c62c4c6d7f8152a80a0f97897
Author: Crystal Wood <crwood@redhat.com>

tracing/osnoise: Per-cpu mutex and fd detachment

Clean up a variety of synchronization issues and related bandaids
by having a per-cpu mutex that guards changes to kthread, and
fd open/close/revoke. Replace the SIGKILL hack with a mutex-protected
detachment mechanism.

> diff --git a/kernel/trace/trace_osnoise.c b/kernel/trace/trace_osnoise.c
> index 0e1265acd1ccc..e2e1ef3f5a617 100644
> --- a/kernel/trace/trace_osnoise.c
> +++ b/kernel/trace/trace_osnoise.c

[ ... ]

> @@ -1688,10 +1704,10 @@ static inline int osnoise_migration_pending(void)
>  	 * The tracers are responsible for cleaning their env before
>  	 * exiting.
>  	 */
> -	mutex_lock(&interface_lock);
> +	mutex_lock(&osn->lock);
>  	this_cpu_osn_var()->kthread = NULL;
>  	cpumask_clear_cpu(smp_processor_id(), &kthread_cpumask);
> -	mutex_unlock(&interface_lock);
> +	mutex_unlock(&osn->lock);
>  
>  	return 1;
>  }

[ ... ]

> @@ -1978,32 +1994,22 @@ static int timerlat_main(void *data)
>   */
>  static void stop_kthread(unsigned int cpu)
>  {
> -	struct task_struct *kthread;
> +	struct osnoise_variables *osn_var = cpu_osn_var(cpu);
>  
> -	kthread = xchg_relaxed(&(per_cpu(per_cpu_osnoise_var, cpu).kthread), NULL);
> -	if (kthread) {
> -		if (cpumask_test_and_clear_cpu(cpu, &kthread_cpumask) &&
> -		    !WARN_ON(!test_bit(OSN_WORKLOAD, &osnoise_options))) {
> -			kthread_stop(kthread);
> -		} else if (!WARN_ON(test_bit(OSN_WORKLOAD, &osnoise_options))) {
> -			/*
> -			 * This is a user thread waiting on the timerlat_fd. We need
> -			 * to close all users, and the best way to guarantee this is
> -			 * by killing the thread. NOTE: this is a purpose specific file.
> -			 */
> -			kill_pid(kthread->thread_pid, SIGKILL, 1);
> -			put_task_struct(kthread);
> -		}
> -	} else {
> -		/* if no workload, just return */
> -		if (!test_bit(OSN_WORKLOAD, &osnoise_options)) {
> -			/*
> -			 * This is set in the osnoise tracer case.
> -			 */
> -			per_cpu(per_cpu_osnoise_var, cpu).sampling = false;
> -			barrier();
> -		}
> +	mutex_lock(&osn_var->lock);
> +
> +	if (osn_var->fdd) {
> +		WARN_ON(test_bit(OSN_WORKLOAD, &osnoise_options));
> +		WARN_ON(osn_var->kthread);
> +		osn_var->fdd->detach(osn_var->fdd);
> +	} else if (osn_var->kthread) {
> +		WARN_ON(!cpumask_test_and_clear_cpu(cpu, &kthread_cpumask));
> +		WARN_ON(!test_bit(OSN_WORKLOAD, &osnoise_options));
> +		kthread_stop(osn_var->kthread);

[Severity: High]
Can holding osn_var->lock while calling kthread_stop() cause a deadlock?

kthread_stop() blocks waiting for the target kthread to exit. However, if
the kthread is currently handling a migration request in
osnoise_migration_pending() (shown slightly earlier in the diff), it will
attempt to acquire this same osn_var->lock before it can clear its state and
exit:

osnoise_migration_pending() {
    ...
    mutex_lock(&osn->lock);
    this_cpu_osn_var()->kthread = NULL;
    ...
}

If stop_kthread() holds the lock while waiting for the thread to exit, and
the kthread waits for the lock to exit, they will deadlock and hang the
system. Is there a way to call kthread_stop() outside of the mutex critical
section?

> +		osn_var->kthread = NULL;
>  	}
> +
> +	mutex_unlock(&osn_var->lock);
>  }

[ ... ]

> @@ -2086,15 +2100,9 @@ static int start_per_cpu_kthreads(void)
>  	 */
>  	cpumask_and(current_mask, cpu_online_mask, &osnoise_cpumask);
>  
> -	for_each_possible_cpu(cpu) {
> -		if (cpumask_test_and_clear_cpu(cpu, &kthread_cpumask)) {
> -			struct task_struct *kthread;
> -
> -			kthread = xchg_relaxed(&(per_cpu(per_cpu_osnoise_var, cpu).kthread), NULL);
> -			if (!WARN_ON(!kthread))
> -				kthread_stop(kthread);
> -		}
> -	}
> +	for_each_possible_cpu(cpu)
> +		if (cpumask_test_and_clear_cpu(cpu, &kthread_cpumask))
> +			stop_kthread(cpu);

[Severity: Medium]
Will this always trigger the WARN_ON() inside stop_kthread()?

Here in start_per_cpu_kthreads(), the kthread_cpumask bit is cleared prior
to calling stop_kthread(cpu). 

But in stop_kthread(cpu) introduced above, it expects the bit to still be
set and asserts:
WARN_ON(!cpumask_test_and_clear_cpu(cpu, &kthread_cpumask));

Since the caller already cleared the bit, won't this result in a guaranteed
warning stack trace whenever this recovery path is executed?

>  
>  	for_each_cpu(cpu, current_mask) {
>  		retval = start_kthread(cpu);

[ ... ]

> @@ -2528,6 +2577,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(&osn_var->lock);
> +	if (fdd != osn_var->fdd || current != fdd->thread) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
>  	/*
>  	 * The timerlat in user-space runs in a different order:
>  	 * the read() starts from the execution of the previous occurrence,
> @@ -2574,6 +2630,11 @@ timerlat_fd_read(struct file *file, char __user *ubuf, size_t count,
>  	/* wait for the next period */
>  	wait_next_period(tlat);
>  
> +	if (fdd != osn_var->fdd) {
> +		ret = -EINVAL;
> +		goto out;
> +	}

[Severity: High]
Is it safe to hold osn_var->lock across a sleep in TASK_INTERRUPTIBLE?

The function acquires osn_var->lock and then calls wait_next_period(),
which sets the task state to TASK_INTERRUPTIBLE and calls schedule().

If a user task calling read() is suspended (e.g., via SIGSTOP), it could
remain in TASK_STOPPED and sleep indefinitely while holding the mutex.
This would permanently block any administrator actions like CPU hotplug
or stopping the tracer, since stop_kthread() attempts to acquire this same
lock.

[ ... ]

> @@ -3157,10 +3203,13 @@ __init static int init_timerlat_tracer(void)
>  
>  __init static int init_osnoise_tracer(void)
>  {
> -	int ret;
> +	int ret, cpu;
>  
>  	mutex_init(&interface_lock);
>  
> +	for_each_online_cpu(cpu)
> +		mutex_init(&cpu_osn_var(cpu)->lock);

[Severity: High]
Does this skip possible CPUs that are offline during initialization?

If a CPU is offline at boot time, its mutex won't be initialized here.
Later, if an administrator hotplugs this CPU, osnoise_cpu_init() will
schedule a workqueue to invoke start_kthread() on the newly online CPU.
start_kthread() will immediately attempt to acquire the uninitialized lock,
leading to a system crash or lockdep splat.

Should this use for_each_possible_cpu(cpu) instead to ensure all CPUs
have their mutex properly initialized for hotplug?

> +
>  	cpumask_copy(&osnoise_cpumask, cpu_all_mask);
>  
>  	ret = register_tracer(&osnoise_tracer);

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