[PATCH v2 0/4] leds: trigger: netdev: fix sysfs_update_group() races

[PATCH v2 0/4] leds: trigger: netdev: fix sysfs_update_group() races

[A. Sverdlin]

From: Alexander Sverdlin <alexander.sverdlin@siemens.com>

The netdev LED trigger refreshes the link_speed attribute group with
sysfs_update_group() from several contexts (the NETDEV_CHANGE notifier,
device_name writes and, indirectly, trigger (de)activation) that share no
common lock.  On a board that emits PHY link events while the trigger is
being (re)armed during boot this is observed as a hard sysfs failure:

  sysfs: cannot create duplicate filename '...green:lan/link_10'
  ...
  led_trigger_set
  led_trigger_write

The window between activate() and device_add_groups() in led_trigger_set()
is narrow, so to reproduce it reliably I artificially widened it with the
debug patch below:

  --- a/drivers/leds/led-triggers.c
  +++ b/drivers/leds/led-triggers.c
  @@ -12,6 +12,7 @@
   #include <linux/list.h>
   #include <linux/spinlock.h>
   #include <linux/device.h>
  +#include <linux/delay.h>
   #include <linux/timer.h>
   #include <linux/rwsem.h>
   #include <linux/leds.h>
  @@ -223,6 +224,8 @@ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
                   if (ret)
                           goto err_activate;

  +               msleep(5000);
  +
                   ret = device_add_groups(led_cdev->dev, trig->groups);
                   if (ret) {
                           dev_err(led_cdev->dev, "Failed to add trigger attributes\n");

With that msleep() in place, writing the trigger while a NETDEV_CHANGE is
delivered from the linkwatch workqueue reproduces the duplicate-filename
splat every time.

The whole series is LOCKDEP-verified.

Changelog:
v2:
- addressed pre-existing issues found by Sashiko (patches 1&2)
- reduced verbosity both in comments and in commit messages
- patch 3 is a combined rework of patches 1&2 from v1
- dropped sync from cancel_delayed_work() (worker now takes the lock)
v1:
https://lore.kernel.org/all/20260914132356.807004-1-alexander.sverdlin@siemens.com/

Alexander Sverdlin (4):
  leds: trigger: netdev: Put netdev on activate error path
  leds: trigger: netdev: Access net_dev under trigger_data->lock in the
    worker
  leds: trigger: netdev: Fix sysfs_update_group() races
  leds: trigger: netdev: Serialize mode/interval stores with trigger
    lock

 drivers/leds/trigger/ledtrig-netdev.c | 54 +++++++++++++++++++++------
 1 file changed, 43 insertions(+), 11 deletions(-)

-- 
2.55.0

[PATCH v2 1/4] leds: trigger: netdev: Put netdev on activate error path

[A. Sverdlin]

From: Alexander Sverdlin <alexander.sverdlin@siemens.com>

When hw control is active, set_device_name() takes a reference on the
netdev via dev_get_by_name(). The register_netdevice_notifier() failure
path frees trigger_data without dropping it, leaking the netdev.

Cc: stable@vger.kernel.org
Closes: https://lore.kernel.org/all/20260914140045.B8F4C1F000FF@smtp.kernel.org/
Fixes: 0316cc5629d1 ("leds: trigger: netdev: init mode if hw control already active")
Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
---
Changelog:
v2:
- new patch

 drivers/leds/trigger/ledtrig-netdev.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 5b4e92c14dbb4..c65df91c4f6dc 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -779,8 +779,10 @@ static int netdev_trig_activate(struct led_classdev *led_cdev)
 	led_set_trigger_data(led_cdev, trigger_data);
 
 	rc = register_netdevice_notifier(&trigger_data->notifier);
-	if (rc)
+	if (rc) {
+		dev_put(trigger_data->net_dev);
 		kfree(trigger_data);
+	}
 
 	return rc;
 }
-- 
2.55.0

[PATCH v2 2/4] leds: trigger: netdev: Access net_dev under trigger_data->lock in the worker

[A. Sverdlin]

From: Alexander Sverdlin <alexander.sverdlin@siemens.com>

netdev_trig_work() dereferences trigger_data->net_dev without the lock,
while set_device_name() and netdev_trig_notify() dev_put() and replace it
under trigger_data->lock.  On NETDEV_UNREGISTER the worker can run
dev_get_stats() on a netdev being freed (UAF).

Take trigger_data->lock in the worker.  cancel_delayed_work_sync() must
then never run under that lock.

Cc: stable@vger.kernel.org
Closes: https://lore.kernel.org/all/20260914142116.6DAC51F000FF@smtp.kernel.org/
Fixes: 06f502f57d0d ("leds: trigger: Introduce a NETDEV trigger")
Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
---
Changelog:
v2:
- new patch

 drivers/leds/trigger/ledtrig-netdev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index c65df91c4f6dc..8b807451b642d 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -679,6 +679,8 @@ static void netdev_trig_work(struct work_struct *work)
 	unsigned long interval;
 	int invert;
 
+	guard(mutex)(&trigger_data->lock);
+
 	/* If we dont have a device, insure we are off */
 	if (!trigger_data->net_dev) {
 		led_set_brightness(trigger_data->led_cdev, LED_OFF);
-- 
2.55.0

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

[A. Sverdlin]

From: Alexander Sverdlin <alexander.sverdlin@siemens.com>

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@vger.kernel.org
Fixes: 06cdca014eca ("leds: trigger: netdev: Display only supported link speed attribute")
Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
---
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;
 
@@ -606,7 +610,6 @@ static const struct attribute_group netdev_trig_attrs_group = {
 
 static const struct attribute_group *netdev_trig_groups[] = {
 	&netdev_trig_attrs_group,
-	&netdev_trig_link_speed_attrs_group,
 	NULL,
 };
 
@@ -654,10 +657,6 @@ static int netdev_trig_notify(struct notifier_block *nb,
 		fallthrough;
 	case NETDEV_CHANGE:
 		get_device_state(trigger_data);
-		/* Refresh link_speed visibility */
-		if (evt == NETDEV_CHANGE)
-			sysfs_update_group(&led_cdev->dev->kobj,
-					   &netdev_trig_link_speed_attrs_group);
 		break;
 	}
 
@@ -665,6 +664,12 @@ static int netdev_trig_notify(struct notifier_block *nb,
 
 	mutex_unlock(&trigger_data->lock);
 
+	if (evt == NETDEV_CHANGE) {
+		guard(mutex)(&trigger_data->attr_lock);
+		sysfs_update_group(&led_cdev->dev->kobj,
+				   &netdev_trig_link_speed_attrs_group);
+	}
+
 	return NOTIFY_DONE;
 }
 
@@ -747,6 +752,7 @@ static int netdev_trig_activate(struct led_classdev *led_cdev)
 		return -ENOMEM;
 
 	mutex_init(&trigger_data->lock);
+	mutex_init(&trigger_data->attr_lock);
 
 	trigger_data->notifier.notifier_call = netdev_trig_notify;
 	trigger_data->notifier.priority = 10;
@@ -780,12 +786,23 @@ static int netdev_trig_activate(struct led_classdev *led_cdev)
 
 	led_set_trigger_data(led_cdev, trigger_data);
 
+	rc = sysfs_create_group(&led_cdev->dev->kobj,
+				&netdev_trig_link_speed_attrs_group);
+	if (rc)
+		goto err_free;
+
 	rc = register_netdevice_notifier(&trigger_data->notifier);
-	if (rc) {
-		dev_put(trigger_data->net_dev);
-		kfree(trigger_data);
-	}
+	if (rc)
+		goto err_remove_group;
 
+	return 0;
+
+err_remove_group:
+	sysfs_remove_group(&led_cdev->dev->kobj,
+			   &netdev_trig_link_speed_attrs_group);
+err_free:
+	dev_put(trigger_data->net_dev);
+	kfree(trigger_data);
 	return rc;
 }
 
@@ -795,6 +812,9 @@ static void netdev_trig_deactivate(struct led_classdev *led_cdev)
 
 	unregister_netdevice_notifier(&trigger_data->notifier);
 
+	sysfs_remove_group(&led_cdev->dev->kobj,
+			   &netdev_trig_link_speed_attrs_group);
+
 	cancel_delayed_work_sync(&trigger_data->work);
 
 	dev_put(trigger_data->net_dev);
-- 
2.55.0

[PATCH v2 4/4] leds: trigger: netdev: Serialize mode/interval stores with trigger lock

[A. Sverdlin]

From: Alexander Sverdlin <alexander.sverdlin@siemens.com>

netdev_led_attr_store() and interval_store() update ->mode and run
set_baseline_state() without trigger_data->lock.  kernfs only serializes
writes to the same file, so two attribute writes race on the non-atomic
read-modify-write of ->mode:

  CPU0 (echo 1 > link_10)       CPU1 (echo 1 > link_100)
  -----------------------       ------------------------
  mode = trigger_data->mode;
                                mode = trigger_data->mode;
  set_bit(LINK_10, &mode);
                                set_bit(LINK_100, &mode);
  trigger_data->mode = mode;
                                trigger_data->mode = mode;  // LINK_10 lost

They also race the notifier's link-state and ->hw_control updates.

Take trigger_data->lock in both stores.  netdev_trig_work() also holds it,
so use the async cancel_delayed_work() (a sync cancel would deadlock).

Cc: stable@vger.kernel.org
Fixes: d5e01266e7f5 ("leds: trigger: netdev: add additional specific link speed mode")
Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
---
Changelog:
v2:
- reduced verbosity both in the comments and in commit message
- dropped sync from cancel_delayed_work() (worker now takes the lock)

 drivers/leds/trigger/ledtrig-netdev.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 54c6913f3afe3..92a6a456130c9 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -385,7 +385,7 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
 {
 	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
 	struct led_classdev *led_cdev = trigger_data->led_cdev;
-	unsigned long state, mode = trigger_data->mode;
+	unsigned long state, mode;
 	int ret;
 	int bit;
 
@@ -417,6 +417,10 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
 		return -EINVAL;
 	}
 
+	/* async cancel: the worker takes this lock */
+	guard(mutex)(&trigger_data->lock);
+
+	mode = trigger_data->mode;
 	if (state)
 		set_bit(bit, &mode);
 	else
@@ -435,7 +439,7 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
 	     test_bit(TRIGGER_NETDEV_LINK_100000, &mode)))
 		return -EINVAL;
 
-	cancel_delayed_work_sync(&trigger_data->work);
+	cancel_delayed_work(&trigger_data->work);
 
 	trigger_data->mode = mode;
 	trigger_data->hw_control = can_hw_control(trigger_data);
@@ -506,10 +510,14 @@ static ssize_t interval_store(struct device *dev,
 
 	/* impose some basic bounds on the timer interval */
 	if (value >= 5 && value <= 10000) {
-		cancel_delayed_work_sync(&trigger_data->work);
+		mutex_lock(&trigger_data->lock);
+
+		cancel_delayed_work(&trigger_data->work);
 
 		atomic_set(&trigger_data->interval, msecs_to_jiffies(value));
 		set_baseline_state(trigger_data);	/* resets timer */
+
+		mutex_unlock(&trigger_data->lock);
 	}
 
 	return size;
-- 
2.55.0

Re: [PATCH v2 1/4] leds: trigger: netdev: Put netdev on activate error path

[Andrew Lunn]

On Mon, Sep 21, 2026 at 11:32:43AM +0200, A. Sverdlin wrote:
> From: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> 
> When hw control is active, set_device_name() takes a reference on the
> netdev via dev_get_by_name(). The register_netdevice_notifier() failure
> path frees trigger_data without dropping it, leaking the netdev.

Maybe it would be better to add a unset_device_name(), which releases
the reference, and does anything else which needs to be undone?

	Andrew

Re: [PATCH v2 2/4] leds: trigger: netdev: Access net_dev under trigger_data->lock in the worker

[Andrew Lunn]

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

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

There is a dislike for the magically guard() construct.

      Andrew

Re: [PATCH v2 1/4] leds: trigger: netdev: Put netdev on activate error path

[Sverdlin, Alexander]

Hi Andrew!

On Sun, 2026-09-27 at 17:02 +0200, Andrew Lunn wrote:
> On Mon, Sep 21, 2026 at 11:32:43AM +0200, A. Sverdlin wrote:
> > From: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> > 
> > When hw control is active, set_device_name() takes a reference on the
> > netdev via dev_get_by_name(). The register_netdevice_notifier() failure
> > path frees trigger_data without dropping it, leaking the netdev.
> 
> Maybe it would be better to add a unset_device_name(), which releases
> the reference, and does anything else which needs to be undone?

ACK

pw-bot: cr

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com

Re: [PATCH v2 2/4] leds: trigger: netdev: Access net_dev under trigger_data->lock in the worker

[Sverdlin, Alexander]

Hi Andrew!

On Sun, 2026-09-27 at 17:05 +0200, Andrew Lunn wrote:
> > +	guard(mutex)(&trigger_data->lock);
> > +
> 
> https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html
> 
> There is a dislike for the magically guard() construct.

ACK

pw-bot: cr

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com

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@siemens.com>
> 
> 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@vger.kernel.org
> Fixes: 06cdca014eca ("leds: trigger: netdev: Display only supported link speed attribute")
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> ---
> 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