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
