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

[Andrew Lunn]

On Mon, Sep 14, 2026 at 03:23:50PM +0200, A. Sverdlin wrote:
> From: Alexander Sverdlin <alexander.sverdlin@xxxxxxxxxxx>
> 
> device_name_store() calls sysfs_update_group() without holding
> trigger_data->lock. The is_visible callback reads
> trigger_data->supported_link_modes, and the sysfs group modification
> itself is not atomic. This races against netdev_trig_notify() in two
> ways:
> 
> 1) Concurrent sysfs_update_group() on the same group:
> 
>     CPU 0 (device_name_store)            CPU 1 (linkwatch workqueue)
>     -------------------------            ---------------------------
>     set_device_name():
>       rtnl_lock()
>       mutex_lock(trigger_data->lock)
>       [sets supported_link_modes]
>       mutex_unlock(trigger_data->lock)
>       rtnl_unlock()
> 
>     sysfs_update_group():
>       remove("link_10")
>                                          netdev_trig_notify():
>                                            mutex_lock(trigger_data->lock)
>                                            sysfs_update_group():
>                                              remove("link_10") [nop]
>                                              add("link_10") ← CREATED
>                                            mutex_unlock(trigger_data->lock)
>       add("link_10") <- EEXIST!
> 
> 2) The netdev_trig_link_speed_visible() reads supported_link_modes without
>    the lock, racing against get_device_state() called from
>    netdev_trig_notify() which modifies supported_link_modes (a multi-word
>    bitmap) under trigger_data->lock. This can cause
>    netdev_trig_link_speed_visible() to observe a partially-updated bitmap.
> 
> Fix by holding trigger_data->lock around the sysfs_update_group() call
> in device_name_store(), matching the locking context used by
> netdev_trig_notify().
> 
> Fixes: 06cdca014eca ("leds: trigger: netdev: Display only supported link speed attribute")
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@xxxxxxxxxxx>
> ---
>  drivers/leds/trigger/ledtrig-netdev.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> 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);

If you look at all the other places where the lock it taken and
released, there are no comments about why the lock is used. I think
your verbose commit message is sufficient.


    Andrew

---
pw-bot: cr