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

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] [day] [month] [year] [list]

Message-ID: <20170807082100.7ba56ac9@xeon-e3>
Date:   Mon, 7 Aug 2017 08:21:00 -0700
From:   Stephen Hemminger <stephen@...workplumber.org>
To:     Vitaly Kuznetsov <vkuznets@...hat.com>
Cc:     devel@...uxdriverproject.org, haiyangz@...rosoft.com,
        sthemmin@...rosoft.com, netdev@...r.kernel.org
Subject: Re: [PATCH net-next 1/1] netvsc: fix rtnl deadlock on unregister of
 vf

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

> Vitaly Kuznetsov <vkuznets@...hat.com> writes:
> 
> > Vitaly Kuznetsov <vkuznets@...hat.com> writes:
> >  
> >> Stephen Hemminger <stephen@...workplumber.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@...hat.com>
> >>> Fixes: 0c195567a8f6 ("netvsc: transparent VF management")
> >>> Signed-off-by: Stephen Hemminger <sthemmin@...rosoft.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.