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

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

[Haiyang Zhang]

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@microsoft.com>
Reviewed-by: K. Y. Srinivasan <kys@microsoft.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;
+	}
+
 	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);
 
-- 
1.7.4.1

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

[Vitaly Kuznetsov]

[-- Attachment #1: Type: text/plain, Size: 2081 bytes --]

Haiyang Zhang <haiyangz@microsoft.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@microsoft.com>
> Reviewed-by: K. Y. Srinivasan <kys@microsoft.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)?

>  	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.

-- 
  Vitaly


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: untangle.patch --]
[-- Type: text/x-patch, Size: 4044 bytes --]

commit b2784f827d2b7b19d3af6025bbe8be5b1450b88c
Author: Vitaly Kuznetsov <vkuznets@redhat.com>
Date:   Fri Dec 11 13:29:40 2015 +0100

    netvsc: untangle rndis_filter_send_request and netvsc_send
    
    Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>

diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
index f5b2145..03da20c 100644
--- a/drivers/net/hyperv/hyperv_net.h
+++ b/drivers/net/hyperv/hyperv_net.h
@@ -189,6 +189,8 @@ int netvsc_device_remove(struct hv_device *device);
 int netvsc_send(struct hv_device *device,
 		struct hv_netvsc_packet *packet,
 		struct rndis_message *rndis_msg);
+int netvsc_send_pkt(struct hv_netvsc_packet *packet,
+		    struct netvsc_device *net_device);
 void netvsc_linkstatus_callback(struct hv_device *device_obj,
 				struct rndis_message *resp);
 void netvsc_xmit_completion(void *context);
@@ -211,6 +213,8 @@ int rndis_filter_receive(struct hv_device *dev,
 int rndis_filter_set_packet_filter(struct rndis_device *dev, u32 new_filter);
 int rndis_filter_set_device_mac(struct hv_device *hdev, char *mac);
 
+struct netvsc_device *netvsc_outbound_net_device(struct hv_device *device);
+
 #define NVSP_INVALID_PROTOCOL_VERSION	((u32)0xFFFFFFFF)
 
 #define NVSP_PROTOCOL_VERSION_1		2
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index 081f14f..eadbb03 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -67,7 +67,7 @@ static void free_netvsc_device(struct netvsc_device *nvdev)
 	kfree(nvdev);
 }
 
-static struct netvsc_device *get_outbound_net_device(struct hv_device *device)
+struct netvsc_device *netvsc_outbound_net_device(struct hv_device *device)
 {
 	struct netvsc_device *net_device;
 
@@ -226,7 +226,7 @@ static int netvsc_init_buf(struct hv_device *device)
 	struct net_device *ndev;
 	int node;
 
-	net_device = get_outbound_net_device(device);
+	net_device = netvsc_outbound_net_device(device);
 	if (!net_device)
 		return -ENODEV;
 	ndev = net_device->ndev;
@@ -478,7 +478,7 @@ static int netvsc_connect_vsp(struct hv_device *device)
 		NVSP_PROTOCOL_VERSION_4, NVSP_PROTOCOL_VERSION_5 };
 	int i, num_ver = 4; /* number of different NVSP versions */
 
-	net_device = get_outbound_net_device(device);
+	net_device = netvsc_outbound_net_device(device);
 	if (!net_device)
 		return -ENODEV;
 	ndev = net_device->ndev;
@@ -740,7 +740,7 @@ static u32 netvsc_copy_to_send_buf(struct netvsc_device *net_device,
 	return msg_size;
 }
 
-static inline int netvsc_send_pkt(
+int netvsc_send_pkt(
 	struct hv_netvsc_packet *packet,
 	struct netvsc_device *net_device)
 {
@@ -850,7 +850,7 @@ int netvsc_send(struct hv_device *device,
 	struct hv_netvsc_packet *msd_send = NULL, *cur_send = NULL;
 	bool try_batch;
 
-	net_device = get_outbound_net_device(device);
+	net_device = netvsc_outbound_net_device(device);
 	if (!net_device)
 		return -ENODEV;
 
@@ -1063,7 +1063,7 @@ static void netvsc_send_table(struct hv_device *hdev,
 	int i;
 	u32 count, *tab;
 
-	nvscdev = get_outbound_net_device(hdev);
+	nvscdev = netvsc_outbound_net_device(hdev);
 	if (!nvscdev)
 		return;
 	ndev = nvscdev->ndev;
diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c
index c8af172..9ee422d 100644
--- a/drivers/net/hyperv/rndis_filter.c
+++ b/drivers/net/hyperv/rndis_filter.c
@@ -210,6 +210,11 @@ static int rndis_filter_send_request(struct rndis_device *dev,
 	int ret;
 	struct hv_netvsc_packet *packet;
 	struct hv_page_buffer page_buf[2];
+	struct netvsc_device *net_device;
+
+	net_device = netvsc_outbound_net_device(dev->net_dev->dev);
+	if (!net_device)
+		return -ENODEV;
 
 	/* Setup the packet to send it */
 	packet = &req->pkt;
@@ -239,8 +244,10 @@ static int rndis_filter_send_request(struct rndis_device *dev,
 
 	packet->completion_func = 0;
 	packet->xmit_more = false;
+	packet->send_buf_index = NETVSC_INVALID_INDEX;
+	packet->cp_partial = false;
 
-	ret = netvsc_send(dev->net_dev->dev, packet, NULL);
+	ret = netvsc_send_pkt(packet, net_device);
 	return ret;
 }
 

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

[Vitaly Kuznetsov]

Vitaly Kuznetsov <vkuznets@redhat.com> writes:

> Haiyang Zhang <haiyangz@microsoft.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@microsoft.com>
>> Reviewed-by: K. Y. Srinivasan <kys@microsoft.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.

-- 
  Vitaly

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

[Haiyang Zhang]

> -----Original Message-----
> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> Sent: Friday, December 11, 2015 8:53 AM
> To: Haiyang Zhang <haiyangz@microsoft.com>
> Cc: davem@davemloft.net; netdev@vger.kernel.org; olaf@aepfle.de;
> jasowang@redhat.com; driverdev-devel@linuxdriverproject.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH net-next] hv_netvsc: Fix race condition on Multi-
> Send Data field
> 
> Vitaly Kuznetsov <vkuznets@redhat.com> writes:
> 
> > Haiyang Zhang <haiyangz@microsoft.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@microsoft.com>
> >> Reviewed-by: K. Y. Srinivasan <kys@microsoft.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.

Thanks for the suggestion.
I still prefer fixing this bug in the netvsc_send(), and keep the common
entry point for sending data & control packets. So, the total lines of
code is smaller. And, when we add more pre-processing steps for send in 
the future, we won't need to change multiple places.

Thanks,
- Haiyang

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[KY Srinivasan]

> -----Original Message-----
> From: devel [mailto:driverdev-devel-bounces@linuxdriverproject.org] On
> Behalf Of Vitaly Kuznetsov
> Sent: Friday, December 11, 2015 5:53 AM
> To: Haiyang Zhang <haiyangz@microsoft.com>
> Cc: olaf@aepfle.de; netdev@vger.kernel.org; jasowang@redhat.com;
> driverdev-devel@linuxdriverproject.org; linux-kernel@vger.kernel.org;
> davem@davemloft.net
> Subject: Re: [PATCH net-next] hv_netvsc: Fix race condition on Multi-Send
> Data field
> 
> Vitaly Kuznetsov <vkuznets@redhat.com> writes:
> 
> > Haiyang Zhang <haiyangz@microsoft.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@microsoft.com>
> >> Reviewed-by: K. Y. Srinivasan <kys@microsoft.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@linuxdriverproject.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

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

[David Miller]

From: Haiyang Zhang <haiyangz@microsoft.com>
Date: Thu, 10 Dec 2015 12:19:35 -0800

> 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@microsoft.com>
> Reviewed-by: K. Y. Srinivasan <kys@microsoft.com>

Applied.