| Message ID | 20250407090455.677846-1-tobias.junghans@inhub.de |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] leds: trigger: netdev: Match net in netdev_trig_notify() | expand |
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 > >
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
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.
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;
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(-)