On 11/1/24 14:15, Hyeonggon Yoo wrote:
> On Fri, Nov 1, 2024 at 10:08 PM Hyeonggon Yoo <42.hyeyoo@xxxxxxxxx> 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@xxxxxxxxx>
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@xxxxxxxxx/
>
>> 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
>>
