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

[Hyeonggon Yoo <42.hyeyoo@gmail.com>]

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>

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