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

From: sashiko-bot@kernel.org
Date: 2026-08-24 21:30:38

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 [off-list ref]

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.

quoted hunk ↗ jump to hunk

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

[ ... ]

quoted hunk ↗ jump to hunk

@@ -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;
 }

[ ... ]

quoted hunk ↗ jump to hunk

@@ -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);
 }

[ ... ]

quoted hunk ↗ jump to hunk

@@ -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);

[ ... ]

quoted hunk ↗ jump to hunk

@@ -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.

[ ... ]

quoted hunk ↗ jump to hunk

@@ -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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help