Re: [PATCH v7 07/14] rv: Introduce DA_MON_ALLOCATION_STRATEGY

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] mm: spinlock_t acquired in tracepoint via DA_ALLOC_POOL
- [High] rcu: unprotected hash_add_rcu() corrupts da_monitor_ht
- [High] rv: execution after free on module unload due to missing rcu_barrier()
- [Low] kmemleak: asymmetric kfree() of kmalloc_nolock() storage
--

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

rv: Introduce DA_MON_ALLOCATION_STRATEGY

Per-object DA storage allocation is currently limited to kmalloc on
demand. Add a compile-time selector so monitors can choose among three
strategies:

  DA_ALLOC_AUTO   (default) - kmalloc per object on the monitor path
  DA_ALLOC_POOL             - pre-allocated fixed-size mempool;
                              selected by defining DA_MON_POOL_SIZE
  DA_ALLOC_MANUAL           - caller pre-inserts storage; framework
                              only links the target field

nomiss switches from the implicit DA_SKIP_AUTO_ALLOC override to an
explicit DA_MON_ALLOCATION_STRATEGY DA_ALLOC_MANUAL; the behaviours are
equivalent (pre-allocated storage, no allocation on the sched_switch
start path).

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

[ ... ]

> @@ -563,6 +601,46 @@ static void da_kfree_cb(struct rcu_head *head)

[ ... ]

> +/*
> + * da_create_pool_storage - pop a free pool slot and insert it into the hash.
> + * Returns NULL if the pool is exhausted.  Caller holds an RCU read-side CS
> + * and the monitor's serialisation lock.
> + */
> +static inline struct da_monitor *
> +da_create_pool_storage(da_id_type id, monitor_target target,
> +		       struct da_monitor *da_mon)
> +{
> +	struct da_monitor_storage *mon_storage;
> +
> +	if (da_mon)
> +		return da_mon;
> +
> +	mon_storage = mempool_alloc_preallocated(&da_monitor_pool);
> +	if (!mon_storage)
> +		return NULL;

[Severity: High]
Does this code acquire a spinlock_t inside an atomic context?

Tracepoint handlers, such as sched_switch handled by nomiss, execute in an
atomic context with preemption disabled.

The mempool_alloc_preallocated() function internally acquires the pool's
spinlock_t lock.

On preempt-rt kernels, spinlock_t maps to a sleeping rt_mutex. Acquiring it
inside an atomic, preemption-disabled context violates the nesting hierarchy,
resulting in "Invalid wait context" lockdep splats and a sleep-in-atomic panic.

> +	memset(mon_storage, 0, sizeof(*mon_storage));
> +
> +	mon_storage->id = id;
> +	mon_storage->target = target;
> +
> +	hash_add_rcu(da_monitor_ht, &mon_storage->node, id);
> +	return &mon_storage->rv.da_mon;
> +}

[Severity: High]
Will concurrent modifications corrupt the global da_monitor_ht?

Tracepoint handlers execute concurrently across multiple CPUs. When starting a
new monitor for an object, the caller only holds an RCU read lock, providing no
mutual exclusion against concurrent writers on other CPUs.

Because hash_add_rcu() uses hlist_add_head_rcu() which modifies the global
bucket's head pointer, simultaneous unprotected writes from different CPUs
tracking different targets could corrupt the bucket's linked list structure
and cause traversal hangs or lost objects.

[ ... ]

> @@ -584,7 +662,10 @@ 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);
> -	call_rcu(&mon_storage->rcu, da_kfree_cb);
> +	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);
>  }

[Severity: Low]
Are there kmemleak warnings triggered by this asymmetric free?

Monitor storage for DA_ALLOC_AUTO and DA_ALLOC_MANUAL is allocated using
kmalloc_nolock() inside da_create_empty_storage().

This path queues da_kfree_cb() which frees the memory using the standard
kfree() API.

Allocations made with kmalloc_nolock() must be paired precisely with
kfree_nolock(). Mixing them bypasses the kmemleak tracking hooks unevenly,
which can cause "Trying to color unknown object" warnings or false memory leak
reports.

[ ... ]

> @@ -629,20 +717,33 @@ static inline void da_monitor_destroy(void)
>  	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);
> +		if (DA_MON_ALLOCATION_STRATEGY == DA_ALLOC_POOL)
> +			mempool_free(mon_storage, &da_monitor_pool);
> +		else
> +			kfree(mon_storage);
> +	}
> +
> +	if (DA_MON_ALLOCATION_STRATEGY == DA_ALLOC_POOL) {
> +		rcu_barrier();
> +		mempool_exit(&da_monitor_pool);
>  	}
>  }

[Severity: High]
Is it safe to skip rcu_barrier() for the other allocation strategies?

For DA_ALLOC_AUTO and DA_ALLOC_MANUAL, call_rcu() is used to queue da_kfree_cb.
Because da_kfree_cb is declared as static inside include/rv/da_monitor.h, its
text is compiled directly into the dynamically loaded RV monitor module.

If da_destroy_storage() is called shortly before the module is unloaded, the
callbacks will be queued. If da_monitor_destroy() doesn't call rcu_barrier() to
wait for pending callbacks, the module will be unloaded and the RCU softirq
will jump to an unmapped callback address, causing a kernel panic.

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