Re: [PATCH 1/2] backlight: led_bl: Add devm_led_backlight_register() helper

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] The devres registration order for the backlight device and the LED disable action is reversed, creating a TOCTOU race condition on device removal that allows the hardware to be improperly turned back on.
- [Medium] Hardcoding `dev_name(dev)` as the backlight device name prevents a single device from registering multiple LED-backed backlights.

Pre-existing issues:
- [High] Dereferencing a potentially NULL `supplier` pointer inside `dev_name()` during an error path causes a kernel panic.
--

--- Patch [1]: [PATCH 1/2] backlight: led_bl: Add devm_led_backlight_register() helper ---
commit 9866a4a9924f34ae7a659947d01fcc9dc9744ec2
Author: Alexander Sverdlin <[email protected]>

backlight: led_bl: Add devm_led_backlight_register() helper

This commit factors out the backlight registration into a led_bl_register()
helper and adds devm_led_backlight_register(). This allows self-contained
LED providers to expose a backlight interface. It also switches to
devres-managed functions to handle teardown, removing the explicit .remove
callback.

> diff --git a/drivers/video/backlight/led_bl.c b/drivers/video/backlight/led_bl.c
> index f7ab9b3607313..f87a48f74186a 100644
> --- a/drivers/video/backlight/led_bl.c
> +++ b/drivers/video/backlight/led_bl.c

[ ... ]

> @@ -203,24 +200,28 @@ static int led_bl_probe(struct platform_device *pdev)
>  	props.brightness = priv->default_brightness;
>  	props.power = (priv->default_brightness > 0) ? BACKLIGHT_POWER_OFF :
>  		      BACKLIGHT_POWER_ON;
> -	priv->bl_dev = backlight_device_register(dev_name(&pdev->dev),
> -			&pdev->dev, priv, &led_bl_ops, &props);
> -	if (IS_ERR(priv->bl_dev)) {
> -		dev_err(&pdev->dev, "Failed to register backlight\n");
> -		return PTR_ERR(priv->bl_dev);
> -	}
> +	priv->bl_dev = devm_backlight_device_register(dev, dev_name(dev), dev,
> +						      priv, &led_bl_ops, &props);

[Severity: Medium]
Since dev_name(dev) is used for the backlight device name, will this prevent
a single device from registering multiple LED-backed backlights?

If a single controller driver calls devm_led_backlight_register() more than
once for different LEDs, it seems the second call will fail due to a sysfs
naming collision (-EEXIST) because the name is hardcoded to the parent
device's name.

> +	if (IS_ERR(priv->bl_dev))
> +		return dev_err_probe(dev, PTR_ERR(priv->bl_dev),
> +				     "Failed to register backlight\n");
>  
>  	for (i = 0; i < priv->nb_leds; i++) {
> +		struct device *supplier = priv->leds[i]->dev->parent;
>  		struct device_link *link;
>  
> -		link = device_link_add(&pdev->dev, priv->leds[i]->dev->parent,
> -				       DL_FLAG_AUTOREMOVE_CONSUMER);
> -		if (!link) {
> -			dev_err(&pdev->dev, "Failed to add devlink (consumer %s, supplier %s)\n",
> -				dev_name(&pdev->dev), dev_name(priv->leds[i]->dev->parent));
> -			backlight_device_unregister(priv->bl_dev);
> -			return -EINVAL;
> -		}
> +		/*
> +		 * BL and the LED are the same device if instantiated via
> +		 * devm_led_backlight_register()
> +		 */
> +		if (supplier == dev)
> +			continue;
> +
> +		link = device_link_add(dev, supplier, DL_FLAG_AUTOREMOVE_CONSUMER);
> +		if (!link)
> +			return dev_err_probe(dev, -EINVAL,
> +					     "Failed to add devlink (consumer %s, supplier %s)\n",
> +					     dev_name(dev), dev_name(supplier));

[Severity: High]
This is a pre-existing issue, but could this dev_name(supplier) call trigger a
NULL pointer dereference?

If an LED classdev without a parent device (where parent = NULL) is passed
to devm_led_backlight_register(), supplier becomes NULL.

Since device_link_add() unconditionally returns NULL when the supplier is
NULL, the error path will execute and dev_name(supplier) will panic by
dereferencing NULL.

>  	}
>  
>  	for (i = 0; i < priv->nb_leds; i++) {
> @@ -229,26 +230,68 @@ static int led_bl_probe(struct platform_device *pdev)
>  		mutex_unlock(&priv->leds[i]->led_access);
>  	}
>  
> +	ret = devm_add_action_or_reset(dev, led_bl_disable, priv);
> +	if (ret)
> +		return ret;

[Severity: High]
Does registering the led_bl_disable action here create a race condition
during device removal?

Because devres actions execute in LIFO order (reverse of registration),
led_bl_disable() will run before the backlight device is unregistered.

This means led_bl_disable() turns off the LED, but the backlight sysfs nodes
are still active. Could userspace write to the backlight's sysfs brightness
attribute during this window, turning the LED back on just before the driver
is removed, thus leaving the hardware powered on?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1