Re: [PATCH 04/10] leds: class: Create "brightness_hw_changed" attribute atomically

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] leds: class: sleeping in atomic context in led_classdev_notify_brightness_hw_changed()
- [High] leds: NULL or ERR_PTR dereference in led_classdev_notify_brightness_hw_changed()
--

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

leds: class: Create "brightness_hw_changed" attribute atomically

This patch converts the brightness_hw_changed sysfs attribute to a static
attribute group with an is_visible callback so that the device core creates
it before sending the uevent to userspace. It also removes the cached
kernfs_node pointer from struct led_classdev and switches
led_classdev_notify_brightness_hw_changed() to use sysfs_notify().

> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 3775ad13762f5..d460f041ed86e 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c

[ ... ]

> @@ -123,51 +116,48 @@ static ssize_t brightness_hw_changed_show(struct device *dev, const struct devic
>  
>  static const DEVICE_ATTR_RO(brightness_hw_changed);
>  
> -static int led_add_brightness_hw_changed(struct led_classdev *led_cdev)
> -{
> -	struct device *dev = led_cdev->dev;
> -	int ret;
> +static const struct attribute *const led_brightness_hw_changed_attrs[] = {
> +	&dev_attr_brightness_hw_changed.attr,
> +	NULL,
> +};

[ ... ]

> +#ifdef CONFIG_LEDS_BRIGHTNESS_HW_CHANGED
>  void led_classdev_notify_brightness_hw_changed(struct led_classdev *led_cdev, unsigned int brightness)
>  {
> -	if (WARN_ON(!led_cdev->brightness_hw_changed_kn))
> -		return;
> -
>  	led_cdev->brightness_hw_changed = brightness;
> -	sysfs_notify_dirent(led_cdev->brightness_hw_changed_kn);
> +	sysfs_notify(&led_cdev->dev->kobj, NULL, "brightness_hw_changed");
>  }
>  EXPORT_SYMBOL_GPL(led_classdev_notify_brightness_hw_changed);

[Severity: High]
Can replacing sysfs_notify_dirent() with sysfs_notify() here cause sleeping
in atomic context?

Previously, sysfs_notify_dirent() called kernfs_notify() on the cached
kernfs_node, which may be called from any context since it only uses
rcu_read_lock(), spin_lock_irqsave(&kernfs_notify_lock, flags), and
schedule_work().

With sysfs_notify(&led_cdev->dev->kobj, NULL, "brightness_hw_changed"), a
non-NULL attribute name causes sysfs_notify() to look up the node via
kernfs_find_and_get() -> kernfs_find_and_get_ns(), which acquires the
sleeping rw_semaphore root->kernfs_rwsem:

fs/sysfs/file.c:sysfs_notify() {
	...
	if (kn && attr) {
		tmp = kernfs_find_and_get(kn, attr);
		kernfs_put(kn);
		kn = tmp;
	}
	...
}

fs/kernfs/dir.c:kernfs_find_and_get_ns() {
	...
	down_read(&root->kernfs_rwsem);
	kn = kernfs_find_ns(parent, name, ns);
	kernfs_get(kn);
	up_read(&root->kernfs_rwsem);
	...
}

In drivers/hid/hid-lg-g15.c, lg_g13_event() and lg_g510_leds_event() call
led_classdev_notify_brightness_hw_changed() when a hardware backlight
brightness change report arrives:

drivers/hid/hid-lg-g15.c:lg_g13_event() {
	...
	if (backlight_disabled ^ g15->backlight_disabled) {
		led_classdev_notify_brightness_hw_changed(
			&g15->leds[LG_G15_KBD_BRIGHTNESS].mcdev.led_cdev,
			backlight_disabled
			? 0 : g15->leds[LG_G15_KBD_BRIGHTNESS].brightness);
		g15->backlight_disabled = backlight_disabled;
	}
	...
}

Those handlers are invoked from lg_g15_raw_event() via the USB interrupt URB
completion path hid_irq_in() -> hid_safe_input_report() ->
__hid_input_report(), which runs in hardirq/atomic context.

[Severity: High]
Also in led_classdev_notify_brightness_hw_changed(), could removing the
WARN_ON(!led_cdev->brightness_hw_changed_kn) guard lead to a NULL or ERR_PTR
dereference on led_cdev->dev (or a use-after-free after commit "leds: class:
Split led_classdev_register_ext()" later in the series)?

Previously, if a classdev was not registered or did not set
LED_BRIGHT_HW_CHANGED, led_cdev->brightness_hw_changed_kn was NULL and the
function warned and returned before touching led_cdev->dev. That behavior is
also documented in Documentation/leds/leds-class.rst:

  Calling led_classdev_notify_brightness_hw_changed on a classdev not
  registered with the LED_BRIGHT_HW_CHANGED flag is a bug and will
  trigger a WARN_ON.

Without a guard, sysfs_notify(&led_cdev->dev->kobj, ...) immediately
dereferences kobj->sd in fs/sysfs/file.c:sysfs_notify().

For example, in drivers/platform/x86/system76_acpi.c, system76_probe() skips
registering data->kb_led when data->kbled_type == KBLED_NONE, leaving
data->kb_led.dev as NULL:

drivers/platform/x86/system76_acpi.c:system76_probe() {
	...
	if (data->kbled_type != KBLED_NONE) {
		err = devm_led_classdev_register(&pdev->dev, &data->kb_led);
		if (err)
			return err;
	}
	...
}

However, system76_notify() is still installed and calls kb_led_notify() ->
led_classdev_notify_brightness_hw_changed(&data->kb_led, ...) through
kb_led_hotkey_hardware(), kb_led_hotkey_toggle(), kb_led_hotkey_down(), and
kb_led_hotkey_up() on ACPI events 0x80-0x83.

Similarly, in drivers/platform/x86/toshiba_acpi.c, toshiba_acpi_probe()
ignores the return value of led_classdev_register(&pdev->dev, &dev->kbd_led):

drivers/platform/x86/toshiba_acpi.c:toshiba_acpi_probe() {
	...
	if (dev->kbd_illum_supported &&
	    (dev->kbd_mode == SCI_KBD_MODE_FNZ || dev->kbd_type == 2)) {
		dev->kbd_led.name = "toshiba::kbd_backlight";
		dev->kbd_led.flags = LED_BRIGHT_HW_CHANGED;
		dev->kbd_led.max_brightness = 1;
		dev->kbd_led.brightness_set = toshiba_kbd_backlight_set;
		dev->kbd_led.brightness_get = toshiba_kbd_backlight_get;
		led_classdev_register(&pdev->dev, &dev->kbd_led);
	}
	...
}

If registration fails, dev->kbd_led.dev is left as NULL or ERR_PTR (or a
freed pointer after "leds: class: Split led_classdev_register_ext()"), while
toshiba_acpi_notify() and toshiba_acpi_kbd_bl_work() still call
led_classdev_notify_brightness_hw_changed(&dev->kbd_led, ...).

> -#else
> -static int led_add_brightness_hw_changed(struct led_classdev *led_cdev)
> -{
> -	return 0;
> -}
> -static void led_remove_brightness_hw_changed(struct led_classdev *led_cdev)
> -{
> -}
>  #endif

[ ... ]

>
>

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