[PATCH 0/2] leds: trigger: netdev: refactor event handling

[PATCH 0/2] leds: trigger: netdev: refactor event handling

[Tobias Junghans]

This series fixes issues in the netdev event handling code which lead to
erroneous netdev associations if e.g. net devices in different net
namespaces have the same name before or after being renamed.

The series based on different previous approaches:
- https://lore.kernel.org/linux-leds/20250401085002.492904-1-tobias.junghans@inhub.de/
- https://lore.kernel.org/linux-leds/20250425132059.393144-1-tobias.junghans@inhub.de/

Signed-off-by: Tobias Junghans <tobias.junghans@inhub.de>

---
Tobias Junghans (2):
  leds: trigger: netdev: separate event checks
  leds: trigger: netdev: refactor netdev_event_requires_handling()

 drivers/leds/trigger/ledtrig-netdev.c | 38 +++++++++++++++++++++------
 1 file changed, 30 insertions(+), 8 deletions(-)

-- 
2.43.0

[PATCH 1/2] leds: trigger: netdev: separate event checks

[Tobias Junghans]

Move event handling checks into a separate function to ease subsequent
refactorings of the actual logic.

Signed-off-by: Tobias Junghans <tobias.junghans@inhub.de>
---
 drivers/leds/trigger/ledtrig-netdev.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 4e048e08c4fd..6e16619719fe 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -575,6 +575,22 @@ static const struct attribute_group *netdev_trig_groups[] = {
 	NULL,
 };
 
+static bool netdev_event_requires_handling(unsigned long evt, struct net_device *dev,
+					   struct led_netdev_data *trigger_data)
+{
+	if (evt != NETDEV_UP && evt != NETDEV_DOWN && evt != NETDEV_CHANGE
+	    && evt != NETDEV_REGISTER && evt != NETDEV_UNREGISTER
+	    && evt != NETDEV_CHANGENAME)
+		return false;
+
+	if (!(dev == trigger_data->net_dev ||
+	     (evt == NETDEV_CHANGENAME && !strcmp(dev->name, trigger_data->device_name)) ||
+	     (evt == NETDEV_REGISTER && !strcmp(dev->name, trigger_data->device_name))))
+		return false;
+
+	return true;
+}
+
 static int netdev_trig_notify(struct notifier_block *nb,
 			      unsigned long evt, void *dv)
 {
@@ -584,14 +600,7 @@ static int netdev_trig_notify(struct notifier_block *nb,
 		container_of(nb, struct led_netdev_data, notifier);
 	struct led_classdev *led_cdev = trigger_data->led_cdev;
 
-	if (evt != NETDEV_UP && evt != NETDEV_DOWN && evt != NETDEV_CHANGE
-	    && evt != NETDEV_REGISTER && evt != NETDEV_UNREGISTER
-	    && evt != NETDEV_CHANGENAME)
-		return NOTIFY_DONE;
-
-	if (!(dev == trigger_data->net_dev ||
-	      (evt == NETDEV_CHANGENAME && !strcmp(dev->name, trigger_data->device_name)) ||
-	      (evt == NETDEV_REGISTER && !strcmp(dev->name, trigger_data->device_name))))
+	if (!netdev_event_requires_handling(evt, dev, trigger_data))
 		return NOTIFY_DONE;
 
 	cancel_delayed_work_sync(&trigger_data->work);
-- 
2.43.0

[PATCH 2/2] leds: trigger: netdev: refactor netdev_event_requires_handling()

[Tobias Junghans]

If there are network devices with the same name in different
namespaces, ledtrig-netdev gets confused easily and switches between
these devices whenever there are NETDEV_CHANGENAME/NETDEV_REGISTER
notifications.  This happens since ledtrig-netdev only checks for
device name equality regardless of previous associations with another
network device with the same name.

Real world example: eth0 is the primary physical network interface and
ledltrig-netdev is associated with that interface. If now Docker creates
a virtual Ethernet interface (vethXXXX), moves it to the
container's net namespace and renames it to eth0, ledtrig-netdev
switches to this device and the LED no longer blinks for the original
(physical) network device.

Fix this by refactoring the conditions under which to handle netdev
events:

- For processing NETDEV_REGISTER events, the device name has to match
  and no association with a net_dev must exist.

- For processing NETDEV_CHANGENAME events, the associated and notified
  network device have to match. Alternatively the device name has to
  match and no association with a net_dev must exist.

- For all other events, the associated and notified network device have
  to match.

Signed-off-by: Tobias Junghans <tobias.junghans@inhub.de>
---
 drivers/leds/trigger/ledtrig-netdev.c | 29 +++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 6e16619719fe..a3f30e6f74b4 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -578,15 +578,28 @@ static const struct attribute_group *netdev_trig_groups[] = {
 static bool netdev_event_requires_handling(unsigned long evt, struct net_device *dev,
 					   struct led_netdev_data *trigger_data)
 {
-	if (evt != NETDEV_UP && evt != NETDEV_DOWN && evt != NETDEV_CHANGE
-	    && evt != NETDEV_REGISTER && evt != NETDEV_UNREGISTER
-	    && evt != NETDEV_CHANGENAME)
-		return false;
-
-	if (!(dev == trigger_data->net_dev ||
-	     (evt == NETDEV_CHANGENAME && !strcmp(dev->name, trigger_data->device_name)) ||
-	     (evt == NETDEV_REGISTER && !strcmp(dev->name, trigger_data->device_name))))
+	switch (evt) {
+	case NETDEV_REGISTER:
+		if (trigger_data->net_dev ||
+		    strcmp(dev->name, trigger_data->device_name))
+			return false;
+		break;
+	case NETDEV_CHANGENAME:
+		if (trigger_data->net_dev != dev &&
+		    (trigger_data->net_dev ||
+		     strcmp(dev->name, trigger_data->device_name)))
+			return false;
+		break;
+	case NETDEV_UNREGISTER:
+	case NETDEV_UP:
+	case NETDEV_DOWN:
+	case NETDEV_CHANGE:
+		if (trigger_data->net_dev != dev)
+			return false;
+		break;
+	default:
 		return false;
+	}
 
 	return true;
 }
-- 
2.43.0

Re: [PATCH 2/2] leds: trigger: netdev: refactor netdev_event_requires_handling()

[Andrew Lunn]

On Tue, Jun 10, 2025 at 01:40:20PM +0200, Tobias Junghans wrote:
> If there are network devices with the same name in different
> namespaces, ledtrig-netdev gets confused easily and switches between
> these devices whenever there are NETDEV_CHANGENAME/NETDEV_REGISTER
> notifications.  This happens since ledtrig-netdev only checks for
> device name equality regardless of previous associations with another
> network device with the same name.
> 
> Real world example: eth0 is the primary physical network interface and
> ledltrig-netdev is associated with that interface. If now Docker creates
> a virtual Ethernet interface (vethXXXX), moves it to the
> container's net namespace and renames it to eth0, ledtrig-netdev
> switches to this device and the LED no longer blinks for the original
> (physical) network device.
> 
> Fix this by refactoring the conditions under which to handle netdev
> events:
> 
> - For processing NETDEV_REGISTER events, the device name has to match
>   and no association with a net_dev must exist.

Sorry for taking a while to review this. It took me a while to think
it through and produce a list of use cases. And might still be missing
some.

Given the complexity here, i actually think we need verbose comments
in the code.

> - For processing NETDEV_CHANGENAME events, the associated and notified
>   network device have to match. Alternatively the device name has to
>   match and no association with a net_dev must exist.
> 
> - For all other events, the associated and notified network device have
>   to match.
> 
> Signed-off-by: Tobias Junghans <tobias.junghans@inhub.de>
> ---
>  drivers/leds/trigger/ledtrig-netdev.c | 29 +++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> index 6e16619719fe..a3f30e6f74b4 100644
> --- a/drivers/leds/trigger/ledtrig-netdev.c
> +++ b/drivers/leds/trigger/ledtrig-netdev.c
> @@ -578,15 +578,28 @@ static const struct attribute_group *netdev_trig_groups[] = {
>  static bool netdev_event_requires_handling(unsigned long evt, struct net_device *dev,
>  					   struct led_netdev_data *trigger_data)
>  {
> -	if (evt != NETDEV_UP && evt != NETDEV_DOWN && evt != NETDEV_CHANGE
> -	    && evt != NETDEV_REGISTER && evt != NETDEV_UNREGISTER
> -	    && evt != NETDEV_CHANGENAME)
> -		return false;
> -
> -	if (!(dev == trigger_data->net_dev ||
> -	     (evt == NETDEV_CHANGENAME && !strcmp(dev->name, trigger_data->device_name)) ||
> -	     (evt == NETDEV_REGISTER && !strcmp(dev->name, trigger_data->device_name))))

At the beginning of this function i would put some background.

/* /sys/class/leds is unaware of network names spaces. All LEDs will
   be visible in all network name spaces. However a network interface
   is only in one network name space. Interface names are unique
   within a network name space, but not across name spaces. The same
   name can appear in multiple name spaces. However the interface dev
   pointer is unique, and does not change when an interface moves
   between namespaces.

   LEDs can either support software blinking, hardware blinking for a
   netdev, or both. When the trigger is activated on an LED which
   supports hardware blinking for a netdev, a request will be made to
   get the struct device it blinks for. This may not be available,
   e.g. the PHY has not been associated to a MAC yet. However, if the
   device is known, the trigger is associated to the device.
*/

Right, in the process of writing this, i found another issue:

netdev_trig_activate() calls led_cdev->hw_control_get_device() to get
the device this LED can hardware blink for. From the device it gets
the name, and calls set_device_name(). set_device_name() then uses:
dev_get_by_name(&init_net, trigger_data->device_name) to convert the
name into a netdev. However, it is looking in the base network name
space, and there is no guarantee that it is where the device actually
is! We should actually use to_net_dev(dev) to get the struct
net_device, and pass that to set_device_name(), or a variant of it.

continuing...

/* If a hardware blinking LED cannot be associated to a device, its
   device name can be set the same as for a software blinking LED, via
   /sys/class/led/<LED>/device_name, once the device actually exists.
   An interface of that name will be found and associated to the
   trigger.
*/

And here is the next issue. It looks for the device in init_net, the
base network namespace. But if i've done something like:

$ ip netns exec n42 bash
$ echo eth24 > /sys/class/led/LED-wan/device_name

we want it to find eth24 in network name space n42.
device_name_store() has to be called in process context, and that
process must have a network namespace associated to it. But i've no
idea how you get to it, this is not something i've done before....

But i think set_device_name() needs to change, and not be hard coded
for init_net. We need to keep the netns in the trigger_data structure.

> +	switch (evt) {
> +	case NETDEV_REGISTER:

So, register...

	/* 1. The name and netns match to those set by
              /sys/class/led/.../device_name. Associate the device to
	      the trigger.

           2. The device was previously associated to this
	      trigger. The device has moved from one netns to another.
	      Reassociate the device to the trigger. The netns in trigger_data
	      needs updating, and the device might of changed name
	*/

> +		if (trigger_data->net_dev ||
> +		    strcmp(dev->name, trigger_data->device_name))
> +			return false;
> +		break;
> +	case NETDEV_CHANGENAME:

	/* 1. The device is associated to this trigger_data. Update
	      trigger_data->device_name with the new name.

           2. The trigger has no device associated to it. The new name
              of the device and its netns match those set via 
              /sys/class/led/.../device_name. Associate the device to
	      the trigger.
        */

> +		if (trigger_data->net_dev != dev &&
> +		    (trigger_data->net_dev ||
> +		     strcmp(dev->name, trigger_data->device_name)))
> +			return false;
> +		break;
> +	case NETDEV_UNREGISTER:

        /* Disconnect the device from the trigger. However, ensure it can be
	   reassociated if the device appears in a different namespace.

> +	case NETDEV_UP:
        /* When the interface was registered, it might not of had a PHY 
	   attached, so check if the PHYs LEDs support hardware blinking.
        */
> +	case NETDEV_DOWN:
> +	case NETDEV_CHANGE:

        /* Update LED state, including the link speed. */
> +		if (trigger_data->net_dev != dev)
> +			return false;
> +		break;

I'm not sure you can have a simple bool needs further
processing/ignore this event. When you get into the details, i think
you will find you will need to take actions depending on various
conditions.

Tomorrow i will look at the socket system calls and see how you get a
processes network namespace.

	Andrew

Re: [PATCH 2/2] leds: trigger: netdev: refactor netdev_event_requires_handling()

[Andrew Lunn]

> Tomorrow i will look at the socket system calls and see how you get a
> processes network namespace.

current->nsproxy->net_ns gives the current processes network
namespace.

current is one of those historical odd things, like
jiffies. Originally it was a global variable, when the kernel
supported a single CPU, and hence only a single processes was ever
running. But with the change to SMP it became a #define for a function
call.

	Andrew

Re: [PATCH 2/2] leds: trigger: netdev: refactor netdev_event_requires_handling()

[Lee Jones]

On Thu, 12 Jun 2025, Andrew Lunn wrote:

> On Tue, Jun 10, 2025 at 01:40:20PM +0200, Tobias Junghans wrote:
> > If there are network devices with the same name in different
> > namespaces, ledtrig-netdev gets confused easily and switches between
> > these devices whenever there are NETDEV_CHANGENAME/NETDEV_REGISTER
> > notifications.  This happens since ledtrig-netdev only checks for
> > device name equality regardless of previous associations with another
> > network device with the same name.
> > 
> > Real world example: eth0 is the primary physical network interface and
> > ledltrig-netdev is associated with that interface. If now Docker creates
> > a virtual Ethernet interface (vethXXXX), moves it to the
> > container's net namespace and renames it to eth0, ledtrig-netdev
> > switches to this device and the LED no longer blinks for the original
> > (physical) network device.
> > 
> > Fix this by refactoring the conditions under which to handle netdev
> > events:
> > 
> > - For processing NETDEV_REGISTER events, the device name has to match
> >   and no association with a net_dev must exist.
> 
> Sorry for taking a while to review this. It took me a while to think
> it through and produce a list of use cases. And might still be missing
> some.
> 
> Given the complexity here, i actually think we need verbose comments
> in the code.

Thanks Andrew - I'll leave it with you.   =:-)

[...]

-- 
Lee Jones [李琼斯]