Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Message-ID: <ce888f7b-70c2-4139-8f18-1cbb3fede9cd@intel.com>
Date:   Fri, 27 Oct 2023 12:21:25 +0200
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH net,v2] hv_netvsc: fix race of netvsc and VF
 register_netdevice
Content-Language: en-US
To:     Haiyang Zhang <haiyangz@microsoft.com>,
        <linux-hyperv@vger.kernel.org>, <netdev@vger.kernel.org>
CC:     <kys@microsoft.com>, <wei.liu@kernel.org>, <decui@microsoft.com>,
        <edumazet@google.com>, <kuba@kernel.org>, <pabeni@redhat.com>,
        <davem@davemloft.net>, <linux-kernel@vger.kernel.org>,
        <stable@vger.kernel.org>
References: <1698355354-12869-1-git-send-email-haiyangz@microsoft.com>
From:   Wojciech Drewek <wojciech.drewek@intel.com>
In-Reply-To: <1698355354-12869-1-git-send-email-haiyangz@microsoft.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?dENtT2kzUHdSOXErd1Q1YVFNSkVvQUk5YzVBTWpKaFZVMmM1UEg4SjdXY1I4?=
 =?utf-8?B?cGdYMmdIa292a0RwR280SlNCa2o1WkcyVjIzWVg4MzVyMk1YR01yZG9DS00z?=
 =?utf-8?B?TUF3SXFxRGhFbmYzWjZXRkZnZDNvS21YaWQ5MjM4QmtLN1J2YnRraG1sbnla?=
 =?utf-8?B?NGloWjdwUFZaTnkzSWhjYXh5NXJORTZmVWpSMk4zOTdlQlR3amtiSVVCVDZK?=
 =?utf-8?B?SHJyN25vUVBVV1JkQWxMOGk0RVpFdXNSTGF4TDNVenlDREhYZDFTbS92MUNB?=
 =?utf-8?B?SnErRUxaZm5FeEltV1Q4ZFVFY2IyUG5vL0NKVWpGODArQ0QyeEZGOTNKd0xE?=
 =?utf-8?B?ZzFnaGtkNzhFNFEyeUJxREIvRHZhT2xSb21OTEdxaDZaZDN3YTdKdjRiMDJP?=
 =?utf-8?B?d0hGZHpuV21jL1U0STZuVWllZXVjYzZYMVJDSnVqNGhFbGZnMkxFV3NPTWlB?=
 =?utf-8?B?SlkrcnRNK2cvbzBRMFRyakVVWGh1Nm8wK3hoOEtkdHI0YVFGVjlvZjhGREdt?=
 =?utf-8?B?OU9iUldud2Q0ZWVUdkk3MHl2dEdJZ1JOMnpwK05vak1tUENVWDNRUjR3dnZ1?=
 =?utf-8?B?Smh2MGx2M0IyL0E4Z3Y0OU51dVg3QW03MUxEQldENFRjOVIzKzlRTjJUL2NF?=
 =?utf-8?B?UkNyMURGa1JiSkYwNlJkeGY0N0RnSU5GeHl6SkI4OWpkS2FmR2p0ZVZ4Zmlv?=
 =?utf-8?B?bXRQSHQrMWJFN2kwRUZJVXcvY01wQmNTNzhGYWRHQzJUdXVzY05KbURucHhK?=
 =?utf-8?B?Nk8wUGZWVFhZUDEycjJ6bjNQczFrUy9nSXhWVUpUczZkd1JLYkxWanRDQTd6?=
 =?utf-8?B?Qk5CcnVkZWZBK2VhMmRabnBkdmc0S2xXdG51NHlqNjJQZHg2c0xSb2ljRkVm?=
 =?utf-8?B?dmIzNEluTzlRbUUyMGdWdnBrNjdTWTd6MHBXYmZCb05RZmJBeThISE5MUXB5?=
 =?utf-8?B?Sk12VXRSTmVMYXVhQW9ZQmUwOWtqRG5vS2o0SGJDc3NCOTJzVEdBdDRaYnFE?=
 =?utf-8?B?Y25LbUY4WHZ2bXZiZ3cwUkNHM2lsU25GT05NZEdsUWVLWEVCcGN6KzRRRjhn?=
 =?utf-8?B?ZVBaUVRXTU1EMS9VUE1Za0F1UmRUMi9wdVZNcEdsczVYZ2NoZlk2eDVvczdB?=
 =?utf-8?B?b2xDU05DbEpRRG0rcXY1RG85YWlGUmM0aWRCS2NZNjJ3U2ZabStPL1lZWUw5?=
 =?utf-8?B?UE1SZWwxby8rRzhrbkxHSk5UN2VXOVNPNDF2VnZMbnZOaWQ1OEIyNllacHlY?=
 =?utf-8?B?cmx5eWtwbkJ4bDZQaXV2OXZQdURMMUphVGsvVVJidkJGTVRZZ2p4d0RuM05Q?=
 =?utf-8?B?RmhiMjQzQVBpOFJVdUVWSWRjb292OHFXV0I5QWR4a0R0a0NFMGtJeldXWUpn?=
 =?utf-8?B?dW4zNTBISFR2SVJ5RHZ2eWRzd3RlOG5hUExEWk54bHlQSmkwcWZ5MTYzdlZ1?=
 =?utf-8?B?T3pLQ1pYdlBERUM2dGlOMXMzLy9iQ0FxaVV5RnlzcFNQNlkxYmVhVVlZUXAw?=
 =?utf-8?B?VWg2b0l2a1ZnQTlXbHZhcm40SDlYMWk4dGNHbHIyWVE4NTQwTmNoaW5ObEZB?=
 =?utf-8?B?WFhZYzA2U3VBUkszdkM4U013Sy82RWd5bEQyWnJ6TFpQcmJVSjl6eHE4bkdQ?=
 =?utf-8?B?dDJEM0VveTJaZm5oQm5RTXVhZHBlNXpvTW5CU2g2OEFscFIyajhVQlFQL1c3?=
 =?utf-8?B?eGZhbmhzYmFwbTlrM0R6TFUrVElVMVhXVFdmS2lrbm1oTFllOEQ1Q0ZrNE9u?=
 =?utf-8?B?eTRzdFJleDdkQ1ZxL0lUaXlvcnZKemQxSDlmaTAybUZOWiswNUtGcnZPTDZs?=
 =?utf-8?B?TWdiUjVDaWl5NDdzSnBDaVUwNTRmTmxMeWE2MitjY2N1R2k2WnFsaCtRakNj?=
 =?utf-8?B?RHgvNExaL0pSMzFXbHhxa1JOZWhUTEZub0ZUUmc0d3FnR3hETm9TNWpRK3Ur?=
 =?utf-8?B?ekkvNDl0RGxteGVWQnVyamJSZXo5a0tWQU9ZSDcrWHBWUWMxVmd4dCtvVWxr?=
 =?utf-8?B?VHY5TnNIdDhCVFdhSDlLcmdqL0NtNEVEdTFINGNMR2FGMEV2d21WSStueUtC?=
 =?utf-8?B?ak9SODFjTWRkMmx2U1dRVmNXRGRpcE5yaFVrT0p6QVVWT1c1bFJYYURZc3lV?=
 =?utf-8?B?VDRKY0VPVVlObG16Z1NTdDR0dWhMdXJaR0NrRlhBUUZtOFRITTJyaEVHVFFj?=
 =?utf-8?B?Q2c9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 0d8fca6c-1333-46e8-1822-08dbd6d67d75
X-MS-Exchange-CrossTenant-AuthSource: MW4PR11MB5776.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Oct 2023 10:21:32.1457
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: GdpSvyUVB8Vle7rsBDrL4K6CwqUEFGO2QCepLSpliuhm2tRzmuwWYrJFtMBvdK/r4SiFw+RnLZNJlM7iXB0vIvurAE97Cj9i0hGcICZC2DA=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR11MB8318
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,
        RCVD_IN_DNSWL_BLOCKED,SPF_HELO_NONE,SPF_NONE,URIBL_BLOCKED
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (snail.vger.email [0.0.0.0]); Fri, 27 Oct 2023 03:22:51 -0700 (PDT)


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@vger.kernel.org
> Fixes: e04e7a7bbd4b ("hv_netvsc: Fix a deadlock by getting rtnl lock earlier in netvsc_probe()")
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>

Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.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;
>  }
>