Re: [PATCH] ftrace: Take trace_array reference before accessing its ftrace_ops

On Fri, 28 Aug 2026 20:13:09 +0000
sashiko-bot@kernel.org 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 [off-list ref]

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/apGORjltZgAiAYHT@gmail.com/ (local)

quoted

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  

[ ... ]

quoted

 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.

[ ... ]

quoted

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

[ ... ]

quoted

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