| Message ID | 1908555.IiAGLGrh1Z@kreacher |
|---|---|
| State | New |
| Headers | show |
| Series | kobject: Avoid premature parent object freeing in kobject_cleanup() | expand |
On 2020/6/5 上午1:46, Rafael J. Wysocki wrote: > From: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > If kobject_del() is invoked by kobject_cleanup() to delete the > target kobject, it may cause its parent kobject to be freed > before invoking the target kobject's ->release() method, which > effectively means freeing the parent before dealing with the > child entirely. > > That is confusing at best and it may also lead to functional > issues if the callers of kobject_cleanup() are not careful enough > about the order in which these calls are made, so avoid the > problem by making kobject_cleanup() drop the last reference to > the target kobject's parent at the end, after invoking the target > kobject's ->release() method. > > [ rjw: Rewrite the subject and changelog, make kobject_cleanup() > drop the parent reference only when __kobject_del() has been > called. ] > > Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org> > Reported-by: kernel test robot <rong.a.chen@intel.com> > Fixes: 7589238a8cf3 ("Revert "software node: Simplify software_node_release() function"") > Suggested-by: Rafael J. Wysocki <rafael@kernel.org> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > > Hi Greg, > > This is a replacement for commit 4ef12f719802 ("kobject: Make sure the parent > does not get released before its children"), that you reverted, because it > broke things and the reason why was that it was incorrect. > > Namely, it called kobject_put() on the target kobject's parent in > kobject_cleanup() unconditionally, but it should only call it after > invoking __kobject_del() on the target kobject. > > That problem is fixed in this patch and a functionally equivalent patch has > been tested by Guenter without issues. > > The underlying issue addressed by the reverted commit is still there and > it may show up again even though the test that triggered it originally was > fixed in the meantime. IMO it is worth fixing even though it may not be > readily visible in the current kernel, so please consider this one for > applying. > > Cheers! > > --- > lib/kobject.c | 33 +++++++++++++++++++++++---------- > 1 file changed, 23 insertions(+), 10 deletions(-) > > Index: linux-pm/lib/kobject.c > =================================================================== > --- linux-pm.orig/lib/kobject.c > +++ linux-pm/lib/kobject.c > @@ -599,14 +599,7 @@ out: > } > EXPORT_SYMBOL_GPL(kobject_move); > > -/** > - * kobject_del() - Unlink kobject from hierarchy. > - * @kobj: object. > - * > - * This is the function that should be called to delete an object > - * successfully added via kobject_add(). > - */ > -void kobject_del(struct kobject *kobj) > +static void __kobject_del(struct kobject *kobj) > { > struct kernfs_node *sd; > const struct kobj_type *ktype; > @@ -625,9 +618,23 @@ void kobject_del(struct kobject *kobj) > > kobj->state_in_sysfs = 0; > kobj_kset_leave(kobj); > - kobject_put(kobj->parent); > kobj->parent = NULL; > } > + > +/** > + * kobject_del() - Unlink kobject from hierarchy. > + * @kobj: object. > + * > + * This is the function that should be called to delete an object > + * successfully added via kobject_add(). > + */ > +void kobject_del(struct kobject *kobj) > +{ > + struct kobject *parent = kobj->parent; > + > + __kobject_del(kobj); > + kobject_put(parent); Could you please add an extra check on kobj before accessing kobj->parent? This patch in fact removes the ability to call kobject_del() on NULL pointer while not cause anything wrong. I know this is not a big deal, but such behavior change has already caused some problem for the incoming btrfs code. (Now I feels guilty just by looking into the old kobject_del()/kobject_put() and utilize that feature in btrfs) Since the old kobject_del() accepts NULL pointer intentionally, it would be much better to keep such behavior. Or at least mention we require a valid kobject pointer. Thanks, Qu > +} > EXPORT_SYMBOL(kobject_del); > > /** > @@ -663,6 +670,7 @@ EXPORT_SYMBOL(kobject_get_unless_zero); > */ > static void kobject_cleanup(struct kobject *kobj) > { > + struct kobject *parent = kobj->parent; > struct kobj_type *t = get_ktype(kobj); > const char *name = kobj->name; > > @@ -684,7 +692,10 @@ static void kobject_cleanup(struct kobje > if (kobj->state_in_sysfs) { > pr_debug("kobject: '%s' (%p): auto cleanup kobject_del\n", > kobject_name(kobj), kobj); > - kobject_del(kobj); > + __kobject_del(kobj); > + } else { > + /* avoid dropping the parent reference unnecessarily */ > + parent = NULL; > } > > if (t && t->release) { > @@ -698,6 +709,8 @@ static void kobject_cleanup(struct kobje > pr_debug("kobject: '%s': free name\n", name); > kfree_const(name); > } > + > + kobject_put(parent); > } > > #ifdef CONFIG_DEBUG_KOBJECT_RELEASE > > > >
On Mon, Aug 3, 2020 at 9:47 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote: > On 2020/6/5 上午1:46, Rafael J. Wysocki wrote: ... > > +/** > > + * kobject_del() - Unlink kobject from hierarchy. > > + * @kobj: object. > > + * > > + * This is the function that should be called to delete an object > > + * successfully added via kobject_add(). > > + */ > > +void kobject_del(struct kobject *kobj) > > +{ > > + struct kobject *parent = kobj->parent; > > + > > + __kobject_del(kobj); > > + kobject_put(parent); > > Could you please add an extra check on kobj before accessing kobj->parent? I do not understand. Where do we access it? kobject_put() is NULL-aware. > This patch in fact removes the ability to call kobject_del() on NULL > pointer while not cause anything wrong. > > I know this is not a big deal, but such behavior change has already > caused some problem for the incoming btrfs code. > (Now I feels guilty just by looking into the old > kobject_del()/kobject_put() and utilize that feature in btrfs) > > Since the old kobject_del() accepts NULL pointer intentionally, it would > be much better to keep such behavior. Can you elaborate, please? -- With Best Regards, Andy Shevchenko
On Mon, Aug 3, 2020 at 10:25 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, Aug 3, 2020 at 9:47 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote: > > On 2020/6/5 上午1:46, Rafael J. Wysocki wrote: > > > +void kobject_del(struct kobject *kobj) > > > +{ > > > + struct kobject *parent = kobj->parent; > > > + > > > + __kobject_del(kobj); > > > + kobject_put(parent); > > > > Could you please add an extra check on kobj before accessing kobj->parent? > > I do not understand. Where do we access it? > kobject_put() is NULL-aware. Ah, I see, now. Should be something like struct kobject *parent = kobj ? kobj->parent : NULL; > > This patch in fact removes the ability to call kobject_del() on NULL > > pointer while not cause anything wrong. > > > > I know this is not a big deal, but such behavior change has already > > caused some problem for the incoming btrfs code. > > (Now I feels guilty just by looking into the old > > kobject_del()/kobject_put() and utilize that feature in btrfs) > > > > Since the old kobject_del() accepts NULL pointer intentionally, it would > > be much better to keep such behavior. -- With Best Regards, Andy Shevchenko
On 2020/8/3 下午3:27, Andy Shevchenko wrote: > On Mon, Aug 3, 2020 at 10:25 AM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: >> On Mon, Aug 3, 2020 at 9:47 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote: >>> On 2020/6/5 上午1:46, Rafael J. Wysocki wrote: > >>>> +void kobject_del(struct kobject *kobj) >>>> +{ >>>> + struct kobject *parent = kobj->parent; >>>> + >>>> + __kobject_del(kobj); >>>> + kobject_put(parent); >>> >>> Could you please add an extra check on kobj before accessing kobj->parent? >> >> I do not understand. Where do we access it? >> kobject_put() is NULL-aware. > > Ah, I see, now. > > Should be something like > struct kobject *parent = kobj ? kobj->parent : NULL; Exactly. Thanks, Qu > >>> This patch in fact removes the ability to call kobject_del() on NULL >>> pointer while not cause anything wrong. >>> >>> I know this is not a big deal, but such behavior change has already >>> caused some problem for the incoming btrfs code. >>> (Now I feels guilty just by looking into the old >>> kobject_del()/kobject_put() and utilize that feature in btrfs) >>> >>> Since the old kobject_del() accepts NULL pointer intentionally, it would >>> be much better to keep such behavior. > >
On Mon, Aug 03, 2020 at 02:31:23PM +0800, Qu Wenruo wrote: > > > On 2020/6/5 上午1:46, Rafael J. Wysocki wrote: > > From: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > > > If kobject_del() is invoked by kobject_cleanup() to delete the > > target kobject, it may cause its parent kobject to be freed > > before invoking the target kobject's ->release() method, which > > effectively means freeing the parent before dealing with the > > child entirely. > > > > That is confusing at best and it may also lead to functional > > issues if the callers of kobject_cleanup() are not careful enough > > about the order in which these calls are made, so avoid the > > problem by making kobject_cleanup() drop the last reference to > > the target kobject's parent at the end, after invoking the target > > kobject's ->release() method. > > > > [ rjw: Rewrite the subject and changelog, make kobject_cleanup() > > drop the parent reference only when __kobject_del() has been > > called. ] > > > > Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org> > > Reported-by: kernel test robot <rong.a.chen@intel.com> > > Fixes: 7589238a8cf3 ("Revert "software node: Simplify software_node_release() function"") > > Suggested-by: Rafael J. Wysocki <rafael@kernel.org> > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > --- > > > > Hi Greg, > > > > This is a replacement for commit 4ef12f719802 ("kobject: Make sure the parent > > does not get released before its children"), that you reverted, because it > > broke things and the reason why was that it was incorrect. > > > > Namely, it called kobject_put() on the target kobject's parent in > > kobject_cleanup() unconditionally, but it should only call it after > > invoking __kobject_del() on the target kobject. > > > > That problem is fixed in this patch and a functionally equivalent patch has > > been tested by Guenter without issues. > > > > The underlying issue addressed by the reverted commit is still there and > > it may show up again even though the test that triggered it originally was > > fixed in the meantime. IMO it is worth fixing even though it may not be > > readily visible in the current kernel, so please consider this one for > > applying. > > > > Cheers! > > > > --- > > lib/kobject.c | 33 +++++++++++++++++++++++---------- > > 1 file changed, 23 insertions(+), 10 deletions(-) > > > > Index: linux-pm/lib/kobject.c > > =================================================================== > > --- linux-pm.orig/lib/kobject.c > > +++ linux-pm/lib/kobject.c > > @@ -599,14 +599,7 @@ out: > > } > > EXPORT_SYMBOL_GPL(kobject_move); > > > > -/** > > - * kobject_del() - Unlink kobject from hierarchy. > > - * @kobj: object. > > - * > > - * This is the function that should be called to delete an object > > - * successfully added via kobject_add(). > > - */ > > -void kobject_del(struct kobject *kobj) > > +static void __kobject_del(struct kobject *kobj) > > { > > struct kernfs_node *sd; > > const struct kobj_type *ktype; > > @@ -625,9 +618,23 @@ void kobject_del(struct kobject *kobj) > > > > kobj->state_in_sysfs = 0; > > kobj_kset_leave(kobj); > > - kobject_put(kobj->parent); > > kobj->parent = NULL; > > } > > + > > +/** > > + * kobject_del() - Unlink kobject from hierarchy. > > + * @kobj: object. > > + * > > + * This is the function that should be called to delete an object > > + * successfully added via kobject_add(). > > + */ > > +void kobject_del(struct kobject *kobj) > > +{ > > + struct kobject *parent = kobj->parent; > > + > > + __kobject_del(kobj); > > + kobject_put(parent); > > Could you please add an extra check on kobj before accessing kobj->parent? Sure, please just send a patch for this. thanks, greg k-h
Index: linux-pm/lib/kobject.c =================================================================== --- linux-pm.orig/lib/kobject.c +++ linux-pm/lib/kobject.c @@ -599,14 +599,7 @@ out: } EXPORT_SYMBOL_GPL(kobject_move); -/** - * kobject_del() - Unlink kobject from hierarchy. - * @kobj: object. - * - * This is the function that should be called to delete an object - * successfully added via kobject_add(). - */ -void kobject_del(struct kobject *kobj) +static void __kobject_del(struct kobject *kobj) { struct kernfs_node *sd; const struct kobj_type *ktype; @@ -625,9 +618,23 @@ void kobject_del(struct kobject *kobj) kobj->state_in_sysfs = 0; kobj_kset_leave(kobj); - kobject_put(kobj->parent); kobj->parent = NULL; } + +/** + * kobject_del() - Unlink kobject from hierarchy. + * @kobj: object. + * + * This is the function that should be called to delete an object + * successfully added via kobject_add(). + */ +void kobject_del(struct kobject *kobj) +{ + struct kobject *parent = kobj->parent; + + __kobject_del(kobj); + kobject_put(parent); +} EXPORT_SYMBOL(kobject_del); /** @@ -663,6 +670,7 @@ EXPORT_SYMBOL(kobject_get_unless_zero); */ static void kobject_cleanup(struct kobject *kobj) { + struct kobject *parent = kobj->parent; struct kobj_type *t = get_ktype(kobj); const char *name = kobj->name; @@ -684,7 +692,10 @@ static void kobject_cleanup(struct kobje if (kobj->state_in_sysfs) { pr_debug("kobject: '%s' (%p): auto cleanup kobject_del\n", kobject_name(kobj), kobj); - kobject_del(kobj); + __kobject_del(kobj); + } else { + /* avoid dropping the parent reference unnecessarily */ + parent = NULL; } if (t && t->release) { @@ -698,6 +709,8 @@ static void kobject_cleanup(struct kobje pr_debug("kobject: '%s': free name\n", name); kfree_const(name); } + + kobject_put(parent); } #ifdef CONFIG_DEBUG_KOBJECT_RELEASE