Re: [PATCH v2] leds: trigger: netdev: Match net in netdev_trig_notify()

Re: [PATCH v2] leds: trigger: netdev: Match net in netdev_trig_notify()

[Lee Jones]

Andrew et al., please could you verify for sanity?

On Mon, 07 Apr 2025, Tobias Junghans wrote:

> Different network devices may have the same device name if they are in
> different network namespaces. This confuses ledtrig-netdev and leads to
> undesired effects in certain situations.
> 
> When setting device_name to eth0, the trigger is attached to the
> corresponding (physical) network device. Later a Docker container is
> started. Docker now creates a virtual Ethernet interface (vethXXXX),
> moves it to the container's net namespace and renames it to "eth0".
> Subsequently ledtrig-netdev receives a NETDEV_CHANGENAME notification,
> recognizes "eth0" as device and and switches its activity over to this
> device. As a result the LED no longer blinks for the original (physical)
> network device.
> 
> The described erroneous behavior can be fixed by tracking and comparing
> the network namespaces of network devices.
> 
> Signed-off-by: Tobias Junghans <tobias.junghans@inhub.de>
> ---
>  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 4b0863db901a..72bcb86cdcdb 100644
> --- a/drivers/leds/trigger/ledtrig-netdev.c
> +++ b/drivers/leds/trigger/ledtrig-netdev.c
> @@ -62,6 +62,7 @@ struct led_netdev_data {
>  
>  	struct led_classdev *led_cdev;
>  	struct net_device *net_dev;
> +	struct net *net;
>  
>  	char device_name[IFNAMSIZ];
>  	atomic_t interval;
> @@ -274,6 +275,7 @@ static int set_device_name(struct led_netdev_data *trigger_data,
>  	if (trigger_data->net_dev) {
>  		dev_put(trigger_data->net_dev);
>  		trigger_data->net_dev = NULL;
> +		trigger_data->net = NULL;
>  	}
>  
>  	memcpy(trigger_data->device_name, name, size);
> @@ -284,6 +286,8 @@ static int set_device_name(struct led_netdev_data *trigger_data,
>  	if (trigger_data->device_name[0] != 0)
>  		trigger_data->net_dev =
>  		    dev_get_by_name(&init_net, trigger_data->device_name);
> +	if (trigger_data->net_dev)
> +		trigger_data->net = dev_net(trigger_data->net_dev);
>  
>  	trigger_data->carrier_link_up = false;
>  	trigger_data->link_speed = SPEED_UNKNOWN;
> @@ -573,15 +577,16 @@ static int netdev_trig_notify(struct notifier_block *nb,
>  	struct led_netdev_data *trigger_data =
>  		container_of(nb, struct led_netdev_data, notifier);
>  	struct led_classdev *led_cdev = trigger_data->led_cdev;
> +	bool same_net = !trigger_data->net || net_eq(dev_net(dev), trigger_data->net);
>  
>  	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 (!((dev == trigger_data->net_dev && same_net) ||
> +	      (evt == NETDEV_CHANGENAME && !strcmp(dev->name, trigger_data->device_name) && same_net) ||
> +	      (evt == NETDEV_REGISTER && !strcmp(dev->name, trigger_data->device_name) && same_net)))
>  		return NOTIFY_DONE;
>  
>  	cancel_delayed_work_sync(&trigger_data->work);
> @@ -597,12 +602,14 @@ static int netdev_trig_notify(struct notifier_block *nb,
>  		dev_put(trigger_data->net_dev);
>  		dev_hold(dev);
>  		trigger_data->net_dev = dev;
> +		trigger_data->net = dev_net(dev);
>  		if (evt == NETDEV_CHANGENAME)
>  			get_device_state(trigger_data);
>  		break;
>  	case NETDEV_UNREGISTER:
>  		dev_put(trigger_data->net_dev);
>  		trigger_data->net_dev = NULL;
> +		trigger_data->net = NULL;
>  		break;
>  	case NETDEV_UP:
>  	case NETDEV_CHANGE:
> @@ -702,6 +709,7 @@ static int netdev_trig_activate(struct led_classdev *led_cdev)
>  
>  	trigger_data->led_cdev = led_cdev;
>  	trigger_data->net_dev = NULL;
> +	trigger_data->net = NULL;
>  	trigger_data->device_name[0] = 0;
>  
>  	trigger_data->mode = 0;
> -- 
> 2.43.0
> 
> 

-- 
Lee Jones [李琼斯]

Re: [PATCH v2] leds: trigger: netdev: Match net in netdev_trig_notify()

[Andrew Lunn]

On Thu, Apr 10, 2025 at 11:17:59AM +0100, Lee Jones wrote:
> Andrew et al., please could you verify for sanity?

Sorry, i did not see this before.

Maybe we need a new MAINTAINER entry for
drivers/leds/triggers/ledtrig-timer.c which points to the netdev list?

Humm:

./scripts/get_maintainer.pl drivers/leds/trigger/ledtrig-netdev.c 
Lee Jones <lee@kernel.org> (maintainer:LED SUBSYSTEM,commit_signer:4/4=100%)
Pavel Machek <pavel@kernel.org> (maintainer:LED SUBSYSTEM)
Andrew Lunn <andrew@lunn.ch> (commit_signer:2/4=50%)
Marek Vasut <marex@denx.de> (commit_signer:2/4=50%,authored:2/4=50%,added_lines:15/36=42%,removed_lines:3/8=38%)
Heiner Kallweit <hkallweit1@gmail.com> (commit_signer:1/4=25%,authored:1/4=25%,removed_lines:2/8=25%)
Lukasz Majewski <lukma@denx.de> (commit_signer:1/4=25%,authored:1/4=25%,added_lines:21/36=58%,removed_lines:3/8=38%)
linux-leds@vger.kernel.org (open list:LED SUBSYSTEM)
linux-kernel@vger.kernel.org (open list)


So i should of been Cc:ed.

Network names spaces and files in /sysfs probably need netdev
involved, in order to get a good review.

> 
> On Mon, 07 Apr 2025, Tobias Junghans wrote:
> 
> > Different network devices may have the same device name if they are in
> > different network namespaces. This confuses ledtrig-netdev and leads to
> > undesired effects in certain situations.
> > 
> > When setting device_name to eth0, the trigger is attached to the
> > corresponding (physical) network device. Later a Docker container is
> > started. Docker now creates a virtual Ethernet interface (vethXXXX),
> > moves it to the container's net namespace and renames it to "eth0".
> > Subsequently ledtrig-netdev receives a NETDEV_CHANGENAME notification,
> > recognizes "eth0" as device and and switches its activity over to this
> > device. As a result the LED no longer blinks for the original (physical)
> > network device.

O.K. That make sense. So far, this has most been used in the embedded
world, which does not normally have containers etc.

> > The described erroneous behavior can be fixed by tracking and comparing
> > the network namespaces of network devices.

I think we need to go through the use cases, because i'm not sure the
netns is always obvious. When device_name_store() is called, what
netns is being referenced? Only some parts of /sysfs are netns aware.

I partially agree with tracking the network name space of
device. However, a struct net_device is unique. An LED trigger bound
to that netdev should follow that netdev as it moves between name
spaces. The trigger itself has no real concept of a name space, it is
the netdev which is in the name space. So where ever possible,
comparisons should be made based on struct netdev.

In order to make this easier to understand, it might be necessary to
break up netdev_trig_notify(). UNREGISTER, UP and CHANGE should match
based on netdev, and the name should not matter.

REGISTER is used to associate a netdev to a trigger, when all we know
is the name of the netdev, but don't yet have a pointer to it. Here i
agree we need to somehow know the netns:name tuple in order for the
match to work.

CHANGENAME falls into both camps. It could be a netdev associated to a
trigger which is changing name, so we need to follow that name
change. Or it could be a trigger which is not yet associated to a
netdev, and the new name of the netdev now matches the trigger, and so
the netdev needs to be associated to the trigger.

So i think in order to get a good understanding of what is going on,
it might be necessary to break this patch up, and have good commit
messages explaining what use cases each patch is addressing.

> > Signed-off-by: Tobias Junghans <tobias.junghans@inhub.de>
> > ---
> >  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 4b0863db901a..72bcb86cdcdb 100644
> > --- a/drivers/leds/trigger/ledtrig-netdev.c
> > +++ b/drivers/leds/trigger/ledtrig-netdev.c
> > @@ -62,6 +62,7 @@ struct led_netdev_data {
> >  
> >  	struct led_classdev *led_cdev;
> >  	struct net_device *net_dev;
> > +	struct net *net;
> >  
> >  	char device_name[IFNAMSIZ];
> >  	atomic_t interval;
> > @@ -274,6 +275,7 @@ static int set_device_name(struct led_netdev_data *trigger_data,
> >  	if (trigger_data->net_dev) {
> >  		dev_put(trigger_data->net_dev);
> >  		trigger_data->net_dev = NULL;
> > +		trigger_data->net = NULL;
> >  	}
> >  
> >  	memcpy(trigger_data->device_name, name, size);
> > @@ -284,6 +286,8 @@ static int set_device_name(struct led_netdev_data *trigger_data,
> >  	if (trigger_data->device_name[0] != 0)
> >  		trigger_data->net_dev =
> >  		    dev_get_by_name(&init_net, trigger_data->device_name);

Given we are talking about network name spaces, is the usage of
init_net correct here?

	Andrew

Re: [PATCH v2] leds: trigger: netdev: Match net in netdev_trig_notify()

[Lee Jones]

On Thu, 10 Apr 2025, Andrew Lunn wrote:

> On Thu, Apr 10, 2025 at 11:17:59AM +0100, Lee Jones wrote:
> > Andrew et al., please could you verify for sanity?
> 
> Sorry, i did not see this before.
> 
> Maybe we need a new MAINTAINER entry for
> drivers/leds/triggers/ledtrig-timer.c which points to the netdev list?

I assume you mean:

  drivers/leds/trigger/ledtrig-netdev.c

In which case, sure, I'm all for that.

Add it to your MAINTAINERS entry and I will Ack it.

> Humm:
> 
> ./scripts/get_maintainer.pl drivers/leds/trigger/ledtrig-netdev.c 
> Lee Jones <lee@kernel.org> (maintainer:LED SUBSYSTEM,commit_signer:4/4=100%)
> Pavel Machek <pavel@kernel.org> (maintainer:LED SUBSYSTEM)
> Andrew Lunn <andrew@lunn.ch> (commit_signer:2/4=50%)
> Marek Vasut <marex@denx.de> (commit_signer:2/4=50%,authored:2/4=50%,added_lines:15/36=42%,removed_lines:3/8=38%)
> Heiner Kallweit <hkallweit1@gmail.com> (commit_signer:1/4=25%,authored:1/4=25%,removed_lines:2/8=25%)
> Lukasz Majewski <lukma@denx.de> (commit_signer:1/4=25%,authored:1/4=25%,added_lines:21/36=58%,removed_lines:3/8=38%)
> linux-leds@vger.kernel.org (open list:LED SUBSYSTEM)
> linux-kernel@vger.kernel.org (open list)
> 
> So i should of been Cc:ed.

To be fair to the contributor, I don't always add commit signers either.

> Network names spaces and files in /sysfs probably need netdev
> involved, in order to get a good review.

Definitely.

-- 
Lee Jones [李琼斯]

[PATCH v2] leds: trigger: netdev: refactor dev matching in netdev_trig_notify()

[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 return early with
NOTIFY_DONE inside netdev_trig_notify():

- 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 4b0863db901a..75d8c8fe9afc 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -574,15 +574,28 @@ 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))))
+	switch (evt) {
+	case NETDEV_REGISTER:
+		if (trigger_data->net_dev ||
+		    strcmp(dev->name, trigger_data->device_name))
+			return NOTIFY_DONE;
+		break;
+	case NETDEV_CHANGENAME:
+		if (trigger_data->net_dev != dev &&
+		    (trigger_data->net_dev ||
+		     strcmp(dev->name, trigger_data->device_name)))
+			return NOTIFY_DONE;
+		break;
+	case NETDEV_UNREGISTER:
+	case NETDEV_UP:
+	case NETDEV_DOWN:
+	case NETDEV_CHANGE:
+		if (trigger_data->net_dev != dev)
+			return NOTIFY_DONE;
+		break;
+	default:
 		return NOTIFY_DONE;
+	}
 
 	cancel_delayed_work_sync(&trigger_data->work);
 
-- 
2.43.0

Re: [PATCH v2] leds: trigger: netdev: refactor dev matching in netdev_trig_notify()

[Lee Jones]

On Wed, 23 Apr 2025, 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 return early with
> NOTIFY_DONE inside netdev_trig_notify():
> 
> - 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(-)

Please send subsequent versions independently, rather than attached to
the end of lengthy (or any for that matter) mail threads.  This gets
confusing, fast!

Also, this is v3.

Please resubmit.

-- 
Lee Jones [李琼斯]