[PATCH net-next 0/1] netvsc: fix deadlock in VF unregister

[PATCH net-next 0/1] netvsc: fix deadlock in VF unregister

[Stephen Hemminger]

There was a race in VF unregister (in net-next only) which can be triggered
if SR-IOV is disabled on host side, which causes PCI hotplug removal.

Stephen Hemminger (1):
  netvsc: fix rtnl deadlock on unregister of vf

 drivers/net/hyperv/netvsc_drv.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

-- 
2.11.0

[PATCH net-next 1/1] netvsc: fix rtnl deadlock on unregister of vf

[Stephen Hemminger]

With new transparent VF support, it is possible to get a deadlock
when some of the deferred work is running and the unregister_vf
is trying to cancel the work element. The solution is to use
trylock and reschedule (similar to bonding and team device).

Reported-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Fixes: 0c195567a8f6 ("netvsc: transparent VF management")
Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
 drivers/net/hyperv/netvsc_drv.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index c71728d82049..e75c0f852a63 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -1601,7 +1601,11 @@ static void netvsc_vf_setup(struct work_struct *w)
 	struct net_device *ndev = hv_get_drvdata(ndev_ctx->device_ctx);
 	struct net_device *vf_netdev;
 
-	rtnl_lock();
+	if (!rtnl_trylock()) {
+		schedule_work(w);
+		return;
+	}
+
 	vf_netdev = rtnl_dereference(ndev_ctx->vf_netdev);
 	if (vf_netdev)
 		__netvsc_vf_setup(ndev, vf_netdev);
@@ -1655,7 +1659,11 @@ static void netvsc_vf_update(struct work_struct *w)
 	struct net_device *vf_netdev;
 	bool vf_is_up;
 
-	rtnl_lock();
+	if (!rtnl_trylock()) {
+		schedule_work(w);
+		return;
+	}
+
 	vf_netdev = rtnl_dereference(ndev_ctx->vf_netdev);
 	if (!vf_netdev)
 		goto unlock;
-- 
2.11.0

Re: [PATCH net-next 1/1] netvsc: fix rtnl deadlock on unregister of vf

[David Miller]

From: Stephen Hemminger <stephen@networkplumber.org>
Date: Fri,  4 Aug 2017 12:14:00 -0700

> With new transparent VF support, it is possible to get a deadlock
> when some of the deferred work is running and the unregister_vf
> is trying to cancel the work element. The solution is to use
> trylock and reschedule (similar to bonding and team device).
> 
> Reported-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> Fixes: 0c195567a8f6 ("netvsc: transparent VF management")
> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>

Applied, thanks.

Re: [PATCH net-next 1/1] netvsc: fix rtnl deadlock on unregister of vf

[Vitaly Kuznetsov]

Stephen Hemminger <stephen@networkplumber.org> writes:

> With new transparent VF support, it is possible to get a deadlock
> when some of the deferred work is running and the unregister_vf
> is trying to cancel the work element. The solution is to use
> trylock and reschedule (similar to bonding and team device).
>
> Reported-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> Fixes: 0c195567a8f6 ("netvsc: transparent VF management")
> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> ---
>  drivers/net/hyperv/netvsc_drv.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
> index c71728d82049..e75c0f852a63 100644
> --- a/drivers/net/hyperv/netvsc_drv.c
> +++ b/drivers/net/hyperv/netvsc_drv.c
> @@ -1601,7 +1601,11 @@ static void netvsc_vf_setup(struct work_struct *w)
>  	struct net_device *ndev = hv_get_drvdata(ndev_ctx->device_ctx);
>  	struct net_device *vf_netdev;
>
> -	rtnl_lock();
> +	if (!rtnl_trylock()) {
> +		schedule_work(w);
> +		return;
> +	}
> +
>  	vf_netdev = rtnl_dereference(ndev_ctx->vf_netdev);
>  	if (vf_netdev)
>  		__netvsc_vf_setup(ndev, vf_netdev);
> @@ -1655,7 +1659,11 @@ static void netvsc_vf_update(struct work_struct *w)
>  	struct net_device *vf_netdev;
>  	bool vf_is_up;
>
> -	rtnl_lock();
> +	if (!rtnl_trylock()) {
> +		schedule_work(w);
> +		return;
> +	}
> +

So in the situation when we're currently in netvsc_unregister_vf() and
trying to do
        cancel_work_sync(&net_device_ctx->vf_takeover);
	cancel_work_sync(&net_device_ctx->vf_notify);

we'll end up not executing netvsc_vf_update() at all, right? Wouldn't it
create an issue as nobody is switching the datapath back to netvsc?

>  	vf_netdev = rtnl_dereference(ndev_ctx->vf_netdev);
>  	if (!vf_netdev)
>  		goto unlock;

-- 
  Vitaly

Re: [PATCH net-next 1/1] netvsc: fix rtnl deadlock on unregister of vf

[Vitaly Kuznetsov]

Vitaly Kuznetsov <vkuznets@redhat.com> writes:

> Stephen Hemminger <stephen@networkplumber.org> writes:
>
>> With new transparent VF support, it is possible to get a deadlock
>> when some of the deferred work is running and the unregister_vf
>> is trying to cancel the work element. The solution is to use
>> trylock and reschedule (similar to bonding and team device).
>>
>> Reported-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> Fixes: 0c195567a8f6 ("netvsc: transparent VF management")
>> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
>> ---
>>  drivers/net/hyperv/netvsc_drv.c | 12 ++++++++++--
>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
>> index c71728d82049..e75c0f852a63 100644
>> --- a/drivers/net/hyperv/netvsc_drv.c
>> +++ b/drivers/net/hyperv/netvsc_drv.c
>> @@ -1601,7 +1601,11 @@ static void netvsc_vf_setup(struct work_struct *w)
>>  	struct net_device *ndev = hv_get_drvdata(ndev_ctx->device_ctx);
>>  	struct net_device *vf_netdev;
>>
>> -	rtnl_lock();
>> +	if (!rtnl_trylock()) {
>> +		schedule_work(w);
>> +		return;
>> +	}
>> +
>>  	vf_netdev = rtnl_dereference(ndev_ctx->vf_netdev);
>>  	if (vf_netdev)
>>  		__netvsc_vf_setup(ndev, vf_netdev);
>> @@ -1655,7 +1659,11 @@ static void netvsc_vf_update(struct work_struct *w)
>>  	struct net_device *vf_netdev;
>>  	bool vf_is_up;
>>
>> -	rtnl_lock();
>> +	if (!rtnl_trylock()) {
>> +		schedule_work(w);
>> +		return;
>> +	}
>> +
>
> So in the situation when we're currently in netvsc_unregister_vf() and
> trying to do
>         cancel_work_sync(&net_device_ctx->vf_takeover);
> 	cancel_work_sync(&net_device_ctx->vf_notify);
>
> we'll end up not executing netvsc_vf_update() at all, right? Wouldn't it
> create an issue as nobody is switching the datapath back to netvsc?
>

Actually, looking more at this I think we have additional issues:

netvsc_unregister_vf() may get executed _before_ netvsc_vf_update() gets
a chance and we just cancel it so the data path is never switched
back. I actually have a VM where I suppose it happens ...

[    7.235566] hv_netvsc 33b7a6f9-6736-451f-8fce-b382eaa50bee eth1: VF up: enP2p0s2
[    7.235569] hv_netvsc 33b7a6f9-6736-451f-8fce-b382eaa50bee eth1: Datapath switched to VF: enP2p0s2

On VF removal:

[   17.675885] mlx4_en: enP2p0s2: Close port called
[   17.727005] hv_netvsc 33b7a6f9-6736-451f-8fce-b382eaa50bee eth1: VF unregistering: enP2p0s2
<and nothing after - so the data path is not switched>

We need to make sure netvsc_vf_update() is always processed on removal.

-- 
  Vitaly

Re: [PATCH net-next 1/1] netvsc: fix rtnl deadlock on unregister of vf

[Vitaly Kuznetsov]

Vitaly Kuznetsov <vkuznets@redhat.com> writes:

> Vitaly Kuznetsov <vkuznets@redhat.com> writes:
>
>> Stephen Hemminger <stephen@networkplumber.org> writes:
>>
>>> With new transparent VF support, it is possible to get a deadlock
>>> when some of the deferred work is running and the unregister_vf
>>> is trying to cancel the work element. The solution is to use
>>> trylock and reschedule (similar to bonding and team device).
>>>
>>> Reported-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>>> Fixes: 0c195567a8f6 ("netvsc: transparent VF management")
>>> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
>>> ---
>>>  drivers/net/hyperv/netvsc_drv.c | 12 ++++++++++--
>>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
>>> index c71728d82049..e75c0f852a63 100644
>>> --- a/drivers/net/hyperv/netvsc_drv.c
>>> +++ b/drivers/net/hyperv/netvsc_drv.c
>>> @@ -1601,7 +1601,11 @@ static void netvsc_vf_setup(struct work_struct *w)
>>>  	struct net_device *ndev = hv_get_drvdata(ndev_ctx->device_ctx);
>>>  	struct net_device *vf_netdev;
>>>
>>> -	rtnl_lock();
>>> +	if (!rtnl_trylock()) {
>>> +		schedule_work(w);
>>> +		return;
>>> +	}
>>> +
>>>  	vf_netdev = rtnl_dereference(ndev_ctx->vf_netdev);
>>>  	if (vf_netdev)
>>>  		__netvsc_vf_setup(ndev, vf_netdev);
>>> @@ -1655,7 +1659,11 @@ static void netvsc_vf_update(struct work_struct *w)
>>>  	struct net_device *vf_netdev;
>>>  	bool vf_is_up;
>>>
>>> -	rtnl_lock();
>>> +	if (!rtnl_trylock()) {
>>> +		schedule_work(w);
>>> +		return;
>>> +	}
>>> +
>>
>> So in the situation when we're currently in netvsc_unregister_vf() and
>> trying to do
>>         cancel_work_sync(&net_device_ctx->vf_takeover);
>> 	cancel_work_sync(&net_device_ctx->vf_notify);
>>
>> we'll end up not executing netvsc_vf_update() at all, right? Wouldn't it
>> create an issue as nobody is switching the datapath back to netvsc?
>>
>
> Actually, looking more at this I think we have additional issues:
>
> netvsc_unregister_vf() may get executed _before_ netvsc_vf_update() gets
> a chance and we just cancel it so the data path is never switched
> back. I actually have a VM where I suppose it happens ...
>
> [    7.235566] hv_netvsc 33b7a6f9-6736-451f-8fce-b382eaa50bee eth1: VF up: enP2p0s2
> [    7.235569] hv_netvsc 33b7a6f9-6736-451f-8fce-b382eaa50bee eth1: Datapath switched to VF: enP2p0s2
>
> On VF removal:
>
> [   17.675885] mlx4_en: enP2p0s2: Close port called
> [   17.727005] hv_netvsc 33b7a6f9-6736-451f-8fce-b382eaa50bee eth1: VF unregistering: enP2p0s2
> <and nothing after - so the data path is not switched>
>
> We need to make sure netvsc_vf_update() is always processed on removal.

So the question I have is: why do we need to call netvsc_vf_update()
from a work? I tried calling it directly from netvsc_netdev_event() (and
with rtnl_lock()/unlock() calls dropped from it as we already have it,
of course) and everything seems to work for me.

Shall I send a patch removing the work?

-- 
  Vitaly

Re: [PATCH net-next 1/1] netvsc: fix rtnl deadlock on unregister of vf

[Stephen Hemminger]

On Mon, 07 Aug 2017 15:37:49 +0200
Vitaly Kuznetsov <vkuznets@redhat.com> wrote:

> Vitaly Kuznetsov <vkuznets@redhat.com> writes:
> 
> > Stephen Hemminger <stephen@networkplumber.org> writes:
> >  
> >> With new transparent VF support, it is possible to get a deadlock
> >> when some of the deferred work is running and the unregister_vf
> >> is trying to cancel the work element. The solution is to use
> >> trylock and reschedule (similar to bonding and team device).
> >>
> >> Reported-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> >> Fixes: 0c195567a8f6 ("netvsc: transparent VF management")
> >> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> >> ---
> >>  drivers/net/hyperv/netvsc_drv.c | 12 ++++++++++--
> >>  1 file changed, 10 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
> >> index c71728d82049..e75c0f852a63 100644
> >> --- a/drivers/net/hyperv/netvsc_drv.c
> >> +++ b/drivers/net/hyperv/netvsc_drv.c
> >> @@ -1601,7 +1601,11 @@ static void netvsc_vf_setup(struct work_struct *w)
> >>  	struct net_device *ndev = hv_get_drvdata(ndev_ctx->device_ctx);
> >>  	struct net_device *vf_netdev;
> >>
> >> -	rtnl_lock();
> >> +	if (!rtnl_trylock()) {
> >> +		schedule_work(w);
> >> +		return;
> >> +	}
> >> +
> >>  	vf_netdev = rtnl_dereference(ndev_ctx->vf_netdev);
> >>  	if (vf_netdev)
> >>  		__netvsc_vf_setup(ndev, vf_netdev);
> >> @@ -1655,7 +1659,11 @@ static void netvsc_vf_update(struct work_struct *w)
> >>  	struct net_device *vf_netdev;
> >>  	bool vf_is_up;
> >>
> >> -	rtnl_lock();
> >> +	if (!rtnl_trylock()) {
> >> +		schedule_work(w);
> >> +		return;
> >> +	}
> >> +  
> >
> > So in the situation when we're currently in netvsc_unregister_vf() and
> > trying to do
> >         cancel_work_sync(&net_device_ctx->vf_takeover);
> > 	cancel_work_sync(&net_device_ctx->vf_notify);
> >
> > we'll end up not executing netvsc_vf_update() at all, right? Wouldn't it
> > create an issue as nobody is switching the datapath back to netvsc?

It worked testing, but most likely only because host is doing it for us.
Not a good thing to rely on.

> 
> Actually, looking more at this I think we have additional issues:
> 
> netvsc_unregister_vf() may get executed _before_ netvsc_vf_update() gets
> a chance and we just cancel it so the data path is never switched
> back. I actually have a VM where I suppose it happens ...
> 
> [    7.235566] hv_netvsc 33b7a6f9-6736-451f-8fce-b382eaa50bee eth1: VF up: enP2p0s2
> [    7.235569] hv_netvsc 33b7a6f9-6736-451f-8fce-b382eaa50bee eth1: Datapath switched to VF: enP2p0s2
> 
> On VF removal:
> 
> [   17.675885] mlx4_en: enP2p0s2: Close port called
> [   17.727005] hv_netvsc 33b7a6f9-6736-451f-8fce-b382eaa50bee eth1: VF unregistering: enP2p0s2
> <and nothing after - so the data path is not switched>
> 
> We need to make sure netvsc_vf_update() is always processed on removal.

The reason vf_update was converted to work queue was because there were some
case the old code could sleep. Probably best to go back to doing it directly
in notifier and handle the special cases.

Re: [PATCH net-next 1/1] netvsc: fix rtnl deadlock on unregister of vf

[Stephen Hemminger]

On Mon, 07 Aug 2017 17:17:19 +0200
Vitaly Kuznetsov <vkuznets@redhat.com> wrote:

> Vitaly Kuznetsov <vkuznets@redhat.com> writes:
> 
> > Vitaly Kuznetsov <vkuznets@redhat.com> writes:
> >  
> >> Stephen Hemminger <stephen@networkplumber.org> writes:
> >>  
> >>> With new transparent VF support, it is possible to get a deadlock
> >>> when some of the deferred work is running and the unregister_vf
> >>> is trying to cancel the work element. The solution is to use
> >>> trylock and reschedule (similar to bonding and team device).
> >>>
> >>> Reported-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> >>> Fixes: 0c195567a8f6 ("netvsc: transparent VF management")
> >>> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> >>> ---
> >>>  drivers/net/hyperv/netvsc_drv.c | 12 ++++++++++--
> >>>  1 file changed, 10 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
> >>> index c71728d82049..e75c0f852a63 100644
> >>> --- a/drivers/net/hyperv/netvsc_drv.c
> >>> +++ b/drivers/net/hyperv/netvsc_drv.c
> >>> @@ -1601,7 +1601,11 @@ static void netvsc_vf_setup(struct work_struct *w)
> >>>  	struct net_device *ndev = hv_get_drvdata(ndev_ctx->device_ctx);
> >>>  	struct net_device *vf_netdev;
> >>>
> >>> -	rtnl_lock();
> >>> +	if (!rtnl_trylock()) {
> >>> +		schedule_work(w);
> >>> +		return;
> >>> +	}
> >>> +
> >>>  	vf_netdev = rtnl_dereference(ndev_ctx->vf_netdev);
> >>>  	if (vf_netdev)
> >>>  		__netvsc_vf_setup(ndev, vf_netdev);
> >>> @@ -1655,7 +1659,11 @@ static void netvsc_vf_update(struct work_struct *w)
> >>>  	struct net_device *vf_netdev;
> >>>  	bool vf_is_up;
> >>>
> >>> -	rtnl_lock();
> >>> +	if (!rtnl_trylock()) {
> >>> +		schedule_work(w);
> >>> +		return;
> >>> +	}
> >>> +  
> >>
> >> So in the situation when we're currently in netvsc_unregister_vf() and
> >> trying to do
> >>         cancel_work_sync(&net_device_ctx->vf_takeover);
> >> 	cancel_work_sync(&net_device_ctx->vf_notify);
> >>
> >> we'll end up not executing netvsc_vf_update() at all, right? Wouldn't it
> >> create an issue as nobody is switching the datapath back to netvsc?
> >>  
> >
> > Actually, looking more at this I think we have additional issues:
> >
> > netvsc_unregister_vf() may get executed _before_ netvsc_vf_update() gets
> > a chance and we just cancel it so the data path is never switched
> > back. I actually have a VM where I suppose it happens ...
> >
> > [    7.235566] hv_netvsc 33b7a6f9-6736-451f-8fce-b382eaa50bee eth1: VF up: enP2p0s2
> > [    7.235569] hv_netvsc 33b7a6f9-6736-451f-8fce-b382eaa50bee eth1: Datapath switched to VF: enP2p0s2
> >
> > On VF removal:
> >
> > [   17.675885] mlx4_en: enP2p0s2: Close port called
> > [   17.727005] hv_netvsc 33b7a6f9-6736-451f-8fce-b382eaa50bee eth1: VF unregistering: enP2p0s2
> > <and nothing after - so the data path is not switched>
> >
> > We need to make sure netvsc_vf_update() is always processed on removal.  
> 
> So the question I have is: why do we need to call netvsc_vf_update()
> from a work? I tried calling it directly from netvsc_netdev_event() (and
> with rtnl_lock()/unlock() calls dropped from it as we already have it,
> of course) and everything seems to work for me.

Switching datapath needs to be waiting for ack and isn't

> 
> Shall I send a patch removing the work?

I will take care of that.