kobject: Avoid premature parent object freeing in kobject_cleanup()
diff mbox series

Message ID 1908555.IiAGLGrh1Z@kreacher
State Accepted
Commit 079ad2fb4bf9eba8a0aaab014b49705cd7f07c66
Headers show
Series
  • kobject: Avoid premature parent object freeing in kobject_cleanup()
Related show

Commit Message

Rafael J. Wysocki June 4, 2020, 5:46 p.m. UTC
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(-)

Comments

Qu Wenruo Aug. 3, 2020, 6:31 a.m. UTC | #1
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
> 
> 
> 
>
Andy Shevchenko Aug. 3, 2020, 7:25 a.m. UTC | #2
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?
Andy Shevchenko Aug. 3, 2020, 7:27 a.m. UTC | #3
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.
Qu Wenruo Aug. 3, 2020, 7:33 a.m. UTC | #4
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.
> 
>
Greg Kroah-Hartman Aug. 3, 2020, 8:12 a.m. UTC | #5
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

Patch
diff mbox series

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