Re: [PATCH] x86/hyperv: Protect set_hv_tscchange_cb() against getting preempted

[Wei Liu]

On Tue, Oct 12, 2021 at 05:50:05PM +0200, Vitaly Kuznetsov wrote:
> The following issue is observed with CONFIG_DEBUG_PREEMPT when KVM loads:
> 
>  KVM: vmx: using Hyper-V Enlightened VMCS
>  BUG: using smp_processor_id() in preemptible [00000000] code: systemd-udevd/488
>  caller is set_hv_tscchange_cb+0x16/0x80
>  CPU: 1 PID: 488 Comm: systemd-udevd Not tainted 5.15.0-rc5+ #396
>  Hardware name: Microsoft Corporation Virtual Machine/Virtual Machine, BIOS Hyper-V UEFI Release v4.0 12/17/2019
>  Call Trace:
>   dump_stack_lvl+0x6a/0x9a
>   check_preemption_disabled+0xde/0xe0
>   ? kvm_gen_update_masterclock+0xd0/0xd0 [kvm]
>   set_hv_tscchange_cb+0x16/0x80
>   kvm_arch_init+0x23f/0x290 [kvm]
>   kvm_init+0x30/0x310 [kvm]
>   vmx_init+0xaf/0x134 [kvm_intel]
>   ...
> 
> set_hv_tscchange_cb() can get preempted in between acquiring
> smp_processor_id() and writing to HV_X64_MSR_REENLIGHTENMENT_CONTROL. This
> is not an issue by itself: HV_X64_MSR_REENLIGHTENMENT_CONTROL is a
> partition-wide MSR and it doesn't matter which particular CPU will be
> used to receive reenlightenment notifications. The only real problem can
> (in theory) be observed if the CPU whose id was acquired with
> smp_processor_id() goes offline before we manage to write to the MSR,
> the logic in hv_cpu_die() won't be able to reassign it correctly.
> 
> Reported-by: Michael Kelley <mikelley@xxxxxxxxxxxxx>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>

Applied to hyperv-next.

> ---
>  arch/x86/hyperv/hv_init.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> index 708a2712a516..179fc173104d 100644
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -139,7 +139,6 @@ void set_hv_tscchange_cb(void (*cb)(void))
>  	struct hv_reenlightenment_control re_ctrl = {
>  		.vector = HYPERV_REENLIGHTENMENT_VECTOR,
>  		.enabled = 1,
> -		.target_vp = hv_vp_index[smp_processor_id()]
>  	};
>  	struct hv_tsc_emulation_control emu_ctrl = {.enabled = 1};
>  
> @@ -153,8 +152,12 @@ void set_hv_tscchange_cb(void (*cb)(void))
>  	/* Make sure callback is registered before we write to MSRs */
>  	wmb();
>  
> +	re_ctrl.target_vp = hv_vp_index[get_cpu()];
> +
>  	wrmsrl(HV_X64_MSR_REENLIGHTENMENT_CONTROL, *((u64 *)&re_ctrl));
>  	wrmsrl(HV_X64_MSR_TSC_EMULATION_CONTROL, *((u64 *)&emu_ctrl));
> +
> +	put_cpu();
>  }
>  EXPORT_SYMBOL_GPL(set_hv_tscchange_cb);
>  
> -- 
> 2.31.1
>