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