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

[Steven Rostedt <rostedt@goodmis.org>]

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