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

[sashiko-bot@kernel.org]

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 <vdonnefort@google.com>

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?

>  	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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260812161340.2417322-1-vdonnefort@google.com?part=2