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>
---
RFC -> v1: Make sysfs optional instead of destroying the cache on sysfs
errors. (Suggested by Christoph)
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
