netdev - RE: [RFC PATCH 1/2] hv_netvsc: Split netvsc_revoke_buf() and netvsc_teardown

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: <CY4PR21MB08379EDA84C9FFAEBC1C5B1ECCFA0@CY4PR21MB0837.namprd21.prod.outlook.com>
Date:   Thu, 1 Feb 2018 22:38:13 +0000
From:   Stephen Hemminger <sthemmin@...rosoft.com>
To:     "mgamal@...hat.com" <mgamal@...hat.com>,
        Stephen Hemminger <stephen@...workplumber.org>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "otubo@...hat.com" <otubo@...hat.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "devel@...uxdriverproject.org" <devel@...uxdriverproject.org>,
        "vkuznets@...hat.com" <vkuznets@...hat.com>
Subject: RE: [RFC PATCH 1/2] hv_netvsc: Split netvsc_revoke_buf() and
 netvsc_teardown_gpadl()

There are multiple issues with some of the parameter change paths.
Still working on getting something stable. Both upstream, and net-next do have crash issues under concurrent changes.

I don't want Linux doing different workaround than Windows if at all possible; because it means that it would require much wider testing against many different versions.
Ps: WS2008r2 still needs to be supported.

-----Original Message-----
From: Mohammed Gamal [mailto:mgamal@...hat.com] 
Sent: Thursday, February 1, 2018 2:34 PM
To: Stephen Hemminger <stephen@...workplumber.org>
Cc: netdev@...r.kernel.org; otubo@...hat.com; Stephen Hemminger <sthemmin@...rosoft.com>; Haiyang Zhang <haiyangz@...rosoft.com>; linux-kernel@...r.kernel.org; devel@...uxdriverproject.org; vkuznets@...hat.com
Subject: Re: [RFC PATCH 1/2] hv_netvsc: Split netvsc_revoke_buf() and netvsc_teardown_gpadl()

On Thu, 2018-02-01 at 09:37 +0100, Mohammed Gamal wrote:
> On Wed, 2018-01-31 at 15:01 -0800, Stephen Hemminger wrote:
> > On Wed, 31 Jan 2018 12:16:49 +0100
> > Mohammed Gamal <mgamal@...hat.com> wrote:
> > 
> > > On Tue, 2018-01-30 at 11:29 -0800, Stephen Hemminger wrote:
> > > > On Tue, 23 Jan 2018 10:34:04 +0100
> > > > Mohammed Gamal <mgamal@...hat.com> wrote:
> > > >   
> > > > > Split each of the functions into two for each of send/recv
> > > > > buffers
> > > > > 
> > > > > Signed-off-by: Mohammed Gamal <mgamal@...hat.com>  
> > > > 
> > > > Splitting these functions is not necessary  
> > > 
> > > How so? We need to send each message independently, and hence the
> > > split
> > > (see cover letter). Is there another way?
> > 
> > This is all that is needed.
> > 
> > 
> > Subject: [PATCH] hv_netvsc: work around for gpadl teardown on older
> > windows
> >  server
> > 
> > On WS2012 the host ignores messages after vmbus channel is closed.
> > Workaround this by doing what Windows does and send the teardown
> > before close on older versions of NVSP protocol.
> > 
> > Reported-by: Mohammed Gamal <mgamal@...hat.com>
> > Fixes: 0cf737808ae7 ("hv_netvsc: netvsc_teardown_gpadl() split")
> > Signed-off-by: Stephen Hemminger <sthemmin@...rosoft.com>
> > ---
> >  drivers/net/hyperv/netvsc.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/hyperv/netvsc.c
> > b/drivers/net/hyperv/netvsc.c
> > index 17e529af79dc..1a3df0eff42f 100644
> > --- a/drivers/net/hyperv/netvsc.c
> > +++ b/drivers/net/hyperv/netvsc.c
> > @@ -574,10 +574,17 @@ void netvsc_device_remove(struct hv_device
> > *device)
> >  	 */
> >  	netdev_dbg(ndev, "net device safe to remove\n");
> >  
> > +	/* Workaround for older versions of Windows require that
> > +	 * buffer be revoked before channel is disabled
> > +	 */
> > +	if (net_device->nvsp_version < NVSP_PROTOCOL_VERSION_4)
> > +		netvsc_teardown_gpadl(device, net_device);
> > +
> >  	/* Now, we can close the channel safely */
> >  	vmbus_close(device->channel);
> >  
> > -	netvsc_teardown_gpadl(device, net_device);
> > +	if (net_device->nvsp_version >= NVSP_PROTOCOL_VERSION_4)
> > +		netvsc_teardown_gpadl(device, net_device);
> >  
> >  	/* And dissassociate NAPI context from device */
> >  	for (i = 0; i < net_device->num_chn; i++)
> 
> I've tried a similar workaround before by calling
> netvsc_teardown_gpadl() after netvsc_revoke_buf(), but before setting
> net_device_ctx->nvdev to NULL and it caused the guest to hang when
> trying to change MTU. 
> 
> Let me try that change and see if it behaves differently.

I tested the patch, but I've actually seen some unexpected behavior.

First, net_device->nvsp_version is actually NVSP_PROTOCOL_VERSION_5 on
both my Win2012 and Win2016 hosts that I tested on, so the condition is
never executed.

Second, when doing the check instead as  if (vmbus_proto_version <
VERSION_WIN10), I get the same behavior I described above where the
guest hangs as the kernel waits indefinitely in vmbus_teardown_gpadl()
for a completion to be signaled. This is actually what lead me to
propose splitting netvsc_revoke_buf() and netvsc_teardown_gpadl() in my
initial patchset so that we keep the same order of messages and avoid
that indefinite wait.