On Mon, 2026-10-12 at 03:39 +0800, [email protected] wrote:
> From: Wen Yang <[email protected]>
>
> da_destroy_storage() cancels the HA timer through da_monitor_reset_hook()
> and only then unlinks the storage with hash_del_rcu(). A handler on
> another CPU that looked the storage up before the unlink can still call
> ha_start_timer_*() after the cancel. The timer then stays queued on a
> storage that kfree_rcu() frees a grace period later, and fires on freed
> memory, reporting a violation for an object that no longer exists.
>
> A monitor cannot close this from its own handlers: any cancel it issues
> precedes the unlink. The first point no reader can race with is the
> RCU callback that frees the storage.
Mmh, looks reasonable, I probably need to check this whole cleanup path again
for consistency and duplication.
>
> Add da_monitor_free_hook(), called right before a per-object storage is
> freed, from the RCU callback and from da_monitor_destroy(). HA monitors
> implement it with ha_cancel_timer_sync(). Replace kfree_rcu() with
> call_rcu() so there is a callback to hook.
>
> A storage whose monitor never started has a zeroed timer, and
> hrtimer_cancel() dereferences timer->base. nomiss pre-creates one such
> storage per possible CPU. Skip them with the new ha_timer_ready().
I struggled with this too, but relying on the internal representation of timers
for this is brittle, can you use da_monitoring() there, like we do in
ha_monitor_reset_env()? We shouldn't have running timers when not monitoring.
I still want to give a deeper look at this when I get more time.
Thanks,
Gabriele
>
> Fixes: 4a24127bd6cb ("rv: Add support for per-object monitors in DA/HA")
> Signed-off-by: Wen Yang <[email protected]>
> ---
> include/rv/da_monitor.h | 23 ++++++++++++++++++++++-
> include/rv/ha_monitor.h | 26 ++++++++++++++++++++++++++
> 2 files changed, 48 insertions(+), 1 deletion(-)
>
> diff --git a/include/rv/da_monitor.h b/include/rv/da_monitor.h
> index e3cf85c9ce55..2cd260208bc1 100644
> --- a/include/rv/da_monitor.h
> +++ b/include/rv/da_monitor.h
> @@ -67,6 +67,17 @@ static struct rv_monitor rv_this;
> #define da_monitor_sync_hook()
> #endif
>
> +/*
> + * Called on the last reference to a storage, right before it is freed.
> + * Monitors embedding a timer disarm it here: an earlier point races with a
> + * reader that resolved the storage before hash_del_rcu(), and
> + * tracepoint_synchronize_unregister() only covers tracepoint probes.
> + * Default: no-op.
> + */
> +#ifndef da_monitor_free_hook
> +#define da_monitor_free_hook(da_mon)
> +#endif
> +
> /*
> * Type for the target id, default to int but can be overridden.
> * A long type can work as hash table key (PER_OBJ) but will be downgraded to
> @@ -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);
> }
> diff --git a/include/rv/ha_monitor.h b/include/rv/ha_monitor.h
> index 499ce60779f9..99b60d0d26b6 100644
> --- a/include/rv/ha_monitor.h
> +++ b/include/rv/ha_monitor.h
> @@ -29,6 +29,8 @@ static inline void ha_monitor_reset_env(struct da_monitor
> *da_mon);
> static inline void ha_setup_timer(struct ha_monitor *ha_mon);
> static inline bool ha_cancel_timer(struct ha_monitor *ha_mon);
> static inline void ha_cancel_timer_sync(struct ha_monitor *ha_mon);
> +static inline bool ha_timer_ready(struct ha_monitor *ha_mon);
> +static inline void ha_monitor_free_env(struct da_monitor *da_mon);
> static bool ha_monitor_handle_constraint(struct da_monitor *da_mon,
> enum states curr_state,
> enum events event,
> @@ -38,6 +40,7 @@ static bool ha_monitor_handle_constraint(struct da_monitor
> *da_mon,
> #define da_monitor_init_hook ha_monitor_init_env
> #define da_monitor_reset_hook ha_monitor_reset_env
> #define da_monitor_sync_hook() synchronize_rcu()
> +#define da_monitor_free_hook ha_monitor_free_env
>
> #if !defined(HA_SKIP_AUTO_CLEANUP) && RV_MON_TYPE == RV_MON_PER_TASK
> /*
> @@ -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);
> }
> #elif HA_TIMER_TYPE == HA_TIMER_HRTIMER
> @@ -524,8 +534,18 @@ static inline bool ha_cancel_timer(struct ha_monitor
> *ha_mon)
> {
> return hrtimer_try_to_cancel(&ha_mon->hrtimer) == 1;
> }
> +/*
> + * hrtimer_cancel() reads timer->base before anything else; a zeroed timer
> + * has it NULL and would fault.
> + */
> +static inline bool ha_timer_ready(struct ha_monitor *ha_mon)
> +{
> + return READ_ONCE(ha_mon->hrtimer.base);
> +}
> static inline void ha_cancel_timer_sync(struct ha_monitor *ha_mon)
> {
> + if (!ha_timer_ready(ha_mon))
> + return;
> hrtimer_cancel(&ha_mon->hrtimer);
> }
> #else /* HA_TIMER_NONE */
> @@ -538,9 +558,15 @@ static inline bool ha_cancel_timer(struct ha_monitor
> *ha_mon)
> {
> return false;
> }
> +static inline bool ha_timer_ready(struct ha_monitor *ha_mon) { return false;
> }
> static inline void ha_cancel_timer_sync(struct ha_monitor *ha_mon) { }
> #endif
>
> +static inline void ha_monitor_free_env(struct da_monitor *da_mon)
> +{
> + ha_cancel_timer_sync(to_ha_monitor(da_mon));
> +}
> +
> #if IS_ENABLED(CONFIG_RV_MONITORS_KUNIT_TEST)
> #ifdef RV_MON_OPS_INIT
> #undef RV_MON_OPS_INIT
