Gabriele Monaco <[email protected]> writes:
> 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]>
> Reviewed-by: Gabriele Monaco <[email protected]>
> Reviewed-by: Nam Cao <[email protected]>
> Signed-off-by: Gabriele Monaco <[email protected]>
> ---
> 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 a7e103654..60dc39f26 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();
> }
Looking at this again, do you need to change it to
static inline void da_monitor_reset(struct da_monitor *da_mon)
{
WRITE_ONCE(da_mon->monitoring, 0);
smp_mb();
da_monitor_reset_hook(da_mon);
da_mon->curr_state = model_get_initial_state();
}
To prevent another task from seeing monitoring=1 while the timer is
already cancelled?
Nam
