Re: [PATCH v2] tracing: Add down_write(trace_event_sem) when adding trace event

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

On Fri, 18 Jul 2025 22:31:58 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: Steven Rostedt <rostedt@goodmis.org>
> 
> When a module is loaded, it adds trace events defined by the module. It
> may also need to modify the modules trace printk formats to replace enum
> names with their values.
> 
> If two modules are loaded at the same time, the adding of the event to the
> ftrace_events list can corrupt the walking of the list in the code that is
> modifying the printk format strings and crash the kernel.
> 
> The addition of the event should take the trace_event_sem for write while
> it adds the new event.
> 
> Also add a lockdep_assert_held() on that semaphore in
> __trace_add_event_dirs() as it iterates the list.

Looks good to me.

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

Thanks!

> 
> Cc: stable@vger.kernel.org
> Reported-by: Fusheng Huang(黄富生)  <Fusheng.Huang@luxshare-ict.com>
> Closes: https://lore.kernel.org/all/20250717105007.46ccd18f@batman.local.home/
> Fixes: 110bf2b764eb6 ("tracing: add protection around module events unload")
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
> Changes since v1: https://lore.kernel.org/20250717131204.74559b88@batman.local.home
> 
> - Replace lockdep_assert_held_read() with lockdep_assert_held() as it is
>   actually held with down_write() when it is called.
> 
>  kernel/trace/trace_events.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 120531268abf..d01e5c910ce1 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -3136,7 +3136,10 @@ __register_event(struct trace_event_call *call, struct module *mod)
>  	if (ret < 0)
>  		return ret;
>  
> +	down_write(&trace_event_sem);
>  	list_add(&call->list, &ftrace_events);
> +	up_write(&trace_event_sem);
> +
>  	if (call->flags & TRACE_EVENT_FL_DYNAMIC)
>  		atomic_set(&call->refcnt, 0);
>  	else
> @@ -3750,6 +3753,8 @@ __trace_add_event_dirs(struct trace_array *tr)
>  	struct trace_event_call *call;
>  	int ret;
>  
> +	lockdep_assert_held(&trace_event_sem);
> +
>  	list_for_each_entry(call, &ftrace_events, list) {
>  		ret = __trace_add_new_event(call, tr);
>  		if (ret < 0)
> -- 
> 2.47.2
> 


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