* [PATCH net-next 1/4] hv_netvsc: netvsc_teardown_gpadl() split
2017-10-31 13:42 [PATCH net-next 0/4] hv_netvsc: fix some crashes and hangs on channel/mtu changes Vitaly Kuznetsov
@ 2017-10-31 13:42 ` Vitaly Kuznetsov
2017-10-31 16:37 ` Stephen Hemminger
2017-10-31 13:42 ` [PATCH net-next 2/4] hv_netvsc: protect nvdev->extension with RCU Vitaly Kuznetsov
` (2 subsequent siblings)
3 siblings, 1 reply; 14+ messages in thread
From: Vitaly Kuznetsov @ 2017-10-31 13:42 UTC (permalink / raw)
To: netdev
Cc: linux-kernel, devel, K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger
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
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH net-next 1/4] hv_netvsc: netvsc_teardown_gpadl() split
2017-10-31 13:42 ` [PATCH net-next 1/4] hv_netvsc: netvsc_teardown_gpadl() split Vitaly Kuznetsov
@ 2017-10-31 16:37 ` Stephen Hemminger
0 siblings, 0 replies; 14+ messages in thread
From: Stephen Hemminger @ 2017-10-31 16:37 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: netdev, devel, Haiyang Zhang, Stephen Hemminger, linux-kernel
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,
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH net-next 2/4] hv_netvsc: protect nvdev->extension with RCU
2017-10-31 13:42 [PATCH net-next 0/4] hv_netvsc: fix some crashes and hangs on channel/mtu changes Vitaly Kuznetsov
2017-10-31 13:42 ` [PATCH net-next 1/4] hv_netvsc: netvsc_teardown_gpadl() split Vitaly Kuznetsov
@ 2017-10-31 13:42 ` Vitaly Kuznetsov
2017-10-31 16:44 ` Stephen Hemminger
2017-10-31 13:42 ` [PATCH net-next 3/4] hv_netvsc: reset net_device_ctx->nvdev with rcu_assign_pointer() Vitaly Kuznetsov
2017-10-31 13:42 ` [PATCH net-next 4/4] hv_netvsc: hide warnings about uninitialized/missing rndis device Vitaly Kuznetsov
3 siblings, 1 reply; 14+ messages in thread
From: Vitaly Kuznetsov @ 2017-10-31 13:42 UTC (permalink / raw)
To: netdev
Cc: linux-kernel, devel, K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger
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
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH net-next 2/4] hv_netvsc: protect nvdev->extension with RCU
2017-10-31 13:42 ` [PATCH net-next 2/4] hv_netvsc: protect nvdev->extension with RCU Vitaly Kuznetsov
@ 2017-10-31 16:44 ` Stephen Hemminger
2017-10-31 17:13 ` Vitaly Kuznetsov
0 siblings, 1 reply; 14+ messages in thread
From: Stephen Hemminger @ 2017-10-31 16:44 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: netdev, devel, Haiyang Zhang, Stephen Hemminger, linux-kernel
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.
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH net-next 2/4] hv_netvsc: protect nvdev->extension with RCU
2017-10-31 16:44 ` Stephen Hemminger
@ 2017-10-31 17:13 ` Vitaly Kuznetsov
2017-11-02 10:27 ` Vitaly Kuznetsov
0 siblings, 1 reply; 14+ messages in thread
From: Vitaly Kuznetsov @ 2017-10-31 17:13 UTC (permalink / raw)
To: Stephen Hemminger
Cc: netdev, devel, Haiyang Zhang, Stephen Hemminger, linux-kernel
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH net-next 2/4] hv_netvsc: protect nvdev->extension with RCU
2017-10-31 17:13 ` Vitaly Kuznetsov
@ 2017-11-02 10:27 ` Vitaly Kuznetsov
0 siblings, 0 replies; 14+ messages in thread
From: Vitaly Kuznetsov @ 2017-11-02 10:27 UTC (permalink / raw)
To: Stephen Hemminger
Cc: netdev, devel, Haiyang Zhang, Stephen Hemminger, linux-kernel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH net-next 3/4] hv_netvsc: reset net_device_ctx->nvdev with rcu_assign_pointer()
2017-10-31 13:42 [PATCH net-next 0/4] hv_netvsc: fix some crashes and hangs on channel/mtu changes Vitaly Kuznetsov
2017-10-31 13:42 ` [PATCH net-next 1/4] hv_netvsc: netvsc_teardown_gpadl() split Vitaly Kuznetsov
2017-10-31 13:42 ` [PATCH net-next 2/4] hv_netvsc: protect nvdev->extension with RCU Vitaly Kuznetsov
@ 2017-10-31 13:42 ` Vitaly Kuznetsov
2017-10-31 14:09 ` Eric Dumazet
2017-10-31 13:42 ` [PATCH net-next 4/4] hv_netvsc: hide warnings about uninitialized/missing rndis device Vitaly Kuznetsov
3 siblings, 1 reply; 14+ messages in thread
From: Vitaly Kuznetsov @ 2017-10-31 13:42 UTC (permalink / raw)
To: netdev
Cc: linux-kernel, devel, K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH net-next 3/4] hv_netvsc: reset net_device_ctx->nvdev with rcu_assign_pointer()
2017-10-31 13:42 ` [PATCH net-next 3/4] hv_netvsc: reset net_device_ctx->nvdev with rcu_assign_pointer() Vitaly Kuznetsov
@ 2017-10-31 14:09 ` Eric Dumazet
2017-10-31 14:40 ` Vitaly Kuznetsov
2017-10-31 16:45 ` Stephen Hemminger
0 siblings, 2 replies; 14+ messages in thread
From: Eric Dumazet @ 2017-10-31 14:09 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: netdev, linux-kernel, devel, K. Y. Srinivasan, Haiyang Zhang,
Stephen Hemminger
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net-next 3/4] hv_netvsc: reset net_device_ctx->nvdev with rcu_assign_pointer()
2017-10-31 14:09 ` Eric Dumazet
@ 2017-10-31 14:40 ` Vitaly Kuznetsov
2017-10-31 14:44 ` David Miller
2017-10-31 16:45 ` Stephen Hemminger
1 sibling, 1 reply; 14+ messages in thread
From: Vitaly Kuznetsov @ 2017-10-31 14:40 UTC (permalink / raw)
To: Eric Dumazet
Cc: netdev, linux-kernel, devel, K. Y. Srinivasan, Haiyang Zhang,
Stephen Hemminger
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net-next 3/4] hv_netvsc: reset net_device_ctx->nvdev with rcu_assign_pointer()
2017-10-31 14:40 ` Vitaly Kuznetsov
@ 2017-10-31 14:44 ` David Miller
2017-10-31 14:50 ` Vitaly Kuznetsov
0 siblings, 1 reply; 14+ messages in thread
From: David Miller @ 2017-10-31 14:44 UTC (permalink / raw)
To: vkuznets
Cc: eric.dumazet, netdev, linux-kernel, devel, kys, haiyangz, sthemmin
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net-next 3/4] hv_netvsc: reset net_device_ctx->nvdev with rcu_assign_pointer()
2017-10-31 14:44 ` David Miller
@ 2017-10-31 14:50 ` Vitaly Kuznetsov
0 siblings, 0 replies; 14+ messages in thread
From: Vitaly Kuznetsov @ 2017-10-31 14:50 UTC (permalink / raw)
To: David Miller
Cc: eric.dumazet, netdev, linux-kernel, devel, kys, haiyangz, sthemmin
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net-next 3/4] hv_netvsc: reset net_device_ctx->nvdev with rcu_assign_pointer()
2017-10-31 14:09 ` Eric Dumazet
2017-10-31 14:40 ` Vitaly Kuznetsov
@ 2017-10-31 16:45 ` Stephen Hemminger
1 sibling, 0 replies; 14+ messages in thread
From: Stephen Hemminger @ 2017-10-31 16:45 UTC (permalink / raw)
To: Eric Dumazet
Cc: Vitaly Kuznetsov, Stephen Hemminger, netdev, Haiyang Zhang,
linux-kernel, devel
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH net-next 4/4] hv_netvsc: hide warnings about uninitialized/missing rndis device
2017-10-31 13:42 [PATCH net-next 0/4] hv_netvsc: fix some crashes and hangs on channel/mtu changes Vitaly Kuznetsov
` (2 preceding siblings ...)
2017-10-31 13:42 ` [PATCH net-next 3/4] hv_netvsc: reset net_device_ctx->nvdev with rcu_assign_pointer() Vitaly Kuznetsov
@ 2017-10-31 13:42 ` Vitaly Kuznetsov
3 siblings, 0 replies; 14+ messages in thread
From: Vitaly Kuznetsov @ 2017-10-31 13:42 UTC (permalink / raw)
To: netdev
Cc: linux-kernel, devel, K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger
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
^ permalink raw reply related [flat|nested] 14+ messages in thread