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

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.

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