KVM: x86: Check for nested events if there is an injectable interrupt
PATCH 1/1 KVM: x86: Check for nested events if there is an injectable interrupt
With commit b6b8a1451fc40412c57d1 that introduced vmx_check_nested_events, checks for injectable interrupts happen at different points in time for L1 and L2 that could potentially cause a race. The regression occurs because KVM_REQ_EVENT is always set when nested_run_pending is set even if there's no pending interrupt. Consequently, there could be a small window when check_nested_events returns without exiting to L1, but an interrupt comes through soon after and it incorrectly, gets injected to L2 by inject_pending_event Fix this by adding a call to check for nested events too when a check for injectable interrupt returns true
--- arch/x86/kvm/x86.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
arch/x86/kvm/x86.c +13 -0
@@ -5907,6 +5907,19 @@ static int inject_pending_event(struct kvm_vcpu *vcpu, bool req_int_win) kvm_x86_ops->set_nmi (vcpu); } }else if (kvm_cpu_has_injectable_intr (vcpu)) { +/* +* TODO/FIXME: We are calling check_nested_events again +* here to avoid a race condition. We should really be +* setting KVM_REQ_EVENT only on certain events +* and not unconditionally. +* See https://lkml.org/lkml/2014/7/2/60 for discussion +* about this proposal and current concerns +*/ +if (is_guest_mode (vcpu) && kvm_x86_ops->check_nested_events) { + r = kvm_x86_ops->check_nested_events (vcpu, req_int_win); +if (r !=0 ) +return r; + }if (kvm_x86_ops->interrupt_allowed (vcpu)) {kvm_queue_interrupt (vcpu,kvm_cpu_get_interrupt (vcpu), false);
Signature · 1 line
1.8.3.1
Paolo Bonzini
maintainerIl 08/07/2014 06:30, Bandan Das ha scritto:
Quoted 32 lines
With commit
b6b8a1451fc40412c57d1that introducedvmx_check_nested_events, checks for injectable interrupts happen at different points in time for L1 and L2 that could potentially cause a race. The regression occurs becauseKVM_REQ_EVENTis always set whennested_run_pendingis set even if there's no pending interrupt. Consequently, there could be a small window whencheck_nested_eventsreturns without exiting to L1, but an interrupt comes through soon after and it incorrectly, gets injected to L2 byinject_pending_eventFix this by adding a call to check for nested events too when a check for injectable interrupt returns trueSigned-off-by Bandan Das <[email protected]>--- arch/x86/kvm/x86.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)arch/x86/kvm/x86.c +6
-0@@ -5907,6 +5907,19 @@ static int inject_pending_event(struct kvm_vcpu *vcpu, bool req_int_win) kvm_x86_ops->set_nmi (vcpu); } }else if (kvm_cpu_has_injectable_intr (vcpu)) { +/* +* TODO/FIXME: We are calling check_nested_events again +* here to avoid a race condition. We should really be +* setting KVM_REQ_EVENT only on certain events +* and not unconditionally. +* See https://lkml.org/lkml/2014/7/2/60 for discussion
arch/x86/kvm/x86.c static int inject_pending_event(struct kvm_vcpu *vcpu, bool req_int_win) · line 5925
Quoted 10 lines
arch/x86/kvm/x86.c +7
-0+* about this proposal and current concerns +*/ +if (is_guest_mode (vcpu) && kvm_x86_ops->check_nested_events) { + r = kvm_x86_ops->check_nested_events (vcpu, req_int_win); +if (r !=0 ) +return r; + }if (kvm_x86_ops->interrupt_allowed (vcpu)) {kvm_queue_interrupt (vcpu,kvm_cpu_get_interrupt (vcpu), false);
I think this should be done for NMI as well.
Jan, what do you think? Can you run Jailhouse through this patch?
Paolo
Quoted 38 lines · Paolo Bonzini
Il 08/07/2014 06:30, Bandan Das ha scritto:
Quoted 37 lines
With commit
b6b8a1451fc40412c57d1that introducedvmx_check_nested_events, checks for injectable interrupts happen at different points in time for L1 and L2 that could potentially cause a race. The regression occurs becauseKVM_REQ_EVENTis always set whennested_run_pendingis set even if there's no pending interrupt. Consequently, there could be a small window whencheck_nested_eventsreturns without exiting to L1, but an interrupt comes through soon after and it incorrectly, gets injected to L2 byinject_pending_eventFix this by adding a call to check for nested events too when a check for injectable interrupt returns trueSigned-off-by Bandan Das <[email protected]>--- arch/x86/kvm/x86.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)arch/x86/kvm/x86.c +11
-0@@ -5907,6 +5907,19 @@ static int inject_pending_event(struct kvm_vcpu *vcpu, bool req_int_win) kvm_x86_ops->set_nmi (vcpu); } }else if (kvm_cpu_has_injectable_intr (vcpu)) { +/* +* TODO/FIXME: We are calling check_nested_events again +* here to avoid a race condition. We should really be +* setting KVM_REQ_EVENT only on certain events +* and not unconditionally. +* See https://lkml.org/lkml/2014/7/2/60 for discussion +* about this proposal and current concerns +*/ +if (is_guest_mode (vcpu) && kvm_x86_ops->check_nested_events) { + r = kvm_x86_ops->check_nested_events (vcpu, req_int_win); +if (r !=0 )
Quoted 8 lines
Quoted 6 lines
arch/x86/kvm/x86.c +2
-0+return r; + }if (kvm_x86_ops->interrupt_allowed (vcpu)) { kvm_queue_interrupt(vcpu, kvm_cpu_get_interrupt(vcpu),false);I think this should be done for NMI as well.
I don't think arch.nmi_pending can flip asynchronously, only in the context of the VCPU thread - in contrast to pending IRQ states.
Quoted 2 lines
Jan, what do you think? Can you run Jailhouse through this patch?
Jailhouse seems fine with it, and it resolves the lockup of nested KVM here as well.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SES-DE Corporate Competence Center Embedded Linux
Paolo Bonzini
maintainerIl 08/07/2014 08:56, Jan Kiszka ha scritto:
Quoted 2 lines
I don't think arch.nmi_pending can flip asynchronously, only in the context of the VCPU thread - in contrast to pending IRQ states.
Right, only nmi_queued is changed from other threads. /me should really look at the code instead of going from memory.
Quoted 4 lines
Quoted 1 line
Jan, what do you think? Can you run Jailhouse through this patch?
Jailhouse seems fine with it, and it resolves the lockup of nested KVM here as well.
Thinking more about it, I think this is the right fix. Not setting KVM_REQ_EVENT in some cases can be an optimization, but it's not necessary. Definitely there are other cases in which KVM_REQ_EVENT is set even though no event is pending---most notably during emulation of invalid guest state.
Paolo
Wanpeng Li
Reviewed-byQuoted 9 lines · Paolo Bonzini
Il 08/07/2014 08:56, Jan Kiszka ha scritto:
Quoted 2 lines
I don't think arch.nmi_pending can flip asynchronously, only in the context of the VCPU thread - in contrast to pending IRQ states.
Right, only
nmi_queuedis changed from other threads. /me should really look at the code instead of going from memory.Quoted 2 lines
Quoted 1 line
Jan, what do you think? Can you run Jailhouse through this patch?
Quoted 8 lines
Quoted 2 lines
Jailhouse seems fine with it, and it resolves the lockup of nested KVM here as well.
Thinking more about it, I think this is the right fix. Not setting
KVM_REQ_EVENTin some cases can be an optimization, but it's not necessary. Definitely there are other cases in whichKVM_REQ_EVENTis set even though no event is pending---most notably during emulation of invalid guest state.
Anyway,
Quoted 2 lines
Paolo