Re: [PATCH v6 06/12] leds: turris-omnia: Implement offloaded() trigger callback and declare hw_control_trigger

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The statically allocated `omnia_hw_trigger` object is registered multiple times via `devm_led_trigger_register()` on probe, causing lock and list corruption if multiple driver instances are bound.
--

commit 836486b7bf6dc3bf19d25d5b547db0edd0d638af
Author: Rong Zhang <i@rong.moe>

leds: turris-omnia: Implement offloaded() trigger callback and declare hw_control_trigger
    
This commit implements the offloaded() callback for the omnia-mcu hardware
control trigger, returning true to reflect its state. It also declares it as
a hardware control trigger in the class device during registration.

> diff --git a/drivers/leds/leds-turris-omnia.c b/drivers/leds/leds-turris-omnia.c
> index ed6a47bbb44f9..32d40d176d3f1 100644
> --- a/drivers/leds/leds-turris-omnia.c
> +++ b/drivers/leds/leds-turris-omnia.c
[ ... ]
> @@ -195,10 +195,16 @@ static void omnia_hwtrig_deactivate(struct led_classdev *cdev)
>  			err);
>  }
>  
> +static bool omnia_hwtrig_offloaded(struct led_classdev *cdev)
> +{
> +	return true;
> +}
> +
>  static struct led_trigger omnia_hw_trigger = {
>  	.name		= "omnia-mcu",
>  	.activate	= omnia_hwtrig_activate,
>  	.deactivate	= omnia_hwtrig_deactivate,
> +	.offloaded	= omnia_hwtrig_offloaded,
>  	.trigger_type	= &omnia_hw_trigger_type,
>  };

[Severity: High]
This is a pre-existing issue, but does this statically allocated singleton
trigger cause list corruption if multiple driver instances are bound?

If a user manually binds a second device instance to the leds-turris-omnia
driver via the sysfs bind interface (or multiple I2C devices probe),
devm_led_trigger_register() in omnia_leds_probe() will be called again with
this exact same statically allocated omnia_hw_trigger structure pointer.

In drivers/leds/led-triggers.c:led_trigger_register(), the core initializes
the lock and list head before checking for duplicate registrations:

    spin_lock_init(&trig->leddev_list_lock);
    INIT_LIST_HEAD(&trig->led_cdevs);

Wouldn't this blindly overwrite the active lock and list state of the
trigger, destroying the lock state while another CPU is iterating and
unlinking any already-registered LEDs, leading to a use-after-free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902-leds-trigger-hw-changed-v6-0-55693cd78877@rong.moe?part=6