[PATCH V2 0/1] mm/slub: release kobject if kobject_init_and_add failed in sysfs_slab_add

[PATCH V2 0/1] mm/slub: release kobject if kobject_init_and_add failed in sysfs_slab_add

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

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

V1:
https://patchwork.kernel.org/project/linux-mm/patch/20220811025258.68684-1-xhao@linux.alibaba.com/

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

[PATCH V2 1/1] mm/slub: release kobject if kobject_init_and_add failed in sysfs_slab_add

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

Signed-off-by: Xin Hao <xhao@linux.alibaba.com>
---
 mm/slub.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index b1281b8654bd..940a3f52e07c 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -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)
-- 
2.31.0

Re: [PATCH V2 1/1] mm/slub: release kobject if kobject_init_and_add failed in sysfs_slab_add

[Hyeonggon Yoo]

On Thu, Aug 11, 2022 at 03:18:44PM +0800, Xin Hao wrote:
> 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!

> 
> Signed-off-by: Xin Hao <xhao@linux.alibaba.com>
> ---
>  mm/slub.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index b1281b8654bd..940a3f52e07c 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -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);

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!

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

-- 
Thanks,
Hyeonggon

Re: [PATCH V2 1/1] mm/slub: release kobject if kobject_init_and_add failed in sysfs_slab_add

[haoxin]

在 2022/8/14 下午4:05, Hyeonggon Yoo 写道:
> On Thu, Aug 11, 2022 at 03:18:44PM +0800, Xin Hao wrote:
>> 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.

> 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!
>
>> Signed-off-by: Xin Hao <xhao@linux.alibaba.com>
>> ---
>>   mm/slub.c | 7 +++----
>>   1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/mm/slub.c b/mm/slub.c
>> index b1281b8654bd..940a3f52e07c 100644
>> --- a/mm/slub.c
>> +++ b/mm/slub.c
>> @@ -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);
> 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!
>
>> -	goto out;
>>   }
>>   
>>   void sysfs_slab_unlink(struct kmem_cache *s)
>> -- 
>> 2.31.0
>>

Re: [PATCH V2 1/1] mm/slub: release kobject if kobject_init_and_add failed in sysfs_slab_add

[Hyeonggon Yoo]

On Sun, Aug 14, 2022 at 09:48:54PM +0800, haoxin wrote:
> 
> 在 2022/8/14 下午4:05, Hyeonggon Yoo 写道:
> > On Thu, Aug 11, 2022 at 03:18:44PM +0800, Xin Hao wrote:
> > > 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.

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!

> > 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!
> > 
> > > Signed-off-by: Xin Hao <xhao@linux.alibaba.com>
> > > ---
> > >   mm/slub.c | 7 +++----
> > >   1 file changed, 3 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/mm/slub.c b/mm/slub.c
> > > index b1281b8654bd..940a3f52e07c 100644
> > > --- a/mm/slub.c
> > > +++ b/mm/slub.c
> > > @@ -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);
> > 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!
> > 
> > > -	goto out;
> > >   }
> > >   void sysfs_slab_unlink(struct kmem_cache *s)
> > > -- 
> > > 2.31.0
> > > 

-- 
Thanks,
Hyeonggon