Re: [PATCH slab/for-next v4 7/8] mm/slab: introduce kfree_rcu_nolock()

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] kfree_call_rcu_nolock() crashes with a NULL pointer dereference when given a ZERO_SIZE_PTR or an unsupported vmalloc/large kmalloc pointer.
- [High] Memory leak and cache destruction panic because deferred_work_barrier() is called after flush_rcu_sheaves_on_cache().
- [High] Calling kfree_rcu_nolock() on PREEMPT_RT triggers an 'Invalid wait context' lockdep warning from local_trylock().
--

--- Patch [7]: [PATCH slab/for-next v4 7/8] mm/slab: introduce kfree_rcu_nolock() ---
commit 278abd26e1c5865b59d3e42ddc961dd9dca8dc17
Author: Harry Yoo (Oracle) <[email protected]>

mm/slab: introduce kfree_rcu_nolock()

This patch introduces the kfree_rcu_nolock() variant to handle
k[v]free_rcu() calls in unknown context. It uses kfree_rcu_sheaf() when
possible and falls back to kvfree_rcu() batching. It also adjusts barrier
placement in kvfree_rcu_barrier_on_cache().

> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index c1c32909fa528..09c457645e039 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
[ ... ]
> @@ -1263,6 +1263,29 @@ EXPORT_TRACEPOINT_SYMBOL(kmem_cache_alloc);
>  EXPORT_TRACEPOINT_SYMBOL(kfree);
>  EXPORT_TRACEPOINT_SYMBOL(kmem_cache_free);
>  
> +void kfree_call_rcu_nolock(struct kvfree_rcu_head *head, void *ptr)
> +{
> +	struct slab *slab;
> +	struct kmem_cache *s;
> +
> +	VM_WARN_ON_ONCE(is_vmalloc_addr(ptr) || !virt_to_slab(ptr));
> +
> +	slab = virt_to_slab(ptr);
> +	s = slab->slab_cache;

[Severity: High]
Does this code properly handle unsupported pointers? For example, if a caller
passes a 0-byte allocation (ZERO_SIZE_PTR) or a large kmalloc object,
virt_to_slab() returns NULL. 

Since the kfree_rcu_nolock() macro only checks if the pointer evaluates to
true (and ZERO_SIZE_PTR is truthy), this would lead to a NULL pointer
dereference when accessing slab->slab_cache. Should this return early if
virt_to_slab() returns NULL instead of just warning?

> +
> +	if (unlikely(IS_ENABLED(CONFIG_NUMA) && slab_nid(slab) != numa_mem_id()))
> +		goto fallback;
> +
> +	if (unlikely(!__kfree_rcu_sheaf(s, ptr, SLAB_FREE_NOLOCK)))

[Severity: High]
Could this trigger an 'Invalid wait context' lockdep warning on PREEMPT_RT?

The kfree_rcu_nolock() variant is intended to be safe for any context,
including under raw spinlocks or in hardirq context. However,
__kfree_rcu_sheaf() acquires s->cpu_sheaves->lock via local_trylock():

mm/slub.c:__kfree_rcu_sheaf() {
    ...
    if (!local_trylock(&s->cpu_sheaves->lock))
        goto fail;
    ...
}

On PREEMPT_RT, local_trylock() maps to a sleepable spin_trylock(). Taking
this lock from a raw spinlock or hardirq context would violate wait-type
nesting rules.

> +		goto fallback;
> +
> +	return;
> +
> +fallback:
> +	defer_kfree_rcu(head);
> +}
> +EXPORT_SYMBOL_GPL(kfree_call_rcu_nolock);
> +
[ ... ]
> @@ -2120,10 +2143,11 @@ void kvfree_rcu_barrier_on_cache(struct kmem_cache *s)
>  		cpus_read_lock();
>  		flush_rcu_sheaves_on_cache(s);
>  		cpus_read_unlock();
> -		deferred_work_barrier();
> -		rcu_barrier();
>  	}
>  
> +	/* kfree_rcu_nolock() might have deferred frees even without sheaves */
> +	deferred_work_barrier();
> +	rcu_barrier();

[Severity: High]
Can this sequence strand objects in the sheaves and cause a cache
destruction panic?

Since deferred_work_barrier() is now called after
flush_rcu_sheaves_on_cache() completes, the deferred work for objects in
objs_by_rcu will call kvfree_call_rcu(). 

If the cache has sheaves, kvfree_call_rcu() will succeed at placing those
objects back into the newly emptied sheaves:

mm/slub.c:deferred_percpu_work_fn() {
    ...
    llnode = llist_del_all(objs_by_rcu);
    llist_for_each_safe(pos, t, llnode) {
        ...
        kvfree_call_rcu(head, objp);
    }
    ...
}

Since there is no further sheaf flush after this repopulation, could these
objects remain stranded and cause kmem_cache_destroy() to fail with a
'Slab cache still has objects' panic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7