Re: [PATCH v5 06/12] leds: trigger: netdev: Implement offloaded() callback

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

New issues:
- [Medium] Unprotected lockless read of shared state in `netdev_trig_offloaded()`

Pre-existing issues:
- [High] Resource leak of `net_device` in `netdev_trig_activate()` error path
--

--- Patch [6]: [PATCH v5 06/12] leds: trigger: netdev: Implement offloaded() callback ---
commit d8144f7505ed647e0e3c94f8c01b2ef2b0814005
Author: Rong Zhang <[email protected]>

leds: trigger: netdev: Implement offloaded() callback

This commit implements the offloaded callback for the netdev trigger,
providing its hardware control state to the LED core. It also updates the
sysfs ABI documentation to clarify the relationship between the custom and
generic offload attributes.

> 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;

[Severity: Medium]
Is it possible for hw_control to be read while it is being updated?

When userspace reads the trigger_may_offload sysfs attribute, it calls
netdev_trig_offloaded() holding only the led_cdev->trigger_lock, which is a
read lock.

Concurrently, a network event can invoke netdev_trig_notify() which
acquires trigger_data->lock and mutates trigger_data->hw_control.

Does this code need to hold trigger_data->lock or use the READ_ONCE() macro
to prevent a data race when reading trigger_data->hw_control here?

> +}
> +
>  static struct led_trigger netdev_led_trigger = {
>  	.name = "netdev",
>  	.activate = netdev_trig_activate,
>  	.deactivate = netdev_trig_deactivate,
> +	.offloaded = netdev_trig_offloaded,
>  	.groups = netdev_trig_groups,
>  };

[Severity: High]
This is a pre-existing issue, but does netdev_trig_activate() leak a
net_device reference in its error path?

If hardware control is supported, set_device_name() is called, which
acquires a net device reference via dev_get_by_name() and saves it to
trigger_data->net_dev.

If the subsequent call to register_netdevice_notifier() fails, the error
path executes:

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

	return rc;
}

Since trigger_data is freed without calling dev_put(trigger_data->net_dev),
does this leave a permanent reference count leak on the net_device?

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