ftrace: Take trace_array reference before accessing its ftrace_ops

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]/ open here

Quoted 4 lines

kernel/trace/ftrace.c +0 -0

index f9d80c7bd9f16..c7cf36f2dd7be 100644

[ ... ]

Quoted 1 line

kernel/trace/ftrace.c +0 -0

@@ -4691,26 +4692,45 @@ ftrace_avail_addrs_open(struct inode *inode, struct file *file)  

[ ... ]

Quoted 5 lines

int

kernel/trace/ftrace.c +1 -1

-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)
 {  

[ ... ]

kernel/trace/ftrace.c ftrace_avail_addrs_open(struct inode *inode, struct file *file) · line 4731

Quoted 7 lines

kernel/trace/ftrace.c +7 -0

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

[ ... ]

Quoted 4 lines

kernel/trace/ftrace.c +0 -0

@@ -4788,21 +4808,19 @@ ftrace_regex_open(struct ftrace_ops *ops, int flag,
 static int
 ftrace_filter_open(struct inode *inode, struct file *file)
 {

kernel/trace/ftrace.c ftrace_regex_open(struct ftrace_ops *ops, int flag, · line 4815

Quoted 10 lines

kernel/trace/ftrace.c +4 -5

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

Quoted 2 lines

kernel/trace/ftrace.c +0 -0

 }  

[ ... ]

kernel/trace/trace_functions.c int ftrace_create_function_files(struct trace_array *tr, · line 107

Quoted 13 lines

kernel/trace/trace_functions.c +1 -1

index cd37f20137581..c879d43a5fbb2 100644
@@ -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)?

From: Steven Rostedt <[email protected]>

The trace instance files set_ftrace_filter and set_ftrace_notrace was updated to work with specific trace instances (trace_arrays). The issue is that when these files are opened, there is a small race window where it will use the ftrace_ops from the inode->private pointer to get a reference to the trace_array and then take its reference. The problem is that the ftrace_ops itself could be freed. If the rmdir on the instance happens at the same time the set_ftrace_filter file is opened, the rmdir could have also freed the ftrace_ops and referencing it will cause a use-after-free bug and crash the kernel.

Instead, pass in the trace_array as the file private data (NULL for the top level instance), and then pass both the trace_array and the ftrace_ops to the ftrace_regex_open() function. If the trace_array is NULL, then it just uses the ftrace_ops without the need to take its reference (like normal). If the ftrace_ops is NULL, that is only the case for the top level instance and the global_ops can be used.

This allows the trace_array to have its reference incremented before touching the ftrace_ops that could also be freed when the instance is.

Cc [email protected]

Fixes 591dffdade9f0 ("ftrace: Allow for function tracing instance to filter functions")

Reported-by Breno Leitao <[email protected]>

Closes https://lore.kernel.org/all/[email protected]/ open here

Signed-off-by Steven Rostedt <[email protected]>