On Wed, 2026-05-20 at 16:28 -0400, Steven Rostedt wrote:
> [ Replying to Sashiko:
> https://sashiko.dev/?list=org.kernel.vger.linux-trace-kernel#/patchset/20260511223035.1475676-1-crwood%40redhat.com
> ]
>
> > commit 859dc1eded9e36304f27e72643f28f30f327f078
> > Author: Crystal Wood <[email protected]>
> > tracing/osnoise: Array printk init and cleanup
> > This commit initializes the trace array buffer for osnoise instances on
> > creation. It also introduces the osnoise_print() helper function and
> > switches
> > the tracer to use trace_array_printk() instead of trace_array_printk_buf().
> > > Use trace_array_printk() instead of trace_array_printk_buf(), as we're
> > > only writing to the main buffer (of a non-main instance) anyway -- and
>
> > Is this description accurate? Can the osnoise tracer also be enabled on the
> > global trace array, for example by writing to current_tracer?
> > If it is used on the global instance, trace_array_init_printk() explicitly
> > returns -EINVAL and refuses to allocate a buffer, and trace_array_printk()
> > will return 0 without printing.
>
> Hmm, I guess it would drop any of these prints if it's using the top level
> trace array. But I believe this is for use with RTLA which always creates a
> trace instance.
It doesn't *have* to be used with rtla, but you won't get the printks if
you use the main instance. The parenthetical was just meant to avoid
potential misunderstanding of the phrase "main buffer".
> > > diff --git a/kernel/trace/trace_osnoise.c b/kernel/trace/trace_osnoise.c
> > > index 75678053b21c5..2be188768ab42 100644
> > > --- a/kernel/trace/trace_osnoise.c
> > > +++ b/kernel/trace/trace_osnoise.c
> > > @@ -83,6 +83,22 @@ struct osnoise_instance {
> > >
> > > static struct list_head osnoise_instances;
> > >
> > > +static void osnoise_print(const char *fmt, ...)
> > > +{
> > > + struct osnoise_instance *inst;
> > > + struct trace_array *tr;
> > > + va_list ap;
> > > +
> > > + rcu_read_lock();
> > > + list_for_each_entry_rcu(inst, &osnoise_instances, list) {
> > > + tr = inst->tr;
> > > + va_start(ap, fmt);
> > > + trace_array_vprintk(tr, _RET_IP_, fmt, ap);
>
> > Does this code create a use-after-free on the trace array if an instance is
> > removed concurrently?
If so, it was already an issue with osnoise_taint(),
osnoise_stop_tracing(), osnoise_stop_exception(), etc. Wouldn't be
surprising, as this file has a number of other synchronization issues as
well.
> > When a user deletes a trace instance via rmdir, the unregister function
> > removes the instance from the list using list_del_rcu(). However, the
> > removal
> > routine does not appear to wait for an RCU grace period before freeing the
> > trace array itself.
> > Could a concurrent execution of this loop inside the rcu_read_lock() section
> > still access the unlinked instance, read the freed inst->tr, and pass it to
> > trace_array_vprintk()? This appears to be an existing issue, but it still
> > affects the loop here.
>
> Hmm, this is interesting. osnoise keeps track of its own instances via a
> osnoise_instances list. But it only use kfree_rcu() to free the list
> descriptor but doesn't take care of the tr being freed before hand!
>
> Something like this could work [not even compiled]
>
> diff --git a/kernel/trace/trace_osnoise.c b/kernel/trace/trace_osnoise.c
> index 75678053b21c..bda1e0e0d2e1 100644
> --- a/kernel/trace/trace_osnoise.c
> +++ b/kernel/trace/trace_osnoise.c
> @@ -476,8 +476,11 @@ static void print_osnoise_headers(struct seq_file *s)
>
> \
> rcu_read_lock();
> \
> list_for_each_entry_rcu(inst, &osnoise_instances, list) {
> \
> + if (trace_array_get(inst->tr) < 0)
> \
> + continue;
> \
> buffer = inst->tr->array_buffer.buffer;
> \
> trace_array_printk_buf(buffer, _THIS_IP_, msg);
> \
> + trace_array_put(inst->tr);
> \
> }
> \
> rcu_read_unlock();
> \
> osnoise_data.tainted = true;
> \
OK, I'll prepare a v3.
> > > @@ -1189,10 +1198,10 @@ static __always_inline void
> > > osnoise_stop_exception(char *msg, int cpu)
> > > rcu_read_lock();
> > > list_for_each_entry_rcu(inst, &osnoise_instances, list) {
> > > tr = inst->tr;
> > > - trace_array_printk_buf(tr->array_buffer.buffer, _THIS_IP_,
> > > - "stop tracing hit on cpu %d due to
> > > exception: %s\n",
> > > - smp_processor_id(),
> > > - msg);
> > > + trace_array_printk(tr, _THIS_IP_,
> > > + "stop tracing hit on cpu %d due to
> > > exception: %s\n",
> > > + smp_processor_id(),
> > > + msg);
>
> > Does this switch to trace_array_printk() silently drop messages when tracing
> > on the global instance?
> > Looking at trace_array_printk(), it explicitly returns 0 if the
> > TRACE_ARRAY_FL_GLOBAL flag is set on the trace array:
> > kernel/trace/trace_printk.c:trace_array_printk() {
> > ...
> > if (tr->flags & TRACE_ARRAY_FL_GLOBAL)
> > return 0;
> > ...
> > }
> > This means stop and exception logs would be discarded if the tracer is
> > running on the global instance.
>
> Yep. But maybe that's not an issue?
I'd actually consider it a fix, if the policy is actually about not
allowing tracers to "spam" the main instance, rather than just avoiding
the percpu allocation. Especially for osnoise_stop_exception(), which
is called only one place, that already printed the same message with
osnoise_taint(). :-P
As I mentioned in the v1 patch, if trace_array_printk_buf() is going to
bypass the global instance check, should probably be internal to the
core trace code.
-Crystal
