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

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 [off-list ref]

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.

quoted

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

[ ... ]

quoted

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