Re: [PATCH v9 2/3] tracing: Remove the backup instance automatically after read

[Google]

On Tue, 31 Mar 2026 17:19:36 -0400
Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:

> On Wed,  1 Apr 2026 01:32:33 +0900
> "Masami Hiramatsu (Google)" <mhiramat@xxxxxxxxxx> wrote:
> 
> > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > index 8cec7bd70438..1d73400a01c7 100644
> > --- a/kernel/trace/trace.c
> > +++ b/kernel/trace/trace.c
> > @@ -539,8 +539,65 @@ void trace_set_ring_buffer_expanded(struct trace_array *tr)
> >  	tr->ring_buffer_expanded = true;
> >  }
> >  
> > +static int __remove_instance(struct trace_array *tr);
> > +
> > +static void trace_array_autoremove(struct work_struct *work)
> > +{
> > +	struct trace_array *tr = container_of(work, struct trace_array, autoremove_work);
> > +
> > +	guard(mutex)(&event_mutex);
> > +	guard(mutex)(&trace_types_lock);
> 
> Hmm, should we do a check if the tr still exists? Couldn't the user delete
> this via a rmdir after the last file closed and this was kicked?
> 
>   CPU 0							CPU 1
>   -----							-----
>   open(trace_pipe);
>   read(..);
>   close(trace_pipe);
>      kick the work queue to delete it....
> 						rmdir();
> 							[instance deleted]

I thought this requires trace_types_lock, and after kicked the queue,
can rmdir() gets the tr? (__trace_array_get() return error if
tr->free_on_close is set)

> 
>   __remove_instance();
> 
>    [ now the tr is freed, and the remove will crash!]
> 
> 
> What would prevent this is this is to use trace_array_destroy() that checks
> this and also adds the proper locking:
> 
> static void trace_array_autoremove(struct work_struct *work)
> {
> 	struct trace_array *tr = container_of(work, struct trace_array, autoremove_work);
> 
> 	trace_array_destroy(tr);
> }

OK, let's use it.

> 
> 
> > +
> > +	/*
> > +	 * This can be fail if someone gets @tr before starting this
> > +	 * function, but in that case, this will be kicked again when
> > +	 * putting it. So we don't care about the result.
> > +	 */
> > +	__remove_instance(tr);
> > +}
> > +
> > +static struct workqueue_struct *autoremove_wq;
> > +
> > +static void trace_array_kick_autoremove(struct trace_array *tr)
> > +{
> > +	if (autoremove_wq && !work_pending(&tr->autoremove_work))
> > +		queue_work(autoremove_wq, &tr->autoremove_work);
> 
> Doesn't queue_work() check if it's pending? Do we really need to check it
> twice?

Indeed, it checked the flag.

> 
> > +}
> > +
> > +static void trace_array_cancel_autoremove(struct trace_array *tr)
> > +{
> > +	if (autoremove_wq && work_pending(&tr->autoremove_work))
> > +		cancel_work(&tr->autoremove_work);
> 
> Same here, as can't this be racy?

Yeah, and this should use cancel_work_sync().

> 
> > +}
> > +
> > +static void trace_array_init_autoremove(struct trace_array *tr)
> > +{
> > +	INIT_WORK(&tr->autoremove_work, trace_array_autoremove);
> > +}
> > +
> > +static void trace_array_start_autoremove(void)
> > +{
> > +	if (autoremove_wq)
> > +		return;
> > +
> > +	autoremove_wq = alloc_workqueue("tr_autoremove_wq",
> > +					WQ_UNBOUND | WQ_HIGHPRI, 0);
> > +	if (!autoremove_wq)
> > +		pr_warn("Unable to allocate tr_autoremove_wq. autoremove
> > disabled.\n"); +}
> > +
> >  LIST_HEAD(ftrace_trace_arrays);
> 
> -- Steve
> 


-- 
Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>