| Message ID | 20220811071844.74020-2-xhao@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/slub: release kobject if kobject_init_and_add failed in sysfs_slab_add | expand |
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 >
在 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 >>
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 > > >
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)
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(-)