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

[Steven Rostedt <rostedt@goodmis.org>]

[ 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