Re: [PATCH v5 5/9] rv: Make da_monitor_reset_hook and EVENT_NONE_LBL overridable

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Overriding `da_monitor_reset_hook` to synchronously cancel timers deadlocks the system
- [Medium] Undocumented API change allowing `da_monitor_sync_hook` to be overridden

Pre-existing issues:
- [High] Monitor teardown sequence causes a Use-After-Free due to improper timer cancellation
- [High] Lockless update of discrete state and hybrid environment variables allows concurrent events to evaluate inconsistent state
--

--- Patch [5]: [PATCH v5 5/9] rv: Make da_monitor_reset_hook and EVENT_NONE_LBL overridable ---
commit 367db4a7d614e2458fbdde565ff9b9fc8d92d1b1
Author: Wen Yang <[email protected]>

rv: Make da_monitor_reset_hook and EVENT_NONE_LBL overridable

This commit wraps da_monitor_reset_hook and EVENT_NONE_LBL with #ifndef guards
to allow HA-based monitors to substitute their own implementations. It is
intended to help tools define custom reset hooks for per-task hrtimer cleanup.

> rv: Make da_monitor_reset_hook and EVENT_NONE_LBL overridable
> 
> Wrap both definitions with #ifndef guards so HA-based monitors can
> substitute their own implementations before including this header.

[Severity: Medium]
The commit message mentions wrapping both definitions (da_monitor_reset_hook
and EVENT_NONE_LBL), but the patch also silently wraps a third definition:
da_monitor_sync_hook(). Could the commit message be updated to reflect this
additional API change, or was this change unintentional?

> diff --git a/include/rv/ha_monitor.h b/include/rv/ha_monitor.h
> index e1738d199b289..807b981eb548a 100644
> --- a/include/rv/ha_monitor.h
> +++ b/include/rv/ha_monitor.h
> @@ -36,8 +36,14 @@ static bool ha_monitor_handle_constraint(struct da_monitor *da_mon,
>  					 da_id_type id);
>  #define da_monitor_event_hook ha_monitor_handle_constraint

[Severity: High]
This isn't a bug introduced by this patch, but can concurrent events evaluate
inconsistent states here?

Looking at the event processing sequence, da_event() in
include/rv/da_monitor.h updates the monitor's discrete state using a lockless
try_cmpxchg() loop. Only after the state is successfully updated does it call
da_monitor_event_hook(), which resolves to ha_monitor_handle_constraint(),
to update the environment variables.

Could a concurrent event on another CPU observe the newly updated discrete
state but still see the stale environment variables before the first CPU
finishes updating them?

>  #define da_monitor_init_hook ha_monitor_init_env
> +
> +/* Overrides must still call ha_monitor_reset_env() or cancel the timer. */
> +#ifndef da_monitor_reset_hook
>  #define da_monitor_reset_hook ha_monitor_reset_env
> +#endif

[Severity: High]
Could following the documentation to cancel the timer here cause a deadlock?

If a developer overrides this hook to synchronously cancel their timers (such
as using hrtimer_cancel() or timer_delete_sync()), it appears this will
deadlock when called from the timer callback itself.

The timer callback __ha_monitor_timer_callback() calls da_monitor_reset(),
which unconditionally invokes da_monitor_reset_hook(). Would this result in
the timer callback attempting to synchronously cancel itself?


[Severity: High]
This isn't a bug introduced by this patch, but is there a use-after-free risk
during monitor teardown due to improper timer cancellation?

When a per-object monitor is destroyed, da_destroy_storage() cancels timers
without waiting (using hrtimer_try_to_cancel() or timer_delete() via
ha_cancel_timer()) and then defers freeing the storage using call_rcu() or
kfree_rcu().

The timer callback __ha_monitor_timer_callback() relies on guard(rcu)() for
synchronization. However, if the timer callback thread is preempted before it
enters the RCU read-side critical section, the grace period could complete and
the monitor storage could be freed.

When the timer callback resumes, would it then dereference the freed monitor
pointer?

> +#ifndef da_monitor_sync_hook
>  #define da_monitor_sync_hook() synchronize_rcu()
> +#endif

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5