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

[sashiko-bot@kernel.org]

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 <wen.yang@linux.dev>

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