linux-kernel - Re: [PATCH RFC 10/19] slab: remove cpu (partial) slabs usage from allocation paths

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5e8e6e92-ba8f-4fee-bd01-39aacdd30dbe@suse.cz>
Date: Thu, 30 Oct 2025 16:35:52 +0100
From: Vlastimil Babka <vbabka@...e.cz>
To: Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc: Harry Yoo <harry.yoo@...cle.com>,
 Andrew Morton <akpm@...ux-foundation.org>, Christoph Lameter
 <cl@...two.org>, David Rientjes <rientjes@...gle.com>,
 Roman Gushchin <roman.gushchin@...ux.dev>,
 Uladzislau Rezki <urezki@...il.com>,
 "Liam R. Howlett" <Liam.Howlett@...cle.com>,
 Suren Baghdasaryan <surenb@...gle.com>,
 Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
 Alexei Starovoitov <ast@...nel.org>, linux-mm <linux-mm@...ck.org>,
 LKML <linux-kernel@...r.kernel.org>, linux-rt-devel@...ts.linux.dev,
 bpf <bpf@...r.kernel.org>, kasan-dev <kasan-dev@...glegroups.com>
Subject: Re: [PATCH RFC 10/19] slab: remove cpu (partial) slabs usage from
 allocation paths

On 10/30/25 16:27, Alexei Starovoitov wrote:
> On Thu, Oct 30, 2025 at 6:09 AM Vlastimil Babka <vbabka@...e.cz> wrote:
>>
>> On 10/30/25 05:32, Harry Yoo wrote:
>> > On Thu, Oct 23, 2025 at 03:52:32PM +0200, Vlastimil Babka wrote:
>> >> diff --git a/mm/slub.c b/mm/slub.c
>> >> index e2b052657d11..bd67336e7c1f 100644
>> >> --- a/mm/slub.c
>> >> +++ b/mm/slub.c
>> >> @@ -4790,66 +4509,15 @@ static void *___slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node,
>> >>
>> >>      stat(s, ALLOC_SLAB);
>> >>
>> >> -    if (IS_ENABLED(CONFIG_SLUB_TINY) || kmem_cache_debug(s)) {
>> >> -            freelist = alloc_single_from_new_slab(s, slab, orig_size, gfpflags);
>> >> -
>> >> -            if (unlikely(!freelist))
>> >> -                    goto new_objects;
>> >> -
>> >> -            if (s->flags & SLAB_STORE_USER)
>> >> -                    set_track(s, freelist, TRACK_ALLOC, addr,
>> >> -                              gfpflags & ~(__GFP_DIRECT_RECLAIM));
>> >> -
>> >> -            return freelist;
>> >> -    }
>> >> -
>> >> -    /*
>> >> -     * No other reference to the slab yet so we can
>> >> -     * muck around with it freely without cmpxchg
>> >> -     */
>> >> -    freelist = slab->freelist;
>> >> -    slab->freelist = NULL;
>> >> -    slab->inuse = slab->objects;
>> >> -    slab->frozen = 1;
>> >> -
>> >> -    inc_slabs_node(s, slab_nid(slab), slab->objects);
>> >> +    freelist = alloc_single_from_new_slab(s, slab, orig_size, gfpflags);
>> >>
>> >> -    if (unlikely(!pfmemalloc_match(slab, gfpflags) && allow_spin)) {
>> >> -            /*
>> >> -             * For !pfmemalloc_match() case we don't load freelist so that
>> >> -             * we don't make further mismatched allocations easier.
>> >> -             */
>> >> -            deactivate_slab(s, slab, get_freepointer(s, freelist));
>> >> -            return freelist;
>> >> -    }
>> >> +    if (unlikely(!freelist))
>> >> +            goto new_objects;
>> >
>> > We may end up in an endless loop in !allow_spin case?
>> > (e.g., kmalloc_nolock() is called in NMI context and n->list_lock is
>> > held in the process context on the same CPU)
>> >
>> > Allocate a new slab, but somebody is holding n->list_lock, so trylock fails,
>> > free the slab, goto new_objects, and repeat.
>>
>> Ugh, yeah. However, AFAICS this possibility already exists prior to this
>> patch, only it's limited to SLUB_TINY/kmem_cache_debug(s). But we should fix
>> it in 6.18 then.
>> How? Grab the single object and defer deactivation of the slab minus one
>> object? Would work except for kmem_cache_debug(s) we open again a race for
>> inconsistency check failure, and we have to undo the simple slab freeing fix
>>  and handle the accounting issue differently again.
>> Fail the allocation for the debug case to avoid the consistency check
>> issues? Would it be acceptable for kmalloc_nolock() users?
> 
> You mean something like:
> diff --git a/mm/slub.c b/mm/slub.c
> index a8fcc7e6f25a..e9a8b75f31d7 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -4658,8 +4658,11 @@ static void *___slab_alloc(struct kmem_cache
> *s, gfp_t gfpflags, int node,
>         if (kmem_cache_debug(s)) {
>                 freelist = alloc_single_from_new_slab(s, slab,
> orig_size, gfpflags);
> 
> -               if (unlikely(!freelist))
> +               if (unlikely(!freelist)) {
> +                       if (!allow_spin)
> +                               return NULL;
>                         goto new_objects;
> +               }
> 
> or I misunderstood the issue?

Yeah that would be the easiest solution, if you can accept the occasional
allocation failures.