Re: [PATCH 2/2] kobject: send KOBJ_REMOVE uevent when the object is removed from sysfs

[Rafael J. Wysocki]

On Wednesday, May 27, 2020 10:34:51 AM CEST Rafael J. Wysocki wrote:
> On Wed, May 27, 2020 at 9:50 AM Heikki Krogerus
> <heikki.krogerus@xxxxxxxxxxxxxxx> wrote:
> >
> > On Tue, May 26, 2020 at 10:26:23AM +0200, Rafael J. Wysocki wrote:
> > > On Tue, May 26, 2020 at 7:58 AM Greg Kroah-Hartman
> > > <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > > >
> > > > On Mon, May 25, 2020 at 03:49:01PM -0700, Dmitry Torokhov wrote:
> > > > > On Sun, May 24, 2020 at 8:34 AM Greg Kroah-Hartman
> > > > > <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > > > > >
> > > > > > It is possible for a KOBJ_REMOVE uevent to be sent to userspace way
> > > > > > after the files are actually gone from sysfs, due to how reference
> > > > > > counting for kobjects work.  This should not be a problem, but it would
> > > > > > be good to properly send the information when things are going away, not
> > > > > > at some later point in time in the future.
> > > > > >
> > > > > > Before this move, if a kobject's parent was torn down before the child,
> > > > >
> > > > > ^^^^ And this is the root of the problem and what has to be fixed.
> > > >
> > > > I fixed that in patch one of this series.  Turns out the user of the
> > > > kobject was not even expecting that to happen.
> > > >
> > > > > > when the call to kobject_uevent() happened, the parent walk to try to
> > > > > > reconstruct the full path of the kobject could be a total mess and cause
> > > > > > crashes.  It's not good to try to tear down a kobject tree from top
> > > > > > down, but let's at least try to not to crash if a user does so.
> > > > >
> > > > > One can try, but if we keep proper reference counting then kobject
> > > > > core should take care of actually releasing objects in the right
> > > > > order. I do not think you should keep this patch, and instead see if
> > > > > we can push call to kobject_put(kobj->parent) into kobject_cleanup().
> > > >
> > > > I tried that, but there was a _lot_ of underflow errors reported, so
> > > > there's something else happening.  Or my attempt was incorrect :)
> > >
> > > So it looks like there is something in there that's been overlooked so far.
> > >
> > > I'll try to look at the Guenter's traces and figure out what went
> > > wrong after the Heikki's patch.
> >
> > At least one problem with that patch was that I was releasing the
> > parent reference unconditionally.
> 
> That actually may be sufficient to explain all of the problems introduced by it.

So Guenter, can you please test the patch below to see if it still introduces
the problems seen by you on ARM?

---
From: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
Subject: [PATCH] kobject: Make sure the parent does not get released before its children

In the function kobject_cleanup(), kobject_del(kobj) is
called before the kobj->release(). That makes it possible to
release the parent of the kobject before the kobject itself.

To fix that, adding function __kboject_del() that does
everything that kobject_del() does except release the parent
reference. kobject_cleanup() then calls __kobject_del()
instead of kobject_del(), and separately decrements the
reference count of the parent kobject after kobj->release()
has been called.

Reported-by: Naresh Kamboju <naresh.kamboju@xxxxxxxxxx>
Reported-by: kernel test robot <rong.a.chen@xxxxxxxxx>
Fixes: 7589238a8cf3 ("Revert "software node: Simplify software_node_release() function"")
Suggested-by: "Rafael J. Wysocki" <rafael@xxxxxxxxxx>
Signed-off-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
[ rjw: Drop parent reference only when called __kobject_del() ]
Signed-off-by: "Rafael J. Wysocki" <rafael.j.wysocki@xxxxxxxxx>
---
 lib/kobject.c |   34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 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,7 +670,9 @@ 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);
+	bool state_in_sysfs = kobj->state_in_sysfs;
 	const char *name = kobj->name;
 
 	pr_debug("kobject: '%s' (%p): %s, parent %p\n",
@@ -681,10 +690,10 @@ static void kobject_cleanup(struct kobje
 	}
 
 	/* remove from sysfs if the caller did not do it */
-	if (kobj->state_in_sysfs) {
+	if (state_in_sysfs) {
 		pr_debug("kobject: '%s' (%p): auto cleanup kobject_del\n",
 			 kobject_name(kobj), kobj);
-		kobject_del(kobj);
+		__kobject_del(kobj);
 	}
 
 	if (t && t->release) {
@@ -698,6 +707,9 @@ static void kobject_cleanup(struct kobje
 		pr_debug("kobject: '%s': free name\n", name);
 		kfree_const(name);
 	}
+
+	if (state_in_sysfs)
+		kobject_put(parent);
 }
 
 #ifdef CONFIG_DEBUG_KOBJECT_RELEASE