Re: [PATCH v2 07/12] rv: Fix monitor start ordering and memory ordering for monitoring flag

[Nam Cao]

Gabriele Monaco <gmonaco@xxxxxxxxxx> writes:
> From: Wen Yang <wen.yang@xxxxxxxxx>
>
> 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 <wen.yang@xxxxxxxxx>
> Reviewed-by: Gabriele Monaco <gmonaco@xxxxxxxxxx>
> Signed-off-by: Gabriele Monaco <gmonaco@xxxxxxxxxx>

Looks correct to me.
Reviewed-by: Nam Cao <namcao@xxxxxxxxxxxxx>

Wen, I am curious, how did you find this issue?

Nam