Re: [PATCH] tracing: Add mutex to trace_parser to fix concurrent write races

On 2026/7/14 10:13, Steven Rostedt wrote:

On Tue, 14 Jul 2026 09:50:55 +0800
Tengda Wu [off-list ref] wrote:

quoted

quoted

quoted

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index f93e34dd2328..ef47e5659283 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c

@@ -5842,6 +5842,8 @@ ftrace_regex_write(struct file *file, const char __user *ubuf,
 	/* iter->hash is a local copy, so we don't need regex_lock */
 
 	parser = &iter->parser;
+
+	guard(mutex)(&parser->lock);
 	read = trace_get_user(parser, ubuf, cnt, ppos);  

Why are the other users of trace_get_user() not a problem? If
anything, trace_get_user() should have a lockdep assert to make sure
the lock is held.

I think we need to add a lockdep assertion in all the callers that use
the parser and we need to make sure it's taken by every user.

-- Steve  

Other places that use trace_get_user():

ftrace_event_write
    trace_parser_get_init
    trace_get_user

trace_pid_write
    trace_parser_get_init
    trace_get_user

In both of these cases, the parser is allocated via trace_parser_get_init()
before use and freed immediately after, with no multi-threaded sharing.

However, ftrace_graph_write() and ftrace_regex_write() are different.
The parser used by these two functions is allocated at open() time and
retrieved from struct file at write() time. Userspace can have multiple
threads invoking write() concurrently, e.g.:

    r0 = openat(AT_FDCWD, path, O_WRONLY | O_CREAT | O_TRUNC |
                O_NOCTTY | O_NONBLOCK, 0);
    r1 = dup(r0);
    write(r1, data1, len1);        /* thread 1 */
    pwrite64(r1, data2, len2, offset); /* thread 2 */

Without proper synchronization in trace_get_user(), the parser state
becomes undefined.

We have locally reproduced a slab-out-of-bounds issue with syzkaller [1],
which appears to be caused by this race.

Regarding the fix, adding a lockdep assertion to trace_get_user() does
not seem necessary. As mentioned above, ftrace_event_write() and
trace_pid_write() do not involve concurrency, so they do not require
locking, and thus do not need a lockdep assertion.

So basically you want to add this inherit coupling between a user of
"parser" and the "parser" itself? That is just bad design. You created a
lock within the trace_parser struct to be used by a single instance but not
all instances.

You're right. That coupling is indeed bad.

It is either added for all users or this is fixed by something else. Yeah,
for now the other users of trace_get_user() do not need locking, but what
happens the usage changes where it adds concurrency? Then it becomes a bug
again. Right?

If one place needs to hold the parser->lock when calling trace_get_user()
and trace_parser_loaded(), then all places should hold that lock. Or we
just need to redesign it better.

Yes, this fixes a race condition when user space does something stupid (and
this is something that requires admin privileges). But if we are going to
fix it, might as well do it properly.

If the issue is only with ftrace, then make the lock part of ftrace and not
the parser. Don't create a lock in parser that is determined by how one
uses it whether the user needs to take the lock or not.

Yes, the issue at hand is currently limited to ftrace internals,
specifically in ftrace_graph_write() and ftrace_regex_write(). I did
consider moving the lock out of the parser and confining it within
ftrace.c, but the approaches I came up with so far all seem less than
ideal. Let me outline them below.

_Approach 1_: Add a global lock at the ftrace.c file level, and hold it
in the relevant write entry points for mutual exclusion.

The obvious downside here is that both of the aforementioned functions are
called by multiple tracefs file write hooks. A single global mutex would
cause unnecessary blocking between writes to different files. On the other
hand, introducing multiple global mutexes (one per file) would scatter the
lock definitions and make maintenance harder.

  ftrace_graph_write() is used by:
    - set_graph_function
    - set_graph_notrace

  ftrace_regex_write() is used by:
    - set_ftrace_filter
    - set_ftrace_notrace

_Approach 2_: Move the lock into structures such as ftrace_graph_data
or ftrace_iterator, both of which already embed a trace_parser.

While this does fix the problem, it feels somewhat redundant. We would be
defining essentially the same lock in multiple places just to serialize
access to the parser. Right now that would mean two definitions, but in the
future, depending on how many parser users need concurrency control, it
could grow further.

_Approach 3_: As you suggested earlier, build on v1 but add proper lockdep
assertions to enforce that the parser lock is held by all callers,
regardless of how the parser is used.

Approach 3 seems cleaner and more straightforward.

Do you happen to have a better suggestion in mind? I'm open to other
approaches if we can keep the design clean and maintainable.

Thanks,
Tengda