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