Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] rcu: missing rcu_barrier() on module unload causes kernel panic - [Low] timers: teardown path uses timer_delete_sync() instead of timer_shutdown_sync() --
commit 659afa957e903e72b692e0af300f112338f0d03d Author: Wen Yang <[email protected]> rv: Disarm HA timers on the storage free path This patch addresses a use-after-free issue where an HA timer can be queued on a storage object that is subsequently freed by kfree_rcu(). It adds a da_monitor_free_hook() callback just before the storage is freed to securely cancel the timer. To support this hook, kfree_rcu() is replaced with call_rcu() and a dedicated callback function. > diff --git a/include/rv/da_monitor.h b/include/rv/da_monitor.h > index 6fc5ef8142aca..5d3468a7a12b7 100644 > --- a/include/rv/da_monitor.h > +++ b/include/rv/da_monitor.h [ ... ] > @@ -543,6 +554,15 @@ static inline monitor_target > da_get_target_by_id(da_id_type id) > return mon_storage->target; > } > > +static void da_kfree_cb(struct rcu_head *head) > +{ > + struct da_monitor_storage *ms = > + container_of(head, struct da_monitor_storage, rcu); > + > + da_monitor_free_hook(&ms->rv.da_mon); > + kfree(ms); > +} > + > /* > * da_destroy_storage - destroy the per-object storage > * > @@ -564,7 +584,7 @@ static inline void da_destroy_storage(da_id_type id) > return; > da_monitor_reset_hook(&mon_storage->rv.da_mon); > hash_del_rcu(&mon_storage->node); > - kfree_rcu(mon_storage, rcu); > + call_rcu(&mon_storage->rcu, da_kfree_cb); > } > > static void __da_monitor_reset_all(void (*reset)(struct da_monitor *)) > @@ -607,6 +627,7 @@ static inline void da_monitor_destroy(void) > * pending, we can safely assume no concurrent user. > */ > hash_for_each_safe(da_monitor_ht, bkt, tmp, mon_storage, node) { > + da_monitor_free_hook(&mon_storage->rv.da_mon); > hash_del_rcu(&mon_storage->node); > kfree(mon_storage); > } [Severity: High] Can this code cause a kernel panic on module unload if a dynamically monitored object was destroyed right before? Since da_kfree_cb() is defined as a static function in da_monitor.h, it will be compiled directly into the loadable monitor module's text segment. If da_destroy_storage() calls call_rcu() just before the module is unloaded, the RCU core could attempt to execute da_kfree_cb() after the module's text has been unmapped, causing an instruction page fault. Should da_monitor_destroy() call rcu_barrier() to wait for these callbacks before the module unloads? I noticed follow-up patches in the series add rcu_barrier() for the pool allocation strategy, but the default auto allocation strategy seems to be left vulnerable. > diff --git a/include/rv/ha_monitor.h b/include/rv/ha_monitor.h > index 499ce60779f9c..99b60d0d26b6d 100644 > --- a/include/rv/ha_monitor.h > +++ b/include/rv/ha_monitor.h [ ... ] > @@ -467,8 +470,15 @@ static inline bool ha_cancel_timer(struct ha_monitor > *ha_mon) > { > return timer_delete(&ha_mon->timer); > } > +/* timer_setup() sets it; the cancel paths here never clear it. */ > +static inline bool ha_timer_ready(struct ha_monitor *ha_mon) > +{ > + return READ_ONCE(ha_mon->timer.function); > +} > static inline void ha_cancel_timer_sync(struct ha_monitor *ha_mon) > { > + if (!ha_timer_ready(ha_mon)) > + return; > timer_delete_sync(&ha_mon->timer); [Severity: Low] Since this is the teardown path prior to freeing the storage object, should this be timer_shutdown_sync() instead? The kernel timer subsystem guidelines mandate the use of timer_shutdown_sync() for teardown paths because it permanently prevents the timer from being re-armed. While concurrent RCU readers cannot reach the object at this point, using timer_delete_sync() for teardown is a documented anti-pattern. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
