Re: [PATCH v4] tracing: Fix race between update_event_fields and, event_define_fields

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

On Mon, 3 Aug 2026 17:40:55 +0800
Michael Wu <michael@allwinnertech.com> wrote:

> event_define_fields() (pri=1 MODULE_STATE_COMING notifier, locked by
> event_mutex) populates class->fields via list_add(), while
> update_event_fields() (called from the pri=0 notifier path via
> trace_event_update_all) traverses class->fields protected only by
> trace_event_sem.  These are two different locks guarding the same
> data structure, so during cross-module loading a reader on one CPU can
> observe partially initialized list nodes being concurrently added by a
> writer on another CPU.
> 
> On arm64 with weak memory ordering, __list_add() writes to two
> different cache lines:
> 
>         next->prev = new;                // (1) ordinary store
>         new->next = next;                // (2) ordinary store
>         new->prev = prev;                // (3) ordinary store
>         WRITE_ONCE(prev->next, new);    // (4) release store
> 
> The store buffer can drain (2) and (4) independently since they target
> different cache lines.  A remote CPU may observe (4) before (2): it
> sees prev->next pointing to the new node, but the new node's link.next
> is still zero (kmem_cache_alloc zero-initialized via KMEM_CACHE with
> SLAB_PANIC).  Since offsetof(struct ftrace_event_field, link) == 0,
> list_for_each_entry() derives field == NULL from link.next == 0 and
> crashes at field->type (offset 0x18):
> 
>   Unable to handle kernel access ... at virtual address 0000000000000018
>   pc : update_event_fields+0xf8/0x368
>   Call trace:
>    update_event_fields+0xf8/0x368
>    trace_event_update_all+0x7c/0x2b4
>    trace_module_notify+0x4c/0x1dc
>    notifier_call_chain+0x84/0x168
>    blocking_notifier_call_chain_robust+0x64/0xd4
>    load_module+0x10c8/0x123c
>    __arm64_sys_finit_module+0x230/0x31c
> 
> Fix by holding event_mutex in trace_event_update_all() before
> down_write(&trace_event_sem).  This serializes the reader against
> event_define_fields() which takes event_mutex for the entire
> write-side critical section.  The lock ordering event_mutex ->
> trace_event_sem is already established at three other sites
> (trace_remove_event_call, event_trace_add_tracer,
> event_trace_del_tracer), so this introduces no ordering conflict.
> 
> An alternative of placing mutex_lock inside update_event_fields() was
> considered but rejected -- it would invert the established lock
> ordering (trace_event_sem -> event_mutex) and risk ABBA deadlock.
> 

Looks good to me!

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

Thank you,

> Fixes: b3bc8547d3be ("tracing: Have TRACE_DEFINE_ENUM affect trace event types as well")
> Cc: stable@vger.kernel.org
> Signed-off-by: Michael Wu <michael@allwinnertech.com>
> ---
>  kernel/trace/trace_events.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 956692856fa8..9632788da5af 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -3566,6 +3566,7 @@ void trace_event_update_all(struct trace_eval_map **map, int len)
>  	int last_i;
>  	int i;
>  
> +	mutex_lock(&event_mutex);
>  	down_write(&trace_event_sem);
>  	list_for_each_entry_safe(call, p, &ftrace_events, list) {
>  		/* events are usually grouped together with systems */
> @@ -3604,6 +3605,7 @@ void trace_event_update_all(struct trace_eval_map **map, int len)
>  		cond_resched();
>  	}
>  	up_write(&trace_event_sem);
> +	mutex_unlock(&event_mutex);
>  }
>  
>  static bool event_in_systems(struct trace_event_call *call,
> -- 
> 2.29.0


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