Re: [PATCH v6 07/12] leds: trigger: netdev: Implement offloaded() callback

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Resource leak of `net_device` in `netdev_trig_activate()` error path.
- [High] Race condition in `netdev_led_attr_store()` leading to lost configuration updates and a Use-After-Free of `net_device`.
--

commit 2e9edb75c55c3a99bfaf5dc0eea11b9a95851115
Author: Rong Zhang <i@rong.moe>

leds: trigger: netdev: Implement offloaded() callback

"netdev" can run in hardware control according to hardware capabilities
and trigger options. Implement offloaded() callback to provide its hardware
control state to the LED core, and document the relation 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 5b4e92c14dbb4..2e9af48a74dbf 100644
> --- a/drivers/leds/trigger/ledtrig-netdev.c
> +++ b/drivers/leds/trigger/ledtrig-netdev.c

[ ... ]

> @@ -798,10 +798,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 is a pre-existing issue, but does netdev_trig_activate() leak the
net_device reference on its error path?

When supports_hw_control is true, setting the device name acquires a
reference to the net_device via dev_get_by_name():

drivers/leds/trigger/ledtrig-netdev.c:netdev_trig_activate() {
    ...
    if (supports_hw_control(led_cdev)) {
        ...
        set_device_name(trigger_data, name, strlen(name));
        ...
    }
    ...

If register_netdevice_notifier() subsequently fails, the error path frees
trigger_data but does not drop the net_device reference:

    rc = register_netdevice_notifier(&trigger_data->notifier);
    if (rc)
        kfree(trigger_data);
    ...
}

Can this leak prevent the network interface from being completely unregistered,
causing operations like network namespace destruction or module removal to hang
indefinitely in the netdev_wait_allrefs loop?

[Severity: High]
This is a pre-existing issue, but is there a race condition in
netdev_led_attr_store() that leads to lost configuration updates and a
use-after-free of net_device?

netdev_led_attr_store() executes concurrently with
netdev_trig_notify(NETDEV_UNREGISTER). Since the sysfs store function
doesn't acquire trigger_data->lock, it can schedule trigger_data->work
locklessly after the notifier has called cancel_delayed_work_sync():

drivers/leds/trigger/ledtrig-netdev.c:netdev_led_attr_store() {
    ...
    cancel_delayed_work_sync(&trigger_data->work);
    ...
    set_baseline_state(trigger_data); // Schedules delayed work outside lock
    ...
}

If netdev_trig_notify() drops the net_device reference and sets it to NULL:

drivers/leds/trigger/ledtrig-netdev.c:netdev_trig_notify() {
    ...
    case NETDEV_UNREGISTER:
        dev_put(trigger_data->net_dev);
        trigger_data->net_dev = NULL;
    ...
}

If the scheduled delayed work (netdev_trig_work()) runs concurrently, it
reads trigger_data->net_dev locklessly. Could it read the pointer before
the notifier sets it to NULL, but execute dev_get_stats() after the notifier
has called dev_put(), resulting in a use-after-free?

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

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