All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net-next] hv_netvsc: Fix race condition on Multi-Send Data field
@ 2015-12-10 20:19 ` Haiyang Zhang
  0 siblings, 0 replies; 14+ messages in thread
From: Haiyang Zhang @ 2015-12-10 20:19 UTC (permalink / raw)
  To: davem, netdev
  Cc: haiyangz, kys, olaf, jasowang, linux-kernel, driverdev-devel

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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH net-next] hv_netvsc: Fix race condition on Multi-Send Data field
@ 2015-12-10 20:19 ` Haiyang Zhang
  0 siblings, 0 replies; 14+ messages in thread
From: Haiyang Zhang @ 2015-12-10 20:19 UTC (permalink / raw)
  To: davem, netdev; +Cc: olaf, jasowang, driverdev-devel, linux-kernel, haiyangz

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH net-next] hv_netvsc: Fix race condition on Multi-Send Data field
@ 2015-12-10 20:19 ` Haiyang Zhang
  0 siblings, 0 replies; 14+ messages in thread
From: Haiyang Zhang @ 2015-12-10 20:19 UTC (permalink / raw)
  To: davem, netdev; +Cc: olaf, jasowang, driverdev-devel, linux-kernel, haiyangz

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

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH net-next] hv_netvsc: Fix race condition on Multi-Send Data field
  2015-12-10 20:19 ` Haiyang Zhang
@ 2015-12-11 12:34   ` Vitaly Kuznetsov
  -1 siblings, 0 replies; 14+ messages in thread
From: Vitaly Kuznetsov @ 2015-12-11 12:34 UTC (permalink / raw)
  To: Haiyang Zhang
  Cc: davem, netdev, olaf, jasowang, driverdev-devel, linux-kernel

[-- 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;
 }
 

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH net-next] hv_netvsc: Fix race condition on Multi-Send Data field
@ 2015-12-11 12:34   ` Vitaly Kuznetsov
  0 siblings, 0 replies; 14+ messages in thread
From: Vitaly Kuznetsov @ 2015-12-11 12:34 UTC (permalink / raw)
  To: Haiyang Zhang
  Cc: olaf, netdev, jasowang, driverdev-devel, linux-kernel, davem

[-- 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;
 }
 

[-- Attachment #3: Type: text/plain, Size: 169 bytes --]

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH net-next] hv_netvsc: Fix race condition on Multi-Send Data field
  2015-12-11 12:34   ` Vitaly Kuznetsov
@ 2015-12-11 13:52     ` Vitaly Kuznetsov
  -1 siblings, 0 replies; 14+ messages in thread
From: Vitaly Kuznetsov @ 2015-12-11 13:52 UTC (permalink / raw)
  To: Haiyang Zhang
  Cc: davem, netdev, olaf, jasowang, driverdev-devel, linux-kernel

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH net-next] hv_netvsc: Fix race condition on Multi-Send Data field
@ 2015-12-11 13:52     ` Vitaly Kuznetsov
  0 siblings, 0 replies; 14+ messages in thread
From: Vitaly Kuznetsov @ 2015-12-11 13:52 UTC (permalink / raw)
  To: Haiyang Zhang
  Cc: olaf, netdev, jasowang, driverdev-devel, linux-kernel, davem

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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: [PATCH net-next] hv_netvsc: Fix race condition on Multi-Send Data field
  2015-12-11 13:52     ` Vitaly Kuznetsov
@ 2015-12-11 15:42       ` Haiyang Zhang
  -1 siblings, 0 replies; 14+ messages in thread
From: Haiyang Zhang @ 2015-12-11 15:42 UTC (permalink / raw)
  To: Vitaly Kuznetsov
  Cc: davem, netdev, olaf, jasowang, driverdev-devel, linux-kernel

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3423 bytes --]



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

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: [PATCH net-next] hv_netvsc: Fix race condition on Multi-Send Data field
@ 2015-12-11 15:42       ` Haiyang Zhang
  0 siblings, 0 replies; 14+ messages in thread
From: Haiyang Zhang @ 2015-12-11 15:42 UTC (permalink / raw)
  To: Vitaly Kuznetsov
  Cc: olaf, netdev, jasowang, driverdev-devel, linux-kernel, davem



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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: [PATCH net-next] hv_netvsc: Fix race condition on Multi-Send Data field
  2015-12-11 13:52     ` Vitaly Kuznetsov
  (?)
@ 2015-12-11 16:00       ` KY Srinivasan
  -1 siblings, 0 replies; 14+ messages in thread
From: KY Srinivasan @ 2015-12-11 16:00 UTC (permalink / raw)
  To: Vitaly Kuznetsov, Haiyang Zhang
  Cc: olaf, netdev, jasowang, driverdev-devel, linux-kernel, davem

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3703 bytes --]



> -----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
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: [PATCH net-next] hv_netvsc: Fix race condition on Multi-Send Data field
@ 2015-12-11 16:00       ` KY Srinivasan
  0 siblings, 0 replies; 14+ messages in thread
From: KY Srinivasan @ 2015-12-11 16:00 UTC (permalink / raw)
  To: Vitaly Kuznetsov, Haiyang Zhang
  Cc: olaf, netdev, jasowang, driverdev-devel, linux-kernel, davem



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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: [PATCH net-next] hv_netvsc: Fix race condition on Multi-Send Data field
@ 2015-12-11 16:00       ` KY Srinivasan
  0 siblings, 0 replies; 14+ messages in thread
From: KY Srinivasan @ 2015-12-11 16:00 UTC (permalink / raw)
  To: Vitaly Kuznetsov, Haiyang Zhang
  Cc: olaf, netdev, jasowang, driverdev-devel, linux-kernel, davem



> -----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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH net-next] hv_netvsc: Fix race condition on Multi-Send Data field
  2015-12-10 20:19 ` Haiyang Zhang
@ 2015-12-14  5:02   ` David Miller
  -1 siblings, 0 replies; 14+ messages in thread
From: David Miller @ 2015-12-14  5:02 UTC (permalink / raw)
  To: haiyangz; +Cc: netdev, kys, olaf, jasowang, linux-kernel, driverdev-devel

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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH net-next] hv_netvsc: Fix race condition on Multi-Send Data field
@ 2015-12-14  5:02   ` David Miller
  0 siblings, 0 replies; 14+ messages in thread
From: David Miller @ 2015-12-14  5:02 UTC (permalink / raw)
  To: haiyangz; +Cc: olaf, netdev, jasowang, driverdev-devel, linux-kernel

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.
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2015-12-14  5:02 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-10 20:19 [PATCH net-next] hv_netvsc: Fix race condition on Multi-Send Data field Haiyang Zhang
2015-12-10 20:19 ` Haiyang Zhang
2015-12-10 20:19 ` Haiyang Zhang
2015-12-11 12:34 ` Vitaly Kuznetsov
2015-12-11 12:34   ` Vitaly Kuznetsov
2015-12-11 13:52   ` Vitaly Kuznetsov
2015-12-11 13:52     ` Vitaly Kuznetsov
2015-12-11 15:42     ` Haiyang Zhang
2015-12-11 15:42       ` Haiyang Zhang
2015-12-11 16:00     ` KY Srinivasan
2015-12-11 16:00       ` KY Srinivasan
2015-12-11 16:00       ` KY Srinivasan
2015-12-14  5:02 ` David Miller
2015-12-14  5:02   ` David Miller

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.