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

[PATCH 0/3] 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

Patches 1 and 2 close this really observed race.  Patch 1 serializes
device_name_store()'s sysfs_update_group() (and its is_visible callback's
read of ->supported_link_modes) against the notifier under
trigger_data->lock.  Patch 2 removes the remaining collision by construction:
the link_speed group is dropped from netdev_led_trigger.groups so the LED
core's device_add_groups() / device_remove_groups() no longer touch it, and
the trigger owns its lifecycle via sysfs_create_group() in activate() and
sysfs_remove_group() in deactivate().

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; with patches 1 and 2 applied it no longer triggers.

Patch 3 addresses a separate race that I deduced by inspection but did not
try to reproduce in practice: netdev_led_attr_store() and interval_store()
mutate the shared trigger_data state (a non-atomic read-modify-write of
->mode, ->hw_control and set_baseline_state()) without holding
trigger_data->lock, so they can race with each other and with the notifier.
It is fixed by taking trigger_data->lock around those updates.

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

Patches 1 and 2 close this really observed race.  Patch 1 serializes
device_name_store()'s sysfs_update_group() (and its is_visible callback's
read of ->supported_link_modes) against the notifier under
trigger_data->lock.  Patch 2 removes the remaining collision by construction:
the link_speed group is dropped from netdev_led_trigger.groups so the LED
core's device_add_groups() / device_remove_groups() no longer touch it, and
the trigger owns its lifecycle via sysfs_create_group() in activate() and
sysfs_remove_group() in deactivate().

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; with patches 1 and 2 applied it no longer triggers.

Patch 3 addresses a separate race that I deduced by inspection but did not
try to reproduce in practice: netdev_led_attr_store() and interval_store()
mutate the shared trigger_data state (a non-atomic read-modify-write of
->mode, ->hw_control and set_baseline_state()) without holding
trigger_data->lock, so they can race with each other and with the notifier.
It is fixed by taking trigger_data->lock around those updates.

The whole series is LOCKDEP-verified.

Alexander Sverdlin (3):
  leds: trigger: netdev: hold lock across sysfs_update_group() in
    device_name_store()
  leds: trigger: netdev: fix sysfs_update_group() vs led_trigger_set()
    races
  leds: trigger: netdev: serialize mode/interval stores with trigger
    lock

 drivers/leds/trigger/ledtrig-netdev.c | 67 +++++++++++++++++++++++++--
 1 file changed, 62 insertions(+), 5 deletions(-)

-- 
2.55.0

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

[A. Sverdlin]

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

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@siemens.com>
---
 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);
 
 	return size;
 }
-- 
2.55.0

[PATCH 2/3] leds: trigger: netdev: fix sysfs_update_group() vs led_trigger_set() races

[A. Sverdlin]

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

The netdev LED trigger calls sysfs_update_group() to refresh link_speed
attribute visibility on NETDEV_CHANGE events and device_name writes.  The
link_speed attribute group was listed in netdev_led_trigger.groups, so it
was also created and destroyed by the LED core via device_add_groups() /
device_remove_groups() in led_trigger_set(), which runs under
trigger_lock(W).  These two owners of the same group share no lock and are
not serialized against each other.

This was observed as a "cannot create duplicate filename" sysfs splat
during boot when the netdev trigger is re-armed while PHY link-up events
are being delivered concurrently:

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

Race 1: activate() vs NETDEV_CHANGE (confirmed with reproducer)

    CPU 0 (led_trigger_set)              CPU 1 (linkwatch workqueue)
    -----------------------              ---------------------------
    [holds trigger_lock(W)]
    activate():
      register_netdevice_notifier()
      <- returns
                                         netdev_trig_notify(NETDEV_CHANGE):
                                           sysfs_update_group()
                                             creates "link_10"
    device_add_groups()
      creates "link_10" <- EEXIST!

Race 2: device_remove_groups() vs NETDEV_CHANGE

    CPU 0 (led_trigger_set)              CPU 1 (linkwatch workqueue)
    -----------------------              ---------------------------
    [holds trigger_lock(W)]
    device_remove_groups()
      removes "link_10"
                                         netdev_trig_notify(NETDEV_CHANGE):
                                           sysfs_update_group()
                                             creates "link_10" <- ORPHANED
    deactivate()
      kfree(trigger_data) <- UAF via orphaned sysfs files

Serializing the trigger's sysfs_update_group() against the core under
trigger_lock is not viable: NETDEV_CHANGE is delivered from linkwatch with
rtnl_mutex held, whereas led_trigger_set() holds trigger_lock(W) and then
takes rtnl_mutex via activate() -> set_device_name().  Acquiring
trigger_lock from the NETDEV_CHANGE handler would invert that order and
deadlock.

Fix it by construction instead: drop the link_speed group from
netdev_led_trigger.groups and let the trigger own its lifecycle.  Create
it with sysfs_create_group() in activate() (before registering the
notifier) and remove it with sysfs_remove_group() in deactivate() (after
unregistering the notifier).  The LED core therefore never touches this
group, so sysfs_update_group() can no longer collide with
device_add_groups() / device_remove_groups() -- eliminating both races
above without taking trigger_lock, and hence without the rtnl_mutex
inversion.

The only remaining concurrency -- device_name_store() and
netdev_trig_notify() both calling sysfs_update_group() on the now
trigger-owned group -- stays serialized by trigger_data->lock.  Creating
the group before notifier registration and removing it after notifier
unregistration guarantees that no sysfs_update_group() can run while the
group is being created or destroyed.

Fixes: 06cdca014eca ("leds: trigger: netdev: Display only supported link speed attribute")
Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
---
 drivers/leds/trigger/ledtrig-netdev.c | 49 +++++++++++++++++++++++----
 1 file changed, 43 insertions(+), 6 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 354d3b640fa53..89b48c522c227 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -337,9 +337,10 @@ static ssize_t device_name_store(struct device *dev,
 		return ret;
 
 	/*
-	 * 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().
+	 * Refresh link_speed attribute visibility.  This trigger owns the
+	 * group (created in activate(), removed in deactivate()), so it is only
+	 * serialized against a concurrent sysfs_update_group() in
+	 * netdev_trig_notify() by trigger_data->lock.
 	 */
 	mutex_lock(&trigger_data->lock);
 	sysfs_update_group(&dev->kobj, &netdev_trig_link_speed_attrs_group);
@@ -610,9 +611,14 @@ static const struct attribute_group netdev_trig_attrs_group = {
 	.attrs = netdev_trig_attrs,
 };
 
+/*
+ * The link_speed attribute group is not listed here: it is created and
+ * destroyed by activate() / deactivate() so that its sysfs_update_group()
+ * refreshes can never race with the LED core's device_add_groups() /
+ * device_remove_groups().
+ */
 static const struct attribute_group *netdev_trig_groups[] = {
 	&netdev_trig_attrs_group,
-	&netdev_trig_link_speed_attrs_group,
 	NULL,
 };
 
@@ -660,7 +666,12 @@ static int netdev_trig_notify(struct notifier_block *nb,
 		fallthrough;
 	case NETDEV_CHANGE:
 		get_device_state(trigger_data);
-		/* Refresh link_speed visibility */
+		/*
+		 * Refresh link_speed attribute visibility.  The group is
+		 * owned by this trigger and never touched by the LED core, so
+		 * updating it here under trigger_data->lock cannot race with
+		 * device_add_groups() / device_remove_groups().
+		 */
 		if (evt == NETDEV_CHANGE)
 			sysfs_update_group(&led_cdev->dev->kobj,
 					   &netdev_trig_link_speed_attrs_group);
@@ -784,10 +795,29 @@ static int netdev_trig_activate(struct led_classdev *led_cdev)
 
 	led_set_trigger_data(led_cdev, trigger_data);
 
+	/*
+	 * Own the link_speed attribute group here instead of listing it in
+	 * netdev_led_trigger.groups, so its later sysfs_update_group() refreshes
+	 * cannot race with the LED core's device_add_groups() /
+	 * device_remove_groups().  Create it before registering the notifier so
+	 * that a NETDEV_CHANGE cannot refresh a not-yet-created group.
+	 */
+	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)
-		kfree(trigger_data);
+		goto err_remove_group;
 
+	return 0;
+
+err_remove_group:
+	sysfs_remove_group(&led_cdev->dev->kobj,
+			   &netdev_trig_link_speed_attrs_group);
+err_free:
+	kfree(trigger_data);
 	return rc;
 }
 
@@ -797,6 +827,13 @@ static void netdev_trig_deactivate(struct led_classdev *led_cdev)
 
 	unregister_netdevice_notifier(&trigger_data->notifier);
 
+	/*
+	 * The notifier is gone, so no sysfs_update_group() can run concurrently
+	 * now; tear down the group we created in activate().
+	 */
+	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 3/3] 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 the shared
trigger_data state and call set_baseline_state() without holding
trigger_data->lock, while netdev_trig_notify() and device_name_store()
do the same work under that lock. Because the store paths do not take
the lock, it provides no mutual exclusion against them.

kernfs only serializes writes to the same attribute file, so writes to
two different files run concurrently. netdev_led_attr_store() does a
non-atomic read-modify-write of ->mode, so one update can be lost:

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

The store path also races with the notifier, which updates the link
state under the lock. set_baseline_state() then programs the LED from a
half-updated snapshot, and both CPUs drive the same led_cdev at once:

  CPU0 (echo 1 > link)               CPU1 (NETDEV_CHANGE)
  -------------------------          --------------------------
                                     mutex_lock(&trigger_data->lock);
                                     carrier_link_up = false;
                                     link_speed = SPEED_UNKNOWN;
  trigger_data->mode = mode;
  set_baseline_state();
    /* reads carrier_link_up == false,
       link_speed == SPEED_UNKNOWN,
       torn intermediate state */
                                     get_device_state(); // refill
                                     set_baseline_state();
                                     mutex_unlock(&trigger_data->lock);

->hw_control is likewise written non-atomically by both the store path
and netdev_trig_notify(), and set_baseline_state() branches on it.

Hold trigger_data->lock around the read-modify-write of ->mode, the
->hw_control update and set_baseline_state() in both stores, matching
the locking already used by device_name_store() and
netdev_trig_notify(). In netdev_led_attr_store() use guard(mutex) so
the lock is released on every return path; the invalid-combination check
is performed on the locked snapshot so a rejected write does not disturb
the running configuration. cancel_delayed_work_sync() is called under
the lock; this is safe because netdev_trig_work() never acquires
trigger_data->lock.

Fixes: d5e01266e7f5 ("leds: trigger: netdev: add additional specific link speed mode")
Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
---
 drivers/leds/trigger/ledtrig-netdev.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 89b48c522c227..6556f6d28547f 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -389,7 +389,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;
 
@@ -421,6 +421,16 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
 		return -EINVAL;
 	}
 
+	/*
+	 * Serialize the read-modify-write of ->mode and the dependent
+	 * ->hw_control update and set_baseline_state() against concurrent
+	 * attribute stores and netdev_trig_notify().  netdev_trig_work() must
+	 * never take this lock, otherwise the cancel_delayed_work_sync() below
+	 * would deadlock.
+	 */
+	guard(mutex)(&trigger_data->lock);
+
+	mode = trigger_data->mode;
 	if (state)
 		set_bit(bit, &mode);
 	else
@@ -510,10 +520,14 @@ static ssize_t interval_store(struct device *dev,
 
 	/* impose some basic bounds on the timer interval */
 	if (value >= 5 && value <= 10000) {
+		mutex_lock(&trigger_data->lock);
+
 		cancel_delayed_work_sync(&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 1/3] leds: trigger: netdev: hold lock across sysfs_update_group() in device_name_store()

[sashiko-bot]

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

Re: [PATCH 2/3] leds: trigger: netdev: fix sysfs_update_group() vs led_trigger_set() races

[sashiko-bot]

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

Pre-existing issues:
- [High] Memory leak of `net_device` reference on error paths in `netdev_trig_activate()`.
--

commit 8000797f0bcd86f582cb708cea2c7a3ec07a48c4
Author: Alexander Sverdlin <alexander.sverdlin@siemens.com>

leds: trigger: netdev: fix sysfs_update_group() vs led_trigger_set() races

The netdev LED trigger is reworked to own the link_speed attribute group
lifecycle instead of letting the LED core manage it. This is achieved by
creating and removing the group in the activate and deactivate callbacks,
fixing sysfs race conditions during concurrent link-up events.

> diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> --- a/drivers/leds/trigger/ledtrig-netdev.c
> +++ b/drivers/leds/trigger/ledtrig-netdev.c

[ ... ]

> @@ -784,10 +795,29 @@ static int netdev_trig_activate(struct led_classdev *led_cdev)
>  
>  	led_set_trigger_data(led_cdev, trigger_data);
>  
> +	/*
> +	 * Own the link_speed attribute group here instead of listing it in
> +	 * netdev_led_trigger.groups, so its later sysfs_update_group() refreshes
> +	 * cannot race with the LED core's device_add_groups() /
> +	 * device_remove_groups().  Create it before registering the notifier so
> +	 * that a NETDEV_CHANGE cannot refresh a not-yet-created group.
> +	 */
> +	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)
> -		kfree(trigger_data);
> +		goto err_remove_group;
>  
> +	return 0;
> +
> +err_remove_group:
> +	sysfs_remove_group(&led_cdev->dev->kobj,
> +			   &netdev_trig_link_speed_attrs_group);
> +err_free:
> +	kfree(trigger_data);
>  	return rc;
>  }

[Severity: High]
This is a pre-existing issue, but this patch adds a new, realistically
reachable failure point (sysfs_create_group) that triggers it.

Earlier in netdev_trig_activate(), if hw control is supported, the driver
calls set_device_name() which can acquire a network device reference via
dev_get_by_name() and store it in trigger_data->net_dev.

If sysfs_create_group() or register_netdevice_notifier() subsequently fail,
the code jumps to the err_free label. It frees trigger_data but misses
calling dev_put(trigger_data->net_dev) to release the acquired reference.

Could this missing dev_put() leak the reference and cause subsequent calls to
unregister_netdevice() to hang indefinitely while waiting for the network
device to become free?

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

Re: [PATCH 3/3] leds: trigger: netdev: serialize mode/interval stores with trigger lock

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Unprotected cancel_delayed_work_sync() allows concurrent execution and UAF of net_device in the worker thread.
--

commit d9c1377958eda5198cea4d7f716e117467e638fb
Author: Alexander Sverdlin <alexander.sverdlin@siemens.com>
Subject: leds: trigger: netdev: serialize mode/interval stores with trigger lock

This patch updates netdev_led_attr_store() and interval_store() to hold
the trigger_data->lock. It prevents concurrent writes from dropping updates,
reading partially-updated device states, and causing data corruption on
shared fields.

> diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> index 89b48c522c227..6556f6d28547f 100644
> --- a/drivers/leds/trigger/ledtrig-netdev.c
> +++ b/drivers/leds/trigger/ledtrig-netdev.c
> @@ -421,6 +421,16 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
>  		return -EINVAL;
>  	}
>  
> +	/*
> +	 * Serialize the read-modify-write of ->mode and the dependent
> +	 * ->hw_control update and set_baseline_state() against concurrent
> +	 * attribute stores and netdev_trig_notify().  netdev_trig_work() must
> +	 * never take this lock, otherwise the cancel_delayed_work_sync() below
> +	 * would deadlock.
> +	 */
> +	guard(mutex)(&trigger_data->lock);
> +
> +	mode = trigger_data->mode;

[Severity: Critical]
This is a pre-existing issue, but since this patch discusses lock interactions
with cancel_delayed_work_sync(), does the unprotected call to
cancel_delayed_work_sync() in set_device_name() allow a concurrent thread
to re-arm the work before the old device reference is put?

A sysfs write to device_name triggers set_device_name(), which calls
cancel_delayed_work_sync() locklessly:

drivers/leds/trigger/ledtrig-netdev.c:set_device_name() {
    ...
    cancel_delayed_work_sync(&trigger_data->work);
    ...
    mutex_lock(&trigger_data->lock);

    dev_put(trigger_data->net_dev);
    ...
}

Concurrently, a sysfs write to a mode attribute (e.g., here in
netdev_led_attr_store()) acquires trigger_data->lock and calls
set_baseline_state(), re-arming the workqueue. The delayed work runs
netdev_trig_work() locklessly, accessing trigger_data->net_dev:

drivers/leds/trigger/ledtrig-netdev.c:netdev_trig_work() {
    ...
    dev_stats = dev_get_stats(trigger_data->net_dev, &temp);
    ...
}

Meanwhile, set_device_name() acquires the lock, assigns a new device, and
calls dev_put() on the old device. Since netdev_trig_work() doesn't hold its
own reference or any locks, this dev_put() can drop the last reference and free
the net_device while the work thread is actively calling dev_get_stats() on it.

A similar race exists in netdev_trig_notify() on the NETDEV_UNREGISTER event.

Can this sequence lead to a Use-After-Free (UAF) of the net_device structure?

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

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@siemens.com>
> 
> 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@siemens.com>
> ---
>  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

Re: [PATCH 2/3] leds: trigger: netdev: fix sysfs_update_group() vs led_trigger_set() races

[Andrew Lunn]

On Mon, Sep 14, 2026 at 03:23:51PM +0200, A. Sverdlin wrote:
> From: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> 
> The netdev LED trigger calls sysfs_update_group() to refresh link_speed
> attribute visibility on NETDEV_CHANGE events and device_name writes.  The
> link_speed attribute group was listed in netdev_led_trigger.groups, so it
> was also created and destroyed by the LED core via device_add_groups() /
> device_remove_groups() in led_trigger_set(), which runs under
> trigger_lock(W).  These two owners of the same group share no lock and are
> not serialized against each other.
> 
> This was observed as a "cannot create duplicate filename" sysfs splat
> during boot when the netdev trigger is re-armed while PHY link-up events
> are being delivered concurrently:
> 
>   sysfs: cannot create duplicate filename '...green:lan/link_10'
>   ...
>   led_trigger_set
>   led_trigger_write
> 
> Race 1: activate() vs NETDEV_CHANGE (confirmed with reproducer)
> 
>     CPU 0 (led_trigger_set)              CPU 1 (linkwatch workqueue)
>     -----------------------              ---------------------------
>     [holds trigger_lock(W)]
>     activate():
>       register_netdevice_notifier()
>       <- returns
>                                          netdev_trig_notify(NETDEV_CHANGE):
>                                            sysfs_update_group()
>                                              creates "link_10"
>     device_add_groups()
>       creates "link_10" <- EEXIST!
> 
> Race 2: device_remove_groups() vs NETDEV_CHANGE
> 
>     CPU 0 (led_trigger_set)              CPU 1 (linkwatch workqueue)
>     -----------------------              ---------------------------
>     [holds trigger_lock(W)]
>     device_remove_groups()
>       removes "link_10"
>                                          netdev_trig_notify(NETDEV_CHANGE):
>                                            sysfs_update_group()
>                                              creates "link_10" <- ORPHANED
>     deactivate()
>       kfree(trigger_data) <- UAF via orphaned sysfs files
> 
> Serializing the trigger's sysfs_update_group() against the core under
> trigger_lock is not viable: NETDEV_CHANGE is delivered from linkwatch with
> rtnl_mutex held, whereas led_trigger_set() holds trigger_lock(W) and then
> takes rtnl_mutex via activate() -> set_device_name().  Acquiring
> trigger_lock from the NETDEV_CHANGE handler would invert that order and
> deadlock.
> 
> Fix it by construction instead: drop the link_speed group from
> netdev_led_trigger.groups and let the trigger own its lifecycle.  Create
> it with sysfs_create_group() in activate() (before registering the
> notifier) and remove it with sysfs_remove_group() in deactivate() (after
> unregistering the notifier).  The LED core therefore never touches this
> group, so sysfs_update_group() can no longer collide with
> device_add_groups() / device_remove_groups() -- eliminating both races
> above without taking trigger_lock, and hence without the rtnl_mutex
> inversion.
> 
> The only remaining concurrency -- device_name_store() and
> netdev_trig_notify() both calling sysfs_update_group() on the now
> trigger-owned group -- stays serialized by trigger_data->lock.  Creating
> the group before notifier registration and removing it after notifier
> unregistration guarantees that no sysfs_update_group() can run while the
> group is being created or destroyed.
> 
> Fixes: 06cdca014eca ("leds: trigger: netdev: Display only supported link speed attribute")
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>

Please reduce the verbosity of the comments in the code.

I know locking is hard, but we don't need a verbose commit message and
verbose code.

	Andrew