Messages in this thread | | | From | Vitaly Kuznetsov <> | Subject | Re: [PATCH v2 10/50] KVM: VMX: Reset eVMCS controls in VP assist page during hardware disabling | Date | Thu, 01 Dec 2022 16:42:58 +0100 |
| |
Sean Christopherson <seanjc@google.com> writes:
> Reset the eVMCS controls in the per-CPU VP assist page during hardware > disabling instead of waiting until kvm-intel's module exit. The controls > are activated if and only if KVM creates a VM, i.e. don't need to be > reset if hardware is never enabled. > > Doing the reset during hardware disabling will naturally fix a potential > NULL pointer deref bug once KVM disables CPU hotplug while enabling and > disabling hardware (which is necessary to fix a variety of bugs). If the > kernel is running as the root partition, the VP assist page is unmapped > during CPU hot unplug, and so KVM's clearing of the eVMCS controls needs > to occur with CPU hot(un)plug disabled, otherwise KVM could attempt to > write to a CPU's VP assist page after it's unmapped. > > Reported-by: Vitaly Kuznetsov <vkuznets@redhat.com> > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/kvm/vmx/vmx.c | 50 +++++++++++++++++++++++++----------------- > 1 file changed, 30 insertions(+), 20 deletions(-) > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index cea8c07f5229..d85d175dca70 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -551,6 +551,33 @@ static int hv_enable_l2_tlb_flush(struct kvm_vcpu *vcpu) > return 0; > } > > +static void hv_reset_evmcs(void) > +{ > + struct hv_vp_assist_page *vp_ap; > + > + if (!static_branch_unlikely(&enable_evmcs)) > + return; > + > + /* > + * KVM should enable eVMCS if and only if all CPUs have a VP assist > + * page, and should reject CPU onlining if eVMCS is enabled the CPU > + * doesn't have a VP assist page allocated. > + */ > + vp_ap = hv_get_vp_assist_page(smp_processor_id()); > + if (WARN_ON_ONCE(!vp_ap)) > + return; > +
In case my understanding is correct, this may actually get triggered for Hyper-V root partition: vmx_hardware_disable() gets called from kvm_dying_cpu() which has its own CPUHP_AP_KVM_STARTING stage. VP page unmapping happens in hv_cpu_die() which uses generic CPUHP_AP_ONLINE_DYN (happens first on CPU oflining AFAIR). I believe we need to introduce a new CPUHP_AP_HYPERV_STARTING stage and put it before CPUHP_AP_KVM_STARTING so it happens after it upon offlining.
The issue is likely theoretical as Hyper-V root partition is a very special case, I'm not sure whether KVM is used there and whether CPU offlining is possible. In any case, WARN_ON_ONCE() is much better than NULL pointer dereference we have now :-)
> + /* > + * Reset everything to support using non-enlightened VMCS access later > + * (e.g. when we reload the module with enlightened_vmcs=0) > + */ > + vp_ap->nested_control.features.directhypercall = 0; > + vp_ap->current_nested_vmcs = 0; > + vp_ap->enlighten_vmentry = 0; > +} > + > +#else /* IS_ENABLED(CONFIG_HYPERV) */ > +static void hv_reset_evmcs(void) {} > #endif /* IS_ENABLED(CONFIG_HYPERV) */ > > /* > @@ -2496,6 +2523,8 @@ static void vmx_hardware_disable(void) > if (cpu_vmxoff()) > kvm_spurious_fault(); > > + hv_reset_evmcs(); > + > intel_pt_handle_vmx(0); > } > > @@ -8462,27 +8491,8 @@ static void vmx_exit(void) > kvm_exit(); > > #if IS_ENABLED(CONFIG_HYPERV) > - if (static_branch_unlikely(&enable_evmcs)) { > - int cpu; > - struct hv_vp_assist_page *vp_ap; > - /* > - * Reset everything to support using non-enlightened VMCS > - * access later (e.g. when we reload the module with > - * enlightened_vmcs=0) > - */ > - for_each_online_cpu(cpu) { > - vp_ap = hv_get_vp_assist_page(cpu); > - > - if (!vp_ap) > - continue; > - > - vp_ap->nested_control.features.directhypercall = 0; > - vp_ap->current_nested_vmcs = 0; > - vp_ap->enlighten_vmentry = 0; > - } > - > + if (static_branch_unlikely(&enable_evmcs)) > static_branch_disable(&enable_evmcs); > - } > #endif > vmx_cleanup_l1d_flush();
Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
-- Vitaly
| |