| 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
| ||
|
Date: Wed, 26 Oct 2016 14:26:20 +0000
From: Haiyang Zhang <haiyangz@...rosoft.com>
To: Vitaly Kuznetsov <vkuznets@...hat.com>,
"devel@...uxdriverproject.org" <devel@...uxdriverproject.org>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"KY Srinivasan" <kys@...rosoft.com>
Subject: RE: [PATCH] Drivers: hv: vmbus: Raise retry/wait limits in
vmbus_post_msg()
> -----Original Message-----
> From: Vitaly Kuznetsov [mailto:vkuznets@...hat.com]
> Sent: Wednesday, October 26, 2016 7:12 AM
> To: devel@...uxdriverproject.org
> Cc: linux-kernel@...r.kernel.org; KY Srinivasan <kys@...rosoft.com>;
> Haiyang Zhang <haiyangz@...rosoft.com>
> Subject: [PATCH] Drivers: hv: vmbus: Raise retry/wait limits in
> vmbus_post_msg()
>
> DoS protection conditions were altered in WS2016 and now it's easy to
> get
> -EAGAIN returned from vmbus_post_msg() (e.g. when we try changing MTU on
> a
> netvsc device in a loop). All vmbus_post_msg() callers don't retry the
> operation and we usually end up with a non-functional device or crash.
>
> While host's DoS protection conditions are unknown to me my tests show
> that
> it can take up to 46 attempts to send a message after changing udelay()
> to
> mdelay() and caping msec at '256', this means we can wait up to 10
> seconds
> before the message is sent so we need to use msleep() instead. Almost
> all
> vmbus_post_msg() callers are ready to sleep but there is one special
> case:
> vmbus_initiate_unload() which can be called from interrupt/NMI context
> and
> we can't sleep there. I'm also not sure about the lonely
> vmbus_send_tl_connect_request() which has no in-tree users but its
> external
> users are most likely waiting for the host to reply so sleeping there is
> also appropriate.
>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@...hat.com>
> ---
> drivers/hv/channel.c | 17 +++++++++--------
> drivers/hv/channel_mgmt.c | 10 ++++++----
> drivers/hv/connection.c | 19 ++++++++++++-------
> drivers/hv/hyperv_vmbus.h | 2 +-
> 4 files changed, 28 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> index 16f91c8..28ca66e 100644
> --- a/drivers/hv/channel.c
> +++ b/drivers/hv/channel.c
> @@ -180,7 +180,7 @@ int vmbus_open(struct vmbus_channel *newchannel, u32
> send_ringbuffer_size,
> spin_unlock_irqrestore(&vmbus_connection.channelmsg_lock, flags);
>
> ret = vmbus_post_msg(open_msg,
> - sizeof(struct vmbus_channel_open_channel));
> + sizeof(struct vmbus_channel_open_channel), true);
>
> if (ret != 0) {
> err = ret;
> @@ -232,7 +232,7 @@ int vmbus_send_tl_connect_request(const uuid_le
> *shv_guest_servie_id,
> conn_msg.guest_endpoint_id = *shv_guest_servie_id;
> conn_msg.host_service_id = *shv_host_servie_id;
>
> - return vmbus_post_msg(&conn_msg, sizeof(conn_msg));
> + return vmbus_post_msg(&conn_msg, sizeof(conn_msg), true);
> }
> EXPORT_SYMBOL_GPL(vmbus_send_tl_connect_request);
>
> @@ -418,7 +418,7 @@ int vmbus_establish_gpadl(struct vmbus_channel
> *channel, void *kbuffer,
> spin_unlock_irqrestore(&vmbus_connection.channelmsg_lock, flags);
>
> ret = vmbus_post_msg(gpadlmsg, msginfo->msgsize -
> - sizeof(*msginfo));
> + sizeof(*msginfo), true);
> if (ret != 0)
> goto cleanup;
>
> @@ -432,8 +432,8 @@ int vmbus_establish_gpadl(struct vmbus_channel
> *channel, void *kbuffer,
> gpadl_body->gpadl = next_gpadl_handle;
>
> ret = vmbus_post_msg(gpadl_body,
> - submsginfo->msgsize -
> - sizeof(*submsginfo));
> + submsginfo->msgsize - sizeof(*submsginfo),
> + true);
> if (ret != 0)
> goto cleanup;
>
> @@ -484,8 +484,8 @@ int vmbus_teardown_gpadl(struct vmbus_channel
> *channel, u32 gpadl_handle)
> list_add_tail(&info->msglistentry,
> &vmbus_connection.chn_msg_list);
> spin_unlock_irqrestore(&vmbus_connection.channelmsg_lock, flags);
> - ret = vmbus_post_msg(msg,
> - sizeof(struct vmbus_channel_gpadl_teardown));
> + ret = vmbus_post_msg(msg, sizeof(struct
> vmbus_channel_gpadl_teardown),
> + true);
>
> if (ret)
> goto post_msg_err;
> @@ -556,7 +556,8 @@ static int vmbus_close_internal(struct vmbus_channel
> *channel)
> msg->header.msgtype = CHANNELMSG_CLOSECHANNEL;
> msg->child_relid = channel->offermsg.child_relid;
>
> - ret = vmbus_post_msg(msg, sizeof(struct
> vmbus_channel_close_channel));
> + ret = vmbus_post_msg(msg, sizeof(struct
> vmbus_channel_close_channel),
> + true);
>
> if (ret) {
> pr_err("Close failed: close post msg return is %d\n", ret);
> diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
> index 96a85cd..160d92e 100644
> --- a/drivers/hv/channel_mgmt.c
> +++ b/drivers/hv/channel_mgmt.c
> @@ -321,7 +321,8 @@ static void vmbus_release_relid(u32 relid)
> memset(&msg, 0, sizeof(struct vmbus_channel_relid_released));
> msg.child_relid = relid;
> msg.header.msgtype = CHANNELMSG_RELID_RELEASED;
> - vmbus_post_msg(&msg, sizeof(struct vmbus_channel_relid_released));
> + vmbus_post_msg(&msg, sizeof(struct vmbus_channel_relid_released),
> + true);
> }
>
> void hv_event_tasklet_disable(struct vmbus_channel *channel)
> @@ -728,7 +729,8 @@ void vmbus_initiate_unload(bool crash)
> init_completion(&vmbus_connection.unload_event);
> memset(&hdr, 0, sizeof(struct vmbus_channel_message_header));
> hdr.msgtype = CHANNELMSG_UNLOAD;
> - vmbus_post_msg(&hdr, sizeof(struct vmbus_channel_message_header));
> + vmbus_post_msg(&hdr, sizeof(struct vmbus_channel_message_header),
> + !crash);
>
> /*
> * vmbus_initiate_unload() is also called on crash and the crash
> can be
> @@ -1116,8 +1118,8 @@ int vmbus_request_offers(void)
> msg->msgtype = CHANNELMSG_REQUESTOFFERS;
>
>
> - ret = vmbus_post_msg(msg,
> - sizeof(struct vmbus_channel_message_header));
> + ret = vmbus_post_msg(msg, sizeof(struct
> vmbus_channel_message_header),
> + true);
> if (ret != 0) {
> pr_err("Unable to request offers - %d\n", ret);
>
> diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
> index 78e6368..da1feb4 100644
> --- a/drivers/hv/connection.c
> +++ b/drivers/hv/connection.c
> @@ -110,7 +110,8 @@ static int vmbus_negotiate_version(struct
> vmbus_channel_msginfo *msginfo,
> spin_unlock_irqrestore(&vmbus_connection.channelmsg_lock, flags);
>
> ret = vmbus_post_msg(msg,
> - sizeof(struct vmbus_channel_initiate_contact));
> + sizeof(struct vmbus_channel_initiate_contact),
> + true);
> if (ret != 0) {
> spin_lock_irqsave(&vmbus_connection.channelmsg_lock, flags);
> list_del(&msginfo->msglistentry);
> @@ -434,12 +435,12 @@ void vmbus_on_event(unsigned long data)
> /*
> * vmbus_post_msg - Send a msg on the vmbus's message connection
> */
> -int vmbus_post_msg(void *buffer, size_t buflen)
> +int vmbus_post_msg(void *buffer, size_t buflen, bool can_sleep)
> {
> union hv_connection_id conn_id;
> int ret = 0;
> int retries = 0;
> - u32 usec = 1;
> + u32 msec = 1;
>
> conn_id.asu32 = 0;
> conn_id.u.id = VMBUS_MESSAGE_CONNECTION_ID;
> @@ -449,7 +450,7 @@ int vmbus_post_msg(void *buffer, size_t buflen)
> * insufficient resources. Retry the operation a couple of
> * times before giving up.
> */
> - while (retries < 20) {
> + while (retries < 100) {
> ret = hv_post_message(conn_id, 1, buffer, buflen);
>
> switch (ret) {
> @@ -472,9 +473,13 @@ int vmbus_post_msg(void *buffer, size_t buflen)
> }
>
> retries++;
> - udelay(usec);
> - if (usec < 2048)
> - usec *= 2;
> + if (can_sleep)
> + msleep(msec);
> + else
> + mdelay(msec);
> +
> + if (msec < 256)
> + msec *= 2;
The change looks fine.
I knew, in case of initializing large trunk of receive/send buffers, vmbus_establish_gpadl() is called many times, and retrying of 1ms or more is needed often. So, using milliseconds delay/sleep is good.
Thanks,
Reviewed-by: Haiyang Zhang <haiyangz@...rosoft.com>
Powered by blists - more mailing lists