[PATCH net-next 0/4] hv_netvsc: fix some crashes and hangs on channel/mtu changes

[PATCH net-next 0/4] hv_netvsc: fix some crashes and hangs on channel/mtu changes

[Vitaly Kuznetsov]

It was found that netvsc driver doesn't survive e.g.

# while true; do ethtool -L eth0 combined 4; ethtool -L eth0 combined 8; done"

test. I was able to identify several issues: hang in guest/host
communication and a couple of crashes. Fix these. While I'm not
convinced I'm fixing everything VMs seem to survive overnight
test. I'll send one more related patch to VMBus core too.

Vitaly Kuznetsov (4):
  hv_netvsc: netvsc_teardown_gpadl() split
  hv_netvsc: protect nvdev->extension with RCU
  hv_netvsc: reset net_device_ctx->nvdev with rcu_assign_pointer()
  hv_netvsc: hide warnings about uninitialized/missing rndis device

 drivers/net/hyperv/hyperv_net.h   |  2 +-
 drivers/net/hyperv/netvsc.c       | 71 ++++++++++++++++++++-------------------
 drivers/net/hyperv/netvsc_drv.c   | 10 +++---
 drivers/net/hyperv/rndis_filter.c | 47 ++++++++++++++++----------
 4 files changed, 74 insertions(+), 56 deletions(-)

-- 
2.13.6

[PATCH net-next 1/4] hv_netvsc: netvsc_teardown_gpadl() split

[Vitaly Kuznetsov]

It was found that in some cases host refuses to teardown GPADL for send/
receive buffers (probably when some work with these buffere is scheduled or
ongoing). Change the teardown logic to be:
1) Send NVSP_MSG1_TYPE_REVOKE_* messages
2) Close the channel
3) Teardown GPADLs.
This seems to work reliably.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 drivers/net/hyperv/netvsc.c | 69 +++++++++++++++++++++++----------------------
 1 file changed, 36 insertions(+), 33 deletions(-)

diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index 5bb6a20072dd..bfc79698b8f4 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -100,12 +100,11 @@ static void free_netvsc_device_rcu(struct netvsc_device *nvdev)
 	call_rcu(&nvdev->rcu, free_netvsc_device);
 }
 
-static void netvsc_destroy_buf(struct hv_device *device)
+static void netvsc_revoke_buf(struct hv_device *device,
+			      struct netvsc_device *net_device)
 {
 	struct nvsp_message *revoke_packet;
 	struct net_device *ndev = hv_get_drvdata(device);
-	struct net_device_context *ndc = netdev_priv(ndev);
-	struct netvsc_device *net_device = rtnl_dereference(ndc->nvdev);
 	int ret;
 
 	/*
@@ -148,28 +147,6 @@ static void netvsc_destroy_buf(struct hv_device *device)
 		net_device->recv_section_cnt = 0;
 	}
 
-	/* Teardown the gpadl on the vsp end */
-	if (net_device->recv_buf_gpadl_handle) {
-		ret = vmbus_teardown_gpadl(device->channel,
-					   net_device->recv_buf_gpadl_handle);
-
-		/* If we failed here, we might as well return and have a leak
-		 * rather than continue and a bugchk
-		 */
-		if (ret != 0) {
-			netdev_err(ndev,
-				   "unable to teardown receive buffer's gpadl\n");
-			return;
-		}
-		net_device->recv_buf_gpadl_handle = 0;
-	}
-
-	if (net_device->recv_buf) {
-		/* Free up the receive buffer */
-		vfree(net_device->recv_buf);
-		net_device->recv_buf = NULL;
-	}
-
 	/* Deal with the send buffer we may have setup.
 	 * If we got a  send section size, it means we received a
 	 * NVSP_MSG1_TYPE_SEND_SEND_BUF_COMPLETE msg (ie sent
@@ -210,7 +187,35 @@ static void netvsc_destroy_buf(struct hv_device *device)
 		}
 		net_device->send_section_cnt = 0;
 	}
-	/* Teardown the gpadl on the vsp end */
+}
+
+static void netvsc_teardown_gpadl(struct hv_device *device,
+				  struct netvsc_device *net_device)
+{
+	struct net_device *ndev = hv_get_drvdata(device);
+	int ret;
+
+	if (net_device->recv_buf_gpadl_handle) {
+		ret = vmbus_teardown_gpadl(device->channel,
+					   net_device->recv_buf_gpadl_handle);
+
+		/* If we failed here, we might as well return and have a leak
+		 * rather than continue and a bugchk
+		 */
+		if (ret != 0) {
+			netdev_err(ndev,
+				   "unable to teardown receive buffer's gpadl\n");
+			return;
+		}
+		net_device->recv_buf_gpadl_handle = 0;
+	}
+
+	if (net_device->recv_buf) {
+		/* Free up the receive buffer */
+		vfree(net_device->recv_buf);
+		net_device->recv_buf = NULL;
+	}
+
 	if (net_device->send_buf_gpadl_handle) {
 		ret = vmbus_teardown_gpadl(device->channel,
 					   net_device->send_buf_gpadl_handle);
@@ -420,7 +425,8 @@ static int netvsc_init_buf(struct hv_device *device,
 	goto exit;
 
 cleanup:
-	netvsc_destroy_buf(device);
+	netvsc_revoke_buf(device, net_device);
+	netvsc_teardown_gpadl(device, net_device);
 
 exit:
 	return ret;
@@ -539,11 +545,6 @@ static int netvsc_connect_vsp(struct hv_device *device,
 	return ret;
 }
 
-static void netvsc_disconnect_vsp(struct hv_device *device)
-{
-	netvsc_destroy_buf(device);
-}
-
 /*
  * netvsc_device_remove - Callback when the root bus device is removed
  */
@@ -557,7 +558,7 @@ void netvsc_device_remove(struct hv_device *device)
 
 	cancel_work_sync(&net_device->subchan_work);
 
-	netvsc_disconnect_vsp(device);
+	netvsc_revoke_buf(device, net_device);
 
 	RCU_INIT_POINTER(net_device_ctx->nvdev, NULL);
 
@@ -570,6 +571,8 @@ void netvsc_device_remove(struct hv_device *device)
 	/* Now, we can close the channel safely */
 	vmbus_close(device->channel);
 
+	netvsc_teardown_gpadl(device, net_device);
+
 	/* And dissassociate NAPI context from device */
 	for (i = 0; i < net_device->num_chn; i++)
 		netif_napi_del(&net_device->chan_table[i].napi);
-- 
2.13.6

[PATCH net-next 2/4] hv_netvsc: protect nvdev->extension with RCU

[Vitaly Kuznetsov]

rndis_filter_receive() is called from interrupt context and may race with
rndis_filter_device_remove() resetting extension pointer. RNDIS_MSG_HALT
does not help, host may still send us messages after it. Protect extension
pointer with RCU.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 drivers/net/hyperv/hyperv_net.h   |  2 +-
 drivers/net/hyperv/netvsc_drv.c   | 10 +++++----
 drivers/net/hyperv/rndis_filter.c | 43 +++++++++++++++++++++++++--------------
 3 files changed, 35 insertions(+), 20 deletions(-)

diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
index 4958bb6b7376..4f003e85781c 100644
--- a/drivers/net/hyperv/hyperv_net.h
+++ b/drivers/net/hyperv/hyperv_net.h
@@ -798,7 +798,7 @@ struct netvsc_device {
 	struct work_struct subchan_work;
 	wait_queue_head_t subchan_open;
 
-	struct rndis_device *extension;
+	struct rndis_device __rcu *extension;
 
 	int ring_size;
 
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index da216ca4f2b2..8ad018305aa5 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -94,7 +94,7 @@ static int netvsc_open(struct net_device *net)
 
 	netif_tx_wake_all_queues(net);
 
-	rdev = nvdev->extension;
+	rdev = rtnl_dereference(nvdev->extension);
 
 	if (!rdev->link_state)
 		netif_carrier_on(net);
@@ -1431,7 +1431,7 @@ static int netvsc_get_rxfh(struct net_device *dev, u32 *indir, u8 *key,
 	if (hfunc)
 		*hfunc = ETH_RSS_HASH_TOP;	/* Toeplitz */
 
-	rndis_dev = ndev->extension;
+	rndis_dev = rtnl_dereference(ndev->extension);
 	if (indir) {
 		for (i = 0; i < ITAB_NUM; i++)
 			indir[i] = rndis_dev->rx_table[i];
@@ -1457,7 +1457,7 @@ static int netvsc_set_rxfh(struct net_device *dev, const u32 *indir,
 	if (hfunc != ETH_RSS_HASH_NO_CHANGE && hfunc != ETH_RSS_HASH_TOP)
 		return -EOPNOTSUPP;
 
-	rndis_dev = ndev->extension;
+	rndis_dev = rtnl_dereference(ndev->extension);
 	if (indir) {
 		for (i = 0; i < ITAB_NUM; i++)
 			if (indir[i] >= ndev->num_chn)
@@ -1640,7 +1640,7 @@ static void netvsc_link_change(struct work_struct *w)
 	if (!net_device)
 		goto out_unlock;
 
-	rdev = net_device->extension;
+	rdev = rtnl_dereference(net_device->extension);
 
 	next_reconfig = ndev_ctx->last_reconfig + LINKCHANGE_INT;
 	if (time_is_after_jiffies(next_reconfig)) {
@@ -2002,7 +2002,9 @@ static int netvsc_probe(struct hv_device *dev,
 	device_info.recv_sections = NETVSC_DEFAULT_RX;
 	device_info.recv_section_size = NETVSC_RECV_SECTION_SIZE;
 
+	rtnl_lock();
 	nvdev = rndis_filter_device_add(dev, &device_info);
+	rtnl_unlock();
 	if (IS_ERR(nvdev)) {
 		ret = PTR_ERR(nvdev);
 		netdev_err(net, "unable to add netvsc device (ret %d)\n", ret);
diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c
index 0648eebda829..1c31e2b0216e 100644
--- a/drivers/net/hyperv/rndis_filter.c
+++ b/drivers/net/hyperv/rndis_filter.c
@@ -402,20 +402,27 @@ int rndis_filter_receive(struct net_device *ndev,
 			 void *data, u32 buflen)
 {
 	struct net_device_context *net_device_ctx = netdev_priv(ndev);
-	struct rndis_device *rndis_dev = net_dev->extension;
+	struct rndis_device *rndis_dev;
 	struct rndis_message *rndis_msg = data;
+	int ret = 0;
+
+	rcu_read_lock_bh();
+
+	rndis_dev = rcu_dereference_bh(net_dev->extension);
 
 	/* Make sure the rndis device state is initialized */
 	if (unlikely(!rndis_dev)) {
 		netif_err(net_device_ctx, rx_err, ndev,
 			  "got rndis message but no rndis device!\n");
-		return NVSP_STAT_FAIL;
+		ret = NVSP_STAT_FAIL;
+		goto unlock;
 	}
 
 	if (unlikely(rndis_dev->state == RNDIS_DEV_UNINITIALIZED)) {
 		netif_err(net_device_ctx, rx_err, ndev,
 			  "got rndis message uninitialized\n");
-		return NVSP_STAT_FAIL;
+		ret = NVSP_STAT_FAIL;
+		goto unlock;
 	}
 
 	if (netif_msg_rx_status(net_device_ctx))
@@ -423,8 +430,9 @@ int rndis_filter_receive(struct net_device *ndev,
 
 	switch (rndis_msg->ndis_msg_type) {
 	case RNDIS_MSG_PACKET:
-		return rndis_filter_receive_data(ndev, rndis_dev, rndis_msg,
-						 channel, data, buflen);
+		ret = rndis_filter_receive_data(ndev, rndis_dev, rndis_msg,
+						channel, data, buflen);
+		break;
 	case RNDIS_MSG_INIT_C:
 	case RNDIS_MSG_QUERY_C:
 	case RNDIS_MSG_SET_C:
@@ -444,7 +452,10 @@ int rndis_filter_receive(struct net_device *ndev,
 		break;
 	}
 
-	return 0;
+unlock:
+	rcu_read_unlock_bh();
+
+	return ret;
 }
 
 static int rndis_filter_query_device(struct rndis_device *dev,
@@ -597,7 +608,7 @@ static int rndis_filter_query_device_mac(struct rndis_device *dev,
 int rndis_filter_set_device_mac(struct netvsc_device *nvdev,
 				const char *mac)
 {
-	struct rndis_device *rdev = nvdev->extension;
+	struct rndis_device *rdev = rtnl_dereference(nvdev->extension);
 	struct rndis_request *request;
 	struct rndis_set_request *set;
 	struct rndis_config_parameter_info *cpi;
@@ -663,7 +674,7 @@ rndis_filter_set_offload_params(struct net_device *ndev,
 				struct netvsc_device *nvdev,
 				struct ndis_offload_params *req_offloads)
 {
-	struct rndis_device *rdev = nvdev->extension;
+	struct rndis_device *rdev = rtnl_dereference(nvdev->extension);
 	struct rndis_request *request;
 	struct rndis_set_request *set;
 	struct ndis_offload_params *offload_params;
@@ -868,7 +879,7 @@ static void rndis_set_multicast(struct work_struct *w)
 
 void rndis_filter_update(struct netvsc_device *nvdev)
 {
-	struct rndis_device *rdev = nvdev->extension;
+	struct rndis_device *rdev = rtnl_dereference(nvdev->extension);
 
 	schedule_work(&rdev->mcast_work);
 }
@@ -1072,7 +1083,7 @@ void rndis_set_subchannel(struct work_struct *w)
 		return;
 	}
 
-	rdev = nvdev->extension;
+	rdev = rtnl_dereference(nvdev->extension);
 	if (!rdev)
 		goto unlock;	/* device was removed */
 
@@ -1167,7 +1178,7 @@ struct netvsc_device *rndis_filter_device_add(struct hv_device *dev,
 	net_device->max_chn = 1;
 	net_device->num_chn = 1;
 
-	net_device->extension = rndis_device;
+	rcu_assign_pointer(net_device->extension, rndis_device);
 	rndis_device->ndev = net;
 
 	/* Send the rndis initialization message */
@@ -1326,12 +1337,14 @@ struct netvsc_device *rndis_filter_device_add(struct hv_device *dev,
 void rndis_filter_device_remove(struct hv_device *dev,
 				struct netvsc_device *net_dev)
 {
-	struct rndis_device *rndis_dev = net_dev->extension;
+	struct rndis_device *rndis_dev = rtnl_dereference(net_dev->extension);
 
 	/* Halt and release the rndis device */
 	rndis_filter_halt_device(rndis_dev);
 
-	net_dev->extension = NULL;
+	rcu_assign_pointer(net_dev->extension, NULL);
+
+	synchronize_rcu();
 
 	netvsc_device_remove(dev);
 	kfree(rndis_dev);
@@ -1345,7 +1358,7 @@ int rndis_filter_open(struct netvsc_device *nvdev)
 	if (atomic_inc_return(&nvdev->open_cnt) != 1)
 		return 0;
 
-	return rndis_filter_open_device(nvdev->extension);
+	return rndis_filter_open_device(rtnl_dereference(nvdev->extension));
 }
 
 int rndis_filter_close(struct netvsc_device *nvdev)
@@ -1356,7 +1369,7 @@ int rndis_filter_close(struct netvsc_device *nvdev)
 	if (atomic_dec_return(&nvdev->open_cnt) != 0)
 		return 0;
 
-	return rndis_filter_close_device(nvdev->extension);
+	return rndis_filter_close_device(rtnl_dereference(nvdev->extension));
 }
 
 bool rndis_filter_opened(const struct netvsc_device *nvdev)
-- 
2.13.6

[PATCH net-next 3/4] hv_netvsc: reset net_device_ctx->nvdev with rcu_assign_pointer()

[Vitaly Kuznetsov]

RCU_INIT_POINTER() is not suitable here as it doesn't give us ordering
guarantees (see the comment in rcupdate.h). This is also not a hotpath.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 drivers/net/hyperv/netvsc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index bfc79698b8f4..12efb3e34775 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -560,7 +560,7 @@ void netvsc_device_remove(struct hv_device *device)
 
 	netvsc_revoke_buf(device, net_device);
 
-	RCU_INIT_POINTER(net_device_ctx->nvdev, NULL);
+	rcu_assign_pointer(net_device_ctx->nvdev, NULL);
 
 	/*
 	 * At this point, no one should be accessing net_device
-- 
2.13.6

[PATCH net-next 4/4] hv_netvsc: hide warnings about uninitialized/missing rndis device

[Vitaly Kuznetsov]

Hyper-V hosts are known to send RNDIS messages even after we halt the
device in rndis_filter_halt_device(). Remove user visible messages
as they are not really useful.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 drivers/net/hyperv/rndis_filter.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c
index 1c31e2b0216e..77cea50d0945 100644
--- a/drivers/net/hyperv/rndis_filter.c
+++ b/drivers/net/hyperv/rndis_filter.c
@@ -412,14 +412,14 @@ int rndis_filter_receive(struct net_device *ndev,
 
 	/* Make sure the rndis device state is initialized */
 	if (unlikely(!rndis_dev)) {
-		netif_err(net_device_ctx, rx_err, ndev,
+		netif_dbg(net_device_ctx, rx_err, ndev,
 			  "got rndis message but no rndis device!\n");
 		ret = NVSP_STAT_FAIL;
 		goto unlock;
 	}
 
 	if (unlikely(rndis_dev->state == RNDIS_DEV_UNINITIALIZED)) {
-		netif_err(net_device_ctx, rx_err, ndev,
+		netif_dbg(net_device_ctx, rx_err, ndev,
 			  "got rndis message uninitialized\n");
 		ret = NVSP_STAT_FAIL;
 		goto unlock;
-- 
2.13.6

Re: [PATCH net-next 3/4] hv_netvsc: reset net_device_ctx->nvdev with rcu_assign_pointer()

[Eric Dumazet]

On Tue, 2017-10-31 at 14:42 +0100, Vitaly Kuznetsov wrote:
> RCU_INIT_POINTER() is not suitable here as it doesn't give us ordering
> guarantees (see the comment in rcupdate.h). This is also not a hotpath.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  drivers/net/hyperv/netvsc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
> index bfc79698b8f4..12efb3e34775 100644
> --- a/drivers/net/hyperv/netvsc.c
> +++ b/drivers/net/hyperv/netvsc.c
> @@ -560,7 +560,7 @@ void netvsc_device_remove(struct hv_device *device)
>  
>  	netvsc_revoke_buf(device, net_device);
>  
> -	RCU_INIT_POINTER(net_device_ctx->nvdev, NULL);
> +	rcu_assign_pointer(net_device_ctx->nvdev, NULL);

I see no point for this patch.

Setting a NULL pointer needs no barrier at all.

Re: [PATCH net-next 3/4] hv_netvsc: reset net_device_ctx->nvdev with rcu_assign_pointer()

[Vitaly Kuznetsov]

Eric Dumazet <eric.dumazet@gmail.com> writes:

> On Tue, 2017-10-31 at 14:42 +0100, Vitaly Kuznetsov wrote:
>> RCU_INIT_POINTER() is not suitable here as it doesn't give us ordering
>> guarantees (see the comment in rcupdate.h). This is also not a hotpath.
>> 
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>>  drivers/net/hyperv/netvsc.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
>> index bfc79698b8f4..12efb3e34775 100644
>> --- a/drivers/net/hyperv/netvsc.c
>> +++ b/drivers/net/hyperv/netvsc.c
>> @@ -560,7 +560,7 @@ void netvsc_device_remove(struct hv_device *device)
>>  
>>  	netvsc_revoke_buf(device, net_device);
>>  
>> -	RCU_INIT_POINTER(net_device_ctx->nvdev, NULL);
>> +	rcu_assign_pointer(net_device_ctx->nvdev, NULL);
>
> I see no point for this patch.
>
> Setting a NULL pointer needs no barrier at all.

Oh, sorry, I got confused by the comment near RCU_INIT_POINTER() in
rcupdate.h. Now looking at their definitions I see.

This patch can of course be dropped from the series.

-- 
  Vitaly

Re: [PATCH net-next 3/4] hv_netvsc: reset net_device_ctx->nvdev with rcu_assign_pointer()

[David Miller]

From: Vitaly Kuznetsov <vkuznets@redhat.com>
Date: Tue, 31 Oct 2017 15:40:06 +0100

> Eric Dumazet <eric.dumazet@gmail.com> writes:
> 
>> On Tue, 2017-10-31 at 14:42 +0100, Vitaly Kuznetsov wrote:
>>> RCU_INIT_POINTER() is not suitable here as it doesn't give us ordering
>>> guarantees (see the comment in rcupdate.h). This is also not a hotpath.
>>> 
>>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>>> ---
>>>  drivers/net/hyperv/netvsc.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>> 
>>> diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
>>> index bfc79698b8f4..12efb3e34775 100644
>>> --- a/drivers/net/hyperv/netvsc.c
>>> +++ b/drivers/net/hyperv/netvsc.c
>>> @@ -560,7 +560,7 @@ void netvsc_device_remove(struct hv_device *device)
>>>  
>>>  	netvsc_revoke_buf(device, net_device);
>>>  
>>> -	RCU_INIT_POINTER(net_device_ctx->nvdev, NULL);
>>> +	rcu_assign_pointer(net_device_ctx->nvdev, NULL);
>>
>> I see no point for this patch.
>>
>> Setting a NULL pointer needs no barrier at all.
> 
> Oh, sorry, I got confused by the comment near RCU_INIT_POINTER() in
> rcupdate.h. Now looking at their definitions I see.
> 
> This patch can of course be dropped from the series.

Any time there is a change to the series, you must resubmit the entire
series.

Thank you.

Re: [PATCH net-next 3/4] hv_netvsc: reset net_device_ctx->nvdev with rcu_assign_pointer()

[Vitaly Kuznetsov]

David Miller <davem@davemloft.net> writes:

> From: Vitaly Kuznetsov <vkuznets@redhat.com>
> Date: Tue, 31 Oct 2017 15:40:06 +0100
>
>> Eric Dumazet <eric.dumazet@gmail.com> writes:
>> 
>>> On Tue, 2017-10-31 at 14:42 +0100, Vitaly Kuznetsov wrote:
>>>> RCU_INIT_POINTER() is not suitable here as it doesn't give us ordering
>>>> guarantees (see the comment in rcupdate.h). This is also not a hotpath.
>>>> 
>>>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>>>> ---
>>>>  drivers/net/hyperv/netvsc.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>> 
>>>> diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
>>>> index bfc79698b8f4..12efb3e34775 100644
>>>> --- a/drivers/net/hyperv/netvsc.c
>>>> +++ b/drivers/net/hyperv/netvsc.c
>>>> @@ -560,7 +560,7 @@ void netvsc_device_remove(struct hv_device *device)
>>>>  
>>>>  	netvsc_revoke_buf(device, net_device);
>>>>  
>>>> -	RCU_INIT_POINTER(net_device_ctx->nvdev, NULL);
>>>> +	rcu_assign_pointer(net_device_ctx->nvdev, NULL);
>>>
>>> I see no point for this patch.
>>>
>>> Setting a NULL pointer needs no barrier at all.
>> 
>> Oh, sorry, I got confused by the comment near RCU_INIT_POINTER() in
>> rcupdate.h. Now looking at their definitions I see.
>> 
>> This patch can of course be dropped from the series.
>
> Any time there is a change to the series, you must resubmit the entire
> series.
>

Sure, will do. Just wanted to give it a couple of days to see if
Microsoft guys or someone else have any comments.

Thanks,

-- 
  Vitaly

Re: [PATCH net-next 1/4] hv_netvsc: netvsc_teardown_gpadl() split

[Stephen Hemminger]

On Tue, 31 Oct 2017 14:42:01 +0100
Vitaly Kuznetsov <vkuznets@redhat.com> wrote:

> It was found that in some cases host refuses to teardown GPADL for send/
> receive buffers (probably when some work with these buffere is scheduled or
> ongoing). Change the teardown logic to be:
> 1) Send NVSP_MSG1_TYPE_REVOKE_* messages
> 2) Close the channel
> 3) Teardown GPADLs.
> This seems to work reliably.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  drivers/net/hyperv/netvsc.c | 69 +++++++++++++++++++++++----------------------
>  1 file changed, 36 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
> index 5bb6a20072dd..bfc79698b8f4 100644
> --- a/drivers/net/hyperv/netvsc.c
> +++ b/drivers/net/hyperv/netvsc.c
> @@ -100,12 +100,11 @@ static void free_netvsc_device_rcu(struct netvsc_device *nvdev)
>  	call_rcu(&nvdev->rcu, free_netvsc_device);
>  }
>  
> -static void netvsc_destroy_buf(struct hv_device *device)
> +static void netvsc_revoke_buf(struct hv_device *device,
> +			      struct netvsc_device *net_device)
>  {
>  	struct nvsp_message *revoke_packet;
>  	struct net_device *ndev = hv_get_drvdata(device);
> -	struct net_device_context *ndc = netdev_priv(ndev);
> -	struct netvsc_device *net_device = rtnl_dereference(ndc->nvdev);
>  	int ret;
>  
>  	/*
> @@ -148,28 +147,6 @@ static void netvsc_destroy_buf(struct hv_device *device)
>  		net_device->recv_section_cnt = 0;
>  	}
>  
> -	/* Teardown the gpadl on the vsp end */
> -	if (net_device->recv_buf_gpadl_handle) {
> -		ret = vmbus_teardown_gpadl(device->channel,
> -					   net_device->recv_buf_gpadl_handle);
> -
> -		/* If we failed here, we might as well return and have a leak
> -		 * rather than continue and a bugchk
> -		 */
> -		if (ret != 0) {
> -			netdev_err(ndev,
> -				   "unable to teardown receive buffer's gpadl\n");
> -			return;
> -		}
> -		net_device->recv_buf_gpadl_handle = 0;
> -	}
> -
> -	if (net_device->recv_buf) {
> -		/* Free up the receive buffer */
> -		vfree(net_device->recv_buf);
> -		net_device->recv_buf = NULL;
> -	}
> -
>  	/* Deal with the send buffer we may have setup.
>  	 * If we got a  send section size, it means we received a
>  	 * NVSP_MSG1_TYPE_SEND_SEND_BUF_COMPLETE msg (ie sent
> @@ -210,7 +187,35 @@ static void netvsc_destroy_buf(struct hv_device *device)
>  		}
>  		net_device->send_section_cnt = 0;
>  	}
> -	/* Teardown the gpadl on the vsp end */
> +}
> +
> +static void netvsc_teardown_gpadl(struct hv_device *device,
> +				  struct netvsc_device *net_device)
> +{
> +	struct net_device *ndev = hv_get_drvdata(device);
> +	int ret;
> +
> +	if (net_device->recv_buf_gpadl_handle) {
> +		ret = vmbus_teardown_gpadl(device->channel,
> +					   net_device->recv_buf_gpadl_handle);
> +
> +		/* If we failed here, we might as well return and have a leak
> +		 * rather than continue and a bugchk
> +		 */
> +		if (ret != 0) {
> +			netdev_err(ndev,
> +				   "unable to teardown receive buffer's gpadl\n");
> +			return;
> +		}
> +		net_device->recv_buf_gpadl_handle = 0;
> +	}
> +
> +	if (net_device->recv_buf) {
> +		/* Free up the receive buffer */
> +		vfree(net_device->recv_buf);
> +		net_device->recv_buf = NULL;
> +	}
> +
>  	if (net_device->send_buf_gpadl_handle) {
>  		ret = vmbus_teardown_gpadl(device->channel,
>  					   net_device->send_buf_gpadl_handle);
> @@ -420,7 +425,8 @@ static int netvsc_init_buf(struct hv_device *device,
>  	goto exit;
>  
>  cleanup:
> -	netvsc_destroy_buf(device);
> +	netvsc_revoke_buf(device, net_device);
> +	netvsc_teardown_gpadl(device, net_device);
>  
>  exit:
>  	return ret;
> @@ -539,11 +545,6 @@ static int netvsc_connect_vsp(struct hv_device *device,
>  	return ret;
>  }
>  
> -static void netvsc_disconnect_vsp(struct hv_device *device)
> -{
> -	netvsc_destroy_buf(device);
> -}
> -
>  /*
>   * netvsc_device_remove - Callback when the root bus device is removed
>   */
> @@ -557,7 +558,7 @@ void netvsc_device_remove(struct hv_device *device)
>  
>  	cancel_work_sync(&net_device->subchan_work);
>  
> -	netvsc_disconnect_vsp(device);
> +	netvsc_revoke_buf(device, net_device);
>  
>  	RCU_INIT_POINTER(net_device_ctx->nvdev, NULL);
>  
> @@ -570,6 +571,8 @@ void netvsc_device_remove(struct hv_device *device)
>  	/* Now, we can close the channel safely */
>  	vmbus_close(device->channel);
>  
> +	netvsc_teardown_gpadl(device, net_device);
> +
>  	/* And dissassociate NAPI context from device */
>  	for (i = 0; i < net_device->num_chn; i++)
>  		netif_napi_del(&net_device->chan_table[i].napi);

This patch makes sense,

Re: [PATCH net-next 2/4] hv_netvsc: protect nvdev->extension with RCU

[Stephen Hemminger]

On Tue, 31 Oct 2017 14:42:02 +0100
Vitaly Kuznetsov <vkuznets@redhat.com> wrote:

> @@ -2002,7 +2002,9 @@ static int netvsc_probe(struct hv_device *dev,
>  	device_info.recv_sections = NETVSC_DEFAULT_RX;
>  	device_info.recv_section_size = NETVSC_RECV_SECTION_SIZE;
>  
> +	rtnl_lock();
>  	nvdev = rndis_filter_device_add(dev, &device_info);
> +	rtnl_unlock();

rtnl is not necessary here. probe can not be bothered by other changes.

> --- a/drivers/net/hyperv/rndis_filter.c
> +++ b/drivers/net/hyperv/rndis_filter.c
> @@ -402,20 +402,27 @@ int rndis_filter_receive(struct net_device *ndev,
>  			 void *data, u32 buflen)
>  {
>  	struct net_device_context *net_device_ctx = netdev_priv(ndev);
> -	struct rndis_device *rndis_dev = net_dev->extension;
> +	struct rndis_device *rndis_dev;
>  	struct rndis_message *rndis_msg = data;
> +	int ret = 0;
> +
> +	rcu_read_lock_bh();
> +
> +	rndis_dev = rcu_dereference_bh(net_dev->extension);

filter_receive is already called only from NAPI only and has RCU lock and soft
irq disabled. This is not necessary.

> -	net_dev->extension = NULL;
> +	rcu_assign_pointer(net_dev->extension, NULL);
> +
> +	synchronize_rcu();

rcu_assign_pointer with NULL is never a good idea.
And synchronize_rcu is slow. Since net_device is already protected
by RCU (for deletion) it should not be necessary.


Thank you for trying to address these races. But it should be
done carefully not by just slapping RCU everywhere.

Re: [PATCH net-next 3/4] hv_netvsc: reset net_device_ctx->nvdev with rcu_assign_pointer()

[Stephen Hemminger]

On Tue, 31 Oct 2017 07:09:58 -0700
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> On Tue, 2017-10-31 at 14:42 +0100, Vitaly Kuznetsov wrote:
> > RCU_INIT_POINTER() is not suitable here as it doesn't give us ordering
> > guarantees (see the comment in rcupdate.h). This is also not a hotpath.
> > 
> > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> > ---
> >  drivers/net/hyperv/netvsc.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
> > index bfc79698b8f4..12efb3e34775 100644
> > --- a/drivers/net/hyperv/netvsc.c
> > +++ b/drivers/net/hyperv/netvsc.c
> > @@ -560,7 +560,7 @@ void netvsc_device_remove(struct hv_device *device)
> >  
> >  	netvsc_revoke_buf(device, net_device);
> >  
> > -	RCU_INIT_POINTER(net_device_ctx->nvdev, NULL);
> > +	rcu_assign_pointer(net_device_ctx->nvdev, NULL);  
> 
> I see no point for this patch.
> 
> Setting a NULL pointer needs no barrier at all.

Agreed with Eric.

Re: [PATCH net-next 2/4] hv_netvsc: protect nvdev->extension with RCU

[Vitaly Kuznetsov]

Stephen Hemminger <stephen@networkplumber.org> writes:

> On Tue, 31 Oct 2017 14:42:02 +0100
> Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>
>> @@ -2002,7 +2002,9 @@ static int netvsc_probe(struct hv_device *dev,
>>  	device_info.recv_sections = NETVSC_DEFAULT_RX;
>>  	device_info.recv_section_size = NETVSC_RECV_SECTION_SIZE;
>>  
>> +	rtnl_lock();
>>  	nvdev = rndis_filter_device_add(dev, &device_info);
>> +	rtnl_unlock();
>
> rtnl is not necessary here. probe can not be bothered by other changes.
>

Yes, this is only to support rtnl_dereference() down the stack.

>> --- a/drivers/net/hyperv/rndis_filter.c
>> +++ b/drivers/net/hyperv/rndis_filter.c
>> @@ -402,20 +402,27 @@ int rndis_filter_receive(struct net_device *ndev,
>>  			 void *data, u32 buflen)
>>  {
>>  	struct net_device_context *net_device_ctx = netdev_priv(ndev);
>> -	struct rndis_device *rndis_dev = net_dev->extension;
>> +	struct rndis_device *rndis_dev;
>>  	struct rndis_message *rndis_msg = data;
>> +	int ret = 0;
>> +
>> +	rcu_read_lock_bh();
>> +
>> +	rndis_dev = rcu_dereference_bh(net_dev->extension);
>
> filter_receive is already called only from NAPI only and has RCU lock and soft
> irq disabled. This is not necessary.
>
>> -	net_dev->extension = NULL;
>> +	rcu_assign_pointer(net_dev->extension, NULL);
>> +
>> +	synchronize_rcu();
>
> rcu_assign_pointer with NULL is never a good idea.
> And synchronize_rcu is slow. Since net_device is already protected
> by RCU (for deletion) it should not be necessary.
>

I thought we don't care that much about the speed of this patch as
rndis_filter_device_remove() is only called on device remove/mtu
change/... and we need to interact with the host -- and this is already
slow.

> Thank you for trying to address these races. But it should be
> done carefully not by just slapping RCU everywhere.

Ok, I may have missed something. I'll try reproducing the crash and
finding a better fine-grained solution.

Thanks for the feedback!

-- 
  Vitaly

Re: [PATCH net-next 2/4] hv_netvsc: protect nvdev->extension with RCU

[Vitaly Kuznetsov]

Vitaly Kuznetsov <vkuznets@redhat.com> writes:

>
> Ok, I may have missed something. I'll try reproducing the crash and
> finding a better fine-grained solution.
>

It's been two days and I'm failing to reproduce the crash in
rndis_filter_receive() in my environment so I'll probably just re-send
patches 1 and 4 of this series as v2.

-- 
  Vitaly