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>
[+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",
> + 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);
> + }
>
> 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
>
