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

[mingming cao <mmc@linux.ibm.com>]

On 8/12/26 7:20 AM, Johan Hovold wrote:
> 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 <johan@kernel.org>
> ---
>   arch/powerpc/platforms/pseries/ibmebus.c | 39 ++++++++++++++----------
>   1 file changed, 23 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/ibmebus.c b/arch/powerpc/platforms/pseries/ibmebus.c
> index 2d0f991da2c8..48076fe6ab30 100644
> --- a/arch/powerpc/platforms/pseries/ibmebus.c
> +++ b/arch/powerpc/platforms/pseries/ibmebus.c
> @@ -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);

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 <mmc@linux.ibm.com>