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 [off-list ref]

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?

[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/cover.1787161646.git.wen.yang@linux.dev?part=5