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

[Lee Jones <lee@kernel.org>]

On Tue, 01 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..1d620149c071 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 == NULL || (dev && net_eq(dev_net(dev), trigger_data->net));

Doesn't looks like 'dev' can be NULL, so:

  !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 [李琼斯]