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

["Sverdlin, Alexander" <alexander.sverdlin@siemens.com>]

Hi all,

On Mon, 2026-08-17 at 17:18 +0000, sashiko-bot@kernel.org wrote:
> 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.

make sense! Will be fixed in v2.

> 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 <alexander.sverdlin@siemens.com>
> 
> 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?

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com