Re: [PATCH] hv_netvsc: Make sure out channel is fully opened on send

From: Stephen Hemminger
Date: Thu Mar 15 2018 - 13:41:07 EST

On Thu, 15 Mar 2018 17:24:13 +0100
Mohammed Gamal <mgamal@xxxxxxxxxx> wrote:

> On Wed, 2018-03-14 at 10:22 +0100, Mohammed Gamal wrote:
> > On Tue, 2018-03-13 at 12:35 -0700, Stephen Hemminger wrote:
> > > On Tue, 13 Mar 2018 20:06:50 +0100
> > > Mohammed Gamal <mgamal@xxxxxxxxxx> wrote:
> > >
> > > > Dring high network traffic changes to network interface
> > > > parameters
> > > > such as number of channels or MTU can cause a kernel panic with a
> > > > NULL
> > > > pointer dereference. This is due to netvsc_device_remove() being
> > > > called and deallocating the channel ring buffers, which can then
> > > > be
> > > > accessed by netvsc_send_pkt() before they're allocated on calling
> > > > netvsc_device_add()
> > > >
> > > > The patch fixes this problem by checking the channel state and
> > > > returning
> > > > ENODEV if not yet opened. We also move the call to
> > > > hv_ringbuf_avail_percent()
> > > > which may access the uninitialized ring buffer.
> > > >
> > > > Signed-off-by: Mohammed Gamal <mgamal@xxxxxxxxxx>
> > > > ---
> > > > Âdrivers/net/hyperv/netvsc.c | 5 +++--
> > > > Â1 file changed, 3 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/net/hyperv/netvsc.c
> > > > b/drivers/net/hyperv/netvsc.c
> > > > index 0265d70..44a8358 100644
> > > > --- a/drivers/net/hyperv/netvsc.c
> > > > +++ b/drivers/net/hyperv/netvsc.c
> > > > @@ -757,7 +757,7 @@ static inline int netvsc_send_pkt(
> > > > Â struct netdev_queue *txq = netdev_get_tx_queue(ndev,
> > > > packet->q_idx);
> > > > Â u64 req_id;
> > > > Â int ret;
> > > > - u32 ring_avail = hv_ringbuf_avail_percent(&out_channel-
> > > > > outbound);
> > > >
> > > > + u32 ring_avail;
> > > > Â
> > > > Â nvmsg.hdr.msg_type = NVSP_MSG1_TYPE_SEND_RNDIS_PKT;
> > > > Â if (skb)
> > > > @@ -773,7 +773,7 @@ static inline int netvsc_send_pkt(
> > > > Â
> > > > Â req_id = (ulong)skb;
> > > > Â
> > > > - if (out_channel->rescind)
> > > > + if (out_channel->rescind || out_channel->state !=
> > > > CHANNEL_OPENED_STATE)
> > > > Â return -ENODEV;
> > > > Â
> > > > Â if (packet->page_buf_cnt) {
> > > > @@ -791,6 +791,7 @@ static inline int netvsc_send_pkt(
> > > > Â ÂÂÂÂÂÂÂVMBUS_DATA_PACKET_FLAG_CO
> > > > MP
> > > > LETION_REQUESTED);
> > > > Â }
> > > > Â
> > > > + ring_avail = hv_ringbuf_avail_percent(&out_channel-
> > > > > outbound);
> > > >
> > > > Â if (ret == 0) {
> > > > Â atomic_inc_return(&nvchan->queue_sends);
> > > > Â
> > >
> > > Thanks for your patch. Yes there are races with the current update
> > > logic. The root cause goes higher up in the flow; the send queues
> > > should
> > > be stopped before netvsc_device_remove is called. Solving it where
> > > you tried
> > > to is racy and not going to work reliably.
> > >
> > > Network patches should go to netdev@xxxxxxxxxxxxxxx
> > >
> > > You can't move the ring_avail check until after the
> > > vmbus_sendpacket
> > > because
> > > that will break the flow control logic.
> > >
> >
> > Why? I don't see ring_avail being used before that point.
>
> Ah, stupid me. vmbus_sendpacket() will write to the ring buffer and
> that means that ring_avail value will be different than the expected.
>
> >
> > > Instead, you should just move the avail_read check until just after
> > > the existing rescind
> > > check.
> > >
> > > Also, you shouldn't need to check for OPENED_STATE, just rescind is
> > > enough.
> >
> > That rarely mitigated the race. channel->rescind flag is set on vmbus
> > exit - called on module unload - and when a rescind offer is received
> > from the host, which AFAICT doesn't happen on every call to
> > netvsc_device_remove, so it's quite possible that the ringbuffer is
> > accessed before it's allocated again on channel open and hence the
> > check for OPENED_STAT - which is only set after all vmbus data is
> > initialized.
> >
>
> Perhaps I haven't been clear enough. The NULL pointer dereference
> happens in the call to hv_ringbuf_avail_percent() which is used to
> calculate ring_avail.Â
>
> So we need to stop the queues before calling it if the channel's ring
> buffers haven't been allocated yet, but OTOH we should only stop the
> queues based upon the value of ring_avail, so this leads into a chicken
> and egg situation.Â
>
> Is my observation here correct? Please correct me if I am wrong,
> Stephen.

This is a far more drastic work of the shutdown logic which I am still working
on. The current netvsc driver is not doing a good job of handling concurrent
send/receives during ring changes.


From a22da18b41ad5024340dddcc989d918987836f6d Mon Sep 17 00:00:00 2001
From: Stephen Hemminger <sthemmin@xxxxxxxxxxxxx>
Date: Tue, 6 Feb 2018 15:05:19 -0800
Subject: [PATCH] hv_netvsc: common detach logic

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

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.

Signed-off-by: Stephen Hemminger <sthemmin@xxxxxxxxxxxxx>
---
drivers/net/hyperv/hyperv_net.h | 1 -
drivers/net/hyperv/netvsc.c | 20 +--
drivers/net/hyperv/netvsc_drv.c | 268 +++++++++++++++++++++-----------------
drivers/net/hyperv/rndis_filter.c | 14 +-
4 files changed, 160 insertions(+), 143 deletions(-)

diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
index cd538d5a7986..32861036c3fc 100644
--- a/drivers/net/hyperv/hyperv_net.h
+++ b/drivers/net/hyperv/hyperv_net.h
@@ -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,
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index 8c95a3797b2f..31b1c6c430bb 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -573,8 +573,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);
@@ -661,14 +659,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++;
+ }
}
}

diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index faea0be18924..721fac7cad81 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -46,7 +46,8 @@

#include "hyperv_net.h"

-#define RING_SIZE_MIN 64
+#define RING_SIZE_MIN 64
+#define RETRY_MAX 2000

#define LINKCHANGE_INT (2 * HZ)
#define VF_TAKEOVER_INT (HZ / 10)
@@ -123,10 +124,8 @@ static int netvsc_open(struct net_device *net)
}

rdev = nvdev->extension;
- if (!rdev->link_state) {
+ if (!rdev->link_state)
netif_carrier_on(net);
- netif_tx_wake_all_queues(net);
- }

if (vf_netdev) {
/* Setting synthetic device up transparently sets
@@ -142,36 +141,25 @@ static int netvsc_open(struct net_device *net)
return 0;
}

-static int netvsc_close(struct net_device *net)
+static int netvsc_wait_until_empty(struct netvsc_device *nvdev)
{
- struct net_device_context *net_device_ctx = netdev_priv(net);
- struct net_device *vf_netdev
- = rtnl_dereference(net_device_ctx->vf_netdev);
- struct netvsc_device *nvdev = rtnl_dereference(net_device_ctx->nvdev);
- int ret = 0;
- u32 aread, i, msec = 10, retry = 0, retry_max = 20;
- struct vmbus_channel *chn;
-
- netif_tx_disable(net);
-
- /* No need to close rndis filter if it is removed already */
- if (!nvdev)
- goto out;
-
- ret = rndis_filter_close(nvdev);
- if (ret != 0) {
- netdev_err(net, "unable to close device (ret %d).\n", ret);
- return ret;
- }
+ unsigned int retry = 0;
+ int i;

/* Ensure pending bytes in ring are read */
- while (true) {
- aread = 0;
+ for (;;) {
+ u32 aread = 0;
+
for (i = 0; i < nvdev->num_chn; i++) {
- chn = nvdev->chan_table[i].channel;
+ struct vmbus_channel *chn
+ = nvdev->chan_table[i].channel;
+
if (!chn)
continue;

+ /* make sure receive not running now */
+ napi_synchronize(&nvdev->chan_table[i].napi);
+
aread = hv_get_bytes_to_read(&chn->inbound);
if (aread)
break;
@@ -181,22 +169,40 @@ static int netvsc_close(struct net_device *net)
break;
}

- retry++;
- if (retry > retry_max || aread == 0)
- break;
+ if (aread == 0)
+ return 0;

- msleep(msec);
+ if (++retry > RETRY_MAX)
+ return -ETIMEDOUT;

- if (msec < 1000)
- msec *= 2;
+ usleep_range(1000, 2000);
}
+}

- if (aread) {
- netdev_err(net, "Ring buffer not empty after closing rndis\n");
- ret = -ETIMEDOUT;
+static int netvsc_close(struct net_device *net)
+{
+ struct net_device_context *net_device_ctx = netdev_priv(net);
+ struct net_device *vf_netdev
+ = rtnl_dereference(net_device_ctx->vf_netdev);
+ struct netvsc_device *nvdev = rtnl_dereference(net_device_ctx->nvdev);
+ int ret;
+
+ netif_tx_disable(net);
+
+ /* No need to close rndis filter if it is removed already */
+ if (!nvdev)
+ return 0;
+
+ ret = rndis_filter_close(nvdev);
+ if (ret != 0) {
+ netdev_err(net, "unable to close device (ret %d).\n", ret);
+ return ret;
}

-out:
+ ret = netvsc_wait_until_empty(nvdev);
+ if (ret)
+ netdev_err(net, "Ring buffer not empty after closing rndis\n");
+
if (vf_netdev)
dev_close(vf_netdev);

@@ -845,16 +851,76 @@ static void netvsc_get_channels(struct net_device *net,
}
}

+static int netvsc_detach(struct net_device *ndev,
+ struct netvsc_device *nvdev)
+{
+ struct net_device_context *ndev_ctx = netdev_priv(ndev);
+ struct hv_device *hdev = ndev_ctx->device_ctx;
+ int ret;
+
+ /* Don't try continuing to try and setup sub channels */
+ if (cancel_work_sync(&nvdev->subchan_work))
+ nvdev->num_chn = 1;
+
+ /* If device was up (receiving) then shutdown */
+ if (netif_running(ndev)) {
+ netif_tx_disable(ndev);
+
+ ret = rndis_filter_close(nvdev);
+ if (ret) {
+ netdev_err(ndev,
+ "unable to close device (ret %d).\n", ret);
+ return ret;
+ }
+
+ ret = netvsc_wait_until_empty(nvdev);
+ if (ret) {
+ netdev_err(ndev,
+ "Ring buffer not empty after closing rndis\n");
+ return ret;
+ }
+ }
+
+ rndis_filter_device_remove(hdev, nvdev);
+ return 0;
+}
+
+static int netvsc_attach(struct net_device *ndev,
+ struct netvsc_device_info *dev_info)
+{
+ struct net_device_context *ndev_ctx = netdev_priv(ndev);
+ struct hv_device *hdev = ndev_ctx->device_ctx;
+ struct netvsc_device *nvdev;
+ struct rndis_device *rdev;
+ int ret;
+
+ nvdev = rndis_filter_device_add(hdev, dev_info);
+ if (IS_ERR(nvdev))
+ return PTR_ERR(nvdev);
+
+ netif_carrier_off(ndev);
+
+ if (netif_running(ndev)) {
+ ret = rndis_filter_open(nvdev);
+ if (ret)
+ return ret;
+
+ rdev = nvdev->extension;
+ if (!rdev->link_state)
+ netif_carrier_on(ndev);
+ }
+
+ return 0;
+}
+
static int netvsc_set_channels(struct net_device *net,
struct ethtool_channels *channels)
{
struct net_device_context *net_device_ctx = netdev_priv(net);
- struct hv_device *dev = net_device_ctx->device_ctx;
struct netvsc_device *nvdev = rtnl_dereference(net_device_ctx->nvdev);
unsigned int orig, count = channels->combined_count;
struct netvsc_device_info device_info;
- bool was_opened;
- int ret = 0;
+ int ret;

/* We do not support separate count for rx, tx, or other */
if (count == 0 ||
@@ -871,9 +937,6 @@ static int netvsc_set_channels(struct net_device *net,
return -EINVAL;

orig = nvdev->num_chn;
- was_opened = rndis_filter_opened(nvdev);
- if (was_opened)
- rndis_filter_close(nvdev);

memset(&device_info, 0, sizeof(device_info));
device_info.num_chn = count;
@@ -882,28 +945,17 @@ static int netvsc_set_channels(struct net_device *net,
device_info.recv_sections = nvdev->recv_section_cnt;
device_info.recv_section_size = nvdev->recv_section_size;

- rndis_filter_device_remove(dev, nvdev);
+ ret = netvsc_detach(net, nvdev);
+ if (ret)
+ return ret;

- nvdev = rndis_filter_device_add(dev, &device_info);
- if (IS_ERR(nvdev)) {
- ret = PTR_ERR(nvdev);
+ ret = netvsc_attach(net, &device_info);
+ if (ret) {
device_info.num_chn = orig;
- nvdev = rndis_filter_device_add(dev, &device_info);
-
- if (IS_ERR(nvdev)) {
- netdev_err(net, "restoring channel setting failed: %ld\n",
- PTR_ERR(nvdev));
- return ret;
- }
+ if (netvsc_attach(net, &device_info))
+ netdev_err(net, "restoring channel setting failed\n");
}

- if (was_opened)
- rndis_filter_open(nvdev);
-
- /* We may have missed link change notifications */
- net_device_ctx->last_reconfig = 0;
- schedule_delayed_work(&net_device_ctx->dwork, 0);
-
return ret;
}

@@ -969,10 +1021,8 @@ static int netvsc_change_mtu(struct net_device *ndev, int mtu)
struct net_device_context *ndevctx = netdev_priv(ndev);
struct net_device *vf_netdev = rtnl_dereference(ndevctx->vf_netdev);
struct netvsc_device *nvdev = rtnl_dereference(ndevctx->nvdev);
- struct hv_device *hdev = ndevctx->device_ctx;
int orig_mtu = ndev->mtu;
struct netvsc_device_info device_info;
- bool was_opened;
int ret = 0;

if (!nvdev || nvdev->destroy)
@@ -985,11 +1035,6 @@ static int netvsc_change_mtu(struct net_device *ndev, int mtu)
return ret;
}

- netif_device_detach(ndev);
- was_opened = rndis_filter_opened(nvdev);
- if (was_opened)
- rndis_filter_close(nvdev);
-
memset(&device_info, 0, sizeof(device_info));
device_info.num_chn = nvdev->num_chn;
device_info.send_sections = nvdev->send_section_cnt;
@@ -997,35 +1042,27 @@ static int netvsc_change_mtu(struct net_device *ndev, int mtu)
device_info.recv_sections = nvdev->recv_section_cnt;
device_info.recv_section_size = nvdev->recv_section_size;

- rndis_filter_device_remove(hdev, nvdev);
+ ret = netvsc_detach(ndev, nvdev);
+ if (ret)
+ goto rollback_vf;

ndev->mtu = mtu;

- nvdev = rndis_filter_device_add(hdev, &device_info);
- if (IS_ERR(nvdev)) {
- ret = PTR_ERR(nvdev);
-
- /* Attempt rollback to original MTU */
- ndev->mtu = orig_mtu;
- nvdev = rndis_filter_device_add(hdev, &device_info);
-
- if (vf_netdev)
- dev_set_mtu(vf_netdev, orig_mtu);
-
- if (IS_ERR(nvdev)) {
- netdev_err(ndev, "restoring mtu failed: %ld\n",
- PTR_ERR(nvdev));
- return ret;
- }
- }
+ ret = netvsc_attach(ndev, &device_info);
+ if (ret)
+ goto rollback;

- if (was_opened)
- rndis_filter_open(nvdev);
+ return 0;

- netif_device_attach(ndev);
+rollback:
+ /* Attempt rollback to original MTU */
+ ndev->mtu = orig_mtu;

- /* We may have missed link change notifications */
- schedule_delayed_work(&ndevctx->dwork, 0);
+ if (netvsc_attach(ndev, &device_info))
+ netdev_err(ndev, "restoring mtu failed\n");
+rollback_vf:
+ if (vf_netdev)
+ dev_set_mtu(vf_netdev, orig_mtu);

return ret;
}
@@ -1531,11 +1568,9 @@ static int netvsc_set_ringparam(struct net_device *ndev,
{
struct net_device_context *ndevctx = netdev_priv(ndev);
struct netvsc_device *nvdev = rtnl_dereference(ndevctx->nvdev);
- struct hv_device *hdev = ndevctx->device_ctx;
struct netvsc_device_info device_info;
struct ethtool_ringparam orig;
u32 new_tx, new_rx;
- bool was_opened;
int ret = 0;

if (!nvdev || nvdev->destroy)
@@ -1560,34 +1595,18 @@ static int netvsc_set_ringparam(struct net_device *ndev,
device_info.recv_sections = new_rx;
device_info.recv_section_size = nvdev->recv_section_size;

- netif_device_detach(ndev);
- was_opened = rndis_filter_opened(nvdev);
- if (was_opened)
- rndis_filter_close(nvdev);
-
- rndis_filter_device_remove(hdev, nvdev);
-
- nvdev = rndis_filter_device_add(hdev, &device_info);
- if (IS_ERR(nvdev)) {
- ret = PTR_ERR(nvdev);
+ ret = netvsc_detach(ndev, nvdev);
+ if (ret)
+ return ret;

+ ret = netvsc_attach(ndev, &device_info);
+ if (ret) {
device_info.send_sections = orig.tx_pending;
device_info.recv_sections = orig.rx_pending;
- nvdev = rndis_filter_device_add(hdev, &device_info);
- if (IS_ERR(nvdev)) {
- netdev_err(ndev, "restoring ringparam failed: %ld\n",
- PTR_ERR(nvdev));
- return ret;
- }
- }
-
- if (was_opened)
- rndis_filter_open(nvdev);
- netif_device_attach(ndev);

- /* We may have missed link change notifications */
- ndevctx->last_reconfig = 0;
- schedule_delayed_work(&ndevctx->dwork, 0);
+ if (netvsc_attach(ndev, &device_info))
+ netdev_err(ndev, "restoring ringparam failed");
+ }

return ret;
}
@@ -2072,8 +2091,8 @@ static int netvsc_probe(struct hv_device *dev,
static int netvsc_remove(struct hv_device *dev)
{
struct net_device_context *ndev_ctx;
- struct net_device *vf_netdev;
- struct net_device *net;
+ struct net_device *vf_netdev, *net;
+ struct netvsc_device *nvdev;

net = hv_get_drvdata(dev);
if (net == NULL) {
@@ -2083,8 +2102,6 @@ static int netvsc_remove(struct hv_device *dev)

ndev_ctx = netdev_priv(net);

- netif_device_detach(net);
-
cancel_delayed_work_sync(&ndev_ctx->dwork);

/*
@@ -2092,14 +2109,19 @@ static int netvsc_remove(struct hv_device *dev)
* removed. Also blocks mtu and channel changes.
*/
rtnl_lock();
+
vf_netdev = rtnl_dereference(ndev_ctx->vf_netdev);
if (vf_netdev)
netvsc_unregister_vf(vf_netdev);

+ nvdev = rtnl_dereference(ndev_ctx->nvdev);
+ if (nvdev) {
+ cancel_work_sync(&nvdev->subchan_work);
+ rndis_filter_device_remove(dev, nvdev);
+ }
+
unregister_netdevice(net);

- rndis_filter_device_remove(dev,
- rtnl_dereference(ndev_ctx->nvdev));
rtnl_unlock();

hv_set_drvdata(dev, NULL);
diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c
index a9746f9fbf4b..3ccb0c6263a8 100644
--- a/drivers/net/hyperv/rndis_filter.c
+++ b/drivers/net/hyperv/rndis_filter.c
@@ -1122,6 +1122,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_tx_wake_all_queues(ndev);
rtnl_unlock();
return;

@@ -1132,6 +1133,7 @@ void rndis_set_subchannel(struct work_struct *w)

nvdev->max_chn = 1;
nvdev->num_chn = 1;
+ netif_wake_queue(ndev);
unlock:
rtnl_unlock();
}
@@ -1326,6 +1328,8 @@ struct netvsc_device *rndis_filter_device_add(struct hv_device *dev,

if (net_device->num_chn > 1)
schedule_work(&net_device->subchan_work);
+ else
+ netif_wake_queue(net);

out:
/* if unavailable, just proceed with one queue */
@@ -1346,9 +1350,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(net_dev, rndis_dev);

@@ -1372,10 +1373,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;
-}
--
2.16.1