netdev - Re: [PATCH net,v2] hv_netvsc: fix race of netvsc and VF register

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: <ce888f7b-70c2-4139-8f18-1cbb3fede9cd@intel.com>
Date: Fri, 27 Oct 2023 12:21:25 +0200
From: Wojciech Drewek <wojciech.drewek@...el.com>
To: Haiyang Zhang <haiyangz@...rosoft.com>, <linux-hyperv@...r.kernel.org>,
	<netdev@...r.kernel.org>
CC: <kys@...rosoft.com>, <wei.liu@...nel.org>, <decui@...rosoft.com>,
	<edumazet@...gle.com>, <kuba@...nel.org>, <pabeni@...hat.com>,
	<davem@...emloft.net>, <linux-kernel@...r.kernel.org>,
	<stable@...r.kernel.org>
Subject: Re: [PATCH net,v2] hv_netvsc: fix race of netvsc and VF
 register_netdevice



On 26.10.2023 23:22, Haiyang Zhang wrote:
> The rtnl lock also needs to be held before rndis_filter_device_add()
> which advertises nvsp_2_vsc_capability / sriov bit, and triggers
> VF NIC offering and registering. If VF NIC finished register_netdev()
> earlier it may cause name based config failure.
> 
> To fix this issue, move the call to rtnl_lock() before
> rndis_filter_device_add(), so VF will be registered later than netvsc
> / synthetic NIC, and gets a name numbered (ethX) after netvsc.
> 
> And, move register_netdevice_notifier() earlier, so the call back
> function is set before probing.
> 
> Cc: stable@...r.kernel.org
> Fixes: e04e7a7bbd4b ("hv_netvsc: Fix a deadlock by getting rtnl lock earlier in netvsc_probe()")
> Signed-off-by: Haiyang Zhang <haiyangz@...rosoft.com>

Reviewed-by: Wojciech Drewek <wojciech.drewek@...el.com>

> 
> ---
> v2:
>   Fix rtnl_unlock() in error handling as found by Wojciech Drewek.
> ---
>  drivers/net/hyperv/netvsc_drv.c | 32 ++++++++++++++++++++------------
>  1 file changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
> index 3ba3c8fb28a5..1d1491da303b 100644
> --- a/drivers/net/hyperv/netvsc_drv.c
> +++ b/drivers/net/hyperv/netvsc_drv.c
> @@ -2531,15 +2531,6 @@ static int netvsc_probe(struct hv_device *dev,
>  		goto devinfo_failed;
>  	}
>  
> -	nvdev = rndis_filter_device_add(dev, device_info);
> -	if (IS_ERR(nvdev)) {
> -		ret = PTR_ERR(nvdev);
> -		netdev_err(net, "unable to add netvsc device (ret %d)\n", ret);
> -		goto rndis_failed;
> -	}
> -
> -	eth_hw_addr_set(net, device_info->mac_adr);
> -
>  	/* We must get rtnl lock before scheduling nvdev->subchan_work,
>  	 * otherwise netvsc_subchan_work() can get rtnl lock first and wait
>  	 * all subchannels to show up, but that may not happen because
> @@ -2547,9 +2538,23 @@ static int netvsc_probe(struct hv_device *dev,
>  	 * -> ... -> device_add() -> ... -> __device_attach() can't get
>  	 * the device lock, so all the subchannels can't be processed --
>  	 * finally netvsc_subchan_work() hangs forever.
> +	 *
> +	 * The rtnl lock also needs to be held before rndis_filter_device_add()
> +	 * which advertises nvsp_2_vsc_capability / sriov bit, and triggers
> +	 * VF NIC offering and registering. If VF NIC finished register_netdev()
> +	 * earlier it may cause name based config failure.
>  	 */
>  	rtnl_lock();
>  
> +	nvdev = rndis_filter_device_add(dev, device_info);
> +	if (IS_ERR(nvdev)) {
> +		ret = PTR_ERR(nvdev);
> +		netdev_err(net, "unable to add netvsc device (ret %d)\n", ret);
> +		goto rndis_failed;
> +	}
> +
> +	eth_hw_addr_set(net, device_info->mac_adr);
> +
>  	if (nvdev->num_chn > 1)
>  		schedule_work(&nvdev->subchan_work);
>  
> @@ -2586,9 +2591,9 @@ static int netvsc_probe(struct hv_device *dev,
>  	return 0;
>  
>  register_failed:
> -	rtnl_unlock();
>  	rndis_filter_device_remove(dev, nvdev);
>  rndis_failed:
> +	rtnl_unlock();
>  	netvsc_devinfo_put(device_info);
>  devinfo_failed:
>  	free_percpu(net_device_ctx->vf_stats);
> @@ -2788,11 +2793,14 @@ static int __init netvsc_drv_init(void)
>  	}
>  	netvsc_ring_bytes = ring_size * PAGE_SIZE;
>  
> +	register_netdevice_notifier(&netvsc_netdev_notifier);
> +
>  	ret = vmbus_driver_register(&netvsc_drv);
> -	if (ret)
> +	if (ret) {
> +		unregister_netdevice_notifier(&netvsc_netdev_notifier);
>  		return ret;
> +	}
>  
> -	register_netdevice_notifier(&netvsc_netdev_notifier);
>  	return 0;
>  }
>