Skip to contentSkip to conversation

powerpc: switch to dynamic root devices

PATCH RESEND In review Johan Hovold 2 participants 5 messages Aug 12 – Oct 12

Subsystems (2)
PowerPC PS3 platform support
lore
Share & tools
lore

Johan Hovold

author
#0 lore raw

v2 cover powerpc: switch to dynamic root devices

[ It's been over three months (and one reminder) since this series was
posted so resending. ]

This series replaces static root devices with dynamically allocated ones. Included is also a related printk cleanup.

Johan

Changes in v2:
- only set ibmebus root pointer on successful registration
- add sanity check when registering ibmebus devices to avoid use-after-free if ibmebus_register_driver() is called after the bus failed to register (Sashiko flagged this path which I had missed)

Johan Hovold (3):
powerpc/ps3: switch to dynamic system bus root device powerpc/pseries: switch to dynamic ibmebus root device powerpc/pseries: clean up ibmebus printks

 arch/powerpc/platforms/ps3/system-bus.c  | 10 ++---
 arch/powerpc/platforms/pseries/ibmebus.c | 57 ++++++++++++------------
 2 files changed, 33 insertions(+), 34 deletions(-)
Signature · 1 line
2.54.0
author
#1 lore raw

PATCH RESEND v2 1/3 powerpc/ps3: switch to dynamic system bus root device

Driver core expects devices to be dynamically allocated and will, for example, complain loudly if a device that lacks a release function is ever freed.

Use root_device_register() to allocate and register the root device instead of open coding using a static device.

Signed-off-by Johan Hovold <[email protected]>
---
 arch/powerpc/platforms/ps3/system-bus.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)
arch/powerpc/platforms/ps3/system-bus.c +4 -6
index 0537a678a32f..4ead6ccea259 100644
@@ -20,9 +20,7 @@
 
 #include "platform.h"
 
-static struct device ps3_system_bus = {
-	.init_name = "ps3_system",
-};
+static struct device *ps3_system_bus;
 
 /* FIXME: need device usage counters! */
 static struct {

@@ -486,8 +484,8 @@ static int __init ps3_system_bus_init(void)
 
 	mutex_init(&usage_hack.mutex);
 
-	result = device_register(&ps3_system_bus);
-	BUG_ON(result);
+	ps3_system_bus = root_device_register("ps3_system");
+	BUG_ON(IS_ERR(ps3_system_bus));
 
 	result = bus_register(&ps3_system_bus_type);
 	BUG_ON(result);

@@ -744,7 +742,7 @@ int ps3_system_bus_device_register(struct ps3_system_bus_device *dev)
 	static unsigned int dev_lpm_count;
 
 	if (!dev->core.parent)
-		dev->core.parent = &ps3_system_bus;
+		dev->core.parent = ps3_system_bus;
 	dev->core.bus = &ps3_system_bus_type;
 	dev->core.release = ps3_system_bus_release_device;
Signature · 1 line
2.54.0
author
#2 lore raw

PATCH RESEND v2 2/3 powerpc/pseries: switch to dynamic ibmebus root device

In reply to Johan Hovold

Driver core expects devices to be dynamically allocated and will, for example, complain loudly if a device that lacks a release function is ever freed.

Use root_device_register() to allocate and register the root device instead of open coding using a static device.

Also add the missing sanity check when registering ibmebus devices to avoid use-after-free if the bus failed to register (which would previously have triggered a bunch of use-after-free warnings).

Signed-off-by Johan Hovold <[email protected]>
---
 arch/powerpc/platforms/pseries/ibmebus.c | 39 ++++++++++++++----------
 1 file changed, 23 insertions(+), 16 deletions(-)
arch/powerpc/platforms/pseries/ibmebus.c +23 -16
index 2d0f991da2c8..48076fe6ab30 100644
@@ -52,9 +52,7 @@
 #include <asm/ibmebus.h>
 #include <asm/machdep.h>
 
-static struct device ibmebus_bus_device = { /* fake "parent" device */
-	.init_name = "ibmebus",
-};
+static struct device *ibmebus_bus_device;	/* fake "parent" device */
 
 const struct bus_type ibmebus_bus_type;
 

@@ -172,7 +170,10 @@ static int ibmebus_create_device(struct device_node *dn)
 	struct platform_device *dev;
 	int ret;
 
-	dev = of_device_alloc(dn, NULL, &ibmebus_bus_device);
+	if (!ibmebus_bus_device)
+		return -ENOENT;
+
+	dev = of_device_alloc(dn, NULL, ibmebus_bus_device);
 	if (!dev)
 		return -ENOMEM;
 

@@ -448,6 +449,7 @@ EXPORT_SYMBOL(ibmebus_bus_type);
 
 static int __init ibmebus_bus_init(void)
 {
+	struct device *root;
 	int err;
 
 	printk(KERN_INFO "IBM eBus Device Driver\n");

@@ -459,23 +461,28 @@ static int __init ibmebus_bus_init(void)
 		return err;
 	}
 
-	err = device_register(&ibmebus_bus_device);
-	if (err) {
-		printk(KERN_WARNING "%s: device_register returned %i\n",
+	root = root_device_register("ibmebus");
+	if (IS_ERR(root)) {
+		err = PTR_ERR(root);
+		printk(KERN_WARNING "%s: root_device_register returned %i\n",
 		       __func__, err);
-		put_device(&ibmebus_bus_device);
-		bus_unregister(&ibmebus_bus_type);
-
-		return err;
+		goto err_deregister_bus;
 	}
 
+	ibmebus_bus_device = root;
+
 	err = ibmebus_create_devices(ibmebus_matches);
-	if (err) {
-		device_unregister(&ibmebus_bus_device);
-		bus_unregister(&ibmebus_bus_type);
-		return err;
-	}
+	if (err)
+		goto err_deregister_root;
 
 	return 0;
+
+err_deregister_root:
+	ibmebus_bus_device = NULL;
+	root_device_unregister(root);
+err_deregister_bus:
+	bus_unregister(&ibmebus_bus_type);
+
+	return err;
 }
 machine_postcore_initcall(pseries, ibmebus_bus_init);
Signature · 1 line
2.54.0
Reviewed-by on v2 2/3
#3 lore raw

Re: [PATCH v2 RESEND 2/3] powerpc/pseries: switch to dynamic ibmebus root device

Quoted 82 lines · Johan Hovold

Driver core expects devices to be dynamically allocated and will, for example, complain loudly if a device that lacks a release function is ever freed.

Use root_device_register() to allocate and register the root device instead of open coding using a static device.

Also add the missing sanity check when registering ibmebus devices to avoid use-after-free if the bus failed to register (which would previously have triggered a bunch of use-after-free warnings).

Signed-off-by Johan Hovold <[email protected]>
---
  arch/powerpc/platforms/pseries/ibmebus.c | 39 ++++++++++++++----------
  1 file changed, 23 insertions(+), 16 deletions(-)
arch/powerpc/platforms/pseries/ibmebus.c +15 -16
index 2d0f991da2c8..48076fe6ab30 100644
@@ -52,9 +52,7 @@
  #include <asm/ibmebus.h>
  #include <asm/machdep.h>
 
-static struct device ibmebus_bus_device = { /* fake "parent" device */
-	.init_name = "ibmebus",
-};
+static struct device *ibmebus_bus_device;	/* fake "parent" device */
 
  const struct bus_type ibmebus_bus_type;
 

@@ -172,7 +170,10 @@ static int ibmebus_create_device(struct device_node *dn)
  	struct platform_device *dev;
  	int ret;
 
-	dev = of_device_alloc(dn, NULL, &ibmebus_bus_device);
+	if (!ibmebus_bus_device)
+		return -ENOENT;
+
+	dev = of_device_alloc(dn, NULL, ibmebus_bus_device);
  	if (!dev)
  		return -ENOMEM;
 

@@ -448,6 +449,7 @@ EXPORT_SYMBOL(ibmebus_bus_type);
 
  static int __init ibmebus_bus_init(void)
  {
+	struct device *root;
  	int err;
 
  	printk(KERN_INFO "IBM eBus Device Driver\n");

@@ -459,23 +461,28 @@ static int __init ibmebus_bus_init(void)
  		return err;
  	}
 
-	err = device_register(&ibmebus_bus_device);
-	if (err) {
-		printk(KERN_WARNING "%s: device_register returned %i\n",
+	root = root_device_register("ibmebus");
+	if (IS_ERR(root)) {
+		err = PTR_ERR(root);
+		printk(KERN_WARNING "%s: root_device_register returned %i\n",
  		       __func__, err);
-		put_device(&ibmebus_bus_device);
-		bus_unregister(&ibmebus_bus_type);
-
-		return err;
+		goto err_deregister_bus;
  	}
 
+	ibmebus_bus_device = root;
+
  	err = ibmebus_create_devices(ibmebus_matches);
-	if (err) {
-		device_unregister(&ibmebus_bus_device);
-		bus_unregister(&ibmebus_bus_type);
-		return err;
-	}
+	if (err)
+		goto err_deregister_root;
 
  	return 0;

arch/powerpc/platforms/pseries/ibmebus.c static int __init ibmebus_bus_init(void) · line 488

Quoted 10 lines
arch/powerpc/platforms/pseries/ibmebus.c +8 -0
+
+err_deregister_root:
+	ibmebus_bus_device = NULL;
+	root_device_unregister(root);
+err_deregister_bus:
+	bus_unregister(&ibmebus_bus_type);
+
+	return err;
  }
  machine_postcore_initcall(pseries, ibmebus_bus_init);

I went through the init error paths and they look right: the global is only set once the root device exists and is cleared before the root is unregistered, so the new NULL check in ibmebus_create_device() stays valid after a failed init.

Reviewed-by Mingming Cao <[email protected]>
author
#4 lore raw

PATCH RESEND v2 3/3 powerpc/pseries: clean up ibmebus printks

In reply to Johan Hovold

Clean up the imbebus printks by switching to pr_warn() and pr_err(), adding a missing newline and making the error messages a bit more uniform.

Signed-off-by Johan Hovold <[email protected]>
---
 arch/powerpc/platforms/pseries/ibmebus.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)
arch/powerpc/platforms/pseries/ibmebus.c +7 -13
index 48076fe6ab30..5cfb0565e712 100644
@@ -207,8 +207,7 @@ static int ibmebus_create_devices(const struct of_device_id *matches)
 
 		ret = ibmebus_create_device(child);
 		if (ret) {
-			printk(KERN_ERR "%s: failed to create device (%i)",
-			       __func__, ret);
+			pr_err("%s: failed to create device: %d\n", __func__, ret);
 			of_node_put(child);
 			break;
 		}

@@ -286,8 +285,7 @@ static ssize_t probe_store(const struct bus_type *bus, const char *buf, size_t c
 			      ibmebus_match_path);
 	if (dev) {
 		put_device(dev);
-		printk(KERN_WARNING "%s: %s has already been probed\n",
-		       __func__, path);
+		pr_warn("%s: %s has already been probed\n", __func__, path);
 		rc = -EEXIST;
 		goto out;
 	}

@@ -296,8 +294,7 @@ static ssize_t probe_store(const struct bus_type *bus, const char *buf, size_t c
 		rc = ibmebus_create_device(dn);
 		of_node_put(dn);
 	} else {
-		printk(KERN_WARNING "%s: no such device node: %s\n",
-		       __func__, path);
+		pr_warn("%s: no such device node: %s\n", __func__, path);
 		rc = -ENODEV;
 	}
 

@@ -326,8 +323,7 @@ static ssize_t remove_store(const struct bus_type *bus, const char *buf, size_t
 		kfree(path);
 		return count;
 	} else {
-		printk(KERN_WARNING "%s: %s not on the bus\n",
-		       __func__, path);
+		pr_warn("%s: %s not on the bus\n", __func__, path);
 
 		kfree(path);
 		return -ENODEV;

@@ -452,20 +448,18 @@ static int __init ibmebus_bus_init(void)
 	struct device *root;
 	int err;
 
-	printk(KERN_INFO "IBM eBus Device Driver\n");
+	pr_info("IBM eBus Device Driver\n");
 
 	err = bus_register(&ibmebus_bus_type);
 	if (err) {
-		printk(KERN_ERR "%s: failed to register IBM eBus.\n",
-		       __func__);
+		pr_err("%s: failed to register IBM eBus\n", __func__);
 		return err;
 	}
 
 	root = root_device_register("ibmebus");
 	if (IS_ERR(root)) {
 		err = PTR_ERR(root);
-		printk(KERN_WARNING "%s: root_device_register returned %i\n",
-		       __func__, err);
+		pr_err("%s: failed to register root device: %d\n", __func__, err);
 		goto err_deregister_bus;
 	}
Signature · 1 line
2.54.0