Re: [PATCH v6 5/9] rv: Make da_monitor_reset_hook and EVENT_NONE_LBL overridable

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Non-synchronous timer cancellation combined with `synchronize_rcu()` or `kfree_rcu()` leads to a Use-After-Free during monitor teardown.
--

--- Patch [5]: [PATCH v6 5/9] rv: Make da_monitor_reset_hook and EVENT_NONE_LBL overridable ---
commit 693c5f5465506e090a4b8b44d5e3c72769ec0685
Author: Wen Yang <wen.yang@linux.dev>

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 provide their own implementations. It enables
monitors like tlob to define custom reset hooks for tasks such as cancelling
per-task hrtimers.

> 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
>  #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
> +#ifndef da_monitor_sync_hook
>  #define da_monitor_sync_hook() synchronize_rcu()
> +#endif

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to rely on
synchronize_rcu() to wait for cancelled timers during monitor teardown?

Looking at the global teardown path:

include/rv/da_monitor.h:da_monitor_destroy() {
    ...
    da_monitor_reset_all();
    da_monitor_sync_hook();
    ...
}

And per-object teardown:

include/rv/da_monitor.h:da_destroy_storage() {
    ...
    da_monitor_reset_hook(&mon_storage->rv.da_mon);
    hash_del_rcu(&mon_storage->node);
    if (DA_MON_ALLOCATION_STRATEGY == DA_ALLOC_POOL)
        call_rcu(&mon_storage->rcu, da_pool_return_cb);
    else
        kfree_rcu(mon_storage, rcu);
    ...
}

Both paths cancel timers non-synchronously in ha_monitor_reset_env():

include/rv/ha_monitor.h:ha_monitor_reset_env() {
    ...
    ha_cancel_timer(ha_mon);
    ...
}

They then rely on RCU grace periods to wait for executing callbacks,
assuming the callbacks execute within an RCU read-side critical section:

include/rv/ha_monitor.h:__ha_monitor_timer_callback() {
    ...
    guard(rcu)();
    ...
}

If the timer callback is dispatched but preempted or delayed before
entering its RCU read-side critical section, synchronize_rcu() will not
wait for it. When the callback resumes, couldn't it access the freed
monitor storage and cause a use-after-free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1787243842.git.wen.yang@linux.dev?part=5