| Message ID | 20210520230339.267445-3-jmattson@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: nVMX: Fix vmcs02 PID use-after-free issue | expand |
On 21/05/21 01:03, Jim Mattson wrote: > At present, there are two reasons why kvm_check_nested_events may > return a non-zero value: > > 1) we just emulated a shutdown VM-exit from L2 to L1. > 2) we need to perform an immediate VM-exit from vmcs02. > > In either case, transition the vCPU to "running." > > Signed-off-by: Jim Mattson <jmattson@google.com> > Reviewed-by: Oliver Upton <oupton@google.com> > --- > arch/x86/kvm/x86.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index d517460db413..d3fea8ea3628 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -9468,8 +9468,8 @@ static inline int vcpu_block(struct kvm *kvm, struct kvm_vcpu *vcpu) > > static inline bool kvm_vcpu_running(struct kvm_vcpu *vcpu) > { > - if (is_guest_mode(vcpu)) > - kvm_check_nested_events(vcpu); > + if (is_guest_mode(vcpu) && kvm_check_nested_events(vcpu)) > + return true; That doesn't make the vCPU running. You still need to go through vcpu_block, which would properly update the vCPU's mp_state. What is this patch fixing? Paolo > return (vcpu->arch.mp_state == KVM_MP_STATE_RUNNABLE && > !vcpu->arch.apf.halted); >
Without this patch, the accompanying selftest never wakes up from HLT in L2. If you can get the selftest to work without this patch, feel free to drop it. On Mon, May 24, 2021 at 8:43 AM Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 21/05/21 01:03, Jim Mattson wrote: > > At present, there are two reasons why kvm_check_nested_events may > > return a non-zero value: > > > > 1) we just emulated a shutdown VM-exit from L2 to L1. > > 2) we need to perform an immediate VM-exit from vmcs02. > > > > In either case, transition the vCPU to "running." > > > > Signed-off-by: Jim Mattson <jmattson@google.com> > > Reviewed-by: Oliver Upton <oupton@google.com> > > --- > > arch/x86/kvm/x86.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > index d517460db413..d3fea8ea3628 100644 > > --- a/arch/x86/kvm/x86.c > > +++ b/arch/x86/kvm/x86.c > > @@ -9468,8 +9468,8 @@ static inline int vcpu_block(struct kvm *kvm, struct kvm_vcpu *vcpu) > > > > static inline bool kvm_vcpu_running(struct kvm_vcpu *vcpu) > > { > > - if (is_guest_mode(vcpu)) > > - kvm_check_nested_events(vcpu); > > + if (is_guest_mode(vcpu) && kvm_check_nested_events(vcpu)) > > + return true; > > That doesn't make the vCPU running. You still need to go through > vcpu_block, which would properly update the vCPU's mp_state. > > What is this patch fixing? > > Paolo > > > return (vcpu->arch.mp_state == KVM_MP_STATE_RUNNABLE && > > !vcpu->arch.apf.halted); > > >
On 24/05/21 18:39, Jim Mattson wrote: > Without this patch, the accompanying selftest never wakes up from HLT > in L2. If you can get the selftest to work without this patch, feel > free to drop it. Ok, that's a pretty good reason. I'll try to debug it. Paolo > On Mon, May 24, 2021 at 8:43 AM Paolo Bonzini <pbonzini@redhat.com> wrote: >> >> On 21/05/21 01:03, Jim Mattson wrote: >>> At present, there are two reasons why kvm_check_nested_events may >>> return a non-zero value: >>> >>> 1) we just emulated a shutdown VM-exit from L2 to L1. >>> 2) we need to perform an immediate VM-exit from vmcs02. >>> >>> In either case, transition the vCPU to "running." >>> >>> Signed-off-by: Jim Mattson <jmattson@google.com> >>> Reviewed-by: Oliver Upton <oupton@google.com> >>> --- >>> arch/x86/kvm/x86.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >>> index d517460db413..d3fea8ea3628 100644 >>> --- a/arch/x86/kvm/x86.c >>> +++ b/arch/x86/kvm/x86.c >>> @@ -9468,8 +9468,8 @@ static inline int vcpu_block(struct kvm *kvm, struct kvm_vcpu *vcpu) >>> >>> static inline bool kvm_vcpu_running(struct kvm_vcpu *vcpu) >>> { >>> - if (is_guest_mode(vcpu)) >>> - kvm_check_nested_events(vcpu); >>> + if (is_guest_mode(vcpu) && kvm_check_nested_events(vcpu)) >>> + return true; >> >> That doesn't make the vCPU running. You still need to go through >> vcpu_block, which would properly update the vCPU's mp_state. >> >> What is this patch fixing? >> >> Paolo >> >>> return (vcpu->arch.mp_state == KVM_MP_STATE_RUNNABLE && >>> !vcpu->arch.apf.halted); >>> >> >
On Mon, May 24, 2021 at 9:43 AM Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 24/05/21 18:39, Jim Mattson wrote: > > Without this patch, the accompanying selftest never wakes up from HLT > > in L2. If you can get the selftest to work without this patch, feel > > free to drop it. > > Ok, that's a pretty good reason. I'll try to debug it. > Note that it works (without this patch) if you change the HLT to an infinite loop.
On Mon, May 24, 2021, Paolo Bonzini wrote: > On 24/05/21 18:39, Jim Mattson wrote: > > Without this patch, the accompanying selftest never wakes up from HLT > > in L2. If you can get the selftest to work without this patch, feel > > free to drop it. > > Ok, that's a pretty good reason. I'll try to debug it. I don't think there's any debug necessary, the hack of unconditionally calling kvm_check_nested_events() in kvm_vcpu_running() handles the case where a pending event/exception causes nested VM-Exit, but doesn't handle the case where KVM can't immediately service the nested VM-Exit. Because the event is never serviced (doesn't cause a VM-Exit) and doesn't show up as a pending event, kvm_vcpu_has_events() and thus kvm_arch_vcpu_runnable() will never become true, i.e. the vCPU gets stuck in L2 HLT. Until Jim's selftest, that was limited to the "request immediate exit" case, which meant that to hit the bug a VM-Exiting event needed to collide with nested VM-Enter that also put L2 into HLT state without a different pending wake event. Jim's selftest adds a more direct path in the form of the -EXNIO when the PI descriptor hits a memslot hole. The proper fix is what we discussed in the other thread; get kvm_vcpu_has_events() to return true if there's a nested event pending. If I'm right, this hack-a-fix should go to stable. Eww... > > On Mon, May 24, 2021 at 8:43 AM Paolo Bonzini <pbonzini@redhat.com> wrote: > > > > > > On 21/05/21 01:03, Jim Mattson wrote: > > > > At present, there are two reasons why kvm_check_nested_events may > > > > return a non-zero value: > > > > > > > > 1) we just emulated a shutdown VM-exit from L2 to L1. > > > > 2) we need to perform an immediate VM-exit from vmcs02. > > > > > > > > In either case, transition the vCPU to "running." > > > > > > > > Signed-off-by: Jim Mattson <jmattson@google.com> > > > > Reviewed-by: Oliver Upton <oupton@google.com> > > > > --- > > > > arch/x86/kvm/x86.c | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > > > index d517460db413..d3fea8ea3628 100644 > > > > --- a/arch/x86/kvm/x86.c > > > > +++ b/arch/x86/kvm/x86.c > > > > @@ -9468,8 +9468,8 @@ static inline int vcpu_block(struct kvm *kvm, struct kvm_vcpu *vcpu) > > > > > > > > static inline bool kvm_vcpu_running(struct kvm_vcpu *vcpu) > > > > { > > > > - if (is_guest_mode(vcpu)) > > > > - kvm_check_nested_events(vcpu); > > > > + if (is_guest_mode(vcpu) && kvm_check_nested_events(vcpu)) > > > > + return true; > > > > > > That doesn't make the vCPU running. You still need to go through > > > vcpu_block, which would properly update the vCPU's mp_state. > > > > > > What is this patch fixing? > > > > > > Paolo > > > > > > > return (vcpu->arch.mp_state == KVM_MP_STATE_RUNNABLE && > > > > !vcpu->arch.apf.halted); > > > > > > > > > >
On Mon, May 24, 2021 at 4:10 PM Sean Christopherson <seanjc@google.com> wrote: > > On Mon, May 24, 2021, Paolo Bonzini wrote: > > On 24/05/21 18:39, Jim Mattson wrote: > > > Without this patch, the accompanying selftest never wakes up from HLT > > > in L2. If you can get the selftest to work without this patch, feel > > > free to drop it. > > > > Ok, that's a pretty good reason. I'll try to debug it. > > I don't think there's any debug necessary, the hack of unconditionally calling > kvm_check_nested_events() in kvm_vcpu_running() ... We don't unconditionally call kvm_check_nested_events() in kvm_vcpu_running(). We still call kvm_check_nested_events() only when is_guest_mode(vcpu). The only change introduced in this patch is that we stop ignoring the result.
On Mon, May 24, 2021, Jim Mattson wrote: > On Mon, May 24, 2021 at 4:10 PM Sean Christopherson <seanjc@google.com> wrote: > > > > On Mon, May 24, 2021, Paolo Bonzini wrote: > > > On 24/05/21 18:39, Jim Mattson wrote: > > > > Without this patch, the accompanying selftest never wakes up from HLT > > > > in L2. If you can get the selftest to work without this patch, feel > > > > free to drop it. > > > > > > Ok, that's a pretty good reason. I'll try to debug it. > > > > I don't think there's any debug necessary, the hack of unconditionally calling > > kvm_check_nested_events() in kvm_vcpu_running() ... > > We don't unconditionally call kvm_check_nested_events() in > kvm_vcpu_running(). We still call kvm_check_nested_events() only when > is_guest_mode(vcpu). The only change introduced in this patch is that > we stop ignoring the result. Doh, sorry, bad use of "unconditionally". I meant "unconditionally when in L2". :-)
On Mon, May 24, 2021 at 4:24 PM Sean Christopherson <seanjc@google.com> wrote: > > On Mon, May 24, 2021, Jim Mattson wrote: > > On Mon, May 24, 2021 at 4:10 PM Sean Christopherson <seanjc@google.com> wrote: > > > > > > On Mon, May 24, 2021, Paolo Bonzini wrote: > > > > On 24/05/21 18:39, Jim Mattson wrote: > > > > > Without this patch, the accompanying selftest never wakes up from HLT > > > > > in L2. If you can get the selftest to work without this patch, feel > > > > > free to drop it. > > > > > > > > Ok, that's a pretty good reason. I'll try to debug it. > > > > > > I don't think there's any debug necessary, the hack of unconditionally calling > > > kvm_check_nested_events() in kvm_vcpu_running() ... > > > > We don't unconditionally call kvm_check_nested_events() in > > kvm_vcpu_running(). We still call kvm_check_nested_events() only when > > is_guest_mode(vcpu). The only change introduced in this patch is that > > we stop ignoring the result. > > Doh, sorry, bad use of "unconditionally". I meant "unconditionally when in L2". :-) Again, the conditions under which we call kvm_check_nested_events are unchanged. The only "hack" here is the hack of not ignoring the return value.
On Mon, May 24, 2021, Jim Mattson wrote: > On Mon, May 24, 2021 at 4:24 PM Sean Christopherson <seanjc@google.com> wrote: > > > > On Mon, May 24, 2021, Jim Mattson wrote: > > > On Mon, May 24, 2021 at 4:10 PM Sean Christopherson <seanjc@google.com> wrote: > > > > > > > > On Mon, May 24, 2021, Paolo Bonzini wrote: > > > > > On 24/05/21 18:39, Jim Mattson wrote: > > > > > > Without this patch, the accompanying selftest never wakes up from HLT > > > > > > in L2. If you can get the selftest to work without this patch, feel > > > > > > free to drop it. > > > > > > > > > > Ok, that's a pretty good reason. I'll try to debug it. > > > > > > > > I don't think there's any debug necessary, the hack of unconditionally calling > > > > kvm_check_nested_events() in kvm_vcpu_running() ... > > > > > > We don't unconditionally call kvm_check_nested_events() in > > > kvm_vcpu_running(). We still call kvm_check_nested_events() only when > > > is_guest_mode(vcpu). The only change introduced in this patch is that > > > we stop ignoring the result. > > > > Doh, sorry, bad use of "unconditionally". I meant "unconditionally when in L2". :-) > > Again, the conditions under which we call kvm_check_nested_events are > unchanged. The only "hack" here is the hack of not ignoring the return > value. I don't disagree, all I'm saying is that the existing code is a hack, and this doesn't fix/cleanse that hack. I agree that this patch is a good intermediate change.
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index d517460db413..d3fea8ea3628 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -9468,8 +9468,8 @@ static inline int vcpu_block(struct kvm *kvm, struct kvm_vcpu *vcpu) static inline bool kvm_vcpu_running(struct kvm_vcpu *vcpu) { - if (is_guest_mode(vcpu)) - kvm_check_nested_events(vcpu); + if (is_guest_mode(vcpu) && kvm_check_nested_events(vcpu)) + return true; return (vcpu->arch.mp_state == KVM_MP_STATE_RUNNABLE && !vcpu->arch.apf.halted);