Re: [PATCH RFC v3 06/11] leds: trigger: netdev: Implement offloaded() callback

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

Pre-existing issues:
- [High] Missing dev_put() on net_device when register_netdevice_notifier() fails in netdev_trig_activate().
- [High] sysfs store functions modify shared trigger state and call set_baseline_state() without locking, racing with network notifiers.
--

--- Patch [6]: [PATCH RFC v3 06/11] leds: trigger: netdev: Implement offloaded() callback ---
commit 0713a09815032fb80c29f765ff7ef7a8acfbd3d3
Author: Rong Zhang <[email protected]>

leds: trigger: netdev: Implement offloaded() callback

This patch adds an offloaded() callback to the netdev LED trigger to
provide its hardware control state to the LED core. It also updates the
documentation to clarify the relationship between the custom offloaded
attribute and the generic trigger_may_offload attribute.

> diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> index 64c078e997f28..a26109ca4b1c9 100644
> --- a/drivers/leds/trigger/ledtrig-netdev.c
> +++ b/drivers/leds/trigger/ledtrig-netdev.c

[ ... ]

> @@ -754,10 +754,18 @@ static void netdev_trig_deactivate(struct led_classdev *led_cdev)
>  	kfree(trigger_data);
>  }
>  
> +static bool netdev_trig_offloaded(struct led_classdev *led_cdev)
> +{
> +	struct led_netdev_data *trigger_data = led_get_trigger_data(led_cdev);
> +
> +	return trigger_data->hw_control;
> +}
> +
>  static struct led_trigger netdev_led_trigger = {
>  	.name = "netdev",
>  	.activate = netdev_trig_activate,

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a missing dev_put()
in the error path of netdev_trig_activate(). If register_netdevice_notifier()
fails, trigger_data is freed but the net_dev reference is leaked:

drivers/leds/trigger/ledtrig-netdev.c:netdev_trig_activate() {
    ...
    rc = register_netdevice_notifier(&trigger_data->notifier);
    if (rc)
        kfree(trigger_data);
    ...
}

Will this permanently leak the network device reference and cause
unregister_netdev() to hang indefinitely?

>  	.deactivate = netdev_trig_deactivate,
> +	.offloaded = netdev_trig_offloaded,
>  	.groups = netdev_trig_groups,

[Severity: High]
This is a pre-existing issue, but while reviewing the netdev trigger groups,
I noticed that the sysfs store functions modify shared state without locking:

drivers/leds/trigger/ledtrig-netdev.c:netdev_led_attr_store() {
    ...
    trigger_data->mode = mode;
    trigger_data->hw_control = can_hw_control(trigger_data);
    ...
    set_baseline_state(trigger_data);
    ...
}

drivers/leds/trigger/ledtrig-netdev.c:interval_store() {
    ...
    atomic_set(&trigger_data->interval, msecs_to_jiffies(value));
    set_baseline_state(trigger_data);
    ...
}

Could concurrent sysfs writes overwrite each other's state or race with
asynchronous network events handled by netdev_trig_notify(), leading to data
races on LED hardware control and timer schedules?

>  };

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