[PATCH v2] tracing/osnoise: Array printk init and cleanup

[PATCH v2] tracing/osnoise: Array printk init and cleanup

[Crystal Wood]

None of the calls to trace_array_printk_buf() will do anything
if we don't initialize the buffer on instance creation (unless
some other tracer called it), so do that.

Add an osnoise_print() function to facilitate adding debug prints
(without tainting).

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
trace_array_printk_buf() skips the check to make sure we're not printing
to the global instance.

Signed-off-by: Crystal Wood <crwood@redhat.com>
---
v2: s/macro/function/ in commit message

v1: https://lore.kernel.org/all/20251112152529.956778-4-crwood@redhat.com/

 kernel/trace/trace_osnoise.c | 39 ++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 15 deletions(-)

diff --git a/kernel/trace/trace_osnoise.c b/kernel/trace/trace_osnoise.c
index 62c2667d97fa..5e83c4f6f2b4 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);
+		va_end(ap);
+	}
+	rcu_read_unlock();
+}
+
 static bool osnoise_has_registered_instances(void)
 {
 	return !!list_first_or_null_rcu(&osnoise_instances,
@@ -123,6 +139,7 @@ static int osnoise_register_instance(struct trace_array *tr)
 	 * trace_types_lock.
 	 */
 	lockdep_assert_held(&trace_types_lock);
+	trace_array_init_printk(tr);
 
 	inst = kmalloc_obj(*inst);
 	if (!inst)
@@ -471,15 +488,7 @@ static void print_osnoise_headers(struct seq_file *s)
  * osnoise_taint - report an osnoise error.
  */
 #define osnoise_taint(msg) ({							\
-	struct osnoise_instance *inst;						\
-	struct trace_buffer *buffer;						\
-										\
-	rcu_read_lock();							\
-	list_for_each_entry_rcu(inst, &osnoise_instances, list) {		\
-		buffer = inst->tr->array_buffer.buffer;				\
-		trace_array_printk_buf(buffer, _THIS_IP_, msg);			\
-	}									\
-	rcu_read_unlock();							\
+	osnoise_print(msg);							\
 	osnoise_data.tainted = true;						\
 })
 
@@ -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);
 
 		if (test_bit(OSN_PANIC_ON_STOP, &osnoise_options))
 			panic("tracer hit on cpu %d due to exception: %s\n",
@@ -1362,8 +1371,8 @@ static __always_inline void osnoise_stop_tracing(void)
 	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\n", smp_processor_id());
+		trace_array_printk(tr, _THIS_IP_,
+				   "stop tracing hit on cpu %d\n", smp_processor_id());
 
 		if (test_bit(OSN_PANIC_ON_STOP, &osnoise_options))
 			panic("tracer hit stop condition on CPU %d\n", smp_processor_id());
-- 
2.54.0

Re: [PATCH v2] tracing/osnoise: Array printk init and cleanup

[Steven Rostedt]

[ 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 <crwood@redhat.com>
> 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.

> > 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?
> 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;						\


> > +		va_end(ap);
> > +	}
> > +	rcu_read_unlock();
> > +}
> [ ... ]
> > @@ -123,6 +139,7 @@ static int osnoise_register_instance(struct trace_array *tr)
> >  	 * trace_types_lock.
> >  	 */
> >  	lockdep_assert_held(&trace_types_lock);
> > +	trace_array_init_printk(tr);

> Could this leave the per-CPU buffer unallocated when running on the global
> instance?
> If the tracer is used on the global trace array, trace_array_init_printk()
> returns -EINVAL. Because this return value is ignored here, the buffer
> remains unallocated, causing trace_array_vprintk() to fail internally.

Well, it will fail anyway because it is the top level trace buffer.

> [ ... ]

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

-- Steve

Re: [PATCH v2] tracing/osnoise: Array printk init and cleanup

[Crystal Wood]

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 <crwood@redhat.com>
> > 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

Re: [PATCH v2] tracing/osnoise: Array printk init and cleanup

[Crystal Wood]

On Thu, 2026-05-21 at 18:48 -0500, Crystal Wood wrote:
> 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 ]
> > 
> > > > 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.

It looks like it's actually also an issue in a bunch more places such as
record_osnoise_sample(), timerlat_dump_stack(), etc.

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

Many osnoise_taint() callers, as well as timerlat_dump_stack(), can have
preemption disabled, so the mutex in trace_array_get() won't work.

What is the intended way for a tracer to record to all of its
instances?  I tried looking at other tracers that allow instances, but
it seems that most of them only allow one instance, apart from
trace_function/trace_function_graph that are driven by a callback
mechanism that doesn't fit here, and that made my brain hurt when I
dove into the code to try to figure out how it ensures a valid tr.

We could have osnoise_unregister_instance() set inst->tr = NULL
under a raw lock, and then require users to hold the raw lock when
manipulating inst->tr (which can't go away until ->stop() completes),
skipping any that are NULL.

It's not great that we lose the ability to do things with tr that are
incompatible with a raw lock, but I don't know how to fix that without
something like changing the tr refcount mechanism to allow an atomic
refcount increase on a known-valid-for-now tr.  It looks like all the
current users of this list are OK with a raw lock.

We could go even further and simplify by un-RCUing the list, requiring
that the raw lock be held over traversal -- I'm not thrilled at the
idea of letting userspace create unbounded instances that are traversed
with preemption disabled, but as noted, we already do that in some places.

In any case, this patch is just moving the code around, not introducing
the problem, so I hope that whatever synchronization overhaul this file
requires (which goes well beyond this one issue) can wait for followup
patches.

-Crystal

Re: [PATCH v2] tracing/osnoise: Array printk init and cleanup

[Steven Rostedt]

On Fri, 29 May 2026 22:33:37 -0500
Crystal Wood <crwood@redhat.com> wrote:
> > > 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.  
> 
> Many osnoise_taint() callers, as well as timerlat_dump_stack(), can have
> preemption disabled, so the mutex in trace_array_get() won't work.

Right. OK, so another solution is to simply call synchronize_rcu() instead
of the kvfree_rcu_mightsleep(inst);

	synchronize_rcu();
	kvfree(inst);

Then there should not be any race, because the rmdir will have to wait for
the synchronization before finishing. This isn't something people should be
running a lot of, so I don't think it would cause too much pain in waiting
to unregister the osnoise tracer.

-- Steve