| Message ID | 20221024121910.1169801-1-yangyingliang@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] kset: fix memory leak when kset_register() returns error | expand |
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@amd.com> > Signed-off-by: Yang Yingliang <yangyingliang@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? > */ > 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? thanks, greg k-h
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@amd.com> >> Signed-off-by: Yang Yingliang <yangyingliang@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 > .
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@amd.com> > > > Signed-off-by: Yang Yingliang <yangyingliang@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
On 2022/10/24 22:53, Greg KH wrote: > 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@amd.com> >>>> Signed-off-by: Yang Yingliang <yangyingliang@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. OK, I can add it in v3. Thanks, Yang > > thanks, > > greg k-h > .
On 2022-10-24 08:19, 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(). "is hold" --> "was set". Also, I'd say "which must be called" instead of "is", since we cannot register kobj/kset without a name--the kobj code crashes, and we want to make this clear. IOW, a novice user may wonder where "is" it called, as opposed to learning that they "must" call it to allocate/set a name, before calling kset_register(). So, I'd say this: "If it fails, the name allocated by kobject_set_name() which must be called before a call to kset_regsiter() is leaked, since refcount of kobj was set 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 Drop "As", start with "A kset". "which needs _to_ be". Also please specify that the release is part of the ktype, like this: "A kset may be embedded in a larger structure which needs to be freed in ktype.release() 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. This is unclear because you're *missing* a verb: "and call an extra kset_put()". Please add the proper verb _between_ "and call", something like, "With this fix, the callers don't need to care about freeing the name of the kset, and _can_ call kset_put() if kset_register() fails." Choose a proper verb here: can, should, cannot, should not, etc. We can do this because you set "kset.kobj.name to NULL, and this is checked for in kobject_cleanup(). We just need to stipulate whether they should/shouldn't have to call kset_put(), or can free the kset and/or the embedding object themselves. This really depends on how we want kset_register() to behave in the future, and on user's own ktype.release implementation... > > Suggested-by: Luben Tuikov <luben.tuikov@amd.com> > Signed-off-by: Yang Yingliang <yangyingliang@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. > */ > 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; > return err; > + } This looks good. It's good you set kset.kobj.name to NULL, so that recovery/free paths don't get confused. Waiting for v3. (I guess this is no different than what we currently do in kobject_cleanup(), so I see it as safe, no-surprises implementation.) Regards, Luben
On 2022-10-24 17:06, Luben Tuikov wrote: > On 2022-10-24 08:19, 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(). > > "is hold" --> "was set". > > Also, I'd say "which must be called" instead of "is", since > we cannot register kobj/kset without a name--the kobj code crashes, > and we want to make this clear. IOW, a novice user may wonder > where "is" it called, as opposed to learning that they "must" > call it to allocate/set a name, before calling kset_register(). > > So, I'd say this: > > "If it fails, the name allocated by kobject_set_name() which must > be called before a call to kset_regsiter() is leaked, since > refcount of kobj was set in kset_init()." Actually, to be a bit more clear: "If kset_register() fails, the name allocated by kobject_set_name(), namely kset.kobj.name, which must be called before a call to kset_register(), may be leaked, if the caller doesn't explicitly free it, say by calling kset_put(). To mitigate this, we free the name in kset_register() when an error is encountered, i.e. when kset_register() returns an error." > >> >> As a kset may be embedded in a larger structure which needs >> be freed in release() function or error path in callers, we > > Drop "As", start with "A kset". "which needs _to_ be". > Also please specify that the release is part of the ktype, > like this: > > "A kset may be embedded in a larger structure which needs to be > freed in ktype.release() 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. > > This is unclear because you're *missing* a verb: > "and call an extra kset_put()". > Please add the proper verb _between_ "and call", something like, > > "With this fix, the callers don't need to care about freeing > the name of the kset, and _can_ call kset_put() if kset_register() fails." > > Choose a proper verb here: can, should, cannot, should not, etc. > > We can do this because you set "kset.kobj.name to NULL, and this > is checked for in kobject_cleanup(). We just need to stipulate > whether they should/shouldn't have to call kset_put(), or can free the kset > and/or the embedding object themselves. This really depends > on how we want kset_register() to behave in the future, and on > user's own ktype.release implementation... Forgot "may", "may not". So, do we want to say "may call kset_put()", like: "With this fix, the callers need not care about freeing the name of the kset, and _may_ call kset_put() if kset_register() fails." Or do we want to say "should" or even "must"--it really depends on what else is (would be) going on in kobj registration. Although, the user may have additional work to be done in the ktype.release() callback for the embedding object. It would be good to give them the freedom, i.e. "may", to call kset_put(). If that's not the case, this must be explicitly stipulated with the proper verb. Regards, Luben
Hi, On 2022/10/25 5:25, Luben Tuikov wrote: > On 2022-10-24 17:06, Luben Tuikov wrote: >> On 2022-10-24 08:19, 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(). >> "is hold" --> "was set". >> >> Also, I'd say "which must be called" instead of "is", since >> we cannot register kobj/kset without a name--the kobj code crashes, >> and we want to make this clear. IOW, a novice user may wonder >> where "is" it called, as opposed to learning that they "must" >> call it to allocate/set a name, before calling kset_register(). >> >> So, I'd say this: >> >> "If it fails, the name allocated by kobject_set_name() which must >> be called before a call to kset_regsiter() is leaked, since >> refcount of kobj was set in kset_init()." > Actually, to be a bit more clear: > > "If kset_register() fails, the name allocated by kobject_set_name(), > namely kset.kobj.name, which must be called before a call to kset_register(), > may be leaked, if the caller doesn't explicitly free it, say by calling kset_put(). > > To mitigate this, we free the name in kset_register() when an error is encountered, > i.e. when kset_register() returns an error." Thanks for you suggestion. > >>> As a kset may be embedded in a larger structure which needs >>> be freed in release() function or error path in callers, we >> Drop "As", start with "A kset". "which needs _to_ be". >> Also please specify that the release is part of the ktype, >> like this: >> >> "A kset may be embedded in a larger structure which needs to be >> freed in ktype.release() 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. >> This is unclear because you're *missing* a verb: >> "and call an extra kset_put()". >> Please add the proper verb _between_ "and call", something like, >> >> "With this fix, the callers don't need to care about freeing >> the name of the kset, and _can_ call kset_put() if kset_register() fails." I was mean the callers don't need to care about freeing the name of the kset and the callers don't need to care about calling kset_put() Thanks, Yang >> >> Choose a proper verb here: can, should, cannot, should not, etc. >> >> We can do this because you set "kset.kobj.name to NULL, and this >> is checked for in kobject_cleanup(). We just need to stipulate >> whether they should/shouldn't have to call kset_put(), or can free the kset >> and/or the embedding object themselves. This really depends >> on how we want kset_register() to behave in the future, and on >> user's own ktype.release implementation... > Forgot "may", "may not". > > So, do we want to say "may call kset_put()", like: > > "With this fix, the callers need not care about freeing > the name of the kset, and _may_ call kset_put() if kset_register() fails." > > Or do we want to say "should" or even "must"--it really depends on > what else is (would be) going on in kobj registration. > > Although, the user may have additional work to be done in the ktype.release() > callback for the embedding object. It would be good to give them the freedom, > i.e. "may", to call kset_put(). If that's not the case, this must be explicitly > stipulated with the proper verb. > > Regards, > Luben > > .
On 2022-10-24 08:19, 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@amd.com> > Signed-off-by: Yang Yingliang <yangyingliang@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. > */ The "need be freed" is confusing here because it is not clear if the user needs to do this or if it is done by the code. Since it is the latter, it should read "_is_ freed". Like this (no "NOTE"): "On error, the kset.kobj.name allocated by kobj_set_name(), which must be called before kset_register() is called, is freed by this function." Regards, Luben > 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; > return err; > + } > kobject_uevent(&k->kobj, KOBJ_ADD); > return 0; > }
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. */ 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; return err; + } kobject_uevent(&k->kobj, KOBJ_ADD); return 0; }
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@amd.com> Signed-off-by: Yang Yingliang <yangyingliang@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(-)