Re: [PATCH v5 02/18] tracing/remotes: Release tracefs,eventfs on registration failure

On Wed, Aug 12, 2026 at 04:25:03PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [High] Expanding the global `trace_remotes_lock` over the external driver callback `cbs->init()` introduces a severe risk of deadlocks and priority inversion.
> 
> Pre-existing issues:
> - [Critical] A Time-Of-Check to Time-Of-Use race condition between `trace_remote_tracefs_open()` and concurrent unregistration leads to a Use-After-Free.
> --
> 
> --- Patch [2]: [PATCH v5 02/18] tracing/remotes: Release tracefs,eventfs on registration failure ---
> commit 70ddc6ce63b80a301a72ec3efb7683b32447865f
> Author: Vincent Donnefort <[email protected]>
> 
> tracing/remotes: Release tracefs,eventfs on registration failure
>     
> In trace_remote_register(), if registration of events or the init
> callback fails, the created tracefs and eventfs directories are leaked.
> Release the entire eventfs and tracefs hierarchy on trace_remote
> registration failure.
> 
> > diff --git a/kernel/trace/trace_remote.c b/kernel/trace/trace_remote.c
> > index ce2d517183b79..0384277070b9e 100644
> > --- a/kernel/trace/trace_remote.c
> > +++ b/kernel/trace/trace_remote.c
> 
> [ ... ]
> 
> > @@ -972,10 +990,9 @@ static int trace_remote_register_events(const char *remote_name, struct trace_re
> >  int trace_remote_register(const char *name, struct trace_remote_callbacks *cbs, void *priv,
> >  			  struct remote_event *events, size_t nr_events)
> >  {
> > -	struct trace_remote *remote;
> > +	struct trace_remote *remote __free(kfree) = kzalloc_obj(*remote);
> >  	int ret;
> >  
> > -	remote = kzalloc_obj(*remote);
> >  	if (!remote)
> >  		return -ENOMEM;
> >  
> > @@ -986,13 +1003,15 @@ int trace_remote_register(const char *name, struct trace_remote_callbacks *cbs,
> >  	mutex_init(&remote->lock);
> >  	init_rwsem(&remote->reader_lock);
> >  
> > -	if (trace_remote_init_tracefs(name, remote)) {
> > -		kfree(remote);
> > -		return -ENOMEM;
> > -	}
> > +	guard(mutex)(&trace_remotes_lock);
> > +
> > +	ret = trace_remote_init_tracefs(name, remote);
> > +	if (ret)
> > +		return ret;
> >  
> >  	ret = trace_remote_register_events(name, remote, events, nr_events);
> >  	if (ret) {
> > +		trace_remote_remove_tracefs(remote);
> >  		pr_err("Failed to register events for trace remote '%s' (%d)\n",
> >  		       name, ret);
> >  		return ret;
> > @@ -1000,13 +1019,16 @@ int trace_remote_register(const char *name, struct trace_remote_callbacks *cbs,
> >  
> >  	ret = cbs->init ? cbs->init(remote->dentry, priv) : 0;
> 
> [Severity: High]
> Does holding the global trace_remotes_lock while calling the external init()
> callback introduce a risk of deadlocks?
> 
> Because trace_remotes_lock was acquired via guard(mutex) earlier in this
> function, calling an opaque external callback with this global subsystem lock
> held establishes an inverted lock dependency.
> 
> If the teardown path of the external driver takes its own locks before
> unregistering the trace remote, could this result in an ABBA deadlock?

Current users are just fine. I do not think that is something worth considering
at the moment.

> 
> >  	if (ret) {
> > +		trace_remote_unregister_events(remote);
> > +		trace_remote_remove_tracefs(remote);
> >  		pr_err("Init failed for trace remote '%s' (%d)\n", name, ret);
> > -	} else {
> > -		guard(mutex)(&trace_remotes_lock);
> > -		list_add(&remote->node, &trace_remotes);
> >  	}
> >  
> > -	return ret;
> > +	list_add(&remote->node, &trace_remotes);
> > +	retain_and_null_ptr(remote);
> > +
> > +	return 0;
> >  }
> 
> [Severity: Critical]
> This is a pre-existing issue, but is there a race condition between
> trace_remote_tracefs_open() and concurrent unregistration?
> 
> In trace_remote_tracefs_open(), the code checks if the remote object exists
> in the global trace_remotes list:
> 
> kernel/trace/trace_remote.c:trace_remote_tracefs_open() {
>     ...
>     guard(mutex)(&trace_remotes_lock);
> 
>     /* i_private is either a struct trace_remote or a struct remote_event */
>     list_for_each_entry(r, &trace_remotes, node) {
>         if (r == i_private)
>             return 0;
>     ...
> }
> 
> However, the lock is dropped on return without incrementing the object's
> reference count.
> 
> Then in trace_open(), the unprotected remote pointer is dereferenced:
> 
> kernel/trace/trace_remote.c:trace_open() {
>     ...
>     ret = trace_remote_tracefs_open(inode, filp);
>     if (ret)
>         return ret;
> 
>     if (!(filp->f_mode & FMODE_READ))
>         return 0;
>     ...
>     guard(mutex)(&remote->lock);
>     ...
> }
> 
> If a concurrent unregistration acquires trace_remotes_lock, removes the
> remote, and frees it after trace_remote_tracefs_open() returns but before
> trace_open() acquires remote->lock, will this result in a use-after-free
> regression?

trace_remote_tracefs_open() will block until the remote is properly registered,
that is, added to the list trace_remotes.

After the remote is added to the list, there is no unregistration possible at
the moment.

I do not see any concurrent unregistration possible.

-- 
Vincent

> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2