netdev - RE: [PATCH net-next] hv_netvsc: Fix race condition on Multi-Send Data field

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

Message-ID: <BY2PR0301MB16546E8D28E58409601E80EEA0EA0@BY2PR0301MB1654.namprd03.prod.outlook.com>
Date:	Fri, 11 Dec 2015 16:00:39 +0000
From:	KY Srinivasan <kys@...rosoft.com>
To:	Vitaly Kuznetsov <vkuznets@...hat.com>,
	Haiyang Zhang <haiyangz@...rosoft.com>
CC:	"olaf@...fle.de" <olaf@...fle.de>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"jasowang@...hat.com" <jasowang@...hat.com>,
	"driverdev-devel@...uxdriverproject.org" 
	<driverdev-devel@...uxdriverproject.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"davem@...emloft.net" <davem@...emloft.net>
Subject: RE: [PATCH net-next] hv_netvsc: Fix race condition on Multi-Send Data
 field



> -----Original Message-----
> From: devel [mailto:driverdev-devel-bounces@...uxdriverproject.org] On
> Behalf Of Vitaly Kuznetsov
> Sent: Friday, December 11, 2015 5:53 AM
> To: Haiyang Zhang <haiyangz@...rosoft.com>
> Cc: olaf@...fle.de; netdev@...r.kernel.org; jasowang@...hat.com;
> driverdev-devel@...uxdriverproject.org; linux-kernel@...r.kernel.org;
> davem@...emloft.net
> Subject: Re: [PATCH net-next] hv_netvsc: Fix race condition on Multi-Send
> Data field
> 
> Vitaly Kuznetsov <vkuznets@...hat.com> writes:
> 
> > Haiyang Zhang <haiyangz@...rosoft.com> writes:
> >
> >> In commit 2a04ae8acb14 ("hv_netvsc: remove locking in netvsc_send()"),
> the
> >> locking for MSD (Multi-Send Data) field was removed. This could cause a
> >> race condition between RNDIS control messages and data packets
> processing,
> >> because these two types of traffic are not synchronized.
> >> This patch fixes this issue by sending control messages out directly
> >> without reading MSD field.
> >>
> >> Signed-off-by: Haiyang Zhang <haiyangz@...rosoft.com>
> >> Reviewed-by: K. Y. Srinivasan <kys@...rosoft.com>
> >> ---
> >>  drivers/net/hyperv/netvsc.c |    9 +++++++++
> >>  1 files changed, 9 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
> >> index 02bab9a..059fc52 100644
> >> --- a/drivers/net/hyperv/netvsc.c
> >> +++ b/drivers/net/hyperv/netvsc.c
> >> @@ -867,6 +867,14 @@ int netvsc_send(struct hv_device *device,
> >>  	packet->send_buf_index = NETVSC_INVALID_INDEX;
> >>  	packet->cp_partial = false;
> >>
> >> +	/* Send control message directly without accessing msd (Multi-Send
> >> +	 * Data) field which may be changed during data packet processing.
> >> +	 */
> >> +	if (!skb) {
> >> +		cur_send = packet;
> >> +		goto send_now;
> >> +	}
> >> +
> >
> > Is is supposed to be applied on top of some other patches? It doesn't
> > compile on top of current net-next:
> >
> > drivers/net/hyperv/netvsc.c: In function ‘netvsc_send’:
> > drivers/net/hyperv/netvsc.c:865:7: error: ‘skb’ undeclared (first use in
> > this function)
> >   if (!skb) {
> >          ^
> >
> > Did you mean to check rndis_msg instead (as skb is not defined here)?
> 
> Oh, my bad, it was me who was looking at the wrong branch... Sorry for
> the noise.
> 
> >
> >>  	msdp = &net_device->msd[q_idx];
> >>
> >>  	/* batch packets in send buffer if possible */
> >> @@ -939,6 +947,7 @@ int netvsc_send(struct hv_device *device,
> >>  		}
> >>  	}
> >>
> >> +send_now:
> >>  	if (cur_send)
> >>  		ret = netvsc_send_pkt(cur_send, net_device, pb, skb);
> >
> > I suppose we untangle these two pathes completely: let
> > rndis_filter_send_request() call netvsc_send_pkt() directly. Please see
> > my patch attached (note: it should be split in 3 patches if
> > submitted). If you like the idea I can send it.
> 
> This patch will need some changes but the suggestion still stands: let's
> untangle sending data and control messages.

Control messages will have skb set to NULL and that is what Haiyang is doing
to distinguish control packets.

K. Y
> 
> --
>   Vitaly
> _______________________________________________
> devel mailing list
> devel@...uxdriverproject.org
> https://na01.safelinks.protection.outlook.com/?url=http%3a%2f%2fdriverde
> v.linuxdriverproject.org%2fmailman%2flistinfo%2fdriverdev-
> devel%0a&data=01%7c01%7ckys%40microsoft.com%7c741026ba8cbb47a993
> 2c08d3023257b7%7c72f988bf86f141af91ab2d7cd011db47%7c1&sdata=VdQiM
> NOzCtlRcsN5%2b%2faf%2bGrPNDHPmrvo2TuUs1T%2frlQ%3d