On Wed, 2026-10-14 at 22:21 +0000, [email protected] wrote: > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) > to consider: > - [High] tracing: Data race between osn_reset_cpu memset and NMI handler > -- > > commit 01cd6ef0e171fa1526bc0f401fc2072fe284c438 > Author: Crystal Wood <[email protected]> > > tracing/osnoise: Split out per-cpu variable resets > > This commit splits out per-cpu variable resets by resetting the per-cpu > state every time a kthread is stopped or an fd is detached. It uses a bulk > memset to clear the zero struct_group in osn_reset_cpu() to avoid zeroing > individual fields explicitly. > > > diff --git a/kernel/trace/trace_osnoise.c b/kernel/trace/trace_osnoise.c > > --- a/kernel/trace/trace_osnoise.c > > +++ b/kernel/trace/trace_osnoise.c > > [ ... ] > > > @@ -409,6 +379,36 @@ static struct osnoise_data { > > #endif > > }; > > > > +static inline void osn_reset_cpu(int cpu) > > +{ > > + struct osnoise_variables *osn = cpu_osn_var(cpu); > > + > > + WARN_ON(osn->kthread); > > + WARN_ON(osn->fdd); > > + memset(&osn->zero, 0, sizeof(osn->zero)); > > [Severity: High] > Does this non-atomic memset race with the NMI handler > trace_osnoise_callback()? > > If osn_reset_cpu() is invoked from a remote CPU while the osnoise tracer is > still globally active (for example, via timerlat_fd_release() or > stop_kthread()), it will concurrently overwrite osn->zero on the target CPU. > > The target CPU's NMI handler could be running concurrently and might bypass > the sampling check if it reads it before or during the zeroing. This would > lead to it executing operations like: > > kernel/trace/trace_osnoise.c:trace_osnoise_callback() { > ... > local_inc(&osn_var->int_counter); > ... > } > > A concurrent memset from another CPU destroys the target CPU's local_t > guarantees and atomic operations, corrupting per-CPU state via torn reads > and writes. This replaces what was previously a safe single-byte > osn_var->sampling = false assignment with a bulk memset over the entire > state.
Ugh... so on one hand I wouldn't call this "high priority" given the way the data is generally read as deltas. Most of it doesn't really need to be reset. It's not limited to NMIs though, as we don't disable interrupts or even preemption here. On the other hand, now that sampling is unconditionally cleared in stop_kthread() as of patch 3, we might be able to just drop this patch... but not without a respin unfortunately. There was apparently a patch shuffling accident in patch 3 where tlat->timer is used after memset() clears it, which this patch undoes. I'm surprised Sashiko didn't mention that... v5 coming. -Crystal
