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 <[email protected]> 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/[email protected]?part=1
