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
