linux-kernel - Re: [PATCH v2] mm/slub: Simplify __kmem_cache

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]

Date:   Wed, 15 Jun 2022 10:37:00 +0200
From:   Vlastimil Babka <vbabka@...e.cz>
To:     Xiongwei Song <sxwjean@...il.com>,
        Hyeonggon Yoo <42.hyeyoo@...il.com>
Cc:     Xiongwei Song <sxwjean@...com>, Christoph Lameter <cl@...ux.com>,
        Pekka Enberg <penberg@...nel.org>,
        David Rientjes <rientjes@...gle.com>,
        Joonsoo Kim <iamjoonsoo.kim@....com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Roman Gushchin <roman.gushchin@...ux.dev>,
        Muchun Song <songmuchun@...edance.com>,
        "linux-mm @ kvack . org" <linux-mm@...ck.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Xiongwei Song <xiongwei.song@...driver.com>
Subject: Re: [PATCH v2] mm/slub: Simplify __kmem_cache_alias()

On 6/5/22 09:04, Xiongwei Song wrote:
> On Sat, Jun 4, 2022 at 5:43 PM Hyeonggon Yoo <42.hyeyoo@...il.com> wrote:
>>
>> On Fri, Jun 03, 2022 at 10:35:55PM +0800, sxwjean@...com wrote:
>> > From: Xiongwei Song <xiongwei.song@...driver.com>
>> >
>> > There is no need to do anything if sysfs_slab_alias() return nonzero
>> > value after getting a mergeable cache.
>> >
>> > Signed-off-by: Xiongwei Song <xiongwei.song@...driver.com>
>> > Reviewed-by: Muchun Song <songmuchun@...edance.com>
>> > ---
>> > v2: Collect Reviewed-by tag from Muchun.

Hmm I added v1 (with the Reviewed tag) before getting to the v2 thread. But
I think it's fine, see below.

>> > ---
>> >  mm/slub.c | 8 +++-----
>> >  1 file changed, 3 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/mm/slub.c b/mm/slub.c
>> > index d8d5abf49f5f..9444277d669a 100644
>> > --- a/mm/slub.c
>> > +++ b/mm/slub.c
>> > @@ -4861,6 +4861,9 @@ __kmem_cache_alias(const char *name, unsigned int size, unsigned int align,
>> >
>> >       s = find_mergeable(size, align, flags, name, ctor);
>> >       if (s) {
>> > +             if (sysfs_slab_alias(s, name))
>> > +                     return NULL;
>> > +
>> >               s->refcount++;
>> >
>>
>> I think we should not expose sysfs attributes before initializing
>> what can be read via sysfs attribute (object_size).

Hmm I don't think they are unitialized. They have an old value from the
cache we are merging with, which is updated if the new aliased cache has a
larger one.
So yeah we might briefly during creation expose an alias that will have an
incorrect value, but I doubt anything will break. The values are not stable
anyway as new aliases are added, as we are bumping them for the 'root' cache
and all aliases that share it already.

>> >               /*
>> > @@ -4869,11 +4872,6 @@ __kmem_cache_alias(const char *name, unsigned int size, unsigned int align,
>> >                */
>> >               s->object_size = max(s->object_size, size);
>>
>> this calculation should be done before sysfs_slab_alias().
> 
> Yeah, understood. Should we restore s->object_size and s->inuse if
> sysfs_slab_alias() returns non zero value?

And by bailing out early this patch effectively achieves that, so I'd say
it's a better state than before the patch so I'll keep it unless proven
otherwise. Thanks!

> Regards,
> Xiongwwei
> 
>>
>> Thanks,
>> Hyeonggon
>>
>> >               s->inuse = max(s->inuse, ALIGN(size, sizeof(void *)));
>> > -
>> > -             if (sysfs_slab_alias(s, name)) {
>> > -                     s->refcount--;
>> > -                     s = NULL;
>> > -             }
>> >       }
>> >
>> >       return s;
>> > --
>> > 2.30.2
>> >
>>