Message ID | 1579749241-712-1-git-send-email-linmiaohe@huawei.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | KVM: nVMX: set rflags to specify success in handle_invvpid() default case | expand |
linmiaohe <linmiaohe@huawei.com> writes: > From: Miaohe Lin <linmiaohe@huawei.com> > > In handle_invvpid() default case, we just skip emulated instruction and > forget to set rflags to specify success. This would result in indefinite > rflags value and thus indeterminate return value for guest. > > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > --- > Chinese New Year is coming. Happy Spring Festival! ^_^ Happy Spring Festival! > --- > arch/x86/kvm/vmx/nested.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c > index 7608924ee8c1..985d3307ec56 100644 > --- a/arch/x86/kvm/vmx/nested.c > +++ b/arch/x86/kvm/vmx/nested.c > @@ -5165,7 +5165,7 @@ static int handle_invvpid(struct kvm_vcpu *vcpu) > break; > default: > WARN_ON_ONCE(1); > - return kvm_skip_emulated_instruction(vcpu); > + break; > } > > return nested_vmx_succeed(vcpu); Your patch seems to do the right thing, however, I started wondering if WARN_ON_ONCE() is the right thing to do. SDM says that "If an unsupported INVVPID type is specified, the instruction fails." and this is similar to INVEPT and I decided to check what handle_invept() does. Well, it does BUG_ON(). Are we doing the right thing in any of these cases?
On 23/01/20 09:55, Vitaly Kuznetsov wrote: >> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c >> index 7608924ee8c1..985d3307ec56 100644 >> --- a/arch/x86/kvm/vmx/nested.c >> +++ b/arch/x86/kvm/vmx/nested.c >> @@ -5165,7 +5165,7 @@ static int handle_invvpid(struct kvm_vcpu *vcpu) >> break; >> default: >> WARN_ON_ONCE(1); >> - return kvm_skip_emulated_instruction(vcpu); >> + break; >> } >> >> return nested_vmx_succeed(vcpu); > Your patch seems to do the right thing, however, I started wondering if > WARN_ON_ONCE() is the right thing to do. SDM says that "If an > unsupported INVVPID type is specified, the instruction fails." and this > is similar to INVEPT and I decided to check what handle_invept() > does. Well, it does BUG_ON(). > > Are we doing the right thing in any of these cases? Yes, both INVEPT and INVVPID catch this earlier. For INVEPT: types = (vmx->nested.msrs.ept_caps >> VMX_EPT_EXTENT_SHIFT) & 6; if (type >= 32 || !(types & (1 << type))) return nested_vmx_failValid(vcpu, VMXERR_INVALID_OPERAND_TO_INVEPT_INVVPID); For INVVPID: types = (vmx->nested.msrs.vpid_caps & VMX_VPID_EXTENT_SUPPORTED_MASK) >> 8; if (type >= 32 || !(types & (1 << type))) return nested_vmx_failValid(vcpu, VMXERR_INVALID_OPERAND_TO_INVEPT_INVVPID); So I'm leaning towards not applying Miaohe's patch. Happy Mouse Year to everyone, here is an ASCII art (except for one Unicode character) mouse: __()() / o) ~~~~\_,__,_>° Thanks,
Paolo Bonzini <pbonzini@redhat.com> writes: > On 23/01/20 09:55, Vitaly Kuznetsov wrote: >>> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c >>> index 7608924ee8c1..985d3307ec56 100644 >>> --- a/arch/x86/kvm/vmx/nested.c >>> +++ b/arch/x86/kvm/vmx/nested.c >>> @@ -5165,7 +5165,7 @@ static int handle_invvpid(struct kvm_vcpu *vcpu) >>> break; >>> default: >>> WARN_ON_ONCE(1); >>> - return kvm_skip_emulated_instruction(vcpu); >>> + break; >>> } >>> >>> return nested_vmx_succeed(vcpu); >> Your patch seems to do the right thing, however, I started wondering if >> WARN_ON_ONCE() is the right thing to do. SDM says that "If an >> unsupported INVVPID type is specified, the instruction fails." and this >> is similar to INVEPT and I decided to check what handle_invept() >> does. Well, it does BUG_ON(). >> >> Are we doing the right thing in any of these cases? > > Yes, both INVEPT and INVVPID catch this earlier. > > For INVEPT: > > types = (vmx->nested.msrs.ept_caps >> VMX_EPT_EXTENT_SHIFT) & 6; > > if (type >= 32 || !(types & (1 << type))) > return nested_vmx_failValid(vcpu, > VMXERR_INVALID_OPERAND_TO_INVEPT_INVVPID); > > > > For INVVPID: > > types = (vmx->nested.msrs.vpid_caps & > VMX_VPID_EXTENT_SUPPORTED_MASK) >> 8; > > if (type >= 32 || !(types & (1 << type))) > return nested_vmx_failValid(vcpu, > VMXERR_INVALID_OPERAND_TO_INVEPT_INVVPID); > Ah, true, thanks for checking! > So I'm leaning towards not applying Miaohe's patch. Well, we may at least want to converge on BUG_ON() for both handle_invvpid()/handle_invept(), there's no need for them to differ.
On 23/01/20 10:45, Vitaly Kuznetsov wrote: >>> SDM says that "If an >>> unsupported INVVPID type is specified, the instruction fails." and this >>> is similar to INVEPT and I decided to check what handle_invept() >>> does. Well, it does BUG_ON(). >>> >>> Are we doing the right thing in any of these cases? >> >> Yes, both INVEPT and INVVPID catch this earlier. >> >> So I'm leaning towards not applying Miaohe's patch. > > Well, we may at least want to converge on BUG_ON() for both > handle_invvpid()/handle_invept(), there's no need for them to differ. WARN_ON_ONCE + nested_vmx_failValid would probably be better, if we really want to change this. Paolo
On Thu, Jan 23, 2020 at 1:54 AM Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 23/01/20 10:45, Vitaly Kuznetsov wrote: > >>> SDM says that "If an > >>> unsupported INVVPID type is specified, the instruction fails." and this > >>> is similar to INVEPT and I decided to check what handle_invept() > >>> does. Well, it does BUG_ON(). > >>> > >>> Are we doing the right thing in any of these cases? > >> > >> Yes, both INVEPT and INVVPID catch this earlier. > >> > >> So I'm leaning towards not applying Miaohe's patch. > > > > Well, we may at least want to converge on BUG_ON() for both > > handle_invvpid()/handle_invept(), there's no need for them to differ. > > WARN_ON_ONCE + nested_vmx_failValid would probably be better, if we > really want to change this. > > Paolo In both cases, something is seriously wrong. The only plausible explanations are compiler error or hardware failure. It would be nice to handle *all* such failures with a KVM_INTERNAL_ERROR exit to userspace. (I'm also thinking of situations like getting a VM-exit for INIT.)
On Thu, Jan 23, 2020 at 10:22:24AM -0800, Jim Mattson wrote: > On Thu, Jan 23, 2020 at 1:54 AM Paolo Bonzini <pbonzini@redhat.com> wrote: > > > > On 23/01/20 10:45, Vitaly Kuznetsov wrote: > > >>> SDM says that "If an > > >>> unsupported INVVPID type is specified, the instruction fails." and this > > >>> is similar to INVEPT and I decided to check what handle_invept() > > >>> does. Well, it does BUG_ON(). > > >>> > > >>> Are we doing the right thing in any of these cases? > > >> > > >> Yes, both INVEPT and INVVPID catch this earlier. > > >> > > >> So I'm leaning towards not applying Miaohe's patch. > > > > > > Well, we may at least want to converge on BUG_ON() for both > > > handle_invvpid()/handle_invept(), there's no need for them to differ. > > > > WARN_ON_ONCE + nested_vmx_failValid would probably be better, if we > > really want to change this. > > > > Paolo > > In both cases, something is seriously wrong. The only plausible > explanations are compiler error or hardware failure. It would be nice > to handle *all* such failures with a KVM_INTERNAL_ERROR exit to > userspace. (I'm also thinking of situations like getting a VM-exit for > INIT.) Ya. Vitaly and I had a similar discussion[*]. The idea we tossed around was to also mark the VM as having encountered a KVM/hardware bug so that the VM is effectively dead. That would also allow gracefully handling bugs that are detected deep in the stack, i.e. can't simply return 0 to get out to userspace. [*] https://lkml.kernel.org/r/20190930153358.GD14693@linux.intel.com
Sean Christopherson <sean.j.christopherson@intel.com> writes: > On Thu, Jan 23, 2020 at 10:22:24AM -0800, Jim Mattson wrote: >> On Thu, Jan 23, 2020 at 1:54 AM Paolo Bonzini <pbonzini@redhat.com> wrote: >> > >> > On 23/01/20 10:45, Vitaly Kuznetsov wrote: >> > >>> SDM says that "If an >> > >>> unsupported INVVPID type is specified, the instruction fails." and this >> > >>> is similar to INVEPT and I decided to check what handle_invept() >> > >>> does. Well, it does BUG_ON(). >> > >>> >> > >>> Are we doing the right thing in any of these cases? >> > >> >> > >> Yes, both INVEPT and INVVPID catch this earlier. >> > >> >> > >> So I'm leaning towards not applying Miaohe's patch. >> > > >> > > Well, we may at least want to converge on BUG_ON() for both >> > > handle_invvpid()/handle_invept(), there's no need for them to differ. >> > >> > WARN_ON_ONCE + nested_vmx_failValid would probably be better, if we >> > really want to change this. >> > >> > Paolo >> >> In both cases, something is seriously wrong. The only plausible >> explanations are compiler error or hardware failure. It would be nice >> to handle *all* such failures with a KVM_INTERNAL_ERROR exit to >> userspace. (I'm also thinking of situations like getting a VM-exit for >> INIT.) > > Ya. Vitaly and I had a similar discussion[*]. The idea we tossed around > was to also mark the VM as having encountered a KVM/hardware bug so that > the VM is effectively dead. That would also allow gracefully handling bugs > that are detected deep in the stack, i.e. can't simply return 0 to get out > to userspace. Yea, I was thinking about introducing a big hammer which would stop the whole VM as soon as possible to make it easier to debug such situations. Something like (not really tested): diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index cf917139de6b..5476f88c9ada 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -8001,6 +8001,15 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) bool req_immediate_exit = false; if (kvm_request_pending(vcpu)) { + /* INTERROR check should always come first */ + if (kvm_check_request(KVM_REQ_INTERROR, vcpu)) { + if (vcpu->run->exit_reason != KVM_EXIT_INTERNAL_ERROR) { + vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR; + vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_OTHERCPU; + } + r = 0; + goto out; + } if (kvm_check_request(KVM_REQ_GET_VMCS12_PAGES, vcpu)) { if (unlikely(!kvm_x86_ops->get_vmcs12_pages(vcpu))) { r = 0; @@ -8510,6 +8519,13 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) kvm_sigset_activate(vcpu); kvm_load_guest_fpu(vcpu); + if (unlikely(vcpu->kvm->vm_bugged)) { + vcpu->run->exit_reason = KVM_REQ_INTERROR; + /* Maybe a suberror for 'attempted to run a vCPU of a bugged VM? */ + r = 0; + goto out; + } + if (unlikely(vcpu->arch.mp_state == KVM_MP_STATE_UNINITIALIZED)) { if (kvm_run->immediate_exit) { r = -EINTR; diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index 538c25e778c0..d003be5fcf42 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -146,6 +146,7 @@ static inline bool is_error_page(struct page *page) #define KVM_REQ_MMU_RELOAD (1 | KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP) #define KVM_REQ_PENDING_TIMER 2 #define KVM_REQ_UNHALT 3 +#define KVM_REQ_INTERROR (4 | KVM_REQUEST_WAIT) #define KVM_REQUEST_ARCH_BASE 8 #define KVM_ARCH_REQ_FLAGS(nr, flags) ({ \ @@ -501,6 +502,9 @@ struct kvm { struct srcu_struct srcu; struct srcu_struct irq_srcu; pid_t userspace_pid; + + /* VM caused internal KVM error */ + bool vm_bugged; }; #define kvm_err(fmt, ...) \ @@ -613,6 +617,7 @@ static inline void kvm_irqfd_exit(void) int kvm_init(void *opaque, unsigned vcpu_size, unsigned vcpu_align, struct module *module); void kvm_exit(void); +void kvm_vm_bug(struct kvm_vcpu *vcpu, u32 error); void kvm_get_kvm(struct kvm *kvm); void kvm_put_kvm(struct kvm *kvm); diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h index f0a16b4adbbd..62505161ae98 100644 --- a/include/uapi/linux/kvm.h +++ b/include/uapi/linux/kvm.h @@ -246,6 +246,8 @@ struct kvm_hyperv_exit { #define KVM_INTERNAL_ERROR_DELIVERY_EV 3 /* Encounter unexpected vm-exit reason */ #define KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON 4 +/* Some other vCPU caused internal KVM error */ +#define KVM_INTERNAL_ERROR_OTHERCPU 5 /* for KVM_RUN, returned by mmap(vcpu_fd, offset=0) */ struct kvm_run { diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 00268290dcbd..4cc268d57714 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -4446,6 +4446,18 @@ void kvm_exit(void) } EXPORT_SYMBOL_GPL(kvm_exit); +void kvm_vm_bug(struct kvm_vcpu *vcpu, u32 error) +{ + vcpu->kvm->vm_bugged = true; + + vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR; + vcpu->run->internal.suberror = error; + /* We can also pass ndata/data ... */ + + kvm_make_all_cpus_request(vcpu->kvm, KVM_REQ_INTERROR); +} +EXPORT_SYMBOL_GPL(kvm_vm_bug); + struct kvm_vm_worker_thread_context { struct kvm *kvm; struct task_struct *parent; If you guys like the idea in general I can prepare patches.
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c index 7608924ee8c1..985d3307ec56 100644 --- a/arch/x86/kvm/vmx/nested.c +++ b/arch/x86/kvm/vmx/nested.c @@ -5165,7 +5165,7 @@ static int handle_invvpid(struct kvm_vcpu *vcpu) break; default: WARN_ON_ONCE(1); - return kvm_skip_emulated_instruction(vcpu); + break; } return nested_vmx_succeed(vcpu);