Re: [PATCH] mm/slub: Flush virtual-slab work before cache destruction

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] mm/slub: undeclared slub_tlbflush_work under nonexistent
  CONFIG_SLAB_VIRTUAL
- [Medium] mm/slub: __kmem_cache_shutdown() flushes work before RCU slab
  free barrier
- [Low] mm/slub: unresolvable Fixes commit 0d6f38b78adda in commit
  message
--

commit eea96dc753bff553cd7a4356f0451c5fa1889295
Author: Felix Hoffmann <f3lix.dev@gmx.de>

> mm/slub: Flush virtual-slab work before cache destruction
>
> Under CONFIG_SLAB_VIRTUAL, freeing a slab queues it to
> slub_tlbflush_work. The worker later follows slab->slab_cache to take the
> cache's virtual-slab lock and update its lists.
>
> __kmem_cache_shutdown() can report success after the last slabs have been
> removed from the cache's accounting but before that deferred work runs.
> The cache can then be released while queued slabs still refer to it. When
> the worker runs, it dereferences the freed kmem_cache and corrupts its
> lists.
>
> Flush the virtual-slab worker after confirming that every cache node is
> empty and before allowing cache destruction to proceed. This drains both
> queued and running work that can still reference the cache.
>
> Fixes: 0d6f38b78adda ("mm/slub: allocate slabs from virtual memory")
> Signed-off-by: Felix Hoffmann <f3lix.dev@gmx.de>

[Severity: Low]
The Fixes: trailer references commit 0d6f38b78adda ("mm/slub: allocate
slabs from virtual memory"), which cannot be resolved in the repository
or tracked maintainer trees.

> diff --git a/mm/slub.c b/mm/slub.c
> index a151268b97263..ec432cff15f58 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -8298,6 +8298,11 @@ int __kmem_cache_shutdown(struct kmem_cache *s)
>  		if (n->nr_partial || node_nr_slabs(n))
>  			return 1;
>  	}
> +
> +#ifdef CONFIG_SLAB_VIRTUAL
> +	/* The TLB flush worker still dereferences slab->slab_cache. */
> +	kthread_flush_work(&slub_tlbflush_work);
> +#endif

[Severity: Medium]
Is this patch missing the prerequisite virtual-slab changes?

Neither CONFIG_SLAB_VIRTUAL nor slub_tlbflush_work exists in the tree,
and mm/slub.c does not include linux/kthread.h for kthread_flush_work().
As a result, this block is unreachable dead code in normal builds and
fails to compile if CONFIG_SLAB_VIRTUAL is defined.

[Severity: Medium]
Can destroying a SLAB_TYPESAFE_BY_RCU cache still race with
slub_tlbflush_work and cause a use-after-free on slab->slab_cache?

During __kmem_cache_shutdown(), discard_slab() decrements the node slab
count via dec_slabs_node() before calling free_slab().  When
SLAB_TYPESAFE_BY_RCU is set, free_slab() defers __free_slab() via
call_rcu(&slab->rcu_head, rcu_free_slab):

mm/slub.c:free_slab() {
    ...
	if (unlikely(s->flags & SLAB_TYPESAFE_BY_RCU))
		call_rcu(&slab->rcu_head, rcu_free_slab);
	else
		__free_slab(s, slab, true);
}

Because node_nr_slabs(n) is already zero, __kmem_cache_shutdown() flushes
slub_tlbflush_work (while holding cpus_read_lock and slab_mutex) before
those rcu_free_slab() callbacks have run.  kmem_cache_destroy() only
waits for rcu_free_slab() afterwards, and then immediately frees the
kmem_cache:

mm/slab_common.c:kmem_cache_destroy() {
    ...
	if (s->flags & SLAB_TYPESAFE_BY_RCU)
		rcu_barrier();

	kmem_cache_release(s, sysfs_ready);
}

Slabs freed by rcu_free_slab() during rcu_barrier() are queued to
slub_tlbflush_work after the flush in __kmem_cache_shutdown() has already
completed, so the worker can still dereference slab->slab_cache after
kmem_cache_release() frees s.  Should kthread_flush_work() be called in
kmem_cache_destroy() after rcu_barrier() (and outside cpus_read_lock and
slab_mutex) instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113221330.162567-1-f3lix.dev@gmx.de?part=1