On Tue, 2026-05-12 at 02:24 +0800, [email protected] wrote: > From: Wen Yang <[email protected]> > > da_monitor_start() set monitoring=1 before calling da_monitor_init_hook(), > may racing with the sched_switch handler: > > da_monitor_start() sched_switch handler > ------------------------- --------------------------------- > da_mon->monitoring = 1; > if (da_monitoring(da_mon)) /* true */ > ha_start_timer_ns(...); > /* hrtimer->base == NULL, crash */ > da_monitor_init_hook(da_mon); > /* hrtimer_setup() sets base */ > > Fix the ordering and pair with release/acquire semantics: > > da_monitor_init_hook(da_mon); > smp_store_release(&da_mon->monitoring, 1); /* da_monitor_start() */ > return smp_load_acquire(&da_mon->monitoring); /* da_monitoring() */ > > On ARM64 a plain STR + LDR does not form a release-acquire pair, so > the load can observe monitoring=1 while hrtimer->base is still NULL. > The plain accesses are also data races under KCSAN. > > Use WRITE_ONCE for the monitoring=0 store in da_monitor_reset() to > cover the reset path. > > Fixes: 792575348ff7 ("rv/include: Add deterministic automata monitor > definition via C macros") > Signed-off-by: Wen Yang <[email protected]>
Thanks for the fix! There are probably more than a few of those bugs since most monitors are implicitly serialised because their events are serialised.. Looks good to me. Reviewed-by: Gabriele Monaco <[email protected]> See minor comments below: > --- > include/rv/da_monitor.h | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/include/rv/da_monitor.h b/include/rv/da_monitor.h > index 39765ff6f098..00ded3d5ab3f 100644 > --- a/include/rv/da_monitor.h > +++ b/include/rv/da_monitor.h > @@ -82,7 +82,7 @@ static void react(enum states curr_state, enum events event) > static inline void da_monitor_reset(struct da_monitor *da_mon) > { > da_monitor_reset_hook(da_mon); > - da_mon->monitoring = 0; > + WRITE_ONCE(da_mon->monitoring, 0); > da_mon->curr_state = model_get_initial_state(); > } > > @@ -95,8 +95,9 @@ static inline void da_monitor_reset(struct da_monitor > *da_mon) > static inline void da_monitor_start(struct da_monitor *da_mon) > { > da_mon->curr_state = model_get_initial_state(); > - da_mon->monitoring = 1; > da_monitor_init_hook(da_mon); > + /* Pairs with smp_load_acquire in da_monitoring(). */ I wonder if these comment are really adding value, pairing smp_load_acquire / smp_store_release is the by-the-book usage and everything is here. But feel free to leave it if you think it's clearer. Thanks, Gabriele > + smp_store_release(&da_mon->monitoring, 1); > } > > /* > @@ -104,7 +105,8 @@ static inline void da_monitor_start(struct da_monitor > *da_mon) > */ > static inline bool da_monitoring(struct da_monitor *da_mon) > { > - return da_mon->monitoring; > + /* Pairs with smp_store_release in da_monitor_start(). */ > + return smp_load_acquire(&da_mon->monitoring); > } > > /*
