Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] ABA problem in trace_array lookup allows a file descriptor to attach to a newly created trace instance instead of failing. Pre-existing issues: - [High] If `allocate_fgraph_ops` fails in `ftrace_create_function_files`, `tr->ops` is freed but not set to `NULL`. During instance removal, this dangling pointer causes a Use-After-Free and Double Free. - [High] Concurrent lockless calls to `ftrace_ops_init` can lead to double `mutex_init` and list corruption. -- commit f37f9bc71afd70bff68b27fdd3afcdd0c07c7ae7 Author: Steven Rostedt <[email protected]> ftrace: Take trace_array reference before accessing its ftrace_ops The trace instance files set_ftrace_filter and set_ftrace_notrace was updated to work with specific trace instances (trace_arrays). This patch changes the files to pass the trace_array as the file private data, allowing the trace_array reference to be incremented before touching the ftrace_ops. Fixes: 591dffdade9f0 ("ftrace: Allow for function tracing instance to filter functions") Closes: https://lore.kernel.org/all/[email protected]/ > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > index f9d80c7bd9f16..c7cf36f2dd7be 100644 > --- a/kernel/trace/ftrace.c > +++ b/kernel/trace/ftrace.c [ ... ] > @@ -4691,26 +4692,45 @@ ftrace_avail_addrs_open(struct inode *inode, struct > file *file) [ ... ] > int > -ftrace_regex_open(struct ftrace_ops *ops, int flag, > +ftrace_regex_open(struct trace_array *tr, struct ftrace_ops *ops, int flag, > struct inode *inode, struct file *file) > { [ ... ] > + if (!ops) > + ops = tr->ops; > + > + if (WARN_ON_ONCE(!ops)) > + goto out; > + > + ftrace_ops_init(ops); [Severity: High] This is a pre-existing issue, but can concurrent lockless calls to ftrace_ops_init() lead to double mutex initialization? In ftrace_regex_open(), this function is called locklessly in the VFS open path. Inside ftrace_ops_init(), it checks if the initialized flag is set: kernel/trace/ftrace.c:ftrace_ops_init() { if (!(ops->flags & FTRACE_OPS_FL_INITIALIZED)) { mutex_init(&ops->local_hash.regex_lock); INIT_LIST_HEAD(&ops->subop_list); ops->func_hash = &ops->local_hash; ops->flags |= FTRACE_OPS_FL_INITIALIZED; } } If multiple threads concurrently open the filter file on a newly created trace instance, both could observe the flag as unset. Could one thread lock the newly initialized mutex just as the second thread calls mutex_init() on it, potentially corrupting the mutex wait list? [ ... ] > @@ -4788,21 +4808,19 @@ ftrace_regex_open(struct ftrace_ops *ops, int flag, > static int > ftrace_filter_open(struct inode *inode, struct file *file) > { > - struct ftrace_ops *ops = inode->i_private; > + struct trace_array *tr = inode->i_private; > > - /* Checks for tracefs lockdown */ > - return ftrace_regex_open(ops, > - FTRACE_ITER_FILTER | FTRACE_ITER_DO_PROBES, > - inode, file); > + return ftrace_regex_open(tr, NULL, > + FTRACE_ITER_FILTER | FTRACE_ITER_DO_PROBES, > + inode, file); [Severity: High] Does this lockless trace_array pointer retrieval introduce an ABA problem? If a thread pauses after reading tr from inode->i_private, a concurrent instance removal could free the original trace_array. If a subsequent mkdir allocates a new trace instance at the exact same memory address, the paused thread would resume with the old address. When ftrace_regex_open() calls tracing_check_open_get_tr(tr), it ultimately validates the pointer in trace_array_get() by comparing the raw pointer against the global list: kernel/trace/trace.c:trace_array_get() { ... list_for_each_entry(this_tr, &ftrace_trace_arrays, list) { if (tr == this_tr) { return __trace_array_get(tr); } } } Because the new instance was allocated at the same address, this comparison would succeed, incorrectly attaching the file descriptor to the new trace instance. Could this lead to cross-instance state corruption? > } [ ... ] > diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c > index cd37f20137581..c879d43a5fbb2 100644 > --- a/kernel/trace/trace_functions.c > +++ b/kernel/trace/trace_functions.c > @@ -101,7 +101,7 @@ int ftrace_create_function_files(struct trace_array *tr, > return ret; > } > > - ftrace_create_filter_files(tr->ops, parent); > + ftrace_create_filter_files(tr, parent); > > return 0; > } [Severity: High] This is a pre-existing issue, but does a failure in allocate_fgraph_ops() leave a dangling tr->ops pointer? Just above this hunk in ftrace_create_function_files(), if allocate_fgraph_ops() fails, it frees tr->ops but doesn't set it to NULL: kernel/trace/trace_functions.c:ftrace_create_function_files() { ret = allocate_fgraph_ops(tr, tr->ops); if (ret) { kfree(tr->ops); return ret; } } Since tracefs setup ignores the error, the instance creation succeeds but leaves a dangling pointer inside the active trace_array. Later, when the instance is removed, ftrace_destroy_function_files() will call ftrace_destroy_filter_files(tr->ops) and ftrace_free_ftrace_ops(tr), which might result in a use-after-free and a double free. Should tr->ops be set to NULL after kfree(tr->ops)? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20260828223901.29e26edb@robin?part=1
