Re: [PATCH 1/3] leds: trigger: netdev: hold lock across sysfs_update_group() in device_name_store()

[sashiko-bot@kernel.org]

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

New issues:
- [High] Cementing the anti-pattern of calling `sysfs_update_group()` under `trigger_data->lock` creates an architectural deadlock trap with sysfs callbacks.
- [Low] The commit message description is not capitalized, violating LED subsystem guidelines.

Pre-existing issues:
- [High] Missing `dev_put()` in the error path of `netdev_trig_activate` causes a permanent reference leak of the network device.
--

commit afc39a712420509489f8fd34d9ef514cdf8ed25a
Author: Alexander Sverdlin <alexander.sverdlin@siemens.com>
Subject: leds: trigger: netdev: hold lock across sysfs_update_group() in device_name_store()

This patch attempts to resolve a race condition between device_name_store() and
netdev_trig_notify() by holding trigger_data->lock around sysfs_update_group()
in device_name_store(). The intent is to match the locking context already used
in netdev_trig_notify() to protect concurrent reads of supported_link_modes.

> leds: trigger: netdev: hold lock across sysfs_update_group() in device_name_store()

[Severity: Low]
This isn't a bug introduced by this patch, but the commit message description
isn't capitalized. Should it be "Hold lock..." to match the LED subsystem
guidelines?

> diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> index 5b4e92c14dbb4..354d3b640fa53 100644
> --- a/drivers/leds/trigger/ledtrig-netdev.c
> +++ b/drivers/leds/trigger/ledtrig-netdev.c
> @@ -336,8 +336,14 @@ static ssize_t device_name_store(struct device *dev,
>  	if (ret < 0)
>  		return ret;
>  
> -	/* Refresh link_speed visibility */
> +	/*
> +	 * Refresh link_speed visibility, serialized against netdev_trig_notify()
> +	 * which may concurrently call sysfs_update_group() on the same group
> +	 * while reading supported_link_modes via netdev_trig_link_speed_visible().
> +	 */
> +	mutex_lock(&trigger_data->lock);
>  	sysfs_update_group(&dev->kobj, &netdev_trig_link_speed_attrs_group);
> +	mutex_unlock(&trigger_data->lock);
>  
>  	return size;
>  }

[Severity: High]
Does expanding this lock coverage to include sysfs_update_group() create a
lock inversion deadlock with sysfs active references?

If a user thread writes to a sysfs attribute (like link_10), the kernfs core
acquires an active reference on that attribute before calling the store
callback. The callback then waits for trigger_data->lock:

netdev_led_attr_store()
    guard(mutex)(&trigger_data->lock);

If concurrently, a network link change processes in netdev_trig_notify(), it
acquires trigger_data->lock and then calls sysfs_update_group().

sysfs_update_group() must drain all active references on the sysfs group before
it can proceed, so it will block waiting for the user thread to finish its
write.

Since netdev_trig_notify() holds the rtnl lock, could this deadlock freeze
the entire network stack?

While reviewing this file, I noticed a pre-existing issue in the activation
sequence:

drivers/leds/trigger/ledtrig-netdev.c:netdev_trig_activate() {
    ...
    if (supports_hw_control(led_cdev)) {
        dev = led_cdev->hw_control_get_device(led_cdev);
        if (dev) {
            const char *name = dev_name(dev);
            ...
            set_device_name(trigger_data, name, strlen(name));
            ...
        }
    }
    ...
    rc = register_netdevice_notifier(&trigger_data->notifier);
    if (rc)
        kfree(trigger_data);

    return rc;
}

[Severity: High]
This is a pre-existing issue, but is there a missing dev_put() on the network
device in the error path for register_netdevice_notifier()?

When hardware control is supported, set_device_name() acquires a reference to
the network device via dev_get_by_name(). If register_netdevice_notifier()
fails later, the error path frees trigger_data but seems to leave the network
device reference held.

Would this permanent reference leak prevent network interfaces from being torn
down or block network namespace destruction?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914132356.807004-1-alexander.sverdlin@siemens.com?part=1