Yang Yingliang
2022-Oct-24 14:39 UTC
[Ocfs2-devel] [PATCH v2] kset: fix memory leak when kset_register() returns error
On 2022/10/24 21:52, Greg KH wrote:> On Mon, Oct 24, 2022 at 08:19:10PM +0800, Yang Yingliang wrote: >> Inject fault while loading module, kset_register() may fail. >> If it fails, the name allocated by kobject_set_name() which >> is called before kset_register() is leaked, because refcount >> of kobject is hold in kset_init(). >> >> As a kset may be embedded in a larger structure which needs >> be freed in release() function or error path in callers, we >> can not call kset_put() in kset_register(), or it will cause >> double free, so just call kfree_const() to free the name and >> set it to NULL. >> >> With this fix, the callers don't need to care about the name >> freeing and call an extra kset_put() if kset_register() fails. >> >> Suggested-by: Luben Tuikov <luben.tuikov at amd.com> >> Signed-off-by: Yang Yingliang <yangyingliang at huawei.com> >> --- >> v1 -> v2: >> Free name inside of kset_register() instead of calling kset_put() >> in drivers. >> --- >> lib/kobject.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/lib/kobject.c b/lib/kobject.c >> index a0b2dbfcfa23..3409a89c81e5 100644 >> --- a/lib/kobject.c >> +++ b/lib/kobject.c >> @@ -834,6 +834,9 @@ EXPORT_SYMBOL_GPL(kobj_sysfs_ops); >> /** >> * kset_register() - Initialize and add a kset. >> * @k: kset. >> + * >> + * NOTE: On error, the kset.kobj.name allocated by() kobj_set_name() >> + * which is called before kset_register() in caller need be freed. > This comment doesn't make any sense anymore. No caller needs to worry > about this, right?With this fix, the name is freed inside of kset_register(), it can not be accessed, if it allocated dynamically, but callers don't know this if no comment here, they may use it in error path (something like to print error message with it), so how about comment like this to tell callers not to use the name: NOTE: On error, the kset.kobj.name allocated by() kobj_set_name() is freed, it can not be used any more.> >> */ >> int kset_register(struct kset *k) >> { >> @@ -844,8 +847,11 @@ int kset_register(struct kset *k) >> >> kset_init(k); >> err = kobject_add_internal(&k->kobj); >> - if (err) >> + if (err) { >> + kfree_const(k->kobj.name); >> + k->kobj.name = NULL; > Why are you setting the name here to NULL?I set it to NULL to avoid accessing bad pointer in callers, if callers use it in error path, current callers won't use this name pointer in error path, so we can remove this assignment? Thanks, Yang> > thanks, > > greg k-h > .
Greg KH
2022-Oct-24 14:53 UTC
[Ocfs2-devel] [PATCH v2] kset: fix memory leak when kset_register() returns error
On Mon, Oct 24, 2022 at 10:39:44PM +0800, Yang Yingliang wrote:> > On 2022/10/24 21:52, Greg KH wrote: > > On Mon, Oct 24, 2022 at 08:19:10PM +0800, Yang Yingliang wrote: > > > Inject fault while loading module, kset_register() may fail. > > > If it fails, the name allocated by kobject_set_name() which > > > is called before kset_register() is leaked, because refcount > > > of kobject is hold in kset_init(). > > > > > > As a kset may be embedded in a larger structure which needs > > > be freed in release() function or error path in callers, we > > > can not call kset_put() in kset_register(), or it will cause > > > double free, so just call kfree_const() to free the name and > > > set it to NULL. > > > > > > With this fix, the callers don't need to care about the name > > > freeing and call an extra kset_put() if kset_register() fails. > > > > > > Suggested-by: Luben Tuikov <luben.tuikov at amd.com> > > > Signed-off-by: Yang Yingliang <yangyingliang at huawei.com> > > > --- > > > v1 -> v2: > > > Free name inside of kset_register() instead of calling kset_put() > > > in drivers. > > > --- > > > lib/kobject.c | 8 +++++++- > > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > > > diff --git a/lib/kobject.c b/lib/kobject.c > > > index a0b2dbfcfa23..3409a89c81e5 100644 > > > --- a/lib/kobject.c > > > +++ b/lib/kobject.c > > > @@ -834,6 +834,9 @@ EXPORT_SYMBOL_GPL(kobj_sysfs_ops); > > > /** > > > * kset_register() - Initialize and add a kset. > > > * @k: kset. > > > + * > > > + * NOTE: On error, the kset.kobj.name allocated by() kobj_set_name() > > > + * which is called before kset_register() in caller need be freed. > > This comment doesn't make any sense anymore. No caller needs to worry > > about this, right? > With this fix, the name is freed inside of kset_register(), it can not be > accessed,Agreed.> if it allocated dynamically, but callers don't know this if no comment here, > they may use it in error path (something like to print error message with > it), > so how about comment like this to tell callers not to use the name: > > NOTE: On error, the kset.kobj.name allocated by() kobj_set_name() > is freed, it can not be used any more.Sure, that's a better way to word it.> > > */ > > > int kset_register(struct kset *k) > > > { > > > @@ -844,8 +847,11 @@ int kset_register(struct kset *k) > > > kset_init(k); > > > err = kobject_add_internal(&k->kobj); > > > - if (err) > > > + if (err) { > > > + kfree_const(k->kobj.name); > > > + k->kobj.name = NULL; > > Why are you setting the name here to NULL? > I set it to NULL to avoid accessing bad pointer in callers, > if callers use it in error path, current callers won't use this > name pointer in error path, so we can remove this assignment?Ah, I didn't think about using it on error paths. Ideally that would never happen, but that's good to set just to make it obvious. How about adding a small comment here saying why you are setting it so we all remember it in 5 years when we look at the code again. thanks, greg k-h