All of lore.kernel.org
 help / color / mirror / Atom feed
From: Harry Yoo <harry@kernel.org>
To: Imre Kaloz <kaloz@kernel.org>
Cc: Vlastimil Babka <vbabka@kernel.org>,
	 Andrew Morton <akpm@linux-foundation.org>,
	Hao Li <hao.li@linux.dev>, Christoph Lameter <cl@gentwo.org>,
	 David Rientjes <rientjes@google.com>,
	Roman Gushchin <roman.gushchin@linux.dev>,
	 Jann Horn <jannh@google.com>,
	linux-mm@kvack.org, linux-kernel@vger.kernel.org,
	 stable@vger.kernel.org
Subject: Re: [PATCH] mm/slab: sample slab_state once in kmem_cache_destroy()
Date: Wed, 14 Oct 2026 14:07:18 +0100	[thread overview]
Message-ID: <as96u-sh8BbjXW24@thinkstation> (raw)
In-Reply-To: <e4184209-75ee-929d-0103-ca15d5886000@kernel.org>

On Tue, Oct 13, 2026 at 06:32:26PM +0200, Imre Kaloz wrote:
> Hi Harry,
> 
> On Tue, 13 Oct 2026, Harry Yoo wrote:
> 
> <snip>
> 
> > > > > There is a related older window: after list_del() and before that
> > > > > single read, slab_sysfs_init() could set FULL, skip this cache, and
> > > > > the single read would then call both unlink and release on an
> > > > > uninitialized kobject.
> > > > 
> > > > No, with this patch slab_sysfs_init() cannot set the state to FULL
> > > > because the slab_caches list and slab_state are protected by slab_mutex.
> > > > 
> > > > > I have not hit that, and this patch does not close it.
> > > > 
> > > > I think the patch does close two bugs.
> > > > 
> > > > One introduced by 4ec10268ed98 (reading slab_state twice),
> > > > another (reading slab_state outside slab_mutex) introduced by, hmm,
> > > > not sure which commit introduced it :) should be very old.
> > > 
> > > You're right, I got that one wrong. Sampling under slab_mutex in the
> > > same section as the list_del() means slab_late_init() either sees the
> > > cache on the list or we see slab_state < FULL, so the older window is
> > > closed too.
> > > 
> > > As for where the outside-mutex read came from, as far as I can tell it's
> > > 0495e337b703 ("mm/slab_common: Deleting kobject in kmem_cache_destroy()
> > > without holding slab_mutex/cpu_hotplug_lock"). Before that, the unlink
> > > and release and their slab_state checks ran in shutdown_cache() under
> > > slab_mutex.
> > 
> > 0495e337b703 Probably yes. Not 100% sure.
> > 
> > It's quite messy, reading slab_state has been done inside, switched
> > to outside, and back to inside, and so on.
> > 
> > But should be fine as long as we can find the first commit that
> > affects -stable kernels (>= 5.10.y)
> > 
> > > SLAB_TYPESAFE_BY_RCU caches were already released from the
> > > workfn outside the mutex since 657dc2f97220.
> > > 
> > > Want me to send a v2 with a second Fixes: for 0495e337b703, or is it
> > > fine to add it when applying?
> > 
> > Could you please verify fixing which bugs resolves the issue on sparc64
> > described in the changelog? Reading it twice, or outside slab_mutex, or
> > both?
> 
> I tested it. I built five variants of rc7 with a small test that
> destroys a SLAB_TYPESAFE_BY_RCU cache while slab_late_init() runs
> (same path the floppy bio slab takes), and booted each 10 times on
> x86 and 8 times on sparc64. Boots that hit the WARN:
> 
>                                              x86     sparc64
>   mainline                                   9/10    8/8
>   one read, in kmem_cache_release()          9/10    8/8
>   unlink check under slab_mutex,
>     release still re-reads                   10/10   8/8
>   one read under slab_mutex (this patch)     0/10    0/8
>   one read right after mutex_unlock()        0/10    0/8
> 
> So it takes both. The read that hurts is the one at release time,
> after rcu_barrier(); fixing only the double read keeps it, and so
> does moving only the unlink check under the mutex. Reading once
> right after the unlock didn't trigger it, but that is still racy,
> just a much smaller window.

Thanks for verifying that.

> For stable, as far as I can tell the release-side read has been
> outside slab_mutex since d50d82faa0c9 ("slub: fix failure when we
> delete and create a slab cache") in v4.18

For SLAB_TYPESAFE_BY_RCU caches, yes.
and others since 0495e337b703.

I think Fixes: d50d82faa0c9 should be fine.

> , so that would cover
> 5.10.y. The patch doesn't apply before 6.12, so the older branches
> need their own backports.

Ack.
Are you willing to backport it to -stable kernels as well?

And could you please post v2 with updated Fixes: and the
commit message ;)

Thanks for fixing this!

-- 
Cheers,
Harry / Hyeonggon


  reply	other threads:[~2026-10-14 13:07 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-09-30 19:51 [PATCH] mm/slab: sample slab_state once in kmem_cache_destroy() Imre Kaloz
2026-09-30 21:26 ` Jose A. Perez de Azpillaga
2026-10-01 14:22 ` Harry Yoo
2026-10-01 15:42   ` Imre Kaloz
2026-10-13 12:38     ` Harry Yoo
2026-10-13 12:51       ` Imre Kaloz
2026-10-13 14:29         ` Harry Yoo
2026-10-13 16:32           ` Imre Kaloz
2026-10-14 13:07             ` Harry Yoo [this message]
2026-10-14 15:21               ` Imre Kaloz

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=as96u-sh8BbjXW24@thinkstation \
    --to=harry@kernel.org \
    --cc=akpm@linux-foundation.org \
    --cc=cl@gentwo.org \
    --cc=hao.li@linux.dev \
    --cc=jannh@google.com \
    --cc=kaloz@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=rientjes@google.com \
    --cc=roman.gushchin@linux.dev \
    --cc=stable@vger.kernel.org \
    --cc=vbabka@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.