Re: [PATCH V2 1/1] mm/slub: release kobject if kobject_init_and_add failed in sysfs_slab_add
From: Hyeonggon Yoo
Date: Sun Aug 14 2022 - 04:05:59 EST
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@xxxxxxxxxxxxxxxxx>
> ---
> 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