From: Maciej S. Szmigiero <[email protected]>
Don't BUG/WARN on interrupt injection due to GIF being cleared if the
injected event is a soft interrupt, which are not actually IRQs and thus
not subject to IRQ blocking conditions. KVM doesn't currently use event
injection to handle incomplete soft interrupts, but it's trivial for
userspace to force the situation via KVM_SET_VCPU_EVENTS.
Opportunistically downgrade the BUG_ON() to WARN_ON(), there's no need to
bring down the whole host just because there might be some issue with
respect to guest GIF handling in KVM, or as evidenced here, an egregious
oversight with respect to KVM's uAPI.
kernel BUG at arch/x86/kvm/svm/svm.c:3386!
invalid opcode: 0000 [#1] SMP
CPU: 15 PID: 926 Comm: smm_test Not tainted 5.17.0-rc3+ #264
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
RIP: 0010:svm_inject_irq+0xab/0xb0 [kvm_amd]
Code: <0f> 0b 0f 1f 00 0f 1f 44 00 00 80 3d ac b3 01 00 00 55 48 89 f5 53
RSP: 0018:ffffc90000b37d88 EFLAGS: 00010246
RAX: 0000000000000000 RBX: ffff88810a234ac0 RCX: 0000000000000006
RDX: 0000000000000000 RSI: ffffc90000b37df7 RDI: ffff88810a234ac0
RBP: ffffc90000b37df7 R08: ffff88810a1fa410 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
R13: ffff888109571000 R14: ffff88810a234ac0 R15: 0000000000000000
FS: 0000000001821380(0000) GS:ffff88846fdc0000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f74fc550008 CR3: 000000010a6fe000 CR4: 0000000000350ea0
Call Trace:
<TASK>
inject_pending_event+0x2f7/0x4c0 [kvm]
kvm_arch_vcpu_ioctl_run+0x791/0x17a0 [kvm]
kvm_vcpu_ioctl+0x26d/0x650 [kvm]
__x64_sys_ioctl+0x82/0xb0
do_syscall_64+0x3b/0xc0
entry_SYSCALL_64_after_hwframe+0x44/0xae
</TASK>
Fixes: 219b65dcf6c0 ("KVM: SVM: Improve nested interrupt injection")
Cc: [email protected]
Signed-off-by: Maciej S. Szmigiero <[email protected]>
Co-developed-by: Sean Christopherson <[email protected]>
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/x86/kvm/svm/svm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 75b4f3ac8b1a..151fba0b405f 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3384,7 +3384,7 @@ static void svm_inject_irq(struct kvm_vcpu *vcpu)
{
struct vcpu_svm *svm = to_svm(vcpu);
- BUG_ON(!(gif_set(svm)));
+ WARN_ON(!vcpu->arch.interrupt.soft && !gif_set(svm));
trace_kvm_inj_virq(vcpu->arch.interrupt.nr);
++vcpu->stat.irq_injections;
--
2.36.0.rc2.479.g8af0fa9b8e-goog
On Sat, 2022-04-23 at 02:14 +0000, Sean Christopherson wrote:
> From: Maciej S. Szmigiero <[email protected]>
>
> Don't BUG/WARN on interrupt injection due to GIF being cleared if the
> injected event is a soft interrupt, which are not actually IRQs and thus
Are any injected events subject to GIF set? I think that EVENTINJ just injects
unconditionaly whatever hypervisor puts in it.
Best regards,
Maxim Levitsky
> not subject to IRQ blocking conditions. KVM doesn't currently use event
> injection to handle incomplete soft interrupts, but it's trivial for
> userspace to force the situation via KVM_SET_VCPU_EVENTS.
>
> Opportunistically downgrade the BUG_ON() to WARN_ON(), there's no need to
> bring down the whole host just because there might be some issue with
> respect to guest GIF handling in KVM, or as evidenced here, an egregious
> oversight with respect to KVM's uAPI.
>
> kernel BUG at arch/x86/kvm/svm/svm.c:3386!
> invalid opcode: 0000 [#1] SMP
> CPU: 15 PID: 926 Comm: smm_test Not tainted 5.17.0-rc3+ #264
> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
> RIP: 0010:svm_inject_irq+0xab/0xb0 [kvm_amd]
> Code: <0f> 0b 0f 1f 00 0f 1f 44 00 00 80 3d ac b3 01 00 00 55 48 89 f5 53
> RSP: 0018:ffffc90000b37d88 EFLAGS: 00010246
> RAX: 0000000000000000 RBX: ffff88810a234ac0 RCX: 0000000000000006
> RDX: 0000000000000000 RSI: ffffc90000b37df7 RDI: ffff88810a234ac0
> RBP: ffffc90000b37df7 R08: ffff88810a1fa410 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
> R13: ffff888109571000 R14: ffff88810a234ac0 R15: 0000000000000000
> FS: 0000000001821380(0000) GS:ffff88846fdc0000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007f74fc550008 CR3: 000000010a6fe000 CR4: 0000000000350ea0
> Call Trace:
> <TASK>
> inject_pending_event+0x2f7/0x4c0 [kvm]
> kvm_arch_vcpu_ioctl_run+0x791/0x17a0 [kvm]
> kvm_vcpu_ioctl+0x26d/0x650 [kvm]
> __x64_sys_ioctl+0x82/0xb0
> do_syscall_64+0x3b/0xc0
> entry_SYSCALL_64_after_hwframe+0x44/0xae
> </TASK>
>
> Fixes: 219b65dcf6c0 ("KVM: SVM: Improve nested interrupt injection")
> Cc: [email protected]
> Signed-off-by: Maciej S. Szmigiero <[email protected]>
> Co-developed-by: Sean Christopherson <[email protected]>
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> arch/x86/kvm/svm/svm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 75b4f3ac8b1a..151fba0b405f 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -3384,7 +3384,7 @@ static void svm_inject_irq(struct kvm_vcpu *vcpu)
> {
> struct vcpu_svm *svm = to_svm(vcpu);
>
> - BUG_ON(!(gif_set(svm)));
> + WARN_ON(!vcpu->arch.interrupt.soft && !gif_set(svm));
>
> trace_kvm_inj_virq(vcpu->arch.interrupt.nr);
> ++vcpu->stat.irq_injections;
On Thu, Apr 28, 2022, Maxim Levitsky wrote:
> On Thu, 2022-04-28 at 15:27 +0200, Maciej S. Szmigiero wrote:
> > On 28.04.2022 09:35, Maxim Levitsky wrote:
> > > On Sat, 2022-04-23 at 02:14 +0000, Sean Christopherson wrote:
> > > > From: Maciej S. Szmigiero <[email protected]>
> > > >
> > > > Don't BUG/WARN on interrupt injection due to GIF being cleared if the
> > > > injected event is a soft interrupt, which are not actually IRQs and thus
> > >
> > > Are any injected events subject to GIF set? I think that EVENTINJ just injects
> > > unconditionaly whatever hypervisor puts in it.
> >
> > That's right, EVENTINJ will pretty much always inject, even when the CPU
> > is in a 'wrong' state (like for example, injecting a hardware interrupt
> > or a NMI with GIF masked).
> >
> > But KVM as a L0 is not supposed to inject a hardware interrupt into guest
> > with GIF unset since the guest is obviously not expecting it then.
> > Hence this WARN_ON().
>
> If you mean L0->L1 injection, that sure, but if L1 injects interrupt to L2,
> then it should always be allowed to do so.
Yes, L1 can inject whatever it wants, whenever it wants.
I kept the WARN_ON() under the assumption that KVM would refuse to inject IRQs
stuffed by userspace if GIF is disabled, but looking at the code again, I have
no idea why I thought that. KVM_SET_VCPU_EVENTS blindly takes whatever userspace
provides, I don't see anything that would prevent userspace from shoving in a
hardware IRQ.
On 28.04.2022 09:35, Maxim Levitsky wrote:
> On Sat, 2022-04-23 at 02:14 +0000, Sean Christopherson wrote:
>> From: Maciej S. Szmigiero <[email protected]>
>>
>> Don't BUG/WARN on interrupt injection due to GIF being cleared if the
>> injected event is a soft interrupt, which are not actually IRQs and thus
>
> Are any injected events subject to GIF set? I think that EVENTINJ just injects
> unconditionaly whatever hypervisor puts in it.
That's right, EVENTINJ will pretty much always inject, even when the CPU
is in a 'wrong' state (like for example, injecting a hardware interrupt
or a NMI with GIF masked).
But KVM as a L0 is not supposed to inject a hardware interrupt into guest
with GIF unset since the guest is obviously not expecting it then.
Hence this WARN_ON().
> Best regards,
> Maxim Levitsky
Thanks,
Maciej
On 28.04.2022 17:04, Sean Christopherson wrote:
> On Thu, Apr 28, 2022, Maxim Levitsky wrote:
>> On Thu, 2022-04-28 at 15:27 +0200, Maciej S. Szmigiero wrote:
>>> On 28.04.2022 09:35, Maxim Levitsky wrote:
>>>> On Sat, 2022-04-23 at 02:14 +0000, Sean Christopherson wrote:
>>>>> From: Maciej S. Szmigiero <[email protected]>
>>>>>
>>>>> Don't BUG/WARN on interrupt injection due to GIF being cleared if the
>>>>> injected event is a soft interrupt, which are not actually IRQs and thus
>>>>
>>>> Are any injected events subject to GIF set? I think that EVENTINJ just injects
>>>> unconditionaly whatever hypervisor puts in it.
>>>
>>> That's right, EVENTINJ will pretty much always inject, even when the CPU
>>> is in a 'wrong' state (like for example, injecting a hardware interrupt
>>> or a NMI with GIF masked).
>>>
>>> But KVM as a L0 is not supposed to inject a hardware interrupt into guest
>>> with GIF unset since the guest is obviously not expecting it then.
>>> Hence this WARN_ON().
>>
>> If you mean L0->L1 injection, that sure, but if L1 injects interrupt to L2,
>> then it should always be allowed to do so.
>
> Yes, L1 can inject whatever it wants, whenever it wants.
>
> I kept the WARN_ON() under the assumption that KVM would refuse to inject IRQs
> stuffed by userspace if GIF is disabled, but looking at the code again, I have
> no idea why I thought that. KVM_SET_VCPU_EVENTS blindly takes whatever userspace
> provides, I don't see anything that would prevent userspace from shoving in a
> hardware IRQ.
You both are right, while KVM itself would not inject IRQ with GIF masked and
a nested VMRUN would enable GIF unconditionally, userspace via KVM_SET_VCPU_EVENTS
does not have this restriction.
Will remove this WARN_ON() then.
Thanks,
Maciej
On Thu, 2022-04-28 at 15:27 +0200, Maciej S. Szmigiero wrote:
> On 28.04.2022 09:35, Maxim Levitsky wrote:
> > On Sat, 2022-04-23 at 02:14 +0000, Sean Christopherson wrote:
> > > From: Maciej S. Szmigiero <[email protected]>
> > >
> > > Don't BUG/WARN on interrupt injection due to GIF being cleared if the
> > > injected event is a soft interrupt, which are not actually IRQs and thus
> >
> > Are any injected events subject to GIF set? I think that EVENTINJ just injects
> > unconditionaly whatever hypervisor puts in it.
>
> That's right, EVENTINJ will pretty much always inject, even when the CPU
> is in a 'wrong' state (like for example, injecting a hardware interrupt
> or a NMI with GIF masked).
>
> But KVM as a L0 is not supposed to inject a hardware interrupt into guest
> with GIF unset since the guest is obviously not expecting it then.
> Hence this WARN_ON().
If you mean L0->L1 injection, that sure, but if L1 injects interrupt to L2,
then it should always be allowed to do so.
I am not sure that I am right here, just noticed something odd. I'll take
a better look at this next week.
Best regards,
Maxim levitsky
>
> > Best regards,
> > Maxim Levitsky
>
> Thanks,
> Maciej
>