Re: [PATCH v7 08/14] rv: Add per-object teardown hook da_extra_cleanup

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] rv: missing rcu_barrier() for non-pool allocations causes UAF on module unload
--

commit f8013d6f70621f02511ed8a820d487338e72eed7
Author: Wen Yang <wen.yang@linux.dev>

rv: Add per-object teardown hook da_extra_cleanup

da_monitor_destroy() frees each per-object storage, but a monitor
whose target is allocated separately has no point to free it: the
target is still reachable from the storage until hash_del_rcu().
Add an optional da_extra_cleanup() hook, called for each storage after
da_monitor_free_hook() has disarmed its timer and before the storage is
unlinked. It defaults to a no-op. tlob, added later in this series,
uses it to retire and free its per-window record.

> diff --git a/include/rv/da_monitor.h b/include/rv/da_monitor.h
> index 7d1026e9edbc6..04caaf529034b 100644
> --- a/include/rv/da_monitor.h
> +++ b/include/rv/da_monitor.h

[ ... ]

> @@ -715,7 +725,9 @@ 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) {
> +		/* Before da_extra_cleanup(): it frees what the timer reads. */
>  		da_monitor_free_hook(&mon_storage->rv.da_mon);
> +		da_extra_cleanup(&mon_storage->rv.da_mon);
>  		hash_del_rcu(&mon_storage->node);
>  		if (DA_MON_ALLOCATION_STRATEGY == DA_ALLOC_POOL)
>  			mempool_free(mon_storage, &da_monitor_pool);

[Severity: High]
Does this missing rcu_barrier() for non-pool allocations cause a use-after-free
on module unload?

Further down in da_monitor_destroy(), rcu_barrier() is only called for
DA_ALLOC_POOL allocations:

include/rv/da_monitor.h:da_monitor_destroy() {
	...
	if (DA_MON_ALLOCATION_STRATEGY == DA_ALLOC_POOL) {
		rcu_barrier();
		mempool_exit(&da_monitor_pool);
	}
}

When a monitor using DA_ALLOC_AUTO or DA_ALLOC_MANUAL is active and an event
calls da_destroy_storage(), it queues the da_kfree_cb() callback:

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

Because da_kfree_cb() is a static function in this header, its code resides
in the loadable module of the specific monitor.

If the user unloads the monitor module immediately after a monitored object is
destroyed, da_monitor_destroy() is called during the module exit path. Since
there is no rcu_barrier() for non-pool allocations, the module's text segment
is unmapped while da_kfree_cb() is still pending in the RCU queues.

This leads to a kernel panic when the RCU core attempts to execute the unmapped
callback.

Should the rcu_barrier() be called unconditionally so that all pending
RCU callbacks finish executing before the module exits and its memory is
unmapped?

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