| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <87k1yyjty6.fsf@vitty.brq.redhat.com>
Date: Fri, 10 Nov 2017 11:42:41 +0100
From: Vitaly Kuznetsov <vkuznets@...hat.com>
To: Stephen Hemminger <stephen@...workplumber.org>
Cc: David Miller <davem@...emloft.net>, sthemmin@...rosoft.com,
eric.dumazet@...il.com, netdev@...r.kernel.org,
haiyangz@...rosoft.com, linux-kernel@...r.kernel.org,
devel@...uxdriverproject.org
Subject: Re: [RFC] hv_netvsc: safer orderly shutdown
Stephen Hemminger <stephen@...workplumber.org> writes:
> Several types of control operations require that the underlying RNDIS
> infrastructure be restarted. This patch changes the ordering of the
> shutdown to avoid race conditions.
> Stop all transmits before doing RNDIS halt. This involves stopping the
> network device transmit queues, then waiting for all outstanding
> sends before informing host to halt.
>
> Also, check for successful restart of the device when after the
> change is done.
>
> For review, not tested on Hyper-V yet.
>
> Signed-off-by: Stephen Hemminger <sthemmin@...rosoft.com>
> ---
> drivers/net/hyperv/netvsc_drv.c | 40 ++++++++++++++++++++++++++++++---------
> drivers/net/hyperv/rndis_filter.c | 23 +++++++++++-----------
> 2 files changed, 42 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
> index da216ca4f2b2..3afa082e093d 100644
> --- a/drivers/net/hyperv/netvsc_drv.c
> +++ b/drivers/net/hyperv/netvsc_drv.c
> @@ -855,8 +855,10 @@ static int netvsc_set_channels(struct net_device *net,
>
> orig = nvdev->num_chn;
> was_opened = rndis_filter_opened(nvdev);
> - if (was_opened)
> + if (was_opened) {
> + netif_tx_disable(net);
> rndis_filter_close(nvdev);
> + }
I was also experimenting with this and I think it may also make sense to
add napi_disable() for all queues here.
It also seems that the usual TX disable pattern is
netif_tx_stop_all_queues(net);
netif_tx_disable(net);
not sure why..
>
> memset(&device_info, 0, sizeof(device_info));
> device_info.num_chn = count;
> @@ -881,8 +883,13 @@ static int netvsc_set_channels(struct net_device *net,
> }
> }
>
> - if (was_opened)
> - rndis_filter_open(nvdev);
> + if (was_opened) {
> + ret = rndis_filter_open(nvdev);
> + if (ret)
> + netdev_err(net, "reopening device failed: %d\n", ret);
> + else
> + netif_tx_start_all_queues(net);
> + }
>
> /* We may have missed link change notifications */
> net_device_ctx->last_reconfig = 0;
> @@ -971,8 +978,10 @@ static int netvsc_change_mtu(struct net_device *ndev, int mtu)
>
> netif_device_detach(ndev);
> was_opened = rndis_filter_opened(nvdev);
> - if (was_opened)
> + if (was_opened) {
> + netif_tx_disable(net);
> rndis_filter_close(nvdev);
> + }
Shall we just nove netif_tx_disable() & friends to rndis_filter_close()?
>
> memset(&device_info, 0, sizeof(device_info));
> device_info.ring_size = ring_size;
> @@ -1004,8 +1013,13 @@ static int netvsc_change_mtu(struct net_device *ndev, int mtu)
> }
> }
>
> - if (was_opened)
> - rndis_filter_open(nvdev);
> + if (was_opened) {
> + ret = rndis_filter_open(nvdev);
> + if (ret)
> + netdev_err(net, "reopening device failed: %d\n", ret);
> + else
> + netif_tx_start_all_queues(net);
> + }
Yea, the main problem here is that we can't do much if we fail, the
device will be completely unusable. That's not something users exepct
doing MTU change...
>
> netif_device_attach(ndev);
>
> @@ -1547,8 +1561,10 @@ static int netvsc_set_ringparam(struct net_device *ndev,
>
> netif_device_detach(ndev);
> was_opened = rndis_filter_opened(nvdev);
> - if (was_opened)
> + if (was_opened) {
> + netif_tx_disable(net);
> rndis_filter_close(nvdev);
> + }
>
> rndis_filter_device_remove(hdev, nvdev);
>
> @@ -1566,8 +1582,14 @@ static int netvsc_set_ringparam(struct net_device *ndev,
> }
> }
>
> - if (was_opened)
> - rndis_filter_open(nvdev);
> + if (was_opened) {
> + ret = rndis_filter_open(nvdev);
> + if (ret)
> + netdev_err(net, "reopening device failed: %d\n", ret);
> + else
> + netif_tx_start_all_queues(net);
> + }
> +
> netif_device_attach(ndev);
>
> /* We may have missed link change notifications */
> diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c
> index 0648eebda829..164f5ffe9c50 100644
> --- a/drivers/net/hyperv/rndis_filter.c
> +++ b/drivers/net/hyperv/rndis_filter.c
> @@ -948,11 +948,20 @@ static void rndis_filter_halt_device(struct rndis_device *dev)
> struct net_device_context *net_device_ctx = netdev_priv(dev->ndev);
> struct netvsc_device *nvdev = rtnl_dereference(net_device_ctx->nvdev);
>
> + /* tell bottom half that deice is being closed */
> + nvdev->destroy = true;
> +
> + /* Force flag to be ordered before waiting */
> + wmb();
> +
> + /* Wait for all send completions */
> + wait_event(nvdev->wait_drain, netvsc_device_idle(nvdev));
> +
> /* Attempt to do a rndis device halt */
> request = get_rndis_request(dev, RNDIS_MSG_HALT,
> RNDIS_MESSAGE_SIZE(struct rndis_halt_request));
> if (!request)
> - goto cleanup;
> + return;
>
> /* Setup the rndis set */
> halt = &request->request_msg.msg.halt_req;
> @@ -963,17 +972,7 @@ static void rndis_filter_halt_device(struct rndis_device *dev)
>
> dev->state = RNDIS_DEV_UNINITIALIZED;
>
> -cleanup:
> - nvdev->destroy = true;
> -
> - /* Force flag to be ordered before waiting */
> - wmb();
> -
> - /* Wait for all send completions */
> - wait_event(nvdev->wait_drain, netvsc_device_idle(nvdev));
> -
> - if (request)
> - put_rndis_request(dev, request);
> + put_rndis_request(dev, request);
> }
>
> static int rndis_filter_open_device(struct rndis_device *dev)
I'll give this patch a shot to see if anything blows up. Thanks!
--
Vitaly
Powered by blists - more mailing lists