software node: implement software_node_unregister()

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, 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.

Cc "Rafael J. Wysocki" <[email protected]>

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
 lib/kobject.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

lib/kobject.c +7 -8

index 83198cb37d8d..2509e22ad74a 100644
@@ -620,6 +620,13 @@ void kobject_del(struct kobject *kobj)
        if (ktype)
                sysfs_remove_groups(kobj, ktype->default_groups);
 
+       /* send "remove" if the caller did not do it but sent "add" */
+       if (kobj->state_add_uevent_sent && !kobj->state_remove_uevent_sent) {
+               pr_debug("kobject: '%s' (%p): auto cleanup 'remove' event\n",
+                        kobject_name(kobj), kobj);
+               kobject_uevent(kobj, KOBJ_REMOVE);
+       }
+
        sysfs_remove_dir(kobj);
        sysfs_put(sd);
 

@@ -673,13 +680,6 @@ static void kobject_cleanup(struct kobject *kobj)
                pr_debug("kobject: '%s' (%p): does not have a release() function, it is broken and must be fixed. See Documentation/kobject.txt.\n",
                         kobject_name(kobj), kobj);
 
-       /* send "remove" if the caller did not do it but sent "add" */
-       if (kobj->state_add_uevent_sent && !kobj->state_remove_uevent_sent) {
-               pr_debug("kobject: '%s' (%p): auto cleanup 'remove' event\n",
-                        kobject_name(kobj), kobj);
-               kobject_uevent(kobj, KOBJ_REMOVE);
-       }
-
        /* remove from sysfs if the caller did not do it */
        if (kobj->state_in_sysfs) {
                pr_debug("kobject: '%s' (%p): auto cleanup kobject_del\n",
--

2.26.2

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,

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.

Quoted 7 lines · Greg Kroah-Hartman

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.

Quoted 1 line

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.

Quoted 4 lines

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().

Quoted 9 lines · Dmitry Torokhov

Quoted 7 lines · Greg Kroah-Hartman

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.

Quoted 3 lines

Quoted 1 line

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.

Quoted 5 lines

Quoted 4 lines

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.

Quoted 4 lines

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 :)

Quoted 28 lines · Greg Kroah-Hartman

Quoted 9 lines · Dmitry Torokhov

Quoted 7 lines · Greg Kroah-Hartman

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.

Quoted 3 lines

Quoted 1 line

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.

Quoted 9 lines

Quoted 4 lines

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().

Quoted 2 lines

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.

Quoted 33 lines · Rafael J. Wysocki

Quoted 30 lines · Greg Kroah-Hartman

Quoted 9 lines · Dmitry Torokhov

Quoted 7 lines · Greg Kroah-Hartman

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.

Quoted 3 lines

Quoted 1 line

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.

Quoted 9 lines

Quoted 4 lines

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 :)

Quoted 4 lines

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.

Quoted 37 lines · Heikki Krogerus

Quoted 35 lines · Rafael J. Wysocki

Quoted 23 lines · Greg Kroah-Hartman

Quoted 9 lines · Dmitry Torokhov

Quoted 7 lines · Greg Kroah-Hartman

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.

Quoted 3 lines

Quoted 1 line

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.

Quoted 5 lines

Quoted 4 lines

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.

Quoted 7 lines

Quoted 4 lines

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.

Quoted 5 lines

Quoted 2 lines

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.

Quoted 42 lines · CEST Rafael J. Wysocki

Quoted 40 lines · Heikki Krogerus

Quoted 37 lines · Rafael J. Wysocki

Quoted 23 lines · Greg Kroah-Hartman

Quoted 9 lines · Dmitry Torokhov

Quoted 7 lines · Greg Kroah-Hartman

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.

Quoted 3 lines

Quoted 1 line

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.

Quoted 5 lines

Quoted 4 lines

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.

Quoted 7 lines

Quoted 4 lines

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.

Quoted 4 lines

Quoted 2 lines

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?

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 <[email protected]>
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 <[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]>

[ rjw: Drop parent reference only when called __kobject_del() ]

Signed-off-by "Rafael J. Wysocki" <[email protected]>

---
 lib/kobject.c |   34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

Index: linux-pm/lib/kobject.c
===================================================================

lib/kobject.c +23 -11

@@ -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

Quoted 4 lines · Rafael J. Wysocki

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

arm64 and arm64be boot tests pass with the patch below.

Some arm boot tests fail, but I think that is due to some other problem with -next. Hard to say for sure at this point because -next is pretty badly broken overall. I'll need to run some bisects to see what is going on.

Quoted 107 lines

---
From: Heikki Krogerus <[email protected]>
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 <[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]>

[ rjw: Drop parent reference only when called __kobject_del() ]

Signed-off-by "Rafael J. Wysocki" <[email protected]>

---
 lib/kobject.c |   34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

Index: linux-pm/lib/kobject.c
===================================================================

lib/kobject.c +23 -11

@@ -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

Quoted 8 lines · Guenter Roeck

Quoted 4 lines · Rafael J. Wysocki

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

arm64 and arm64be boot tests pass with the patch below.

Great, thanks!

Quoted 3 lines

Some arm boot tests fail, but I think that is due to some other problem with -next. Hard to say for sure at this point because -next is pretty badly broken overall. I'll need to run some bisects to see what is going on.

I see.

Thanks for giving this one a go.

Quoted 107 lines

Quoted 107 lines

---
From: Heikki Krogerus <[email protected]>
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 <[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]>

[ rjw: Drop parent reference only when called __kobject_del() ]

Signed-off-by "Rafael J. Wysocki" <[email protected]>

---
 lib/kobject.c |   34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

Index: linux-pm/lib/kobject.c
===================================================================

lib/kobject.c +23 -11

@@ -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

Quoted 16 lines · Rafael J. Wysocki

Quoted 8 lines · Guenter Roeck

Quoted 4 lines · Rafael J. Wysocki

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

arm64 and arm64be boot tests pass with the patch below.

Great, thanks!

Quoted 3 lines

Some arm boot tests fail, but I think that is due to some other problem with -next. Hard to say for sure at this point because -next is pretty badly broken overall. I'll need to run some bisects to see what is going on.

Quoted 2 lines

I see.

The failing arm boot tests are due to various dts changes (commit "arm64:
dts: vexpress: Move fixed devices out of bus node" and associated), unrelated to this patch.

Sometimes it is better to unregister individual nodes instead of trying to do them all at once with software_node_unregister_nodes(), so create software_node_unregister() so that you can unregister them one at a time.

This is especially important when creating nodes in a hierarchy, with parent -> children representations. Children always need to be removed before a parent is, as the swnode logic assumes this is going to be the case.

Fix up the lib/test_printf.c fwnode_pointer() test which to use this new function as it had the problem of tearing things down in the backwards order.

Fixes f1ce39df508d ("lib/test_printf: Add tests for %pfw printk modifier")

Reported-by Naresh Kamboju <[email protected]>

Reported-by kernel test robot <[email protected]>

Cc stable <[email protected]>

Cc Andy Shevchenko <[email protected]>

Cc Brendan Higgins <[email protected]>

Cc Dmitry Torokhov <[email protected]>

Cc Heikki Krogerus <[email protected]>

Cc Petr Mladek <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

Cc Randy Dunlap <[email protected]>

Cc Rasmus Villemoes <[email protected]>

Cc Sakari Ailus <[email protected]>

Cc Sergey Senozhatsky <[email protected]>

Cc Steven Rostedt <[email protected]>

Signed-off-by Greg Kroah-Hartman <[email protected]>

Quoted 14 lines · Greg Kroah-Hartman

Sometimes it is better to unregister individual nodes instead of trying to do them all at once with software_node_unregister_nodes(), so create software_node_unregister() so that you can unregister them one at a time.

This is especially important when creating nodes in a hierarchy, with parent -> children representations. Children always need to be removed before a parent is, as the swnode logic assumes this is going to be the case.

Fix up the lib/test_printf.c fwnode_pointer() test which to use this new function as it had the problem of tearing things down in the backwards order.

Quoted 16 lines

Fixes f1ce39df508d ("lib/test_printf: Add tests for %pfw printk modifier")

Reported-by Naresh Kamboju <[email protected]>

Reported-by kernel test robot <[email protected]>

Cc stable <[email protected]>

Cc Andy Shevchenko <[email protected]>

Cc Brendan Higgins <[email protected]>

Cc Dmitry Torokhov <[email protected]>

Cc Heikki Krogerus <[email protected]>

Cc Petr Mladek <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

Cc Randy Dunlap <[email protected]>

Cc Rasmus Villemoes <[email protected]>

Cc Sakari Ailus <[email protected]>

Cc Sergey Senozhatsky <[email protected]>

Cc Steven Rostedt <[email protected]>

Signed-off-by Greg Kroah-Hartman <[email protected]>

Both patches pass my boot tests on arm64 and arm64be (I didn't test any others). So, FWIW,

Tested-by Guenter Roeck <[email protected]>

I wasn't sure it the two patches replace or fix commit 4ef12f719802 ("kobject:
Make sure the parent does not get released before its children"), so I tried to re-apply 4ef12f719802 on top of the two patches. Unfortunately that still results in crashes and UAF messages.

Sometimes it is better to unregister individual nodes instead of trying to do them all at once with software_node_unregister_nodes(), so create software_node_unregister() so that you can unregister them one at a time.

This is especially important when creating nodes in a hierarchy, with parent -> children representations. Children always need to be removed before a parent is, as the swnode logic assumes this is going to be the case.

Fix up the lib/test_printf.c fwnode_pointer() test which to use this new function as it had the problem of tearing things down in the backwards order.

Fixes f1ce39df508d ("lib/test_printf: Add tests for %pfw printk modifier")

Reported-by Naresh Kamboju <[email protected]>

Reported-by kernel test robot <[email protected]>

Cc stable <[email protected]>

Cc Andy Shevchenko <[email protected]>

Cc Brendan Higgins <[email protected]>

Cc Dmitry Torokhov <[email protected]>

Cc Heikki Krogerus <[email protected]>

Cc Petr Mladek <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

Cc Randy Dunlap <[email protected]>

Cc Rasmus Villemoes <[email protected]>

Cc Sakari Ailus <[email protected]>

Cc Sergey Senozhatsky <[email protected]>

Cc Steven Rostedt <[email protected]>

Signed-off-by Greg Kroah-Hartman <[email protected]>

Sometimes it is better to unregister individual nodes instead of trying to do them all at once with software_node_unregister_nodes(), so create software_node_unregister() so that you can unregister them one at a time.

This is especially important when creating nodes in a hierarchy, with parent -> children representations. Children always need to be removed before a parent is, as the swnode logic assumes this is going to be the case.

Fix up the lib/test_printf.c fwnode_pointer() test which to use this new function as it had the problem of tearing things down in the backwards order.

Fixes f1ce39df508d ("lib/test_printf: Add tests for %pfw printk modifier")

Reported-by Naresh Kamboju <[email protected]>

Reported-by kernel test robot <[email protected]>

Cc stable <[email protected]>

Cc Andy Shevchenko <[email protected]>

Cc Brendan Higgins <[email protected]>

Cc Dmitry Torokhov <[email protected]>

Cc Heikki Krogerus <[email protected]>

Cc Petr Mladek <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

Cc Randy Dunlap <[email protected]>

Cc Rasmus Villemoes <[email protected]>

Cc Sakari Ailus <[email protected]>

Cc Sergey Senozhatsky <[email protected]>

Cc Steven Rostedt <[email protected]>

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
 drivers/base/swnode.c    | 27 +++++++++++++++++++++------
 include/linux/property.h |  1 +
 lib/test_printf.c        |  4 +++-
 3 files changed, 25 insertions(+), 7 deletions(-)

drivers/base/swnode.c +21 -6

index de8d3543e8fe..770b1f47a625 100644
@@ -712,17 +712,18 @@ EXPORT_SYMBOL_GPL(software_node_register_nodes);
  * @nodes: Zero terminated array of software nodes to be unregistered
  *
  * Unregister multiple software nodes at once.
+ *
+ * NOTE: Be careful using this call if the nodes had parent pointers set up in
+ * them before registering.  If so, it is wiser to remove the nodes
+ * individually, in the correct order (child before parent) instead of relying
+ * on the sequential order of the list of nodes in the array.
  */
 void software_node_unregister_nodes(const struct software_node *nodes)
 {
-	struct swnode *swnode;
 	int i;
 
-	for (i = 0; nodes[i].name; i++) {
-		swnode = software_node_to_swnode(&nodes[i]);
-		if (swnode)
-			fwnode_remove_software_node(&swnode->fwnode);
-	}
+	for (i = 0; nodes[i].name; i++)
+		software_node_unregister(&nodes[i]);
 }
 EXPORT_SYMBOL_GPL(software_node_unregister_nodes);
 

@@ -741,6 +742,20 @@ int software_node_register(const struct software_node *node)
 }
 EXPORT_SYMBOL_GPL(software_node_register);
 
+/**
+ * software_node_unregister - Unregister static software node
+ * @node: The software node to be unregistered
+ */
+void software_node_unregister(const struct software_node *node)
+{
+	struct swnode *swnode;
+
+	swnode = software_node_to_swnode(node);
+	if (swnode)
+		fwnode_remove_software_node(&swnode->fwnode);
+}
+EXPORT_SYMBOL_GPL(software_node_unregister);
+
 struct fwnode_handle *
 fwnode_create_software_node(const struct property_entry *properties,
 			    const struct fwnode_handle *parent)

include/linux/property.h +1 -0

index d86de017c689..0d4099b4ce1f 100644
@@ -441,6 +441,7 @@ int software_node_register_nodes(const struct software_node *nodes);
 void software_node_unregister_nodes(const struct software_node *nodes);
 
 int software_node_register(const struct software_node *node);
+void software_node_unregister(const struct software_node *node);
 
 int software_node_notify(struct device *dev, unsigned long action);
 

lib/test_printf.c +3 -1

index 6b1622f4d7c2..fc63b8959d42 100644
@@ -637,7 +637,9 @@ static void __init fwnode_pointer(void)
 	test(second_name, "%pfwP", software_node_fwnode(&softnodes[1]));
 	test(third_name, "%pfwP", software_node_fwnode(&softnodes[2]));
 
-	software_node_unregister_nodes(softnodes);
+	software_node_unregister(&softnodes[2]);
+	software_node_unregister(&softnodes[1]);
+	software_node_unregister(&softnodes[0]);
 }
 
 static void __init

Signature · 1 line

2.26.2

Sometimes it is better to unregister individual nodes instead of trying to do them all at once with software_node_unregister_nodes(), so create software_node_unregister() so that you can unregister them one at a time.

This is especially important when creating nodes in a hierarchy, with parent -> children representations. Children always need to be removed before a parent is, as the swnode logic assumes this is going to be the case.

Fix up the lib/test_printf.c fwnode_pointer() test which to use this new function as it had the problem of tearing things down in the backwards order.

Fixes f1ce39df508d ("lib/test_printf: Add tests for %pfw printk modifier")

Reported-by Naresh Kamboju <[email protected]>

Reported-by kernel test robot <[email protected]>

Cc stable <[email protected]>

Cc Andy Shevchenko <[email protected]>

Cc Brendan Higgins <[email protected]>

Cc Dmitry Torokhov <[email protected]>

Cc Heikki Krogerus <[email protected]>

Cc Petr Mladek <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

Cc Randy Dunlap <[email protected]>

Cc Rasmus Villemoes <[email protected]>

Cc Sakari Ailus <[email protected]>

Cc Sergey Senozhatsky <[email protected]>

Cc Steven Rostedt <[email protected]>

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
 drivers/base/swnode.c    | 27 +++++++++++++++++++++------
 include/linux/property.h |  1 +
 lib/test_printf.c        |  4 +++-
 3 files changed, 25 insertions(+), 7 deletions(-)

drivers/base/swnode.c +21 -6

index de8d3543e8fe..770b1f47a625 100644
@@ -712,17 +712,18 @@ EXPORT_SYMBOL_GPL(software_node_register_nodes);
  * @nodes: Zero terminated array of software nodes to be unregistered
  *
  * Unregister multiple software nodes at once.
+ *
+ * NOTE: Be careful using this call if the nodes had parent pointers set up in
+ * them before registering.  If so, it is wiser to remove the nodes
+ * individually, in the correct order (child before parent) instead of relying
+ * on the sequential order of the list of nodes in the array.
  */
 void software_node_unregister_nodes(const struct software_node *nodes)
 {
-	struct swnode *swnode;
 	int i;
 
-	for (i = 0; nodes[i].name; i++) {
-		swnode = software_node_to_swnode(&nodes[i]);
-		if (swnode)
-			fwnode_remove_software_node(&swnode->fwnode);
-	}
+	for (i = 0; nodes[i].name; i++)
+		software_node_unregister(&nodes[i]);
 }
 EXPORT_SYMBOL_GPL(software_node_unregister_nodes);
 

@@ -741,6 +742,20 @@ int software_node_register(const struct software_node *node)
 }
 EXPORT_SYMBOL_GPL(software_node_register);
 
+/**
+ * software_node_unregister - Unregister static software node
+ * @node: The software node to be unregistered
+ */
+void software_node_unregister(const struct software_node *node)
+{
+	struct swnode *swnode;
+
+	swnode = software_node_to_swnode(node);
+	if (swnode)
+		fwnode_remove_software_node(&swnode->fwnode);
+}
+EXPORT_SYMBOL_GPL(software_node_unregister);
+
 struct fwnode_handle *
 fwnode_create_software_node(const struct property_entry *properties,
 			    const struct fwnode_handle *parent)

include/linux/property.h +1 -0

index d86de017c689..0d4099b4ce1f 100644
@@ -441,6 +441,7 @@ int software_node_register_nodes(const struct software_node *nodes);
 void software_node_unregister_nodes(const struct software_node *nodes);
 
 int software_node_register(const struct software_node *node);
+void software_node_unregister(const struct software_node *node);
 
 int software_node_notify(struct device *dev, unsigned long action);
 

lib/test_printf.c +3 -1

index 6b1622f4d7c2..fc63b8959d42 100644
@@ -637,7 +637,9 @@ static void __init fwnode_pointer(void)
 	test(second_name, "%pfwP", software_node_fwnode(&softnodes[1]));
 	test(third_name, "%pfwP", software_node_fwnode(&softnodes[2]));
 
-	software_node_unregister_nodes(softnodes);
+	software_node_unregister(&softnodes[2]);
+	software_node_unregister(&softnodes[1]);
+	software_node_unregister(&softnodes[0]);
 }
 
 static void __init

Sometimes it is better to unregister individual nodes instead of trying to do them all at once with software_node_unregister_nodes(), so create software_node_unregister() so that you can unregister them one at a time.

This is especially important when creating nodes in a hierarchy, with parent -> children representations. Children always need to be removed before a parent is, as the swnode logic assumes this is going to be the case.

Fix up the lib/test_printf.c fwnode_pointer() test which to use this new function as it had the problem of tearing things down in the backwards order.

Fixes f1ce39df508d ("lib/test_printf: Add tests for %pfw printk modifier")

Reported-by Naresh Kamboju <[email protected]>

Reported-by kernel test robot <[email protected]>

Cc stable <[email protected]>

Cc Andy Shevchenko <[email protected]>

Cc Brendan Higgins <[email protected]>

Cc Dmitry Torokhov <[email protected]>

Cc Heikki Krogerus <[email protected]>

Cc Petr Mladek <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

Cc Randy Dunlap <[email protected]>

Cc Rasmus Villemoes <[email protected]>

Cc Sakari Ailus <[email protected]>

Cc Sergey Senozhatsky <[email protected]>

Cc Steven Rostedt <[email protected]>

Signed-off-by Greg Kroah-Hartman <[email protected]>