Re: [PATCH 08/10] leds: class: Split led_classdev_register_ext()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] leds: dangling led_cdev->dev pointer on error in led_classdev_init()
- [High] leds: premature device_set_node() in led_classdev_init() breaks fw_devlink
--

commit 7efb333e9420ea13e705d9a114384e841e92d2b2
Author: Armin Wolf <W_Armin@gmx.de>

leds: class: Split led_classdev_register_ext()

This commit splits led_classdev_register_ext() into led_classdev_init() and
led_classdev_add() so that LED class devices can be initialized before
being added to the subsystem.

> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 77a0fce039528..ace417a2388e0 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c

[ ... ]

> @@ -539,23 +550,54 @@ int led_classdev_register_ext(struct device *parent,
>  	if (!led_cdev->max_brightness)
>  		led_cdev->max_brightness = LED_FULL;
>  
> -	mutex_lock(&led_cdev->led_access);
> -	led_cdev->dev = device_create_with_groups(&leds_class, parent, 0, led_cdev,
> -						  led_cdev->groups, "%s", final_name);
> -	if (IS_ERR(led_cdev->dev)) {
> -		mutex_unlock(&led_cdev->led_access);
> -		return PTR_ERR(led_cdev->dev);
> +	led_cdev->dev = kzalloc_obj(*led_cdev->dev);
> +	if (!led_cdev->dev)
> +		return -ENOMEM;
> +
> +	device_initialize(led_cdev->dev);
> +	led_cdev->dev->class = &leds_class;
> +	led_cdev->dev->parent = parent;
> +	led_cdev->dev->groups = led_cdev->groups;
> +	dev_set_drvdata(led_cdev->dev, led_cdev);
> +	ret = kobject_set_name(&led_cdev->dev->kobj, "%s", final_name);
> +	if (ret < 0)
> +	{
> +		put_device(led_cdev->dev);
> +		return ret;
>  	}

[Severity: High]
Does this leave led_cdev->dev as a dangling pointer when kobject_set_name()
fails here, or when led_classdev_add() fails in led_classdev_register_ext()
below?

Previously, led_cdev->dev was assigned the return value of
device_create_with_groups(), so on any failure it remained either NULL or an
ERR_PTR(). Commit 1dbb9fb4082ce2 ("leds: Allow to call
led_classdev_unregister() unconditionally") added an IS_ERR_OR_NULL() check
to led_classdev_unregister() so drivers can call it unconditionally in their
error and removal paths:

drivers/leds/led-class.c:led_classdev_unregister() {
	if (IS_ERR_OR_NULL(led_cdev->dev))
		return;
...
	led_stop_software_blink(led_cdev);
...
	flush_work(&led_cdev->set_brightness_work);

	device_unregister(led_cdev->dev);

	down_write(&leds_list_lock);
	list_del(&led_cdev->node);
	up_write(&leds_list_lock);
...
}

Many drivers rely on this guarantee during error unwind or removal, such as
asus_wmi_led_init() -> asus_wmi_led_exit() in drivers/platform/x86/asus-wmi.c,
asus_led_init() -> asus_led_exit() in drivers/platform/x86/asus-laptop.c, and
samsung_leds_init() -> samsung_leds_exit() in
drivers/platform/x86/samsung-laptop.c.

With this change, put_device(led_cdev->dev) frees the device via
led_classdev_release(), but led_cdev->dev is not reset to NULL. A subsequent
call to led_classdev_unregister() will bypass the IS_ERR_OR_NULL(led_cdev->dev)
check, operate on uninitialized blink_timer and set_brightness_work structures
if kobject_set_name() failed before led_init_core(), trigger a use-after-free
and double-free in device_unregister(led_cdev->dev), and crash in
list_del(&led_cdev->node).

>  
>  	if (init_data && init_data->fwnode)
>  		device_set_node(led_cdev->dev, init_data->fwnode);

[Severity: High]
Could calling device_set_node() in led_classdev_init() before device_add() is
called in led_classdev_add() break fw_devlink for child LED nodes?

Previously, device_create_with_groups() called device_add() while
led_cdev->dev->fwnode was still NULL, and device_set_node() was only called
after device_add() returned. When the parent LED controller finished probing,
device_links_driver_bound() -> fw_devlink_pickup_dangling_consumers() marked
the child LED fwnodes with FWNODE_FLAG_NOT_DEVICE and attached consumer links
to the parent LED controller device.

Now that led_cdev->dev->fwnode is populated before led_classdev_add() calls
device_add(led_cdev->dev), this block in device_add() will trigger during the
parent driver's probe:

drivers/base/core.c:device_add() {
...
	if (dev->fwnode && !dev->fwnode->dev) {
		dev->fwnode->dev = dev;
		fw_devlink_link_device(dev);
	}
...
}

Because led_cdev->dev is a class device that never binds to a driver:

1. __fw_devlink_link_to_consumers(led_cdev->dev) converts consumer fwnode_links
   on init_data->fwnode into managed device links with led_cdev->dev as the
   supplier instead of the parent LED controller. During boot (while
   fw_devlink_drv_reg_done is false), consumers remain blocked with
   -EPROBE_DEFER until fw_devlink_drivers_done() runs, and are never linked to
   the parent controller for runtime PM, suspend/resume, or unbind ordering.

2. __fw_devlink_link_to_suppliers(led_cdev->dev, fwnode) creates managed device
   links with led_cdev->dev as the consumer for any supplier properties on the
   child LED node (such as gpios in gpio-leds or pwms in pwm-leds). Since
   led_cdev->dev never probes a driver, those links stay in DL_STATE_AVAILABLE
   and prevent the GPIO or PWM suppliers from reaching sync_state().

> -
> -	led_update_brightness(led_cdev);
> -
>  	led_cdev->wq = leds_wq;
>  
>  	led_init_core(led_cdev);
>  
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(led_classdev_init);
> +
> +/**
> + * led_classdev_add - Add a new LED class device to the subsystem.
> + * @led_cdev: the led_classdev structure for this device.
> + *
> + * Adds a new LED class device to the subsystem, allowing userspace applications
> + * and/or kernel consumers to access it.
> + *
> + * Returns: Zero on success or a negative error code on failure.
> + */
> +int led_classdev_add(struct led_classdev *led_cdev)
> +{
> +	int ret;
> +
> +	guard(mutex)(&led_cdev->led_access);
> +
> +	ret = device_add(led_cdev->dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	led_update_brightness(led_cdev);
> +
>  	/* add to the list of leds */
>  	down_write(&leds_list_lock);
>  	list_add_tail(&led_cdev->node, &leds_list);
> @@ -565,13 +607,36 @@ int led_classdev_register_ext(struct device *parent,
>  	led_trigger_set_default(led_cdev);
>  #endif
>  
> -	mutex_unlock(&led_cdev->led_access);
> -
> -	dev_dbg(parent, "Registered led device: %s\n",
> -			led_cdev->name);
> +	dev_dbg(led_cdev->dev->parent, "Registered led device: %s\n", led_cdev->name);
>  
>  	return 0;
>  }
> +EXPORT_SYMBOL_GPL(led_classdev_add);
> +
> +/**
> + * led_classdev_register_ext - register a new object of led_classdev class
> + *                             with init data.
> + *
> + * @parent: parent of LED device.
> + * @led_cdev: the led_classdev structure for this device.
> + * @init_data: LED class device initialization data.
> + */
> +int led_classdev_register_ext(struct device *parent,
> +                              struct led_classdev *led_cdev,
> +                              struct led_init_data *init_data)
> +{
> +	int ret;
> +
> +	ret = led_classdev_init(parent, led_cdev, init_data);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = led_classdev_add(led_cdev);
> +	if (ret < 0)
> +		put_device(led_cdev->dev);
> +
> +	return ret;
> +}
>  EXPORT_SYMBOL_GPL(led_classdev_register_ext);

[ ... ]

>
> -- 
> 2.51.2
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261018135339.1109271-1-W_Armin@gmx.de?part=8