Re: [PATCH v7 04/14] rv: Disarm HA timers on the storage free path

[Gabriele Monaco <gmonaco@redhat.com>]

On Mon, 2026-10-12 at 03:39 +0800, wen.yang@linux.dev wrote:
> From: Wen Yang <wen.yang@linux.dev>
> 
> 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 <wen.yang@linux.dev>
> ---
>  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