Re: [PATCH v2 3/4] leds: trigger: netdev: Fix sysfs_update_group() races

[Sverdlin, Alexander]

Hi Andrew,

On Mon, 2026-09-21 at 11:32 +0200, A. Sverdlin wrote:
> From: Alexander Sverdlin <alexander.sverdlin@xxxxxxxxxxx>
> 
> The link_speed attribute group was listed in netdev_led_trigger.groups,
> so the LED core creates and destroys it via device_add_groups() /
> device_remove_groups() in led_trigger_set() (under trigger_lock), while
> the trigger also refreshes it with sysfs_update_group() from
> netdev_trig_notify() and device_name writes.  With no shared lock this was
> observed as a sysfs splat when the trigger is re-armed during PHY link-up:
> 
>   sysfs: cannot create duplicate filename '...green:lan/link_10'
> 
>     CPU 0 (led_trigger_set)        CPU 1 (linkwatch workqueue)
>     -----------------------        ---------------------------
>     activate():
>       register_netdevice_notifier()
>     .                              netdev_trig_notify(NETDEV_CHANGE):
>     .                                sysfs_update_group() creates "link_10"
>     device_add_groups()
>       creates "link_10"  <- EEXIST!
> 
> The mirror case (device_remove_groups() racing the notifier) leaves an
> orphaned sysfs file pointing at freed trigger_data, i.e. a UAF.
> 
> Manage the group in the trigger: drop it from netdev_led_trigger.groups
> and create/destroy it in activate()/deactivate(), so the LED core never
> touches it.  The two remaining refreshers, netdev_trig_notify() and
> device_name_store(), are serialized against each other by a dedicated
> attr_lock, and the is_visible callback takes trigger_data->lock for its
> supported_link_modes read.  sysfs_update_group() is never called under
> trigger_data->lock, so it cannot deadlock against a concurrent link_*
> store that takes that lock.
> 
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: 06cdca014eca ("leds: trigger: netdev: Display only supported link speed attribute")
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@xxxxxxxxxxx>
> ---
> Changelog:
> v2:
> - this patch is a combined rework of patches 1&2 from v1
> 
>  drivers/leds/trigger/ledtrig-netdev.c | 42 ++++++++++++++++++++-------
>  1 file changed, 31 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> index 8b807451b642d..54c6913f3afe3 100644
> --- a/drivers/leds/trigger/ledtrig-netdev.c
> +++ b/drivers/leds/trigger/ledtrig-netdev.c
> @@ -57,6 +57,8 @@
>  
>  struct led_netdev_data {
>  	struct mutex lock;
> +	/* Serializes link_speed group refreshes; never taken by attr stores */
> +	struct mutex attr_lock;
>  
>  	struct delayed_work work;
>  	struct notifier_block notifier;
> @@ -336,8 +338,9 @@ static ssize_t device_name_store(struct device *dev,
>  	if (ret < 0)
>  		return ret;
>  
> -	/* Refresh link_speed visibility */
> -	sysfs_update_group(&dev->kobj, &netdev_trig_link_speed_attrs_group);
> +	/* Serialize the link_speed visibility refresh against netdev_trig_notify() */
> +	scoped_guard(mutex, &trigger_data->attr_lock)
> +		sysfs_update_group(&dev->kobj, &netdev_trig_link_speed_attrs_group);
>  
>  	return size;
>  }
> @@ -547,6 +550,7 @@ static umode_t netdev_trig_link_speed_visible(struct kobject *kobj,
>  	 * Stop at the first matching entry as we care only to check if a particular
>  	 * speed is supported and not the kind.
>  	 */
> +	guard(mutex)(&trigger_data->lock);
>  	for_each_set_bit(mode, supported_link_modes, __ETHTOOL_LINK_MODE_MASK_NBITS) {
>  		struct ethtool_link_ksettings link_ksettings;
>  

I can change this to scoped_guard() as it "is considered more readable" [1],
but switching to explicit mutex_unlock() makes things quite ugly because
CHECK_LINK_MODE_ATTR() would  either do mutex_unlock() (of a lock outside of
macro scope) or a goto (outside of macro).

Therefore at least a scoped_guard() really makes sense here IMO.

[1] https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com