Re: [PATCH v2] ftrace: Synchronize the initialization of ftrace_ops

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

On Wed, 2 Sep 2026 09:55:01 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: Steven Rostedt <rostedt@goodmis.org>
> 
> There's some internal state that ftrace_ops needs to have set, but since
> it can be declared outside of the ftrace.c code, it calls
> ftrace_ops_init() on the ops in every global function. The issue is that
> if two tasks call it on the same ops at the same time it is possible to
> have the initialization of one corrupt the initialization of the other
> call.
> 
> Create a ops_mutex to use to synchronize every initialization of the
> ftrace_ops. The mutex is taken within checking the ftrace_ops flag that
> states it was initializied but the flag is checked again after the mutex
> has been taken. Checking first outside the mutex allows it to shortcut
> having to take the mutex. But then the check needs to be done again after
> the mute is taken in case of races.
> 

Looks good to me.

Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Thanks,

> Cc: stable@vger.kernel.org
> Fixes: f04f24fb7e48d ("ftrace, kprobes: Fix a deadlock on ftrace_regex_lock")
> Reported-by: sashiko-bot@kernel.org
> Close: https://lore.kernel.org/all/20260829025528.49A831F000E9@smtp.kernel.org/
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> ---
> Changes since v1: https://patch.msgid.link/20260901202020.09a1119a@robin
> 
> - Added smp_load_acquire() and smp_store_release() to make sure that
>   the flags value that is compared is really updated after the code has
>   been initialized. (Sashiko)
> 
>  kernel/trace/ftrace.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index c7cf36f2dd7b..53d5db60bfa5 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -75,6 +75,8 @@
>  	.func_hash		= &opsname.local_hash,			\
>  	.local_hash.regex_lock	= __MUTEX_INITIALIZER(opsname.local_hash.regex_lock), \
>  	.subop_list		= LIST_HEAD_INIT(opsname.subop_list),
> +/* Used only to synchronize the initialization of ftrace_ops */
> +static DEFINE_MUTEX(ops_mutex);
>  #else
>  #define INIT_OPS_HASH(opsname)
>  #endif
> @@ -159,11 +161,18 @@ const struct ftrace_ops ftrace_nop_ops = {
>  static inline void ftrace_ops_init(struct ftrace_ops *ops)
>  {
>  #ifdef CONFIG_DYNAMIC_FTRACE
> -	if (!(ops->flags & FTRACE_OPS_FL_INITIALIZED)) {
> +	unsigned long flags = smp_load_acquire(&ops->flags);
> +
> +	if (!(flags & FTRACE_OPS_FL_INITIALIZED)) {
> +		guard(mutex)(&ops_mutex);
> +		/* Could have been initialized before lock taken */
> +		if (unlikely(ops->flags & FTRACE_OPS_FL_INITIALIZED))
> +			return;
>  		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;
> +		flags = ops->flags | FTRACE_OPS_FL_INITIALIZED;
> +		smp_store_release(&ops->flags, flags);
>  	}
>  #endif
>  }
> -- 
> 2.53.0
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>