From: Heikki Krogerus <[email protected]>
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 <[email protected]>
Reported-by: kernel test robot <[email protected]>
Fixes: 7589238a8cf3 ("Revert "software node: Simplify software_node_release() function"")
Suggested-by: Rafael J. Wysocki <[email protected]>
Signed-off-by: Heikki Krogerus <[email protected]>
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
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);
+}
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 2020/6/5 上午1:46, Rafael J. Wysocki wrote:
> From: Heikki Krogerus <[email protected]>
>
> 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 <[email protected]>
> Reported-by: kernel test robot <[email protected]>
> Fixes: 7589238a8cf3 ("Revert "software node: Simplify software_node_release() function"")
> Suggested-by: Rafael J. Wysocki <[email protected]>
> Signed-off-by: Heikki Krogerus <[email protected]>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
>
> 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 <[email protected]> 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
<[email protected]> wrote:
> On Mon, Aug 3, 2020 at 9:47 AM Qu Wenruo <[email protected]> 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
> <[email protected]> wrote:
>> On Mon, Aug 3, 2020 at 9:47 AM Qu Wenruo <[email protected]> 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 <[email protected]>
> >
> > 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 <[email protected]>
> > Reported-by: kernel test robot <[email protected]>
> > Fixes: 7589238a8cf3 ("Revert "software node: Simplify software_node_release() function"")
> > Suggested-by: Rafael J. Wysocki <[email protected]>
> > Signed-off-by: Heikki Krogerus <[email protected]>
> > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > ---
> >
> > 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