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

[PATCH v3 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:
v3:
- introduced unset_device_name() as a counterpart of set_device_name()
- guard() has been converted to either explicit mutex_lock()/mutex_unlock()
  or scoped_guard()
- rebased onto next-20260925
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)
- https://lore.kernel.org/all/20260921093249.3481413-1-alexander.sverdlin@siemens.com/
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 | 112 +++++++++++++++++++-------
 1 file changed, 81 insertions(+), 31 deletions(-)

-- 
2.55.0

[PATCH v3 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:
v3:
- introduced unset_device_name() as a counterpart of set_device_name()

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

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 5b0132484594c..e2a6c95a0dc97 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -324,6 +324,13 @@ static int set_device_name(struct led_netdev_data *trigger_data,
 	return 0;
 }
 
+static void unset_device_name(struct led_netdev_data *trigger_data)
+{
+	dev_put(trigger_data->net_dev);
+	trigger_data->net_dev = NULL;
+	trigger_data->device_name[0] = 0;
+}
+
 static ssize_t device_name_store(struct device *dev,
 				 struct device_attribute *attr, const char *buf,
 				 size_t size)
@@ -776,8 +783,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) {
+		unset_device_name(trigger_data);
 		kfree(trigger_data);
+	}
 
 	return rc;
 }
@@ -790,7 +799,7 @@ static void netdev_trig_deactivate(struct led_classdev *led_cdev)
 
 	cancel_delayed_work_sync(&trigger_data->work);
 
-	dev_put(trigger_data->net_dev);
+	unset_device_name(trigger_data);
 
 	kfree(trigger_data);
 }
-- 
2.55.0

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

[sashiko-bot]

> 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>

Sashiko has reviewed this patch and found no issues. It looks great!

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

[PATCH v3 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:
v3:
- explicit mutex_lock()/mutex_unlock() instead of guard()

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

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index e2a6c95a0dc97..d62a7b0168523 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -683,9 +683,12 @@ static void netdev_trig_work(struct work_struct *work)
 	unsigned long interval;
 	int invert;
 
+	mutex_lock(&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);
+		mutex_unlock(&trigger_data->lock);
 		return;
 	}
 
@@ -693,8 +696,10 @@ static void netdev_trig_work(struct work_struct *work)
 	if (!test_bit(TRIGGER_NETDEV_TX, &trigger_data->mode) &&
 	    !test_bit(TRIGGER_NETDEV_RX, &trigger_data->mode) &&
 	    !test_bit(TRIGGER_NETDEV_TX_ERR, &trigger_data->mode) &&
-	    !test_bit(TRIGGER_NETDEV_RX_ERR, &trigger_data->mode))
+	    !test_bit(TRIGGER_NETDEV_RX_ERR, &trigger_data->mode)) {
+		mutex_unlock(&trigger_data->lock);
 		return;
+	}
 
 	dev_stats = dev_get_stats(trigger_data->net_dev, &temp);
 	new_activity =
@@ -735,6 +740,8 @@ static void netdev_trig_work(struct work_struct *work)
 
 	schedule_delayed_work(&trigger_data->work,
 			(atomic_read(&trigger_data->interval)*2));
+
+	mutex_unlock(&trigger_data->lock);
 }
 
 static int netdev_trig_activate(struct led_classdev *led_cdev)
-- 
2.55.0

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

[sashiko-bot]

> 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>

Sashiko has reviewed this patch and found no issues. It looks great!

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

[PATCH v3 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:
v3:
- changed guard() to scoped_guard() and dev_put() to unset_device_name()
v2:
- this patch is a combined rework of patches 1&2 from v1

 drivers/leds/trigger/ledtrig-netdev.c | 74 +++++++++++++++++----------
 1 file changed, 47 insertions(+), 27 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index d62a7b0168523..b27a453bbb859 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;
@@ -343,8 +345,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;
 }
@@ -551,22 +554,23 @@ 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.
 	 */
-	for_each_set_bit(mode, supported_link_modes, __ETHTOOL_LINK_MODE_MASK_NBITS) {
-		struct ethtool_link_ksettings link_ksettings;
-
-		ethtool_params_from_link_mode(&link_ksettings, mode);
-
-		CHECK_LINK_MODE_ATTR(10);
-		CHECK_LINK_MODE_ATTR(100);
-		CHECK_LINK_MODE_ATTR(1000);
-		CHECK_LINK_MODE_ATTR(2500);
-		CHECK_LINK_MODE_ATTR(5000);
-		CHECK_LINK_MODE_ATTR(10000);
-		CHECK_LINK_MODE_ATTR(25000);
-		CHECK_LINK_MODE_ATTR(40000);
-		CHECK_LINK_MODE_ATTR(50000);
-		CHECK_LINK_MODE_ATTR(100000);
-	}
+	scoped_guard(mutex, &trigger_data->lock)
+		for_each_set_bit(mode, supported_link_modes, __ETHTOOL_LINK_MODE_MASK_NBITS) {
+			struct ethtool_link_ksettings link_ksettings;
+
+			ethtool_params_from_link_mode(&link_ksettings, mode);
+
+			CHECK_LINK_MODE_ATTR(10);
+			CHECK_LINK_MODE_ATTR(100);
+			CHECK_LINK_MODE_ATTR(1000);
+			CHECK_LINK_MODE_ATTR(2500);
+			CHECK_LINK_MODE_ATTR(5000);
+			CHECK_LINK_MODE_ATTR(10000);
+			CHECK_LINK_MODE_ATTR(25000);
+			CHECK_LINK_MODE_ATTR(40000);
+			CHECK_LINK_MODE_ATTR(50000);
+			CHECK_LINK_MODE_ATTR(100000);
+		}
 
 	return 0;
 }
@@ -610,7 +614,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,
 };
 
@@ -658,10 +661,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;
 	}
 
@@ -669,6 +668,12 @@ static int netdev_trig_notify(struct notifier_block *nb,
 
 	mutex_unlock(&trigger_data->lock);
 
+	if (evt == NETDEV_CHANGE) {
+		scoped_guard(mutex, &trigger_data->attr_lock)
+			sysfs_update_group(&led_cdev->dev->kobj,
+					   &netdev_trig_link_speed_attrs_group);
+	}
+
 	return NOTIFY_DONE;
 }
 
@@ -756,6 +761,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;
@@ -789,12 +795,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) {
-		unset_device_name(trigger_data);
-		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:
+	unset_device_name(trigger_data);
+	kfree(trigger_data);
 	return rc;
 }
 
@@ -804,6 +821,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);
 
 	unset_device_name(trigger_data);
-- 
2.55.0

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

[sashiko-bot]

> 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()
> [ ... ]
> 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>

Sashiko has reviewed this patch and found no issues. It looks great!

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

[PATCH v3 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:
v3:
- explicit mutex_lock()/mutex_unlock() instead of guard()
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 | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index b27a453bbb859..7ad61c4be282e 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -392,7 +392,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;
 
@@ -424,6 +424,10 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
 		return -EINVAL;
 	}
 
+	/* async cancel: the worker takes this lock */
+	mutex_lock(&trigger_data->lock);
+
+	mode = trigger_data->mode;
 	assign_bit(bit, &mode, state);
 
 	if (test_bit(TRIGGER_NETDEV_LINK, &mode) &&
@@ -436,20 +440,26 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
 	     test_bit(TRIGGER_NETDEV_LINK_25000, &mode) ||
 	     test_bit(TRIGGER_NETDEV_LINK_40000, &mode) ||
 	     test_bit(TRIGGER_NETDEV_LINK_50000, &mode) ||
-	     test_bit(TRIGGER_NETDEV_LINK_100000, &mode)))
+	     test_bit(TRIGGER_NETDEV_LINK_100000, &mode))) {
+		mutex_unlock(&trigger_data->lock);
 		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);
 
 	if (!led_cdev->brightness_set && !led_cdev->brightness_set_blocking &&
-	    !trigger_data->hw_control)
+	    !trigger_data->hw_control) {
+		mutex_unlock(&trigger_data->lock);
 		return -EOPNOTSUPP;
+	}
 
 	set_baseline_state(trigger_data);
 
+	mutex_unlock(&trigger_data->lock);
+
 	return size;
 }
 
@@ -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) {
-		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 v3 4/4] 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:
- [Medium] leds: netdev: async work cancel allows zombie worker to overwrite hw offload
--

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

leds: trigger: netdev: Serialize mode/interval stores with trigger lock

This commit adds explicit mutex locking to serialize mode and interval stores.
It replaces the synchronous delayed work cancellation with an asynchronous
cancellation while holding the trigger lock.

> diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> index b27a453bbb859..7ad61c4be282e 100644
> --- a/drivers/leds/trigger/ledtrig-netdev.c
> +++ b/drivers/leds/trigger/ledtrig-netdev.c
[ ... ]
> @@ -424,6 +424,10 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
>  		return -EINVAL;
>  	}
>  
> +	/* async cancel: the worker takes this lock */
> +	mutex_lock(&trigger_data->lock);
> +
> +	mode = trigger_data->mode;
>  	assign_bit(bit, &mode, state);
>  
>  	if (test_bit(TRIGGER_NETDEV_LINK, &mode) &&
> @@ -436,20 +440,26 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
>  	     test_bit(TRIGGER_NETDEV_LINK_25000, &mode) ||
>  	     test_bit(TRIGGER_NETDEV_LINK_40000, &mode) ||
>  	     test_bit(TRIGGER_NETDEV_LINK_50000, &mode) ||
> -	     test_bit(TRIGGER_NETDEV_LINK_100000, &mode)))
> +	     test_bit(TRIGGER_NETDEV_LINK_100000, &mode))) {
> +		mutex_unlock(&trigger_data->lock);
>  		return -EINVAL;
> +	}
>  
> -	cancel_delayed_work_sync(&trigger_data->work);
> +	cancel_delayed_work(&trigger_data->work);

[Severity: Medium]
If the worker netdev_trig_work() is concurrently executing and blocked on
trigger_data->lock, will this asynchronous cancel fail to stop it?

Once netdev_led_attr_store() finishes configuring hardware offload and
releases the lock, could the worker unblock and continue to execute?

If it does, the worker doesn't appear to check trigger_data->hw_control
before falling back to software blinking and rescheduling itself:

drivers/leds/trigger/ledtrig-netdev.c:netdev_trig_work() {
	...
	/* If we are not looking for RX/TX then return  */
	if (!test_bit(TRIGGER_NETDEV_TX, &trigger_data->mode) &&
	    !test_bit(TRIGGER_NETDEV_RX, &trigger_data->mode) &&
	    !test_bit(TRIGGER_NETDEV_TX_ERR, &trigger_data->mode) &&
	    !test_bit(TRIGGER_NETDEV_RX_ERR, &trigger_data->mode)) {
		mutex_unlock(&trigger_data->lock);
		return;
	}
	...
	/* no check for hw_control */
	...
		led_blink_set_oneshot(trigger_data->led_cdev, ...);
		trigger_data->last_activity = new_activity;
	}

	schedule_delayed_work(&trigger_data->work, ...);

	mutex_unlock(&trigger_data->lock);
}

Will this cause the zombie worker to permanently overwrite the hardware
offload state configured here and waste CPU by continuously rescheduling
itself?

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

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

[Sverdlin, Alexander]

Hi all,

On Tue, 2026-09-29 at 12:19 +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] leds: netdev: async work cancel allows zombie worker to overwrite hw offload
> --
> 
> commit d51dca8c4abbb112c1f8e31383d3f2334d50c585
> Author: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> 
> leds: trigger: netdev: Serialize mode/interval stores with trigger lock
> 
> This commit adds explicit mutex locking to serialize mode and interval stores.
> It replaces the synchronous delayed work cancellation with an asynchronous
> cancellation while holding the trigger lock.
> 
> > diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> > index b27a453bbb859..7ad61c4be282e 100644
> > --- a/drivers/leds/trigger/ledtrig-netdev.c
> > +++ b/drivers/leds/trigger/ledtrig-netdev.c
> [ ... ]
> > @@ -424,6 +424,10 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
> >   		return -EINVAL;
> >   	}
> >   
> > +	/* async cancel: the worker takes this lock */
> > +	mutex_lock(&trigger_data->lock);
> > +
> > +	mode = trigger_data->mode;
> >   	assign_bit(bit, &mode, state);
> >   
> >   	if (test_bit(TRIGGER_NETDEV_LINK, &mode) &&
> > @@ -436,20 +440,26 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
> >   	     test_bit(TRIGGER_NETDEV_LINK_25000, &mode) ||
> >   	     test_bit(TRIGGER_NETDEV_LINK_40000, &mode) ||
> >   	     test_bit(TRIGGER_NETDEV_LINK_50000, &mode) ||
> > -	     test_bit(TRIGGER_NETDEV_LINK_100000, &mode)))
> > +	     test_bit(TRIGGER_NETDEV_LINK_100000, &mode))) {
> > +		mutex_unlock(&trigger_data->lock);
> >   		return -EINVAL;
> > +	}
> >   
> > -	cancel_delayed_work_sync(&trigger_data->work);
> > +	cancel_delayed_work(&trigger_data->work);
> 
> [Severity: Medium]
> If the worker netdev_trig_work() is concurrently executing and blocked on
> trigger_data->lock, will this asynchronous cancel fail to stop it?
> 
> Once netdev_led_attr_store() finishes configuring hardware offload and
> releases the lock, could the worker unblock and continue to execute?
> 
> If it does, the worker doesn't appear to check trigger_data->hw_control
> before falling back to software blinking and rescheduling itself:
> 
> drivers/leds/trigger/ledtrig-netdev.c:netdev_trig_work() {
> 	...
> 	/* If we are not looking for RX/TX then return  */
> 	if (!test_bit(TRIGGER_NETDEV_TX, &trigger_data->mode) &&
> 	    !test_bit(TRIGGER_NETDEV_RX, &trigger_data->mode) &&
> 	    !test_bit(TRIGGER_NETDEV_TX_ERR, &trigger_data->mode) &&
> 	    !test_bit(TRIGGER_NETDEV_RX_ERR, &trigger_data->mode)) {
> 		mutex_unlock(&trigger_data->lock);
> 		return;
> 	}
> 	...
> 	/* no check for hw_control */
> 	...
> 		led_blink_set_oneshot(trigger_data->led_cdev, ...);
> 		trigger_data->last_activity = new_activity;
> 	}
> 
> 	schedule_delayed_work(&trigger_data->work, ...);
> 
> 	mutex_unlock(&trigger_data->lock);
> }
> 
> Will this cause the zombie worker to permanently overwrite the hardware
> offload state configured here and waste CPU by continuously rescheduling
> itself?

Sashiko is right here, this requires re-work.

pw-bot: cr

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com