Re: [PATCH] leds: trigger: netdev: fix refcnt leak on interface rename

[Jacek Anaszewski <jacek.anaszewski@xxxxxxxxx>]

Hi Rafał,

Thank you for the patch.

On 2/28/19 10:57 PM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@xxxxxxxxxx>
>
> Renaming a netdev-trigger-tracked interface was resulting in an
> unbalanced dev_hold().
>
> Example:
> > iw phy phy0 interface add foo type __ap
> > echo netdev > trigger
> > echo foo > device_name
> > ip link set foo name bar
> > iw dev bar del
> [  237.355366] unregister_netdevice: waiting for bar to become free. Usage count = 1
> [  247.435362] unregister_netdevice: waiting for bar to become free. Usage count = 1
> [  257.545366] unregister_netdevice: waiting for bar to become free. Usage count = 1
>
> Above problem was caused by trigger checking a dev->name which obviously
> changes after renaming an interface. It meant missing all further events
> including the NETDEV_UNREGISTER which is required for calling dev_put().
>
> This change fixes that by:
> 1) Comparing device struct *address* for notification-filtering purposes
> 2) Dropping unneeded NETDEV_CHANGENAME code (no behavior change)
>
> Fixes: 06f502f57d0d ("leds: trigger: Introduce a NETDEV trigger")
> Signed-off-by: Rafał Miłecki <rafal@xxxxxxxxxx>
> ---
>   drivers/leds/trigger/ledtrig-netdev.c | 13 +++++--------
>   1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> index 3dd3ed46d473..167a94c02d05 100644
> --- a/drivers/leds/trigger/ledtrig-netdev.c
> +++ b/drivers/leds/trigger/ledtrig-netdev.c
> @@ -301,11 +301,11 @@ static int netdev_trig_notify(struct notifier_block *nb,
>   		container_of(nb, struct led_netdev_data, notifier);
>   
>   	if (evt != NETDEV_UP && evt != NETDEV_DOWN && evt != NETDEV_CHANGE
> -	    && evt != NETDEV_REGISTER && evt != NETDEV_UNREGISTER
> -	    && evt != NETDEV_CHANGENAME)
> +	    && evt != NETDEV_REGISTER && evt != NETDEV_UNREGISTER)
>   		return NOTIFY_DONE;
>   
> -	if (strcmp(dev->name, trigger_data->device_name))
> +	if (!(dev == trigger_data->net_dev ||
> +	      (evt == NETDEV_REGISTER && !strcmp(dev->name, trigger_data->device_name))))
>   		return NOTIFY_DONE;
>   
>   	cancel_delayed_work_sync(&trigger_data->work);
> @@ -320,12 +320,9 @@ static int netdev_trig_notify(struct notifier_block *nb,
>   		dev_hold(dev);
>   		trigger_data->net_dev = dev;
>   		break;
> -	case NETDEV_CHANGENAME:
>   	case NETDEV_UNREGISTER:
> -		if (trigger_data->net_dev) {
> -			dev_put(trigger_data->net_dev);
> -			trigger_data->net_dev = NULL;
> -		}
> +		dev_put(trigger_data->net_dev);
> +		trigger_data->net_dev = NULL;
>   		break;
>   	case NETDEV_UP:
>   	case NETDEV_CHANGE:

Applied to the for-5.2 branch of linux-leds.git.

--
Best regards,
Jacek Anaszewski