[v1] hv_netvsc: fix race of netvsc, VF register, and slave bit

[PATCH net,v4, 2/3] hv_netvsc: Fix race of register_netdevice_notifier and VF register

Posted by Haiyang Zhang 2 years, 1 month ago

If VF NIC is registered earlier, NETDEV_REGISTER event is replayed,
but NETDEV_POST_INIT is not.

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()")
Reported-by: Dexuan Cui <decui@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>

---
v3:
  Divide it into two patches, suggested by Jakub Kicinski.
v2:
  Fix rtnl_unlock() in error handling as found by Wojciech Drewek.
---
 drivers/net/hyperv/netvsc_drv.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 5e528a76f5f5..1d1491da303b 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -2793,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;
 }
 
-- 
2.25.1

RE: [PATCH net,v4, 2/3] hv_netvsc: Fix race of register_netdevice_notifier and VF register

Posted by Dexuan Cui 2 years, 1 month ago

> From: LKML haiyangz <lkmlhyz@microsoft.com> On Behalf Of Haiyang Zhang
> Sent: Friday, November 10, 2023 9:39 AM
> [...]
> If VF NIC is registered earlier, NETDEV_REGISTER event is replayed,
> but NETDEV_POST_INIT is not.
> 
> 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()")
> Reported-by: Dexuan Cui <decui@microsoft.com>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>

Reviewed-by: Dexuan Cui <decui@microsoft.com>

It's better to post a new version that follows Simon Horman's suggestion,
i.e., use a more idiomatic form for the error path.

Re: [PATCH net,v4, 2/3] hv_netvsc: Fix race of register_netdevice_notifier and VF register

Posted by Simon Horman 2 years, 1 month ago

On Fri, Nov 10, 2023 at 06:38:59AM -0800, Haiyang Zhang wrote:
> If VF NIC is registered earlier, NETDEV_REGISTER event is replayed,
> but NETDEV_POST_INIT is not.
> 
> 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()")
> Reported-by: Dexuan Cui <decui@microsoft.com>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>
> 
> ---
> v3:
>   Divide it into two patches, suggested by Jakub Kicinski.
> v2:
>   Fix rtnl_unlock() in error handling as found by Wojciech Drewek.
> ---
>  drivers/net/hyperv/netvsc_drv.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
> index 5e528a76f5f5..1d1491da303b 100644
> --- a/drivers/net/hyperv/netvsc_drv.c
> +++ b/drivers/net/hyperv/netvsc_drv.c
> @@ -2793,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;
>  }

Hi Haiyang Zhang,

functionally this change looks good to me, thanks!

I'm wondering if we could improve things slightly by using a more idiomatic
form for the error path. Something like the following (completely untested!).

My reasoning is that this way things are less likely go to wrong if more
error conditions are added to this function later.

	...

	register_netdevice_notifier(&netvsc_netdev_notifier);

	ret = vmbus_driver_register(&netvsc_drv);
	if (ret)
		goto err_unregister_netdevice_notifier;

	return 0;

err_unregister_netdevice_notifier:
	unregister_netdevice_notifier(&netvsc_netdev_notifier);
	return ret;
}

RE: [PATCH net,v4, 2/3] hv_netvsc: Fix race of register_netdevice_notifier and VF register

Posted by Haiyang Zhang 2 years, 1 month ago


> -----Original Message-----
> From: Simon Horman <horms@kernel.org>
> Sent: Sunday, November 12, 2023 4:41 AM
> To: Haiyang Zhang <haiyangz@microsoft.com>
> Cc: linux-hyperv@vger.kernel.org; netdev@vger.kernel.org; KY Srinivasan
> <kys@microsoft.com>; wei.liu@kernel.org; Dexuan Cui
> <decui@microsoft.com>; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; davem@davemloft.net; linux-kernel@vger.kernel.org;
> stable@vger.kernel.org
> Subject: Re: [PATCH net,v4, 2/3] hv_netvsc: Fix race of
> register_netdevice_notifier and VF register
> 
> On Fri, Nov 10, 2023 at 06:38:59AM -0800, Haiyang Zhang wrote:
> > If VF NIC is registered earlier, NETDEV_REGISTER event is replayed,
> > but NETDEV_POST_INIT is not.
> >
> > 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()")
> > Reported-by: Dexuan Cui <decui@microsoft.com>
> > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>
> >
> > ---
> > v3:
> >   Divide it into two patches, suggested by Jakub Kicinski.
> > v2:
> >   Fix rtnl_unlock() in error handling as found by Wojciech Drewek.
> > ---
> >  drivers/net/hyperv/netvsc_drv.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/hyperv/netvsc_drv.c
> b/drivers/net/hyperv/netvsc_drv.c
> > index 5e528a76f5f5..1d1491da303b 100644
> > --- a/drivers/net/hyperv/netvsc_drv.c
> > +++ b/drivers/net/hyperv/netvsc_drv.c
> > @@ -2793,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;
> >  }
> 
> Hi Haiyang Zhang,
> 
> functionally this change looks good to me, thanks!
> 
> I'm wondering if we could improve things slightly by using a more idiomatic
> form for the error path. Something like the following (completely untested!).
> 
> My reasoning is that this way things are less likely go to wrong if more
> error conditions are added to this function later.
> 
> 	...
> 
> 	register_netdevice_notifier(&netvsc_netdev_notifier);
> 
> 	ret = vmbus_driver_register(&netvsc_drv);
> 	if (ret)
> 		goto err_unregister_netdevice_notifier;
> 
> 	return 0;
> 
> err_unregister_netdevice_notifier:
> 	unregister_netdevice_notifier(&netvsc_netdev_notifier);
> 	return ret;
> }

Thanks for the suggested idiomatic form. I will update it.

- Haiyang