Re: [PATCH v1] mm/slab: Allow cache creation to proceed even if sysfs registration fails

[Vlastimil Babka <vbabka@suse.cz>]

On 11/1/24 14:15, Hyeonggon Yoo wrote:
> On Fri, Nov 1, 2024 at 10:08 PM Hyeonggon Yoo <42.hyeyoo@gmail.com> wrote:
>>
>> When kobject_init_and_add() fails during cache creation,
>> kobj->name can be leaked because SLUB does not call kobject_put(),
>> which should be invoked per the kobject API documentation.
>> This has a bit of historical context, though; SLUB does not call
>> kobject_put() to avoid double-free for struct kmem_cache because
>> 1) simply calling it would free all resources related to the cache, and
>> 2) struct kmem_cache descriptor is always freed by cache_cache()'s
>> error handling path, causing struct kmem_cache to be freed twice.
>>
>> This issue can be reproduced by creating new slab caches while applying
>> failslab for kernfs_node_cache. This makes kobject_add_varg() succeed,
>> but causes kobject_add_internal() to fail in kobject_init_and_add()
>> during cache creation.
>>
>> Historically, this issue has attracted developers' attention several times.
>> Each time a fix addressed either the leak or the double-free,
>> it caused the other issue. Let's summarize a bit of history here:
>>
>>   The leak has existed since the early days of SLUB.
>>
>>   Commit 54b6a731025f ("slub: fix leak of 'name' in sysfs_slab_add")
>>   introduced a double-free bug while fixing the leak.
>>
>>   Commit 80da026a8e5d ("mm/slub: fix slab double-free in case of duplicate
>>   sysfs filename") re-introduced the leak while fixing the double-free
>>   error.
>>
>>   Commit dde3c6b72a16 ("mm/slub: fix a memory leak in sysfs_slab_add()")
>>   fixed the memory leak, but it was later reverted by commit 757fed1d0898
>>   ("Revert "mm/slub: fix a memory leak in sysfs_slab_add()"") to avoid
>>   the double-free error.
>>
>>   This is where we are now: we've chosen a memory leak over a double-free.
>>
>> To resolve this memory leak, skip creating sysfs files if it fails
>> and continue with cache creation regardless (as suggested by Christoph).
>> This resolves the memory leak because both the cache and the kobject
>> remain alive on kobject_init_and_add() failure.
>>
>> If SLUB tries to create an alias for a cache without sysfs files,
>> its symbolic link will not be generated.
>>
>> Since a slab cache might not have associated sysfs files, call kobject_del()
>> only if such files exist.
>>
>> Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>

Thanks, added to slab/for-next

> 
> [+Cc Jinjie, Liu]
> 
>> ---
>>
>> RFC -> v1: Make sysfs optional instead of destroying the cache on sysfs
>> errors. (Suggested by Christoph)
> 
> RFC version for reference:
> https://lore.kernel.org/linux-mm/20241021091413.154775-1-42.hyeyoo@gmail.com/
> 
>>  mm/slub.c | 21 +++++++++++++++++----
>>  1 file changed, 17 insertions(+), 4 deletions(-)
>>
>> diff --git a/mm/slub.c b/mm/slub.c
>> index 151a987dc3a0..b4b211468f77 100644
>> --- a/mm/slub.c
>> +++ b/mm/slub.c
>> @@ -6116,7 +6116,8 @@ __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;
>> +                       pr_err("SLUB: Unable to add slab alias %s to sysfs\n",

I've changed "slab" to "cache".

>> +                              name);
>>
>>                 s->refcount++;
>>
>> @@ -6204,9 +6205,15 @@ int do_kmem_cache_create(struct kmem_cache *s, const char *name,
>>                 goto out;
>>         }
>>
>> +       /*
>> +        * Failing to create sysfs files is not critical to SLUB functionality.
>> +        * If it fails, proceed with cache creation without these files.
>> +        */
>>         err = sysfs_slab_add(s);
>> -       if (err)
>> -               goto out;
>> +       if (err) {
>> +               err = 0;
>> +               pr_err("SLUB: Unable to add slab %s to sysfs\n", s->name);

Also here, and simplified to "if (sysfs_slab_add(s)) ... " to avoid dealing
with err.

>> +       }
>>
>>         if (s->flags & SLAB_STORE_USER)
>>                 debugfs_slab_add(s);
>> @@ -7276,7 +7283,8 @@ static int sysfs_slab_add(struct kmem_cache *s)
>>
>>  void sysfs_slab_unlink(struct kmem_cache *s)
>>  {
>> -       kobject_del(&s->kobj);
>> +       if (s->kobj.state_in_sysfs)
>> +               kobject_del(&s->kobj);
>>  }
>>
>>  void sysfs_slab_release(struct kmem_cache *s)
>> @@ -7305,6 +7313,11 @@ static int sysfs_slab_alias(struct kmem_cache *s, const char *name)
>>                  * If we have a leftover link then remove it.
>>                  */
>>                 sysfs_remove_link(&slab_kset->kobj, name);
>> +               /*
>> +                * The original cache may have failed to generate sysfs file.
>> +                * In that case, sysfs_create_link() returns -ENOENT and
>> +                * symbolic link creation is skipped.
>> +                */
>>                 return sysfs_create_link(&slab_kset->kobj, &s->kobj, name);
>>         }
>>
>> --
>> 2.45.0
>>