[PATCH] tracing: Fix race between update_event_fields and event_define_fields

[PATCH] tracing: Fix race between update_event_fields and event_define_fields

[Michael Wu]

Kernel: Linux version 6.18.21
Platform: Android

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 — 
field's cache line
         new->prev = prev;                // (3) ordinary store
         WRITE_ONCE(prev->next, new);    // (4) release store — head's 
cache line

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.

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


-- 
Regards,
Michael Wu

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

[Masami Hiramatsu]

Hi Michael,

On Thu, 30 Jul 2026 13:37:59 +0800
Michael Wu <michael@allwinnertech.com> wrote:

> Kernel: Linux version 6.18.21
> Platform: Android

We will accept the patch which is for the upstream kernel here.
Can you rebase the patch on top of trace/fixes branch of 
https://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace.git
or linus tree?

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

I confirmed this is still valid on upstream kernel.
I have some comments, see below;

> 
> 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 — 
> field's cache line
>          new->prev = prev;                // (3) ordinary store
>          WRITE_ONCE(prev->next, new);    // (4) release store — head's 
> cache line
> 
> 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.
> 

Also add:

Fixes: b3bc8547d3be ("tracing: Have TRACE_DEFINE_ENUM affect trace event types as well")
Cc: stable@vger.kernel.org

Thank you,

> 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
> 
> 
> -- 
> Regards,
> Michael Wu


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

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

[Michael Wu]

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 — 
field's cache line
         new->prev = prev;                // (3) ordinary store
         WRITE_ONCE(prev->next, new);    // (4) release store — head's 
cache line

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.

Fixes: b3bc8547d3be ("tracing: Have TRACE_DEFINE_ENUM affect trace event 
types as well")
Cc: stable@vger.kernel.org
---
  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

On 7/30/2026 4:31 PM, Masami Hiramatsu (Google) wrote:
> Hi Michael,
> 
> On Thu, 30 Jul 2026 13:37:59 +0800
> Michael Wu <michael@allwinnertech.com> wrote:
> 
>> Kernel: Linux version 6.18.21
>> Platform: Android
> 
> We will accept the patch which is for the upstream kernel here.
> Can you rebase the patch on top of trace/fixes branch of
> https://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace.git
> or linus tree?
> 
>>
>> 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.
> 
> I confirmed this is still valid on upstream kernel.
> I have some comments, see below;
> 
>>
>> 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 —
>> field's cache line
>>           new->prev = prev;                // (3) ordinary store
>>           WRITE_ONCE(prev->next, new);    // (4) release store — head's
>> cache line
>>
>> 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.
>>
> 
> Also add:
> 
> Fixes: b3bc8547d3be ("tracing: Have TRACE_DEFINE_ENUM affect trace event types as well")
> Cc: stable@vger.kernel.org
> 
> Thank you,
> 
>> 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
>>
>>
>> -- 
>> Regards,
>> Michael Wu
> 
> 


-- 
Regards,
Michael Wu

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

[Michael Wu]

Hi Masami,
   Thanks for the review. I've rebased the patch. The new version is below.

---
 From ec00b31c58b56f4cbbe6dd67c9f88a1d87d8d5ac Mon Sep 17 00:00:00 2001
From: Michael Wu <michael@allwinnertech.com>
Date: Wed, 29 Jul 2026 17:34:09 +0800
Subject: [PATCH v2] tracing: Fix race between update_event_fields and
  event_define_fields

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 — 
field's cache line
         new->prev = prev;                // (3) ordinary store
         WRITE_ONCE(prev->next, new);    // (4) release store — head's 
cache line

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.

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

On 7/30/2026 6:54 PM, Michael Wu 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 — 
> field's cache line
>          new->prev = prev;                // (3) ordinary store
>          WRITE_ONCE(prev->next, new);    // (4) release store — head's 
> cache line
> 
> 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.
> 
> Fixes: b3bc8547d3be ("tracing: Have TRACE_DEFINE_ENUM affect trace event 
> types as well")
> Cc: stable@vger.kernel.org
> ---
>   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,


-- 
Regards,
Michael Wu