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

[PATCH v4 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.
v3->v4: Fix error in kfree_const of kobject's name.

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 v4 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 v4 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 v4 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..25575bce0c3c 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 the name
+	 * for early allocated kmem_cache.
+	 */
+	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

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

[Hyeonggon Yoo]

On Sat, Nov 12, 2022 at 07:46:02PM +0800, Liu Shixin wrote:
> 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..25575bce0c3c 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)

free_slab is confusing.
Maybe 'release' or 'shutdown'?

>  {
>  	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 the name
> +	 * for early allocated kmem_cache.
> +	 */
> +	if (free_slab)
> +		kobject_put(&s->kobj);
> +	else
> +		kfree_const(s->kobj.name);

This is bypassing kobject API - can we initialize it
with different ktype that has different .release function,
depending on the boolean parameter?

>  	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
> 
> 

-- 
Thanks,
Hyeonggon

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

[Jinjie Ruan]

On 2022/11/12 19:46, Liu Shixin wrote:
> 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


Hi，every one,

I found the same problem and it solve this problem with the patch, is
there any plan to update the patch and solve it.

> 
> 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..25575bce0c3c 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 the name
> +	 * for early allocated kmem_cache.
> +	 */
> +	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);

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

[Hyeonggon Yoo]

On Thu, Sep 5, 2024 at 12:41 PM Jinjie Ruan <ruanjinjie@huawei.com> wrote:
>
>
>
> On 2022/11/12 19:46, Liu Shixin wrote:
> > 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
>
>
> Hi，every one,

Hi.

> I found the same problem and it solve this problem with the patch, is
> there any plan to update the patch and solve it.

What kernel version do you use,
and when do you encounter it or how do you reproduce it?

--
Hyeonggon

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

[Jinjie Ruan]

On 2024/9/5 21:59, Hyeonggon Yoo wrote:
> On Thu, Sep 5, 2024 at 12:41 PM Jinjie Ruan <ruanjinjie@huawei.com> wrote:
>>
>>
>>
>> On 2022/11/12 19:46, Liu Shixin wrote:
>>> 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
>>
>>
>> Hi，every one,
> 
> Hi.
> 
>> I found the same problem and it solve this problem with the patch, is
>> there any plan to update the patch and solve it.
> 
> What kernel version do you use,

6.11.0-rc6

> and when do you encounter it or how do you reproduce it?

Hi, Hyeonggon,

Thank you, I encounter it when doing inject fault test while modprobe
amdgpu.ko.

> 
> --
> Hyeonggon

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

[Vlastimil Babka]

On 9/6/24 10:10, Jinjie Ruan wrote:
> 
> 
> On 2024/9/5 21:59, Hyeonggon Yoo wrote:
>> On Thu, Sep 5, 2024 at 12:41 PM Jinjie Ruan <ruanjinjie@huawei.com> wrote:
>>>
>>>
>>>
>>> On 2022/11/12 19:46, Liu Shixin wrote:
>>>> 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
>>>
>>>
>>> Hi，every one,
>> 
>> Hi.
>> 
>>> I found the same problem and it solve this problem with the patch, is
>>> there any plan to update the patch and solve it.

Hmm looks like back in 2022, Hyeonggon had some feedback to the series which
was not answered and then it got forgotten. Feel free to take over and send
an updated version.

>> What kernel version do you use,
> 
> 6.11.0-rc6
> 
>> and when do you encounter it or how do you reproduce it?
> 
> Hi, Hyeonggon,
> 
> Thank you, I encounter it when doing inject fault test while modprobe
> amdgpu.ko.

So I wonder where's the problem that results in kobject_init_and_add()
failing. If it's genuinely duplicate name as commit 80da026a8e5d suggests,
6.12-rc1 will have a warning to prevent that. Delayed destruction of
SLAB_TYPESAFE_BY_RCU caches should also no longer happen with 6.12-rc1. So
worth retrying with that and if it's still failing, we should look at the
root cause perhaps.

>> 
>> --
>> Hyeonggon

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

[Hyeonggon Yoo]

> On Sep 13, 2024, at 11:10 PM, Vlastimil Babka <vbabka@suse.cz> wrote:
> 
> On 9/6/24 10:10, Jinjie Ruan wrote:
>> 
>> 
>> On 2024/9/5 21:59, Hyeonggon Yoo wrote:
>>> On Thu, Sep 5, 2024 at 12:41 PM Jinjie Ruan <ruanjinjie@huawei.com> wrote:
>>>> 
>>>> 
>>>> 
>>>> On 2022/11/12 19:46, Liu Shixin wrote:
>>>>> 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
>>>> 
>>>> 
>>>> Hi，every one,
>>> 
>>> Hi.
>>> 
>>>> I found the same problem and it solve this problem with the patch, is
>>>> there any plan to update the patch and solve it.
> 
> Hmm looks like back in 2022, Hyeonggon had some feedback to the series which
> was not answered and then it got forgotten. Feel free to take over and send
> an updated version.


I was thinking of what the fix would be with my feedback,
and I still think passing different kobj_type (with a dummy release function) for early kmem_caches
will be a more appropriate approach.

However, there is one concern: people that wrote kobject.rst might not like it :(

in Documentation/core-api/kobject.rst:
> One important point cannot be overstated: every kobject must have a release() method,
> and the kobject must persist (in a consistent state) until that method is called. If these constraints are not met,
> the code is flawed. Note that the kernel will warn you if you forget to provide a release() method.
> Do not try to get rid of this warning by providing an "empty" release function.

But obviously we don't want to release caches just because the kernel failed to add it to sysfs.

>>> What kernel version do you use,
>> 
>> 6.11.0-rc6
>> 
>>> and when do you encounter it or how do you reproduce it?
>> 
>> Hi, Hyeonggon,
>> 
>> Thank you, I encounter it when doing inject fault test while modprobe
>> amdgpu.ko.
> 
> So I wonder where's the problem that results in kobject_init_and_add()
> failing. If it's genuinely duplicate name as commit 80da026a8e5d suggests,
> 6.12-rc1 will have a warning to prevent that. Delayed destruction of
> SLAB_TYPESAFE_BY_RCU caches should also no longer happen with 6.12-rc1. So
> worth retrying with that and if it's still failing, we should look at the
> root cause perhaps.

I thought it was because the memory allocation for a name string failed due to fault injection?

> 
>>> 
>>> --
>>> Hyeonggon

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

[Vlastimil Babka]

On 9/13/24 17:00, Hyeonggon Yoo wrote:
> 
> 
>> On Sep 13, 2024, at 11:10 PM, Vlastimil Babka <vbabka@suse.cz> wrote:
>> 
>> On 9/6/24 10:10, Jinjie Ruan wrote:
>>> 
>>> 
>>> On 2024/9/5 21:59, Hyeonggon Yoo wrote:
>>>> On Thu, Sep 5, 2024 at 12:41 PM Jinjie Ruan <ruanjinjie@huawei.com> wrote:
>>>>> 
>>>>> 
>>>>> 
>>>>> On 2022/11/12 19:46, Liu Shixin wrote:
>>>>>> 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
>>>>> 
>>>>> 
>>>>> Hi，every one,
>>>> 
>>>> Hi.
>>>> 
>>>>> I found the same problem and it solve this problem with the patch, is
>>>>> there any plan to update the patch and solve it.
>> 
>> Hmm looks like back in 2022, Hyeonggon had some feedback to the series which
>> was not answered and then it got forgotten. Feel free to take over and send
>> an updated version.
> 
> 
> I was thinking of what the fix would be with my feedback,
> and I still think passing different kobj_type (with a dummy release function) for early kmem_caches
> will be a more appropriate approach.
> 
> However, there is one concern: people that wrote kobject.rst might not like it :(
> 
> in Documentation/core-api/kobject.rst:
>> One important point cannot be overstated: every kobject must have a release() method,
>> and the kobject must persist (in a consistent state) until that method is called. If these constraints are not met,
>> the code is flawed. Note that the kernel will warn you if you forget to provide a release() method.
>> Do not try to get rid of this warning by providing an "empty" release function.
> 
> But obviously we don't want to release caches just because the kernel failed to add it to sysfs.
> 
>>>> What kernel version do you use,
>>> 
>>> 6.11.0-rc6
>>> 
>>>> and when do you encounter it or how do you reproduce it?
>>> 
>>> Hi, Hyeonggon,
>>> 
>>> Thank you, I encounter it when doing inject fault test while modprobe
>>> amdgpu.ko.
>> 
>> So I wonder where's the problem that results in kobject_init_and_add()
>> failing. If it's genuinely duplicate name as commit 80da026a8e5d suggests,
>> 6.12-rc1 will have a warning to prevent that. Delayed destruction of
>> SLAB_TYPESAFE_BY_RCU caches should also no longer happen with 6.12-rc1. So
>> worth retrying with that and if it's still failing, we should look at the
>> root cause perhaps.
> 
> I thought it was because the memory allocation for a name string failed due to fault injection?

Well in any case 6.12-rc1 introduced a new one, fixed by:
https://git.kernel.org/pub/scm/linux/kernel/git/vbabka/slab.git/commit/?h=slab/for-6.12-rc1/fixes&id=77ced98f0f03fdc196561d1afbe652899c318073

So once that's mainline, we can see if anything remains

>> 
>>>> 
>>>> --
>>>> Hyeonggon
> 
> 

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

[Jinjie Ruan]

On 2024/10/2 19:35, Vlastimil Babka wrote:
> On 9/13/24 17:00, Hyeonggon Yoo wrote:
>>
>>
>>> On Sep 13, 2024, at 11:10 PM, Vlastimil Babka <vbabka@suse.cz> wrote:
>>>
>>> On 9/6/24 10:10, Jinjie Ruan wrote:
>>>>
>>>>
>>>> On 2024/9/5 21:59, Hyeonggon Yoo wrote:
>>>>> On Thu, Sep 5, 2024 at 12:41 PM Jinjie Ruan <ruanjinjie@huawei.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 2022/11/12 19:46, Liu Shixin wrote:
>>>>>>> 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
>>>>>>
>>>>>>
>>>>>> Hi，every one,
>>>>>
>>>>> Hi.
>>>>>
>>>>>> I found the same problem and it solve this problem with the patch, is
>>>>>> there any plan to update the patch and solve it.
>>>
>>> Hmm looks like back in 2022, Hyeonggon had some feedback to the series which
>>> was not answered and then it got forgotten. Feel free to take over and send
>>> an updated version.
>>
>>
>> I was thinking of what the fix would be with my feedback,
>> and I still think passing different kobj_type (with a dummy release function) for early kmem_caches
>> will be a more appropriate approach.
>>
>> However, there is one concern: people that wrote kobject.rst might not like it :(
>>
>> in Documentation/core-api/kobject.rst:
>>> One important point cannot be overstated: every kobject must have a release() method,
>>> and the kobject must persist (in a consistent state) until that method is called. If these constraints are not met,
>>> the code is flawed. Note that the kernel will warn you if you forget to provide a release() method.
>>> Do not try to get rid of this warning by providing an "empty" release function.
>>
>> But obviously we don't want to release caches just because the kernel failed to add it to sysfs.
>>
>>>>> What kernel version do you use,
>>>>
>>>> 6.11.0-rc6
>>>>
>>>>> and when do you encounter it or how do you reproduce it?
>>>>
>>>> Hi, Hyeonggon,
>>>>
>>>> Thank you, I encounter it when doing inject fault test while modprobe
>>>> amdgpu.ko.
>>>
>>> So I wonder where's the problem that results in kobject_init_and_add()
>>> failing. If it's genuinely duplicate name as commit 80da026a8e5d suggests,
>>> 6.12-rc1 will have a warning to prevent that. Delayed destruction of
>>> SLAB_TYPESAFE_BY_RCU caches should also no longer happen with 6.12-rc1. So
>>> worth retrying with that and if it's still failing, we should look at the
>>> root cause perhaps.
>>
>> I thought it was because the memory allocation for a name string failed due to fault injection?
> 
> Well in any case 6.12-rc1 introduced a new one, fixed by:
> https://git.kernel.org/pub/scm/linux/kernel/git/vbabka/slab.git/commit/?h=slab/for-6.12-rc1/fixes&id=77ced98f0f03fdc196561d1afbe652899c318073
> 
> So once that's mainline, we can see if anything remains

Using the newest 6.12.0-rc4, the issue still exists in slab:

unreferenced object 0xffffff80ce6da9c0 (size 16):
  comm "modprobe", pid 12782, jiffies 4299073226
  hex dump (first 16 bytes):
    6f 76 6c 5f 69 6e 6f 64 65 00 6d ce 80 ff ff ff  ovl_inode.m.....
  backtrace (crc 1a460899):
    [<00000000edf3be8b>] kmemleak_alloc+0x34/0x40
    [<0000000004121c8d>] __kmalloc_node_track_caller_noprof+0x304/0x3e8
    [<00000000515e9eda>] kstrdup+0x48/0x84
    [<000000005d2d0c1a>] kstrdup_const+0x34/0x40
    [<00000000d14076ce>] kvasprintf_const+0x170/0x1e0
    [<0000000060f79972>] kobject_set_name_vargs+0x5c/0x12c
    [<00000000299f544a>] kobject_init_and_add+0xd4/0x168
    [<000000008ceb40f4>] sysfs_slab_add+0x190/0x21c
    [<00000000027371b9>] do_kmem_cache_create+0x354/0x5cc
    [<00000000cc9eb2aa>] __kmem_cache_create_args+0x1b8/0x2c8
    [<000000006a3e21cc>] 0xffffffea545a409c
    [<0000000002f945b3>] do_one_initcall+0x110/0x77c
    [<0000000024f23211>] do_init_module+0x1dc/0x5c8
    [<00000000a16337d6>] load_module+0x4acc/0x4e90
    [<00000000be447e77>] init_module_from_file+0xd4/0x128
    [<0000000048065de1>] idempotent_init_module+0x2d4/0x57c

> 
>>>
>>>>>
>>>>> --
>>>>> Hyeonggon
>>
>>
> 

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

[Hyeonggon Yoo]

On Fri, Oct 25, 2024 at 11:10 AM Jinjie Ruan <ruanjinjie@huawei.com> wrote:
>
>
>
> On 2024/10/2 19:35, Vlastimil Babka wrote:
> > On 9/13/24 17:00, Hyeonggon Yoo wrote:
> >>
> >>
> >>> On Sep 13, 2024, at 11:10 PM, Vlastimil Babka <vbabka@suse.cz> wrote:
> >>>
> >>> On 9/6/24 10:10, Jinjie Ruan wrote:
> >>>>
> >>>>
> >>>> On 2024/9/5 21:59, Hyeonggon Yoo wrote:
> >>>>> On Thu, Sep 5, 2024 at 12:41 PM Jinjie Ruan <ruanjinjie@huawei.com> wrote:
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> On 2022/11/12 19:46, Liu Shixin wrote:
> >>>>>>> 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
> >>>>>>
> >>>>>>
> >>>>>> Hi，every one,
> >>>>>
> >>>>> Hi.
> >>>>>
> >>>>>> I found the same problem and it solve this problem with the patch, is
> >>>>>> there any plan to update the patch and solve it.
> >>>
> >>> Hmm looks like back in 2022, Hyeonggon had some feedback to the series which
> >>> was not answered and then it got forgotten. Feel free to take over and send
> >>> an updated version.
> >>
> >>
> >> I was thinking of what the fix would be with my feedback,
> >> and I still think passing different kobj_type (with a dummy release function) for early kmem_caches
> >> will be a more appropriate approach.
> >>
> >> However, there is one concern: people that wrote kobject.rst might not like it :(
> >>
> >> in Documentation/core-api/kobject.rst:
> >>> One important point cannot be overstated: every kobject must have a release() method,
> >>> and the kobject must persist (in a consistent state) until that method is called. If these constraints are not met,
> >>> the code is flawed. Note that the kernel will warn you if you forget to provide a release() method.
> >>> Do not try to get rid of this warning by providing an "empty" release function.
> >>
> >> But obviously we don't want to release caches just because the kernel failed to add it to sysfs.
> >>
> >>>>> What kernel version do you use,
> >>>>
> >>>> 6.11.0-rc6
> >>>>
> >>>>> and when do you encounter it or how do you reproduce it?
> >>>>
> >>>> Hi, Hyeonggon,
> >>>>
> >>>> Thank you, I encounter it when doing inject fault test while modprobe
> >>>> amdgpu.ko.
> >>>
> >>> So I wonder where's the problem that results in kobject_init_and_add()
> >>> failing. If it's genuinely duplicate name as commit 80da026a8e5d suggests,
> >>> 6.12-rc1 will have a warning to prevent that. Delayed destruction of
> >>> SLAB_TYPESAFE_BY_RCU caches should also no longer happen with 6.12-rc1. So
> >>> worth retrying with that and if it's still failing, we should look at the
> >>> root cause perhaps.
> >>
> >> I thought it was because the memory allocation for a name string failed due to fault injection?
> >
> > Well in any case 6.12-rc1 introduced a new one, fixed by:
> > https://git.kernel.org/pub/scm/linux/kernel/git/vbabka/slab.git/commit/?h=slab/for-6.12-rc1/fixes&id=77ced98f0f03fdc196561d1afbe652899c318073
> >
> > So once that's mainline, we can see if anything remains
>
> Using the newest 6.12.0-rc4, the issue still exists in slab:
>
> unreferenced object 0xffffff80ce6da9c0 (size 16):
>   comm "modprobe", pid 12782, jiffies 4299073226
>   hex dump (first 16 bytes):
>     6f 76 6c 5f 69 6e 6f 64 65 00 6d ce 80 ff ff ff  ovl_inode.m.....
>   backtrace (crc 1a460899):
>     [<00000000edf3be8b>] kmemleak_alloc+0x34/0x40
>     [<0000000004121c8d>] __kmalloc_node_track_caller_noprof+0x304/0x3e8
>     [<00000000515e9eda>] kstrdup+0x48/0x84
>     [<000000005d2d0c1a>] kstrdup_const+0x34/0x40
>     [<00000000d14076ce>] kvasprintf_const+0x170/0x1e0
>     [<0000000060f79972>] kobject_set_name_vargs+0x5c/0x12c
>     [<00000000299f544a>] kobject_init_and_add+0xd4/0x168
>     [<000000008ceb40f4>] sysfs_slab_add+0x190/0x21c
>     [<00000000027371b9>] do_kmem_cache_create+0x354/0x5cc
>     [<00000000cc9eb2aa>] __kmem_cache_create_args+0x1b8/0x2c8
>     [<000000006a3e21cc>] 0xffffffea545a409c
>     [<0000000002f945b3>] do_one_initcall+0x110/0x77c
>     [<0000000024f23211>] do_init_module+0x1dc/0x5c8
>     [<00000000a16337d6>] load_module+0x4acc/0x4e90
>     [<00000000be447e77>] init_module_from_file+0xd4/0x128
>     [<0000000048065de1>] idempotent_init_module+0x2d4/0x57c

Hi Jinjie,

Oh, I should've Cc'd you...

https://lore.kernel.org/linux-mm/20241021091413.154775-1-42.hyeyoo@gmail.com/

I wrote a fix for this and I'm adjusting feedback from Christoph and Vlastimil.

Thanks,
Hyeonggon