Re: [PATCH v2] x86/hyperv: fix kexec crash due to VP assist page corruption

[Vitaly Kuznetsov <vkuznets@redhat.com>]

Anirudh Rayabharam <anirudh@anirudhrb.com> writes:

> From: Anirudh Rayabharam (Microsoft) <anirudh@anirudhrb.com>
>
> commit 9636be85cc5b ("x86/hyperv: Fix hyperv_pcpu_input_arg handling when
> CPUs go online/offline") introduces a new cpuhp state for hyperv
> initialization.
>
> cpuhp_setup_state() returns the state number if state is
> CPUHP_AP_ONLINE_DYN or CPUHP_BP_PREPARE_DYN and 0 for all other states.
> For the hyperv case, since a new cpuhp state was introduced it would
> return 0. However, in hv_machine_shutdown(), the cpuhp_remove_state() call
> is conditioned upon "hyperv_init_cpuhp > 0". This will never be true and
> so hv_cpu_die() won't be called on all CPUs. This means the VP assist page
> won't be reset. When the kexec kernel tries to setup the VP assist page
> again, the hypervisor corrupts the memory region of the old VP assist page
> causing a panic in case the kexec kernel is using that memory elsewhere.
> This was originally fixed in commit dfe94d4086e4 ("x86/hyperv: Fix kexec
> panic/hang issues").
>
> Get rid of hyperv_init_cpuhp entirely since we are no longer using a
> dynamic cpuhp state and use CPUHP_AP_HYPERV_ONLINE directly with
> cpuhp_remove_state().
>
> Cc: stable@vger.kernel.org
> Fixes: 9636be85cc5b ("x86/hyperv: Fix hyperv_pcpu_input_arg handling when CPUs go online/offline")
> Signed-off-by: Anirudh Rayabharam (Microsoft) <anirudh@anirudhrb.com>
> ---
>
> v1->v2:
> - Remove hyperv_init_cpuhp entirely and use CPUHP_AP_HYPERV_ONLINE directly
>   with cpuhp_remove_state().

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

Thanks!

>
> v1: https://lore.kernel.org/linux-hyperv/87wmk2xt5i.fsf@redhat.com/T/#m54b8ae17e98d65e77a09002e478669d15d9830d0
>
> ---
>  arch/x86/hyperv/hv_init.c       | 5 +----
>  arch/x86/include/asm/mshyperv.h | 1 -
>  arch/x86/kernel/cpu/mshyperv.c  | 4 ++--
>  3 files changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> index 17a71e92a343..95eada2994e1 100644
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -35,7 +35,6 @@
>  #include <clocksource/hyperv_timer.h>
>  #include <linux/highmem.h>
>  
> -int hyperv_init_cpuhp;
>  u64 hv_current_partition_id = ~0ull;
>  EXPORT_SYMBOL_GPL(hv_current_partition_id);
>  
> @@ -607,8 +606,6 @@ void __init hyperv_init(void)
>  
>  	register_syscore_ops(&hv_syscore_ops);
>  
> -	hyperv_init_cpuhp = cpuhp;
> -
>  	if (cpuid_ebx(HYPERV_CPUID_FEATURES) & HV_ACCESS_PARTITION_ID)
>  		hv_get_partition_id();
>  
> @@ -637,7 +634,7 @@ void __init hyperv_init(void)
>  clean_guest_os_id:
>  	wrmsrl(HV_X64_MSR_GUEST_OS_ID, 0);
>  	hv_ivm_msr_write(HV_X64_MSR_GUEST_OS_ID, 0);
> -	cpuhp_remove_state(cpuhp);
> +	cpuhp_remove_state(CPUHP_AP_HYPERV_ONLINE);
>  free_ghcb_page:
>  	free_percpu(hv_ghcb_pg);
>  free_vp_assist_page:
> diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
> index 390c4d13956d..5f0bc6a6d025 100644
> --- a/arch/x86/include/asm/mshyperv.h
> +++ b/arch/x86/include/asm/mshyperv.h
> @@ -40,7 +40,6 @@ static inline unsigned char hv_get_nmi_reason(void)
>  }
>  
>  #if IS_ENABLED(CONFIG_HYPERV)
> -extern int hyperv_init_cpuhp;
>  extern bool hyperv_paravisor_present;
>  
>  extern void *hv_hypercall_pg;
> diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
> index e0fd57a8ba84..e98db51f25ba 100644
> --- a/arch/x86/kernel/cpu/mshyperv.c
> +++ b/arch/x86/kernel/cpu/mshyperv.c
> @@ -199,8 +199,8 @@ static void hv_machine_shutdown(void)
>  	 * Call hv_cpu_die() on all the CPUs, otherwise later the hypervisor
>  	 * corrupts the old VP Assist Pages and can crash the kexec kernel.
>  	 */
> -	if (kexec_in_progress && hyperv_init_cpuhp > 0)
> -		cpuhp_remove_state(hyperv_init_cpuhp);
> +	if (kexec_in_progress)
> +		cpuhp_remove_state(CPUHP_AP_HYPERV_ONLINE);
>  
>  	/* The function calls stop_other_cpus(). */
>  	native_machine_shutdown();

-- 
Vitaly