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

[Michael Wu <michael@allwinnertech.com>]

On 8/11/2026 9:00 PM, Steven Rostedt wrote:
> This is still way too verbose. Is this AI written? If so, AI is *not* your friend.
This commit was written by me, not by AI. I apologize for wasting your time. Thank you for rewriting the change.
> Why are the priorities of the notifiers important here?
I probably wanted to describe the entire process, so I've gone on to say too much.
> Less is more when it comes to describing a bug.
Got it.

> On Tue, 11 Aug 2026 14:00:05 +0800
> Michael Wu <michael@allwinnertech.com> wrote:
> 
>>> What does the above mean? Are you loading two modules at the same time?  
>> Two modules (A and B) are loaded simultaneously on different CPUs. On the arm64,
>>  when CPU0's trace_module_notify [pri=1] and CPU1's trace_module_notify [pri=0] 
>> simultaneously perform operations on call_A, because they are in different cache lines,
>> CPU1 may observe WRITE_ONCE(head->next, &f->link) in step (4) before f->link.next=next in step (2).
>>  At this time, CPU1 reads an uninitialized f->link.next and performs an operation that causes to crash.
> 
> This is still way too verbose. Is this AI written? If so, AI is *not* your friend.
> 
> 
>>
>>> What does "pri=X notifier" mean? What function calls are these coming from?  
>> `pri=X notifier` represents `trace_events.c:trace_module_notify [pri=1]` and `trace.c:trace_module_notify [pri=0]`, respectively.
> 
> Why are the priorities of the notifiers important here?
> 
> I honestly didn't know one was allowed to load two modules at the same time
> and thought that it the module logic would prevent that. But if that's not
> the case, then yeah, we need protection.
> 
> 
>>  CPU0 (loads module A)                      CPU1 (loads module B)
>>   ===============================            ===============================
>>   load_module(A)                             load_module(B)
>>     blocking_notifier_call_chain_robust        blocking_notifier_call_chain_robust
>>          notifier_call_chain                     notifier_call_chain
>>           nb = trace_events.c:                     nb = trace.c:
>>           trace_module_notify [pri=1]              trace_module_notify [pri=0]
>>             mutex_lock(&event_mutex)                 trace_event_update_all() 
>>             trace_module_add_events(A)               down_write(&trace_event_sem) 
>>               __register_event(call_A)                
>>               __add_event_to_tracers(call_A)           
>>                 event_define_fields(call_A)            
>>                   for each f:                          
>>                     f = kmem_cache_alloc()             
>>                     list_add(&f->link,               	
>>                              &class->fields)           
>>                       f->link.next=next; (2)       	   
>>                       WRITE_ONCE(head->next,        
>>                                 &f->link); (4)       update_event_fields(call_A)   											           
>>             mutex_unlock(&event_mutex)	               list_for_each_entry(field, 
>>                                                          &class->fields, link) 
>>                                                          field = class->fields->next 
>>                                                                = &f->link 
>>                                                        	       = f (offset 0) 
>>                                                       up_write(&trace_event_sem)  
> 
> Basically this can be summed up to being:
> 
>  CPU0 (loads module A)                      CPU1 (loads module B)
>  ===============================            ===============================
>  load_module(A)                             load_module(B)
>    notifier_call_chain                        notifier_call_chain
>      trace_module_notify                        trace_module_notify
>        mutex_lock(&event_mutex)                   trace_event_update_all() 
>          trace_module_add_events(A)                 down_write(&trace_event_sem) 
>             __register_event(call_A)                
>               __add_event_to_tracers(call_A)           
>                 event_define_fields(call_A)            
>                   for each f:                         list_for_each_entry(field, 
>                     list_add(&f->link,                                    &class->fields, link) 
>                              &class->fields)            field = class->fields->next;
> 
> Where you can see that one is being read while the other is being written
> to. You do not need to go into details of the cache visibility here because
> this is an obvious race condition. All that information just distracts from
> the real issue that is being fixed.
> 
> Less is more when it comes to describing a bug.
> 
> I'll rewrite you change log and take the patch.
> 
> Thanks,
> 
> -- Steve


-- 
Regards,
Michael Wu