| 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: <874kub5i02.fsf@vitty.brq.redhat.com>
Date: Thu, 26 Mar 2020 15:05:17 +0100
From: Vitaly Kuznetsov <vkuznets@...hat.com>
To: "Andrea Parri \(Microsoft\)" <parri.andrea@...il.com>,
Dexuan Cui <decui@...rosoft.com>
Cc: "K . Y . Srinivasan" <kys@...rosoft.com>,
Haiyang Zhang <haiyangz@...rosoft.com>,
Stephen Hemminger <sthemmin@...rosoft.com>,
Wei Liu <wei.liu@...nel.org>, linux-hyperv@...r.kernel.org,
Michael Kelley <mikelley@...rosoft.com>,
Boqun Feng <boqun.feng@...il.com>, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 01/11] Drivers: hv: vmbus: Always handle the VMBus messages on CPU0
"Andrea Parri (Microsoft)" <parri.andrea@...il.com> writes:
> A Linux guest have to pick a "connect CPU" to communicate with the
> Hyper-V host. This CPU can not be taken offline because Hyper-V does
> not provide a way to change that CPU assignment.
>
> Current code sets the connect CPU to whatever CPU ends up running the
> function vmbus_negotiate_version(), and this will generate problems if
> that CPU is taken offine.
>
> Establish CPU0 as the connect CPU, and add logics to prevents the
> connect CPU from being taken offline. We could pick some other CPU,
> and we could pick that "other CPU" dynamically if there was a reason to
> do so at some point in the future. But for now, #defining the connect
> CPU to 0 is the most straightforward and least complex solution.
>
> While on this, add inline comments explaining "why" offer and rescind
> messages should not be handled by a same serialized work queue.
>
> Suggested-by: Dexuan Cui <decui@...rosoft.com>
> Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@...il.com>
> ---
> drivers/hv/connection.c | 20 +-------------------
> drivers/hv/hv.c | 7 +++++++
> drivers/hv/hyperv_vmbus.h | 11 ++++++-----
> drivers/hv/vmbus_drv.c | 20 +++++++++++++++++---
> 4 files changed, 31 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
> index 74e77de89b4f3..f4bd306d2cef9 100644
> --- a/drivers/hv/connection.c
> +++ b/drivers/hv/connection.c
> @@ -69,7 +69,6 @@ MODULE_PARM_DESC(max_version,
> int vmbus_negotiate_version(struct vmbus_channel_msginfo *msginfo, u32 version)
> {
> int ret = 0;
> - unsigned int cur_cpu;
> struct vmbus_channel_initiate_contact *msg;
> unsigned long flags;
>
> @@ -102,24 +101,7 @@ int vmbus_negotiate_version(struct vmbus_channel_msginfo *msginfo, u32 version)
>
> msg->monitor_page1 = virt_to_phys(vmbus_connection.monitor_pages[0]);
> msg->monitor_page2 = virt_to_phys(vmbus_connection.monitor_pages[1]);
> - /*
> - * We want all channel messages to be delivered on CPU 0.
> - * This has been the behavior pre-win8. This is not
> - * perf issue and having all channel messages delivered on CPU 0
> - * would be ok.
> - * For post win8 hosts, we support receiving channel messagges on
> - * all the CPUs. This is needed for kexec to work correctly where
> - * the CPU attempting to connect may not be CPU 0.
> - */
> - if (version >= VERSION_WIN8_1) {
> - cur_cpu = get_cpu();
> - msg->target_vcpu = hv_cpu_number_to_vp_number(cur_cpu);
> - vmbus_connection.connect_cpu = cur_cpu;
> - put_cpu();
> - } else {
> - msg->target_vcpu = 0;
> - vmbus_connection.connect_cpu = 0;
> - }
> + msg->target_vcpu = hv_cpu_number_to_vp_number(VMBUS_CONNECT_CPU);
>
> /*
> * Add to list before we send the request since we may
> diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
> index 6098e0cbdb4b0..e2b3310454640 100644
> --- a/drivers/hv/hv.c
> +++ b/drivers/hv/hv.c
> @@ -249,6 +249,13 @@ int hv_synic_cleanup(unsigned int cpu)
> bool channel_found = false;
> unsigned long flags;
>
> + /*
> + * Hyper-V does not provide a way to change the connect CPU once
> + * it is set; we must prevent the connect CPU from going offline.
> + */
> + if (cpu == VMBUS_CONNECT_CPU)
> + return -EBUSY;
> +
> /*
> * Search for channels which are bound to the CPU we're about to
> * cleanup. In case we find one and vmbus is still connected we need to
> diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
> index 70b30e223a578..67fb1edcbf527 100644
> --- a/drivers/hv/hyperv_vmbus.h
> +++ b/drivers/hv/hyperv_vmbus.h
> @@ -212,12 +212,13 @@ enum vmbus_connect_state {
>
> #define MAX_SIZE_CHANNEL_MESSAGE HV_MESSAGE_PAYLOAD_BYTE_COUNT
>
> -struct vmbus_connection {
> - /*
> - * CPU on which the initial host contact was made.
> - */
> - int connect_cpu;
> +/*
> + * The CPU that Hyper-V will interrupt for VMBUS messages, such as
> + * CHANNELMSG_OFFERCHANNEL and CHANNELMSG_RESCIND_CHANNELOFFER.
> + */
> +#define VMBUS_CONNECT_CPU 0
>
> +struct vmbus_connection {
> u32 msg_conn_id;
>
> atomic_t offer_in_progress;
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 029378c27421d..7600615e13754 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -1056,14 +1056,28 @@ void vmbus_on_msg_dpc(unsigned long data)
> /*
> * If we are handling the rescind message;
> * schedule the work on the global work queue.
> + *
> + * The OFFER message and the RESCIND message should
> + * not be handled by the same serialized work queue,
> + * because the OFFER handler may call vmbus_open(),
> + * which tries to open the channel by sending an
> + * OPEN_CHANNEL message to the host and waits for
> + * the host's response; however, if the host has
> + * rescinded the channel before it receives the
> + * OPEN_CHANNEL message, the host just silently
> + * ignores the OPEN_CHANNEL message; as a result,
> + * the guest's OFFER handler hangs for ever, if we
> + * handle the RESCIND message in the same serialized
> + * work queue: the RESCIND handler can not start to
> + * run before the OFFER handler finishes.
> */
> - schedule_work_on(vmbus_connection.connect_cpu,
> + schedule_work_on(VMBUS_CONNECT_CPU,
> &ctx->work);
> break;
>
> case CHANNELMSG_OFFERCHANNEL:
> atomic_inc(&vmbus_connection.offer_in_progress);
> - queue_work_on(vmbus_connection.connect_cpu,
> + queue_work_on(VMBUS_CONNECT_CPU,
> vmbus_connection.work_queue,
> &ctx->work);
> break;
> @@ -1110,7 +1124,7 @@ static void vmbus_force_channel_rescinded(struct vmbus_channel *channel)
>
> INIT_WORK(&ctx->work, vmbus_onmessage_work);
>
> - queue_work_on(vmbus_connection.connect_cpu,
> + queue_work_on(VMBUS_CONNECT_CPU,
> vmbus_connection.work_queue,
> &ctx->work);
> }
I tried to refresh my memory on why 'connect_cpu' was introduced and it
all comes down to the following commit:
commit 7268644734f6a300353a4c4ff8bf3e013ba80f89
Author: Alex Ng <alexng@...rosoft.com>
Date: Fri Feb 26 15:13:22 2016 -0800
Drivers: hv: vmbus: Support kexec on ws2012 r2 and above
which for some unknown reason kept hardcoding '0' for pre-win2012-r2 (
hv_context.vp_index[smp_processor_id()] in all cases would do exactly
the same I guess ). Later, 'connect_cpu' appeared just to remember our
choice, I can't see why we didn't go with CPU0 for simplicity.
Reviewed-by: Vitaly Kuznetsov <vkuznets@...hat.com>
--
Vitaly
Powered by blists - more mailing lists