| Message ID | 20220811025258.68684-1-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 10:52:58AM +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 Hello and thanks! Should kobject_del() be called when kobject_add() failed? its comments says: 597 * kobject_del() - Unlink kobject from hierarchy. 598 * @kobj: object. 599 * 600 * This is the function that should be called to delete an object 601 * successfully added via kobject_add(). AFAIK kobject_put() is proper function to call when kobject_init_and_add() failed as stated in its comment: 417 /** 418 * kobject_init_and_add() - Initialize a kobject structure and add it to 419 * the kobject hierarchy. 420 * @kobj: pointer to the kobject to initialize 421 * @ktype: pointer to the ktype for this kobject. 422 * @parent: pointer to the parent of this kobject. 423 * @fmt: the name of the kobject. 424 * 425 * This function combines the call to kobject_init() and kobject_add(). 426 * 427 * If this function returns an error, kobject_put() must be called to 428 * properly clean up the memory associated with the object. This is the 429 * same type of error handling after a call to kobject_add() and kobject 430 * lifetime rules are the same here. > 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..63b0a8a3a71f 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); > - return err; > -out_del_kobj: > kobject_del(&s->kobj); > - goto out; > + return err; > } > > void sysfs_slab_unlink(struct kmem_cache *s) > -- > 2.31.0
在 2022/8/11 上午11:31, Hyeonggon Yoo 写道: > On Thu, Aug 11, 2022 at 10:52:58AM +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 > Hello and thanks! > > Should kobject_del() be called when kobject_add() failed? Yes, I'm afraid think so， you can see the comment about kobject_add() * Return: If this function returns an error, kobject_put() must be * called to properly clean up the memory associated with the * object. Under no instance should the kobject that is passed * to this function be directly freed with a call to kfree(), * that can leak memory. * * If this function returns success, kobject_put() must also be called * in order to properly clean up the memory associated with the object. * * In short, once this function is called, kobject_put() MUST be called * when the use of the object is finished in order to properly free * everything. */ int kobject_add(struct kobject *kobj, struct kobject *parent, const char *fmt, ...) > > its comments says: > > 597 * kobject_del() - Unlink kobject from hierarchy. > 598 * @kobj: object. > 599 * > 600 * This is the function that should be called to delete an object > 601 * successfully added via kobject_add(). > > AFAIK kobject_put() is proper function to call when > kobject_init_and_add() failed as stated in its comment: > > 417 /** > 418 * kobject_init_and_add() - Initialize a kobject structure and add it to > 419 * the kobject hierarchy. > 420 * @kobj: pointer to the kobject to initialize > 421 * @ktype: pointer to the ktype for this kobject. > 422 * @parent: pointer to the parent of this kobject. > 423 * @fmt: the name of the kobject. > 424 * > 425 * This function combines the call to kobject_init() and kobject_add(). > 426 * > 427 * If this function returns an error, kobject_put() must be called to > 428 * properly clean up the memory associated with the object. This is the > 429 * same type of error handling after a call to kobject_add() and kobject > 430 * lifetime rules are the same here. > >> 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..63b0a8a3a71f 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); >> - return err; >> -out_del_kobj: >> kobject_del(&s->kobj); Maybe there use kobject_put will be better. >> - goto out; >> + return err; >> } >> >> void sysfs_slab_unlink(struct kmem_cache *s) >> -- >> 2.31.0
diff --git a/mm/slub.c b/mm/slub.c index b1281b8654bd..63b0a8a3a71f 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); - return err; -out_del_kobj: kobject_del(&s->kobj); - goto out; + return err; } 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(-)