On Fri, 28 Aug 2026 20:13:09 +0000
[email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s)
> to consider:
> - [Critical] Tracefs lockdown and tracing_disabled checks are bypassed for
> the global ftrace instance and stack tracer.
> - [High] Module and probe filters for the global ftrace instance cannot be
> cleared or read.
> --
>
> commit 42c008f904dcd516dc1681a548fae6a4bba9a162
> 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 are
> updated to work with specific trace instances (trace_arrays). This
> patch passes the trace_array as the file private data to allow incrementing
> its reference before touching the ftrace_ops, preventing a use-after-free.
>
> Closes: https://lore.kernel.org/all/[email protected]/
>
> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > index f9d80c7bd9f16..4babd86c7be00 100644
> > --- a/kernel/trace/ftrace.c
> > +++ b/kernel/trace/ftrace.c
>
> [ ... ]
>
> > 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)
> > {
> > - struct ftrace_iterator *iter;
> > + struct ftrace_iterator *iter = NULL;
> > struct ftrace_hash *hash;
> > struct list_head *mod_head;
> > - struct trace_array *tr = ops->private;
> > - int ret = -ENOMEM;
> > -
> > - ftrace_ops_init(ops);
> > + int ret = -ENODEV;
> >
> > if (unlikely(ftrace_disabled))
> > return -ENODEV;
> >
> > - if (tracing_check_open_get_tr(tr))
> > + if (tr && tracing_check_open_get_tr(tr))
> > return -ENODEV;
>
> [Severity: Critical]
> By adding the `tr &&` short-circuit here, does this cause a regression
> where tracefs lockdown and tracing_disabled checks are bypassed for the
> global ftrace instance and the stack tracer?
>
> Since `tr` is explicitly passed as `NULL` for these instances,
> `tracing_check_open_get_tr(NULL)` is never called.
Yeah, right after sending this I noticed that the if statement isn't needed.
>
> [ ... ]
>
> > @@ -4788,21 +4801,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]
> For the global ftrace instance, `tr` is `NULL`. Does passing `NULL` for `tr`
> break the module and probe filters?
>
> In `ftrace_regex_open()`, the `mod_head` pointer is initialized like this:
>
> mod_head = tr ? &tr->mod_trace : NULL;
>
> Because `tr` is `NULL`, `mod_head` will also be `NULL`. When opening the
> file with `O_TRUNC`, `clear_ftrace_mod_list(mod_head)` will do nothing,
> failing to clear the module filters.
>
> Additionally, `iter->tr` is set to `NULL`. When reading the file,
> `t_mod_start()` relies on `iter->tr` and will return early:
>
> /* probes are only available if tr is set */
> if (!iter->tr)
> return NULL;
>
> Could this skip the enumeration of active module and probe filters for the
> global instance entirely?
Hmm, yeah.
I guess I need to add a:
perhaps I need to add at the start:
if (!tr && ops)
tr = ops->private;
>
> [ ... ]
>
> > diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
> > index 0aa2514a6593b..e7f4e523587d0 100644
> > --- a/kernel/trace/trace_stack.c
> > +++ b/kernel/trace/trace_stack.c
> > @@ -499,7 +499,7 @@ stack_trace_filter_open(struct inode *inode, struct
> > file *file)
> > struct ftrace_ops *ops = inode->i_private;
> >
> > /* Checks for tracefs lockdown */
> > - return ftrace_regex_open(ops, FTRACE_ITER_FILTER,
> > + return ftrace_regex_open(NULL, ops, FTRACE_ITER_FILTER,
> > inode, file);
> > }
>
> [Severity: Critical]
> The stack tracer explicitly passes `NULL` for the trace array. Due to the
> short-circuit mentioned earlier, doesn't this also bypass the lockdown
> check documented in the comment right above this call?
>
Yes, and I mentioned at the start I need to remove the if statement.
-- Steve
