mm/slub: release kobject if kobject_init_and_add failed in sysfs_slab_add

PATCH In review Xin Hao 2 participants 5 messages Aug 11, 2022 – Aug 14, 2022

Subsystems (2)

Memory management Slab allocator

lore

Share & tools

lore

Xin Hao

author

2022-08-11 07:18 UTC #0 lore raw

v2 cover mm/slub: release kobject if kobject_init_and_add failed in sysfs_slab_add

In kobject_init_and_add() function, the refcount is setted by calling kobject_init() function, regardless of whether the return value is zero or not, therefore, we must call kobject_del(&s->kobj) to prevent memory of s->kobj is leaked.

V1 -> V2
use kobject_put() instead kobject_del().

V1:
https://patchwork.kernel.org/project/linux-mm/patch/[email protected]/

Xin Hao (1):
mm/slub: release kobject if kobject_init_and_add failed in
sysfs_slab_add

 mm/slub.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

--
2.31.0

Xin Hao

author

2022-08-11 07:18 UTC #1 lore raw

PATCH v2 1/1 mm/slub: release kobject if kobject_init_and_add failed in sysfs_slab_add

Signed-off-by Xin Hao <[email protected]>

---
 mm/slub.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

mm/slub.c +3 -4

index b1281b8654bd..940a3f52e07c 100644
@@ -5981,19 +5981,18 @@ static int sysfs_slab_add(struct kmem_cache *s)
 
 	err = sysfs_create_group(&s->kobj, &slab_attr_group);
 	if (err)
-		goto out_del_kobj;
+		goto out;
 
 	if (!unmergeable) {
 		/* Setup first alias */
 		sysfs_slab_alias(s, s->name);
 	}
+	return err;
 out:
 	if (!unmergeable)
 		kfree(name);
+	kobject_put(&s->kobj);
 	return err;
-out_del_kobj:
-	kobject_del(&s->kobj);
-	goto out;
 }
 
 void sysfs_slab_unlink(struct kmem_cache *s)

Signature · 1 line

2.31.0

Hyeonggon Yoo

2022-08-14 08:05 UTC #2 lore raw

Quoted 4 lines · Xin Hao

In kobject_init_and_add() function, the refcount is setted by calling kobject_init() function, regardless of whether the return value is zero or not, therefore, we must call kobject_del(&s->kobj) to prevent memory of s->kobj is leaked.

TL;DR: IIUC current code works just fine

After thinking more, I don't think the memory leak you said exist.
The space for s->kobj is freed in create_cache() when __kmem_cache_create() failed.

The situation here is:

create_cache() {
	s = kmem_cache_alloc(kmem_cache, GFP_KERNEL)
	err = __kmem_cache_create()
	if (err)
		goto out_free_cache;

out_free_cache:

	kmem_cache_free(s) // s is freed here (including its kobject)
	[...]
}

__kmem_cache_create() {
	[...]

	err = sysfs_slab_add();
	if (err) {
		__kmem_cache_release(s);
		return err;
	}
}

The primary goal of kobject_put() is to call release() function of kobj_type (when reference becomes zero), which is kmem_cache_release().

kmem_cache_release() {
	__kmem_cache_release(s)
	kfree_const(s->name)
	kmem_cache_free(s)
}

But when slab_sysfs_add() failed, __kmem_cache_release() and create_cache() releases resources related to the cache. (Also its name is freed in kmem_cache_create_usercopy().)

So IIUC current code works just fine!

Quoted 19 lines

Signed-off-by Xin Hao <[email protected]>

---
 mm/slub.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

mm/slub.c +1 -1

index b1281b8654bd..940a3f52e07c 100644
@@ -5981,19 +5981,18 @@ static int sysfs_slab_add(struct kmem_cache *s)
 
 	err = sysfs_create_group(&s->kobj, &slab_attr_group);
 	if (err)
-		goto out_del_kobj;
+		goto out;
 
 	if (!unmergeable) {
 		/* Setup first alias */

mm/slub.c static int sysfs_slab_add(struct kmem_cache *s) · line 5995

Quoted 10 lines

mm/slub.c +2 -2

 		sysfs_slab_alias(s, s->name);
 	}
+	return err;
 out:
 	if (!unmergeable)
 		kfree(name);
+	kobject_put(&s->kobj);
 	return err;
-out_del_kobj:
-	kobject_del(&s->kobj);

So related resources are released in create_cache(), instead of by calling kobject_put().

But kobject_del() is still needed because it should unlink kobject hierarchy when kobject_add() succeeded but sysfs_create_group() failed!

Quoted 7 lines

mm/slub.c +0 -1

-	goto out;
 }
 
 void sysfs_slab_unlink(struct kmem_cache *s)

Signature · 1 line

2.31.0

Signature · 2 lines

Thanks,
Hyeonggon

haoxin

author

2022-08-14 13:49 UTC #3 lore raw

Quoted 9 lines · Hyeonggon Yoo

Quoted 4 lines · Xin Hao
In kobject_init_and_add() function, the refcount is setted by calling kobject_init() function, regardless of whether the return value is zero or not, therefore, we must call kobject_del(&s->kobj) to prevent memory of s->kobj is leaked.
TL;DR: IIUC current code works just fine
After thinking more, I don't think the memory leak you said exist.
The space for s->kobj is freed in create_cache() when __kmem_cache_create() failed.

Yes, Agree what you explain, but in slab_sysfs_init() function, it also call slab_sysfs_add() and there no other function to release it, so i think the memory leak still exist.

Quoted 79 lines

The situation here is:
create_cache() {
	s = kmem_cache_alloc(kmem_cache, GFP_KERNEL)
	err = __kmem_cache_create()
	if (err)
		goto out_free_cache;
out_free_cache:
	kmem_cache_free(s) // s is freed here (including its kobject)
	[...]
}
__kmem_cache_create() {
	[...]

	err = sysfs_slab_add();
	if (err) {
		__kmem_cache_release(s);
		return err;
	}
}
The primary goal of kobject_put() is to call release() function of kobj_type (when reference becomes zero), which is kmem_cache_release().
kmem_cache_release() {
	__kmem_cache_release(s)
	kfree_const(s->name)
	kmem_cache_free(s)
}
But when slab_sysfs_add() failed, __kmem_cache_release() and create_cache() releases resources related to the cache. (Also its name is freed in kmem_cache_create_usercopy().)
So IIUC current code works just fine!
Quoted 18 lines
Signed-off-by Xin Hao <[email protected]>
---
  mm/slub.c | 7 +++----
  1 file changed, 3 insertions(+), 4 deletions(-)
mm/slub.c +1 -1
index b1281b8654bd..940a3f52e07c 100644
@@ -5981,19 +5981,18 @@ static int sysfs_slab_add(struct kmem_cache *s)
 
  	err = sysfs_create_group(&s->kobj, &slab_attr_group);
  	if (err)
-		goto out_del_kobj;
+		goto out;
 
  	if (!unmergeable) {
  		/* Setup first alias */
mm/slub.c static int sysfs_slab_add(struct kmem_cache *s) · line 5995
Quoted 10 lines
mm/slub.c +2 -2
  		sysfs_slab_alias(s, s->name);
  	}
+	return err;
  out:
  	if (!unmergeable)
  		kfree(name);
+	kobject_put(&s->kobj);
  	return err;
-out_del_kobj:
-	kobject_del(&s->kobj);
So related resources are released in create_cache(), instead of by calling kobject_put().
But kobject_del() is still needed because it should unlink kobject hierarchy when kobject_add() succeeded but sysfs_create_group() failed!
Quoted 7 lines
mm/slub.c +0 -1
-	goto out;
  }
 
  void sysfs_slab_unlink(struct kmem_cache *s)
Signature · 1 line
2.31.0

Hyeonggon Yoo

2022-08-14 15:07 UTC #4 lore raw

Quoted 7 lines · haoxin

Quoted 5 lines · Hyeonggon Yoo
Quoted 4 lines · Xin Hao
In kobject_init_and_add() function, the refcount is setted by calling kobject_init() function, regardless of whether the return value is zero or not, therefore, we must call kobject_del(&s->kobj) to prevent memory of s->kobj is leaked.

Quoted 8 lines

Quoted 4 lines
TL;DR: IIUC current code works just fine
After thinking more, I don't think the memory leak you said exist.
The space for s->kobj is freed in create_cache() when __kmem_cache_create() failed.
Yes, Agree what you explain, but in slab_sysfs_init() function, it also call slab_sysfs_add() and there no other function to release it, so i think the memory leak still exist.

That is in fact not a memory leak.

some early caches are made when sysfs subsystem is not up, so it cannot call sysfs_slab_add().

So the solution is to call sysfs_slab_add() in late boot process by using initcall mechanism.

One difference is that early caches does not fail to create because it failed to register sysfs group - because you do it later.

Then, if kernel succeeded to create caches and then failed to register sysfs group later, what should it do? release resources that are being used actively? or just shutdown the system?

No, it just normally uses caches but just ignore the failure. Because resources of caches are being used normally, it is not a *leak*.

Thanks!

Quoted 76 lines

Quoted 76 lines
The situation here is:
create_cache() {
	s = kmem_cache_alloc(kmem_cache, GFP_KERNEL)
	err = __kmem_cache_create()
	if (err)
		goto out_free_cache;
out_free_cache:
	kmem_cache_free(s) // s is freed here (including its kobject)
	[...]
}
__kmem_cache_create() {
	[...]

	err = sysfs_slab_add();
	if (err) {
		__kmem_cache_release(s);
		return err;
	}
}
The primary goal of kobject_put() is to call release() function of kobj_type (when reference becomes zero), which is kmem_cache_release().
kmem_cache_release() {
	__kmem_cache_release(s)
	kfree_const(s->name)
	kmem_cache_free(s)
}
But when slab_sysfs_add() failed, __kmem_cache_release() and create_cache() releases resources related to the cache. (Also its name is freed in kmem_cache_create_usercopy().)
So IIUC current code works just fine!
Quoted 16 lines
Signed-off-by Xin Hao <[email protected]>
---
  mm/slub.c | 7 +++----
  1 file changed, 3 insertions(+), 4 deletions(-)
mm/slub.c +1 -1
index b1281b8654bd..940a3f52e07c 100644
@@ -5981,19 +5981,18 @@ static int sysfs_slab_add(struct kmem_cache *s)
  	err = sysfs_create_group(&s->kobj, &slab_attr_group);
  	if (err)
-		goto out_del_kobj;
+		goto out;
  	if (!unmergeable) {
  		/* Setup first alias */
mm/slub.c static int sysfs_slab_add(struct kmem_cache *s) · line 5993
Quoted 10 lines
mm/slub.c +2 -2
  		sysfs_slab_alias(s, s->name);
  	}
+	return err;
  out:
  	if (!unmergeable)
  		kfree(name);
+	kobject_put(&s->kobj);
  	return err;
-out_del_kobj:
-	kobject_del(&s->kobj);
So related resources are released in create_cache(), instead of by calling kobject_put().
But kobject_del() is still needed because it should unlink kobject hierarchy when kobject_add() succeeded but sysfs_create_group() failed!
Quoted 6 lines
mm/slub.c +0 -1
-	goto out;
  }
  void sysfs_slab_unlink(struct kmem_cache *s)
Signature · 1 line
2.31.0

Signature · 2 lines

Thanks,
Hyeonggon