Re: [PATCH RFC v3 08/11] leds: trigger: Add led_trigger_notify_hw_control_changed() interface

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

Pre-existing issues:
- [High] Race condition between asynchronous hardware notification (or sysfs writes) and LED unregistration leads to a Use-After-Free of the led_classdev.
--

--- Patch [8]: [PATCH RFC v3 08/11] leds: trigger: Add led_trigger_notify_hw_control_changed() interface ---
commit 9103c7dadab6a0041a92ad3edeff977ceadf9820
Author: Rong Zhang <[email protected]>

leds: trigger: Add led_trigger_notify_hw_control_changed() interface

This commit adds a new interface led_trigger_notify_hw_control_changed() to
allow LED drivers to notify the LED core about hardware-initiated control
transitions. It handles switching between "none" and a private trigger based
on autonomous hardware state changes.

> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> index 726fa7bf88cfd..6ae28cbd1c778 100644
> --- a/drivers/leds/led-triggers.c
> +++ b/drivers/leds/led-triggers.c

[ ... ]

> @@ -467,6 +488,63 @@ int devm_led_trigger_register(struct device *dev,

[ ... ]

> +void led_trigger_notify_hw_control_changed(struct led_classdev *led_cdev, bool activate)
> +{
> +	struct led_trigger *trig;
> +
> +	/* Restricted to private triggers. */
> +	if (WARN_ON(!(led_cdev->flags & LED_TRIG_HW_CHANGED) ||
> +		    !led_cdev->hw_control_trigger || !led_cdev->trigger_type))
> +		return;
> +
> +	scoped_guard(rwsem_read, &triggers_list_lock) {
> +		list_for_each_entry(trig, &trigger_list, next_trig) {
> +			if (trig->trigger_type == led_cdev->trigger_type &&
> +			    !strcmp(trig->name, led_cdev->hw_control_trigger)) {
> +				guard(rwsem_write)(&led_cdev->trigger_lock);
> +
> +				led_trigger_do_hw_control_transition(led_cdev, activate, trig);

[Severity: High]
This isn't a bug introduced by this patch, but could this new API expose a
race condition with device unregistration?

Looking at led_classdev_unregister() in drivers/leds/led-class.c, the
trigger_lock is released before the LED_UNREGISTERING flag is set:

led_classdev_unregister(struct led_classdev *led_cdev) {
    ...
    led_trigger_set(led_cdev, NULL);
    up_write(&led_cdev->trigger_lock);
    ...
    led_cdev->flags |= LED_UNREGISTERING;
}

If device removal (like a USB unplug or sysfs unbind) happens concurrently
with a hardware transition event, could led_trigger_notify_hw_control_changed()
acquire the trigger_lock here and successfully attach a trigger to the dying
LED device?

If led_classdev_unregister() then finishes and the LED memory is freed,
wouldn't this leave a dangling pointer in the trigger's led_cdevs list,
leading to a use-after-free memory corruption when the trigger later fires?

> +				return;
> +			}
> +		}
> +	}
> +
> +	dev_err(led_cdev->dev,
> +		"%s() is called, but the private trigger (%s) is not properly registered\n",
> +		__func__, led_cdev->hw_control_trigger);
> +}
> +EXPORT_SYMBOL_GPL(led_trigger_notify_hw_control_changed);

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