hv_netvsc: fix races during shutdown and changes
Subsystems (3)
cover hv_netvsc: fix races during shutdown and changes
This set of patches fixes issues identified by Vitaly Kuznetsov and Mohammed Gamal related to state changes in Hyper-v network driver.
A lot of the issues are because setting up the netvsc device requires a second step (in work queue) to get all the sub-channels running.
Stephen Hemminger (4):
hv_netvsc: disable NAPI before channel close
hv_netvsc: use RCU to fix concurrent rx and queue changes
hv_netvsc: change GPAD teardown order on older versions
hv_netvsc: common detach logic
drivers/net/hyperv/hyperv_net.h | 1 - drivers/net/hyperv/netvsc.c | 52 +++---- drivers/net/hyperv/netvsc_drv.c | 278 +++++++++++++++++++++----------------- drivers/net/hyperv/rndis_filter.c | 56 ++++---- 4 files changed, 204 insertions(+), 183 deletions(-)
Signature · 1 line
2.16.2
PATCH 1/4 hv_netvsc: disable NAPI before channel close
This makes sure that no CPU is still process packets when the channel is closed.
Fixes 76bb5db5c749 ("netvsc: fix use after free on module removal")
Signed-off-by Stephen Hemminger <[email protected]>
--- drivers/net/hyperv/netvsc.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/net/hyperv/netvsc.c +4 -4
@@ -568,6 +568,10 @@ void netvsc_device_remove(struct hv_device *device)RCU_INIT_POINTER (net_device_ctx->nvdev,NULL ); +/* And disassociate NAPI context from device */ +for (i =0 ; i < net_device->num_chn; i++) +netif_napi_del (&net_device->chan_table[i].napi); +/* * At this point, no one should be accessing net_device * except in here
@@ -579,10 +583,6 @@ void netvsc_device_remove(struct hv_device *device)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); -/* Release all resources */ free_netvsc_device_rcu (net_device); }
Signature · 1 line
2.16.2
PATCH 2/4 hv_netvsc: use RCU to fix concurrent rx and queue changes
In reply to Stephen HemmingerThe receive processing may continue to happen while the internal network device state is in RCU grace period. The internal RNDIS structure is associated with the internal netvsc_device structure; both have the same RCU lifetime.
Defer freeing all associated parts until after grace period.
Fixes 0cf737808ae7 ("hv_netvsc: netvsc_teardown_gpadl() split")
Signed-off-by Stephen Hemminger <[email protected]>
--- drivers/net/hyperv/netvsc.c | 17 +++++------------ drivers/net/hyperv/rndis_filter.c | 39 ++++++++++++++++----------------------- 2 files changed, 21 insertions(+), 35 deletions(-)
drivers/net/hyperv/netvsc.c +5 -12
@@ -90,6 +90,11 @@ static void free_netvsc_device(struct rcu_head *head) = container_of(head, struct netvsc_device, rcu);int i; +kfree (nvdev->extension); +vfree (nvdev->recv_buf); +vfree (nvdev->send_buf); +kfree (nvdev->send_section_map); +for (i =0 ; i <VRSS_CHANNEL_MAX ; i++)vfree (nvdev->chan_table[i].mrc.slots);
@@ -211,12 +216,6 @@ static void netvsc_teardown_gpadl(struct hv_device *device, 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);
@@ -231,12 +230,6 @@ static void netvsc_teardown_gpadl(struct hv_device *device, } net_device->send_buf_gpadl_handle =0 ; } -if (net_device->send_buf) { -/* Free up the send buffer */ -vfree (net_device->send_buf); - net_device->send_buf =NULL ; - } -kfree (net_device->send_section_map); }int netvsc_alloc_recv_comp_ring (struct netvsc_device *net_device,u32 q_idx)
drivers/net/hyperv/rndis_filter.c +16 -23
@@ -264,13 +264,23 @@ static void rndis_set_link_state(struct rndis_device *rdev, } } -static void rndis_filter_receive_response (struct rndis_device *dev, -struct rndis_message *resp) +static void rndis_filter_receive_response (struct net_device *ndev, +struct netvsc_device *nvdev, +const struct rndis_message *resp) { +struct rndis_device *dev = nvdev->extension;struct rndis_request *request =NULL ;bool found = false;unsigned long flags; -struct net_device *ndev = dev->ndev; + +/* This should never happen, it means control message +* response received after device removed. +*/ +if (dev->state ==RNDIS_DEV_UNINITIALIZED ) { +netdev_err (ndev, +"got rndis message uninitialized\n" ); +return ; + }spin_lock_irqsave (&dev->request_lock, flags);list_for_each_entry (request, &dev->req_list, list_ent) {
@@ -352,7 +362,6 @@ static inline void *rndis_get_ppi(struct rndis_packet *rpkt, u32 type)static int rndis_filter_receive_data (struct net_device *ndev,struct netvsc_device *nvdev, -struct rndis_device *dev,struct rndis_message *msg,struct vmbus_channel *channel,void *data,u32 data_buflen)
@@ -372,7 +381,7 @@ static int rndis_filter_receive_data(struct net_device *ndev,* should be the data packet size plus the trailer padding size */ if (unlikely (data_buflen < rndis_pkt->data_len)) { -netdev_err (dev->ndev,"rndis message buffer " +netdev_err (ndev,"rndis message buffer " "overflow detected (got %u, min %u)" "...dropping this message!\n" , data_buflen, rndis_pkt->data_len);
@@ -400,35 +409,20 @@ 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_message *rndis_msg = data; -/* Make sure the rndis device state is initialized */ -if (unlikely (!rndis_dev)) { -netif_dbg (net_device_ctx, rx_err, ndev, -"got rndis message but no rndis device!\n" ); -return NVSP_STAT_FAIL ; - } - -if (unlikely (rndis_dev->state ==RNDIS_DEV_UNINITIALIZED )) { -netif_dbg (net_device_ctx, rx_err, ndev, -"got rndis message uninitialized\n" ); -return NVSP_STAT_FAIL ; - } -if (netif_msg_rx_status (net_device_ctx))dump_rndis_message (ndev, rndis_msg);switch (rndis_msg->ndis_msg_type) {case RNDIS_MSG_PACKET : -return rndis_filter_receive_data (ndev, net_dev, - rndis_dev, rndis_msg, +return rndis_filter_receive_data (ndev, net_dev, rndis_msg, channel, data, buflen);case RNDIS_MSG_INIT_C :case RNDIS_MSG_QUERY_C :case RNDIS_MSG_SET_C :/* completion msgs */ -rndis_filter_receive_response (rndis_dev, rndis_msg); +rndis_filter_receive_response (ndev, net_dev, rndis_msg);break ;case RNDIS_MSG_INDICATE :
@@ -1357,7 +1351,6 @@ void rndis_filter_device_remove(struct hv_device *dev, net_dev->extension =NULL ;netvsc_device_remove (dev); -kfree (rndis_dev); }int rndis_filter_open (struct netvsc_device *nvdev)
Signature · 1 line
2.16.2
PATCH 3/4 hv_netvsc: change GPAD teardown order on older versions
In reply to Stephen HemmingerOn older versions of Windows, the host ignores messages after vmbus channel is closed.
Workaround this by doing what Windows does and send the teardown before close on older versions of NVSP protocol.
Reported-by Mohammed Gamal <[email protected]>
Fixes 0cf737808ae7 ("hv_netvsc: netvsc_teardown_gpadl() split")
Signed-off-by Stephen Hemminger <[email protected]>
--- drivers/net/hyperv/netvsc.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/net/hyperv/netvsc.c +6 -1
@@ -571,10 +571,15 @@ void netvsc_device_remove(struct hv_device *device)*/ netdev_dbg (ndev,"net device safe to remove\n" ); +/* older versions require that buffer be revoked before close */ +if (net_device->nvsp_version <NVSP_PROTOCOL_VERSION_4 ) +netvsc_teardown_gpadl (device, net_device); +/* Now, we can close the channel safely */ vmbus_close (device->channel); -netvsc_teardown_gpadl (device, net_device); +if (net_device->nvsp_version >=NVSP_PROTOCOL_VERSION_4 ) +netvsc_teardown_gpadl (device, net_device);/* Release all resources */ free_netvsc_device_rcu (net_device);
Signature · 1 line
2.16.2
PATCH 4/4 hv_netvsc: common detach logic
In reply to Stephen HemmingerMake common function for detaching internals of device during changes to MTU and RSS. Make sure no more packets are transmitted and all packets have been received before doing device teardown.
Change the wait logic to be common and use usleep_range().
Changes transmit enabling logic so that transmit queues are disabled during the period when lower device is being changed. And enabled only after sub channels are setup. This avoids issue where it could be that a packet was being sent while subchannel was not initialized.
Fixes 8195b1396ec8 ("hv_netvsc: fix deadlock on hotplug")
Signed-off-by Stephen Hemminger <[email protected]>
--- drivers/net/hyperv/hyperv_net.h | 1 - drivers/net/hyperv/netvsc.c | 20 +-- drivers/net/hyperv/netvsc_drv.c | 278 +++++++++++++++++++++----------------- drivers/net/hyperv/rndis_filter.c | 17 +-- 4 files changed, 173 insertions(+), 143 deletions(-)
drivers/net/hyperv/hyperv_net.h +0 -1
@@ -212,7 +212,6 @@ void netvsc_channel_cb(void *context);int netvsc_poll (struct napi_struct *napi,int budget);void rndis_set_subchannel (struct work_struct *w); -bool rndis_filter_opened (const struct netvsc_device *nvdev);int rndis_filter_open (struct netvsc_device *nvdev);int rndis_filter_close (struct netvsc_device *nvdev); struct netvsc_device *rndis_filter_device_add(struct hv_device *dev,
drivers/net/hyperv/netvsc.c +11 -9
@@ -555,8 +555,6 @@ void netvsc_device_remove(struct hv_device *device) = rtnl_dereference(net_device_ctx->nvdev);int i; -cancel_work_sync (&net_device->subchan_work); -netvsc_revoke_buf (device, net_device);RCU_INIT_POINTER (net_device_ctx->nvdev,NULL );
@@ -643,14 +641,18 @@ static void netvsc_send_tx_complete(struct netvsc_device *net_device, queue_sends =atomic_dec_return (&net_device->chan_table[q_idx].queue_sends); -if (net_device->destroy && queue_sends ==0 ) -wake_up (&net_device->wait_drain); +if (unlikely (net_device->destroy)) { +if (queue_sends ==0 ) +wake_up (&net_device->wait_drain); + }else { +struct netdev_queue *txq =netdev_get_tx_queue (ndev, q_idx); -if (netif_tx_queue_stopped (netdev_get_tx_queue (ndev, q_idx)) && - (hv_ringbuf_avail_percent (&channel->outbound) >RING_AVAIL_PERCENT_HIWATER || - queue_sends <1 )) { -netif_tx_wake_queue (netdev_get_tx_queue (ndev, q_idx)); - ndev_ctx->eth_stats.wake_queue++; +if (netif_tx_queue_stopped (txq) && + (hv_ringbuf_avail_percent (&channel->outbound) >RING_AVAIL_PERCENT_HIWATER || + queue_sends <1 )) { +netif_tx_wake_queue (txq); + ndev_ctx->eth_stats.wake_queue++; + } } }
drivers/net/hyperv/netvsc_drv.c +155 -123
413 lines not on this page · raw message
drivers/net/hyperv/rndis_filter.c +7 -10
@@ -1118,6 +1118,7 @@ void rndis_set_subchannel(struct work_struct *w)for (i =0 ; i <VRSS_SEND_TAB_SIZE ; i++) ndev_ctx->tx_table[i] = i % nvdev->num_chn; +netif_device_attach (ndev);rtnl_unlock ();return ;
@@ -1128,6 +1129,8 @@ void rndis_set_subchannel(struct work_struct *w) nvdev->max_chn =1 ; nvdev->num_chn =1 ; + +netif_device_attach (ndev); unlock:rtnl_unlock (); }
@@ -1330,6 +1333,10 @@ struct netvsc_device *rndis_filter_device_add(struct hv_device *dev, net_device->num_chn =1 ; } +/* No sub channels, device is ready */ +if (net_device->num_chn ==1 ) +netif_device_attach (net); +return net_device; err_dev_remv:
@@ -1342,9 +1349,6 @@ void rndis_filter_device_remove(struct hv_device *dev, {struct rndis_device *rndis_dev = net_dev->extension; -/* Don't try and setup sub channels if about to halt */ -cancel_work_sync (&net_dev->subchan_work); -/* Halt and release the rndis device */ rndis_filter_halt_device (rndis_dev);
@@ -1368,10 +1372,3 @@ int rndis_filter_close(struct netvsc_device *nvdev)return rndis_filter_close_device (nvdev->extension); } - -bool rndis_filter_opened (const struct netvsc_device *nvdev) -{ -const struct rndis_device *dev = nvdev->extension; - -return dev->state ==RNDIS_DEV_DATAINITIALIZED ; -}
Signature · 1 line
2.16.2
David Miller
maintainerFrom: Stephen Hemminger <[email protected]>
Date: Tue, 20 Mar 2018 15:03:01 -0700
Quoted 5 lines
This set of patches fixes issues identified by Vitaly Kuznetsov and Mohammed Gamal related to state changes in Hyper-v network driver.
A lot of the issues are because setting up the netvsc device requires a second step (in work queue) to get all the sub-channels running.
Series applied, thanks Stephen.