Re: [BUG] arch/x86/kvm/vmx/vmx_onhyperv.h:109:36: error: dereference of NULL ‘0’

[Vitaly Kuznetsov]

Sean Christopherson <seanjc@xxxxxxxxxx> writes:

> On Wed, Aug 14, 2024, Vitaly Kuznetsov wrote:
>> Sean Christopherson <seanjc@xxxxxxxxxx> writes:
>> 
>> > On Wed, Aug 14, 2024, Vitaly Kuznetsov wrote:
>> >> What I meant is something along these lines (untested):
>> >> 
>> >> diff --git a/arch/x86/kvm/vmx/vmx_onhyperv.h b/arch/x86/kvm/vmx/vmx_onhyperv.h
>> >> index eb48153bfd73..e2d8c67d0cad 100644
>> >> --- a/arch/x86/kvm/vmx/vmx_onhyperv.h
>> >> +++ b/arch/x86/kvm/vmx/vmx_onhyperv.h
>> >> @@ -104,6 +104,14 @@ static inline void evmcs_load(u64 phys_addr)
>> >>         struct hv_vp_assist_page *vp_ap =
>> >>                 hv_get_vp_assist_page(smp_processor_id());
>> >>  
>> >> +       /*
>> >> +        * When enabling eVMCS, KVM verifies that every CPU has a valid hv_vp_assist_page()
>> >> +        * and aborts enabling the feature otherwise. CPU onlining path is also checked in
>> >> +        * vmx_hardware_enable(). With this, it is impossible to reach here with vp_ap == NULL
>> >> +        * but compilers may still complain.
>> >> +        */
>> >> +       BUG_ON(!vp_ap);
>> >
>> > A full BUG_ON() is overkill, and easily avoided.  If we want to add a sanity
>> > check here and do more than just WARN, then it's easy enough to plumb in @vcpu
>> > and make this a KVM_BUG_ON() so that the VM dies, i.e. so that KVM doesn't risk
>> > corrupting the guest somehow.
>> >
>> 
>> I'm still acting under the impression this is an absolutely impossible
>> situation :-)
>> 
>> AFAICS, we only call evmcs_load() from vmcs_load() but this one doesn't
>> have @vcpu/@kvm either and I wasn't sure it's worth the effort to do the
>> plumbing (or am I missing an easy way to go back from @vmcs to
>> @vcpu?). On the other hand, vmcs_load() should not be called that ofter
>> so if we prefer to have @vcpu there for some other reason -- why not.
>
> kvm_get_running_vcpu(), though I honestly purposely didn't suggest it earlier
> because I am not a fan of using kvm_get_running_vcpu() unless it's absolutely
> necessary.  But for this situation, I'd be fine with using it.

Ah, nice, so we don't even need the plumbing then I guess? Compile-tested only:

diff --git a/arch/x86/kvm/vmx/vmx_onhyperv.h b/arch/x86/kvm/vmx/vmx_onhyperv.h
index eb48153bfd73..318f5f95f211 100644
--- a/arch/x86/kvm/vmx/vmx_onhyperv.h
+++ b/arch/x86/kvm/vmx/vmx_onhyperv.h
@@ -104,6 +104,19 @@ static inline void evmcs_load(u64 phys_addr)
        struct hv_vp_assist_page *vp_ap =
                hv_get_vp_assist_page(smp_processor_id());
 
+       /*
+        * When enabling eVMCS, KVM verifies that every CPU has a valid hv_vp_assist_page()
+        * and aborts enabling the feature otherwise. CPU onlining path is also checked in
+        * vmx_hardware_enable(). With this, it is impossible to reach here with vp_ap == NULL
+        * but compilers may still complain.
+        */
+       if (!vp_ap) {
+               struct kvm_vcpu *vcpu = kvm_get_running_vcpu();
+
+               KVM_BUG_ON(1, vcpu->kvm);
+               return;
+       }
+
        if (current_evmcs->hv_enlightenments_control.nested_flush_hypercall)
                vp_ap->nested_control.features.directhypercall = 1;
        vp_ap->current_nested_vmcs = phys_addr;

(I hope we can't reach here with kvm_running_vcpu unset, can we?)

-- 
Vitaly