netdev - Re: [PATCH v2] leds: trigger: netdev: Match net in netdev_trig

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <45822bb5-a408-42c1-85b7-e179789d586a@lunn.ch>
Date: Thu, 10 Apr 2025 22:39:38 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Lee Jones <lee@...nel.org>
Cc: Tobias Junghans <tobias.junghans@...ub.de>,
	Andrew Lunn <andrew+netdev@...n.ch>, linux-leds@...r.kernel.org,
	netdev@...r.kernel.org
Subject: Re: [PATCH v2] leds: trigger: netdev: Match net in
 netdev_trig_notify()

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@...nel.org> (maintainer:LED SUBSYSTEM,commit_signer:4/4=100%)
Pavel Machek <pavel@...nel.org> (maintainer:LED SUBSYSTEM)
Andrew Lunn <andrew@...n.ch> (commit_signer:2/4=50%)
Marek Vasut <marex@...x.de> (commit_signer:2/4=50%,authored:2/4=50%,added_lines:15/36=42%,removed_lines:3/8=38%)
Heiner Kallweit <hkallweit1@...il.com> (commit_signer:1/4=25%,authored:1/4=25%,removed_lines:2/8=25%)
Lukasz Majewski <lukma@...x.de> (commit_signer:1/4=25%,authored:1/4=25%,added_lines:21/36=58%,removed_lines:3/8=38%)
linux-leds@...r.kernel.org (open list:LED SUBSYSTEM)
linux-kernel@...r.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@...ub.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