Re: [PATCH v2] mm/slab: sample slab_state once in kmem_cache_destroy()

[Vlastimil Babka (SUSE)]

On 10/15/26 16:48, Harry Yoo wrote:
> On Thu, Oct 15, 2026 at 09:21:43AM +0200, Vlastimil Babka (SUSE) wrote:
>> On 10/14/26 17:08, Imre Kaloz wrote:
>> > kmem_cache_release() tests slab_state >= FULL after kmem_cache_destroy()
>> > has dropped slab_mutex and unlinked the cache, and for a
>> > SLAB_TYPESAFE_BY_RCU cache after rcu_barrier(). If slab_late_init() runs
>> > before that test it sets slab_state to FULL but skips the unlinked cache,
>> > which never got a sysfs entry, so sysfs_slab_release() calls kobject_put()
>> > on a kobject that was never initialized:
>> > 
>> >   WARNING: lib/kobject.c:734 at kobject_put+0x64/0x2c0, CPU#1: kworker/u8:3/55
>> >   kobject: '(null)' ((____ptrval____)): is not initialized, yet kobject_put() is being called.
>> >   refcount_t: underflow; use-after-free.
>> >   kobject_put+0x64/0x2c0
>> >   sysfs_slab_release+0xc/0x20
>> >   kmem_cache_destroy+0x104/0x1e0
>> >   bioset_exit+0x13c/0x1e0
>> >   disk_release+0x54/0x140
>> >   put_disk+0x18/0x40
>> >   floppy_async_init+0xbec/0xd10
>> > 
>> > The release-time test has run outside slab_mutex since at least v4.4 for
>> > SLAB_TYPESAFE_BY_RCU caches, and since v6.0 for the others. Since v6.12
>> > the unlink-time test in kmem_cache_destroy() is a separate read of its
>> > own, also after mutex_unlock(), so the two can disagree. Both tests ask
>> 
>> It would be useful to mention the commits here along with the versions. Also
>> this suggests 3 commits but there's only 2 in Fixes.
> 
> Agreed.
> 
> At second look I think we should improve the changelog a bit....
> And the history of this race is really... messy.
> 
> I'll just rewrite this part a little bit with specific commits and
> add Fixes: tags for each commit that introduced for re-introduced
> the commit, along with some history so that people* can decide whether
> they* should pick this up.
> 
> *who may maintain downstream stable Linux kernels
> 
>> > the unlink-time test in kmem_cache_destroy() is a separate read of its
>> > own, also after mutex_unlock(), so the two can disagree.
> 
> This part is unnecessary because reading slab_state outside
> slab_mutex is what we should fix. As long as it's read under
> slab_mutex, reading it once or twice doesn't really matter.
> 
> (See full changelog at the end of this email)
> 
>> I would guess the 6.0 change is not a crucial one?
> 
> Uh, I thought the Fixes: was 6.0 one, not 6.12 one :/
> 
>> Reviewed-by: Vlastimil Babka (SUSE) <vbabka@xxxxxxxxxx>
> 
> Thanks!
> 
> So... below is my suggestion. What do you think?

Oh wow, what a complicated history :)

LGTM, thanks!

> commit 13564e1cfc847aa540019b55a1f8e588670892fe
> Author: Imre Kaloz <kaloz@xxxxxxxxxx>
> Date:   Wed Oct 14 17:08:43 2026 +0200
> 
>     mm/slab: sample slab_state once in kmem_cache_destroy()
>     
>     kmem_cache_release() tests slab_state >= FULL after kmem_cache_destroy()
>     has dropped slab_mutex and unlinked the cache, and for a
>     SLAB_TYPESAFE_BY_RCU cache after rcu_barrier(). If slab_late_init() runs
>     before that test it sets slab_state to FULL but skips the unlinked cache,
>     which never got a sysfs entry, so sysfs_slab_release() calls kobject_put()
>     on a kobject that was never initialized:
>     
>       WARNING: lib/kobject.c:734 at kobject_put+0x64/0x2c0, CPU#1: kworker/u8:3/55
>       kobject: '(null)' ((____ptrval____)): is not initialized, yet kobject_put() is being called.
>       refcount_t: underflow; use-after-free.
>       kobject_put+0x64/0x2c0
>       sysfs_slab_release+0xc/0x20
>       kmem_cache_destroy+0x104/0x1e0
>       bioset_exit+0x13c/0x1e0
>       disk_release+0x54/0x140
>       put_disk+0x18/0x40
>       floppy_async_init+0xbec/0xd10
>     
>     slab_state >= FULL asks whether the cache has a kobject, which cache
>     creation skips while slab_state <= UP. UP is the state just below FULL,
>     so a false sample cannot skip a kobject_put() that was owed.
>     
>     Seen on sparc64 at boot, where the asynchronous floppy init tears down
>     its bio slab while the late initcalls are running.
>     
>     Sample slab_state once, under slab_mutex which slab_late_init() holds
>     when it sets FULL, and use the result for both the unlink and the
>     release.
>     
>     The history of this problem dates back to v3.2:
>     
>       Commit 69cb8e6b7c29 ("slub: free slabs without holding locks") in
>       v3.2 moved sysfs_slab_remove() outside slub_lock and introduced
>       a race condition between kmem_cache_destroy() and slab_sysfs_init().
>     
>       Commit 945cf2b6199be ("mm/sl[aou]b: Extract a common function for
>       kmem_cache_destroy") in v3.7 accidentally closed the race while
>       extracting a common implementation for kmem_cache_destroy().
>     
>       Commit 5413dfba88d6 ("slub: drop mutex before deleting sysfs entry")
>       in v3.8 re-opened the race by dropping slab_mutex around
>       sysfs_slab_remove() in __kmem_cache_shutdown(), to avoid a lockdep
>       splat between slab_mutex and the sysfs lock.
>     
>       Commit 657dc2f97220 ("slab: remove synchronous rcu_barrier() call in
>       memcg cache release path") in v4.11 shifted from synchronous
>       rcu_barrier() to asynchronous RCU work for SLAB_TYPESAFE_BY_RCU caches.
>       That commit, again, accidentally closed the race by moving
>       sysfs_slab_release() to shutdown_cache(), but only for
>       !SLAB_TYPESAFE_BY_RCU caches because destruction of
>       SLAB_TYPESAFE_BY_RCU caches is done in an RCU work.
>     
>       Commit 0495e337b703 ("mm/slab_common: Deleting kobject in
>       kmem_cache_destroy() without holding slab_mutex/cpu_hotplug_lock") in
>       v6.0 re-introduced the race for !SLAB_TYPESAFE_BY_RCU caches.
>     
>     Fixes: 69cb8e6b7c29 ("slub: free slabs without holding locks")
>     Fixes: 5413dfba88d6 ("slub: drop mutex before deleting sysfs entry")
>     Fixes: 0495e337b703 ("mm/slab_common: Deleting kobject in kmem_cache_destroy() without holding slab_mutex/cpu_hotplug_lock")
>     Cc: stable@xxxxxxxxxxxxxxx
>     Signed-off-by: Imre Kaloz <kaloz@xxxxxxxxxx>
>     Reviewed-by: Jose A. Perez de Azpillaga <azpijr@xxxxxxxxx>
>     Reviewed-by: Harry Yoo (Meta) <harry@xxxxxxxxxx>
>     Link: https://patch.msgid.link/20261014150844.8536-1-kaloz@xxxxxxxxxx
>     Reviewed-by: Vlastimil Babka (SUSE) <vbabka@xxxxxxxxxx>
>     Signed-off-by: Harry Yoo (Meta) <harry@xxxxxxxxxx>
>