mirror of https://lore.kernel.org/lkml/
 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: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-09-30 19:51 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]

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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®