Re: [PATCH v3 0/3] Refactor __kmem_cache_create() and fix memory leak

Re: [PATCH v3 0/3] Refactor __kmem_cache_create() and fix memory leak

[Liu Shixin]

Please ignore this set of patches. I need fix and will resend then.


thanks,

Liu Shixin

.

On 2022/11/12 19:20, Liu Shixin wrote:
> I found a memory leak of kobj->name in sysfs_slab_add() which is introduced
> by 80da026a8e5d ("mm/slub: fix slab double-free in case of duplicate sysfs filename").
> Following the rules stated in the comment for kobject_init_and_add():
>
>  If this function returns an error, kobject_put() must be called to
>  properly clean up the memory associated with the object.
>
> We should use kobject_put() to free kobject.
>
> But we can't simply add kobject_put() since it will free kmem_cache too.
> If we use kobject_put(), we need to skip other release functions.
>
> In this series, We refactor the code to separate sysfs_slab_add() and
> debugfs_slab_add() from __kmem_cache_create(), and then use kobject_put()
> to free kobject in sysfs_slab_add(). This can fix the memory leak of
> kobject->name.
>
> v1->v2: Fix build error reported by kernel test robot <lkp@intel.com>.
> v2->v3: Don't free kmem_cache that create early.
>
> Liu Shixin (3):
>   mm/slab_common: Move cache_name to create_cache()
>   mm/slub: Refactor __kmem_cache_create()
>   mm/slub: Fix memory leak of kobj->name in sysfs_slab_add()
>
>  include/linux/slub_def.h | 11 +++++++
>  mm/slab_common.c         | 44 +++++++++++++++-------------
>  mm/slub.c                | 63 +++++++++++++++-------------------------
>  3 files changed, 58 insertions(+), 60 deletions(-)
>

[PATCH v3 0/3] Refactor __kmem_cache_create() and fix memory leak

[Liu Shixin]

I found a memory leak of kobj->name in sysfs_slab_add() which is introduced
by 80da026a8e5d ("mm/slub: fix slab double-free in case of duplicate sysfs filename").
Following the rules stated in the comment for kobject_init_and_add():

 If this function returns an error, kobject_put() must be called to
 properly clean up the memory associated with the object.

We should use kobject_put() to free kobject.

But we can't simply add kobject_put() since it will free kmem_cache too.
If we use kobject_put(), we need to skip other release functions.

In this series, We refactor the code to separate sysfs_slab_add() and
debugfs_slab_add() from __kmem_cache_create(), and then use kobject_put()
to free kobject in sysfs_slab_add(). This can fix the memory leak of
kobject->name.

v1->v2: Fix build error reported by kernel test robot <lkp@intel.com>.
v2->v3: Don't free kmem_cache that create early.

Liu Shixin (3):
  mm/slab_common: Move cache_name to create_cache()
  mm/slub: Refactor __kmem_cache_create()
  mm/slub: Fix memory leak of kobj->name in sysfs_slab_add()

 include/linux/slub_def.h | 11 +++++++
 mm/slab_common.c         | 44 +++++++++++++++-------------
 mm/slub.c                | 63 +++++++++++++++-------------------------
 3 files changed, 58 insertions(+), 60 deletions(-)

-- 
2.25.1

[PATCH v3 1/3] mm/slab_common: Move cache_name to create_cache()

[Liu Shixin]

The string cache_name and its kmem_cache have same life cycle. The latter
is allocated in create_cache() so move cache_name to create_cache() too
for better error handing.

Signed-off-by: Liu Shixin <liushixin2@huawei.com>
---
 mm/slab_common.c | 34 ++++++++++++++--------------------
 1 file changed, 14 insertions(+), 20 deletions(-)

diff --git a/mm/slab_common.c b/mm/slab_common.c
index 33b1886b06eb..e5f430a17d95 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -209,17 +209,21 @@ static struct kmem_cache *create_cache(const char *name,
 		struct kmem_cache *root_cache)
 {
 	struct kmem_cache *s;
-	int err;
+	const char *cache_name;
+	int err = -ENOMEM;
 
 	if (WARN_ON(useroffset + usersize > object_size))
 		useroffset = usersize = 0;
 
-	err = -ENOMEM;
 	s = kmem_cache_zalloc(kmem_cache, GFP_KERNEL);
 	if (!s)
-		goto out;
+		return ERR_PTR(err);
 
-	s->name = name;
+	cache_name = kstrdup_const(name, GFP_KERNEL);
+	if (!cache_name)
+		goto out_free_cache;
+
+	s->name = cache_name;
 	s->size = s->object_size = object_size;
 	s->align = align;
 	s->ctor = ctor;
@@ -228,18 +232,17 @@ static struct kmem_cache *create_cache(const char *name,
 
 	err = __kmem_cache_create(s, flags);
 	if (err)
-		goto out_free_cache;
+		goto out_free_name;
 
 	s->refcount = 1;
 	list_add(&s->list, &slab_caches);
-out:
-	if (err)
-		return ERR_PTR(err);
 	return s;
 
+out_free_name:
+	kfree_const(s->name);
 out_free_cache:
 	kmem_cache_free(kmem_cache, s);
-	goto out;
+	return ERR_PTR(err);
 }
 
 /**
@@ -278,7 +281,6 @@ kmem_cache_create_usercopy(const char *name,
 		  void (*ctor)(void *))
 {
 	struct kmem_cache *s = NULL;
-	const char *cache_name;
 	int err;
 
 #ifdef CONFIG_SLUB_DEBUG
@@ -326,19 +328,11 @@ kmem_cache_create_usercopy(const char *name,
 	if (s)
 		goto out_unlock;
 
-	cache_name = kstrdup_const(name, GFP_KERNEL);
-	if (!cache_name) {
-		err = -ENOMEM;
-		goto out_unlock;
-	}
-
-	s = create_cache(cache_name, size,
+	s = create_cache(name, size,
 			 calculate_alignment(flags, align, size),
 			 flags, useroffset, usersize, ctor, NULL);
-	if (IS_ERR(s)) {
+	if (IS_ERR(s))
 		err = PTR_ERR(s);
-		kfree_const(cache_name);
-	}
 
 out_unlock:
 	mutex_unlock(&slab_mutex);
-- 
2.25.1

[PATCH v3 2/3] mm/slub: Refactor __kmem_cache_create()

[Liu Shixin]

Separate sysfs_slab_add() and debugfs_slab_add() from __kmem_cache_create()
can help to fix a memory leak about kobject. After this patch, we can fix
the memory leak naturally by calling kobject_put() to free kobject and
associated kmem_cache when sysfs_slab_add() failed.

Besides, after that, we can easy to provide sysfs and debugfs support for
other allocators too.

Signed-off-by: Liu Shixin <liushixin2@huawei.com>
---
 include/linux/slub_def.h | 11 ++++++++++
 mm/slab_common.c         | 12 +++++++++++
 mm/slub.c                | 44 +++++++---------------------------------
 3 files changed, 30 insertions(+), 37 deletions(-)

diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h
index f9c68a9dac04..26d56c4c74d1 100644
--- a/include/linux/slub_def.h
+++ b/include/linux/slub_def.h
@@ -144,9 +144,14 @@ struct kmem_cache {
 
 #ifdef CONFIG_SYSFS
 #define SLAB_SUPPORTS_SYSFS
+int sysfs_slab_add(struct kmem_cache *);
 void sysfs_slab_unlink(struct kmem_cache *);
 void sysfs_slab_release(struct kmem_cache *);
 #else
+static inline int sysfs_slab_add(struct kmem_cache *s)
+{
+	return 0;
+}
 static inline void sysfs_slab_unlink(struct kmem_cache *s)
 {
 }
@@ -155,6 +160,12 @@ static inline void sysfs_slab_release(struct kmem_cache *s)
 }
 #endif
 
+#if defined(CONFIG_DEBUG_FS) && defined(CONFIG_SLUB_DEBUG)
+void debugfs_slab_add(struct kmem_cache *);
+#else
+static inline void debugfs_slab_add(struct kmem_cache *s) { }
+#endif
+
 void *fixup_red_left(struct kmem_cache *s, void *p);
 
 static inline void *nearest_obj(struct kmem_cache *cache, const struct slab *slab,
diff --git a/mm/slab_common.c b/mm/slab_common.c
index e5f430a17d95..55e2cf064dfe 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -234,6 +234,18 @@ static struct kmem_cache *create_cache(const char *name,
 	if (err)
 		goto out_free_name;
 
+#ifdef SLAB_SUPPORTS_SYSFS
+	/* Mutex is not taken during early boot */
+	if (slab_state >= FULL) {
+		err = sysfs_slab_add(s);
+		if (err) {
+			slab_kmem_cache_release(s);
+			return ERR_PTR(err);
+		}
+		debugfs_slab_add(s);
+	}
+#endif
+
 	s->refcount = 1;
 	list_add(&s->list, &slab_caches);
 	return s;
diff --git a/mm/slub.c b/mm/slub.c
index ba94eb6fda78..a1ad759753ce 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -299,20 +299,12 @@ struct track {
 enum track_item { TRACK_ALLOC, TRACK_FREE };
 
 #ifdef CONFIG_SYSFS
-static int sysfs_slab_add(struct kmem_cache *);
 static int sysfs_slab_alias(struct kmem_cache *, const char *);
 #else
-static inline int sysfs_slab_add(struct kmem_cache *s) { return 0; }
 static inline int sysfs_slab_alias(struct kmem_cache *s, const char *p)
 							{ return 0; }
 #endif
 
-#if defined(CONFIG_DEBUG_FS) && defined(CONFIG_SLUB_DEBUG)
-static void debugfs_slab_add(struct kmem_cache *);
-#else
-static inline void debugfs_slab_add(struct kmem_cache *s) { }
-#endif
-
 static inline void stat(const struct kmem_cache *s, enum stat_item si)
 {
 #ifdef CONFIG_SLUB_STATS
@@ -4297,7 +4289,7 @@ static int calculate_sizes(struct kmem_cache *s)
 	return !!oo_objects(s->oo);
 }
 
-static int kmem_cache_open(struct kmem_cache *s, slab_flags_t flags)
+int __kmem_cache_create(struct kmem_cache *s, slab_flags_t flags)
 {
 	s->flags = kmem_cache_flags(s->size, flags, s->name);
 #ifdef CONFIG_SLAB_FREELIST_HARDENED
@@ -4900,30 +4892,6 @@ __kmem_cache_alias(const char *name, unsigned int size, unsigned int align,
 	return s;
 }
 
-int __kmem_cache_create(struct kmem_cache *s, slab_flags_t flags)
-{
-	int err;
-
-	err = kmem_cache_open(s, flags);
-	if (err)
-		return err;
-
-	/* Mutex is not taken during early boot */
-	if (slab_state <= UP)
-		return 0;
-
-	err = sysfs_slab_add(s);
-	if (err) {
-		__kmem_cache_release(s);
-		return err;
-	}
-
-	if (s->flags & SLAB_STORE_USER)
-		debugfs_slab_add(s);
-
-	return 0;
-}
-
 #ifdef CONFIG_SYSFS
 static int count_inuse(struct slab *slab)
 {
@@ -5913,7 +5881,7 @@ static char *create_unique_id(struct kmem_cache *s)
 	return name;
 }
 
-static int sysfs_slab_add(struct kmem_cache *s)
+int sysfs_slab_add(struct kmem_cache *s)
 {
 	int err;
 	const char *name;
@@ -6236,10 +6204,13 @@ static const struct file_operations slab_debugfs_fops = {
 	.release = slab_debug_trace_release,
 };
 
-static void debugfs_slab_add(struct kmem_cache *s)
+void debugfs_slab_add(struct kmem_cache *s)
 {
 	struct dentry *slab_cache_dir;
 
+	if (!(s->flags & SLAB_STORE_USER))
+		return;
+
 	if (unlikely(!slab_debugfs_root))
 		return;
 
@@ -6264,8 +6235,7 @@ static int __init slab_debugfs_init(void)
 	slab_debugfs_root = debugfs_create_dir("slab", NULL);
 
 	list_for_each_entry(s, &slab_caches, list)
-		if (s->flags & SLAB_STORE_USER)
-			debugfs_slab_add(s);
+		debugfs_slab_add(s);
 
 	return 0;
 
-- 
2.25.1

[PATCH v3 3/3] mm/slub: Fix memory leak of kobj->name in sysfs_slab_add()

[Liu Shixin]

There is a memory leak of kobj->name in sysfs_slab_add():

 unreferenced object 0xffff88817e446440 (size 32):
   comm "insmod", pid 4085, jiffies 4296564501 (age 126.272s)
   hex dump (first 32 bytes):
     75 62 69 66 73 5f 69 6e 6f 64 65 5f 73 6c 61 62  ubifs_inode_slab
     00 65 44 7e 81 88 ff ff 00 00 00 00 00 00 00 00  .eD~............
   backtrace:
     [<000000005b30fbbd>] __kmalloc_node_track_caller+0x4e/0x150
     [<000000002f70da0c>] kstrdup_const+0x4b/0x80
     [<00000000c6712c61>] kobject_set_name_vargs+0x2f/0xb0
     [<00000000b151218e>] kobject_init_and_add+0xb0/0x120
     [<00000000e56a4cf5>] sysfs_slab_add+0x17d/0x220
     [<000000009326fd57>] __kmem_cache_create+0x406/0x590
     [<00000000dde33cff>] kmem_cache_create_usercopy+0x1fc/0x300
     [<00000000fe90cedb>] kmem_cache_create+0x12/0x20
     [<000000007a6531c8>] 0xffffffffa02d802d
     [<000000000e3b13c7>] do_one_initcall+0x87/0x2a0
     [<00000000995ecdcf>] do_init_module+0xdf/0x320
     [<000000008821941f>] load_module+0x2f98/0x3330
     [<00000000ef51efa4>] __do_sys_finit_module+0x113/0x1b0
     [<000000009339fbce>] do_syscall_64+0x35/0x80
     [<000000006b7f2033>] entry_SYSCALL_64_after_hwframe+0x46/0xb0

Following the rules stated in the comment for kobject_init_and_add():
 If this function returns an error, kobject_put() must be called to
 properly clean up the memory associated with the object.

kobject_put() is more appropriate for error handling after kobject_init().
And we can use this function to solve this problem.

For the cache created early, the related sysfs_slab_add() is called in
slab_sysfs_init(). Skip free these kmem_cache since they are important
for system. Keep them working without sysfs.

Fixes: 80da026a8e5d ("mm/slub: fix slab double-free in case of duplicate sysfs filename")
Signed-off-by: Liu Shixin <liushixin2@huawei.com>
---
 include/linux/slub_def.h |  4 ++--
 mm/slab_common.c         |  6 ++----
 mm/slub.c                | 21 +++++++++++++++++----
 3 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h
index 26d56c4c74d1..90c3e06b77b1 100644
--- a/include/linux/slub_def.h
+++ b/include/linux/slub_def.h
@@ -144,11 +144,11 @@ struct kmem_cache {
 
 #ifdef CONFIG_SYSFS
 #define SLAB_SUPPORTS_SYSFS
-int sysfs_slab_add(struct kmem_cache *);
+int sysfs_slab_add(struct kmem_cache *, bool);
 void sysfs_slab_unlink(struct kmem_cache *);
 void sysfs_slab_release(struct kmem_cache *);
 #else
-static inline int sysfs_slab_add(struct kmem_cache *s)
+static inline int sysfs_slab_add(struct kmem_cache *s, bool free_slab)
 {
 	return 0;
 }
diff --git a/mm/slab_common.c b/mm/slab_common.c
index 55e2cf064dfe..30808a1d1b32 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -237,11 +237,9 @@ static struct kmem_cache *create_cache(const char *name,
 #ifdef SLAB_SUPPORTS_SYSFS
 	/* Mutex is not taken during early boot */
 	if (slab_state >= FULL) {
-		err = sysfs_slab_add(s);
-		if (err) {
-			slab_kmem_cache_release(s);
+		err = sysfs_slab_add(s, true);
+		if (err)
 			return ERR_PTR(err);
-		}
 		debugfs_slab_add(s);
 	}
 #endif
diff --git a/mm/slub.c b/mm/slub.c
index a1ad759753ce..06a3223fc833 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -5881,7 +5881,7 @@ static char *create_unique_id(struct kmem_cache *s)
 	return name;
 }
 
-int sysfs_slab_add(struct kmem_cache *s)
+int sysfs_slab_add(struct kmem_cache *s, bool free_slab)
 {
 	int err;
 	const char *name;
@@ -5911,14 +5911,17 @@ int sysfs_slab_add(struct kmem_cache *s)
 		 * for the symlinks.
 		 */
 		name = create_unique_id(s);
-		if (IS_ERR(name))
+		if (IS_ERR(name)) {
+			if (free_slab)
+				slab_kmem_cache_release(s);
 			return PTR_ERR(name);
+		}
 	}
 
 	s->kobj.kset = kset;
 	err = kobject_init_and_add(&s->kobj, &slab_ktype, NULL, "%s", name);
 	if (err)
-		goto out;
+		goto out_put_kobj;
 
 	err = sysfs_create_group(&s->kobj, &slab_attr_group);
 	if (err)
@@ -5934,6 +5937,16 @@ int sysfs_slab_add(struct kmem_cache *s)
 	return err;
 out_del_kobj:
 	kobject_del(&s->kobj);
+out_put_kobj:
+	/*
+	 * Skip free kmem_cache that create early since they are important
+	 * for system. Keep them working without sysfs. Only free name
+	 * for this case.
+	 */
+	if (free_slab)
+		kobject_put(&s->kobj);
+	else
+		kfree_const(&s->kobj.name);
 	goto out;
 }
 
@@ -6002,7 +6015,7 @@ static int __init slab_sysfs_init(void)
 	slab_state = FULL;
 
 	list_for_each_entry(s, &slab_caches, list) {
-		err = sysfs_slab_add(s);
+		err = sysfs_slab_add(s, false);
 		if (err)
 			pr_err("SLUB: Unable to add boot slab %s to sysfs\n",
 			       s->name);
-- 
2.25.1