On Sat, 29 Aug 2026 02:55:27 +0000
[email protected] wrote:
> 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?
Hmm, yeah this probably needs some locking to synchronize.
>
> [ ... ]
> > @@ -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?
The would at most open the wrong file in the new instance that was
meant for the old instance. It may confuse the administrator doing the
open, but if they are also deleting and creating instances (which are
very big operations that do a lot) and opening one of the files that
they are deleting, then we know they are likely already confused!
As long as it doesn't crash the kernel, this side effect is an issue
for the administrator to not do stupid things or they may get stupid
results.
>
> > }
> [ ... ]
> > 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)?
>
Probably, will investigate.
-- Steve
