KVM: VMX: Fix locking order in handle_invalid_guest_state

Release and re-acquire preemption and IRQ lock in the same order as vcpu_enter_guest does.

Signed-off-by Jan Kiszka <[email protected]>

---

 arch/x86/kvm/vmx.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

arch/x86/kvm/vmx.c +2 -2

index d75c271..4f914c3 100644
@@ -3324,8 +3324,8 @@ static void handle_invalid_guest_state(struct kvm_vcpu *vcpu,
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 	enum emulation_result err = EMULATE_DONE;
 
-	preempt_enable();
 	local_irq_enable();
+	preempt_enable();
 
 	while (!guest_state_valid(vcpu)) {
 		err = emulate_instruction(vcpu, kvm_run, 0, 0, 0);

@@ -3344,8 +3344,8 @@ static void handle_invalid_guest_state(struct kvm_vcpu *vcpu,
 			schedule();
 	}
 
-	local_irq_disable();
 	preempt_disable();
+	local_irq_disable();
 
 	vmx->invalid_state_emulation_result = err;
 }

Quoted 2 lines · Jan Kiszka

Release and re-acquire preemption and IRQ lock in the same order as vcpu_enter_guest does.

This should happen in vcpu_enter_guest, before it decides to disable preemption/irqs (so you consolidate the control there).

Maybe add a new member to x86_ops?

Quoted 31 lines

Signed-off-by Jan Kiszka <[email protected]>

---

 arch/x86/kvm/vmx.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

arch/x86/kvm/vmx.c +2 -2

index d75c271..4f914c3 100644
@@ -3324,8 +3324,8 @@ static void handle_invalid_guest_state(struct kvm_vcpu *vcpu,
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 	enum emulation_result err = EMULATE_DONE;
 
-	preempt_enable();
 	local_irq_enable();
+	preempt_enable();
 
 	while (!guest_state_valid(vcpu)) {
 		err = emulate_instruction(vcpu, kvm_run, 0, 0, 0);

@@ -3344,8 +3344,8 @@ static void handle_invalid_guest_state(struct kvm_vcpu *vcpu,
 			schedule();
 	}
 
-	local_irq_disable();
 	preempt_disable();
+	local_irq_disable();
 
 	vmx->invalid_state_emulation_result = err;
 }

Quoted 8 lines · Marcelo Tosatti

Quoted 3 lines · Jan Kiszka

Release and re-acquire preemption and IRQ lock in the same order as vcpu_enter_guest does.

This should happen in vcpu_enter_guest, before it decides to disable preemption/irqs (so you consolidate the control there).

Maybe, maybe not. handle_invalid_guest_state is an alternative way of "executing" guest code, and it currently shares the setup and tear-down with vmx_vcpu_run. If it has to share parts that actually require preemption and IRQ lock, then moving makes not much sense. Can anyone comment on what the requirements for handle_invalid_guest_state are?

I would suggest to merge this fix first and then decide about and potentially merge a refactoring patch.

Quoted 11 lines · Jan Kiszka

Quoted 8 lines · Marcelo Tosatti

Quoted 3 lines · Jan Kiszka

Release and re-acquire preemption and IRQ lock in the same order as vcpu_enter_guest does.

This should happen in vcpu_enter_guest, before it decides to disable preemption/irqs (so you consolidate the control there).

Quoted 6 lines

Maybe, maybe not. handle_invalid_guest_state is an alternative way of "executing" guest code, and it currently shares the setup and tear-down with vmx_vcpu_run. If it has to share parts that actually require preemption and IRQ lock, then moving makes not much sense. Can anyone comment on what the requirements for handle_invalid_guest_state are?

Like you said, it's an alternative to vmx entry/exit, so it shares the same requirements. It must run with interrupts and preemption enabled, but any code that normally runs in the entry critical section (like interrupt injection) must continue to run in a critical section.

Quoted 3 lines

I would suggest to merge this fix first and then decide about and potentially merge a refactoring patch.

btw, what does it fix? a debug warning?

Quoted 3 lines

btw, what does it fix? a debug warning?

I haven't seen anything in the wild, and I don't think it would raise a warning. All it should cause is a potential delay of some pending reschedule as preempt_enable will not fire under local_irq_disable.

Quoted 4 lines · Jan Kiszka

Quoted 3 lines

btw, what does it fix? a debug warning?

Quoted 4 lines

I haven't seen anything in the wild, and I don't think it would raise a warning. All it should cause is a potential delay of some pending reschedule as preempt_enable will not fire under local_irq_disable.

Ah, okay, then it is a real fix. Preempt-correctness is important.

(but won't local_irq_enable() reschedule?)

Quoted 11 lines · Avi Kivity

Quoted 9 lines · Jan Kiszka

Quoted 4 lines

btw, what does it fix? a debug warning?

I haven't seen anything in the wild, and I don't think it would raise a warning. All it should cause is a potential delay of some pending reschedule as preempt_enable will not fire under local_irq_disable.

Quoted 4 lines

Ah, okay, then it is a real fix. Preempt-correctness is important.

(but won't local_irq_enable() reschedule?)

The last time I checked it was essentially a plain 'sti'.

Quoted 18 lines · Jan Kiszka

Quoted 15 lines · Avi Kivity

Quoted 9 lines · Jan Kiszka

Quoted 4 lines

btw, what does it fix? a debug warning?

I haven't seen anything in the wild, and I don't think it would raise a warning. All it should cause is a potential delay of some pending reschedule as preempt_enable will not fire under local_irq_disable.

Ah, okay, then it is a real fix. Preempt-correctness is important.

(but won't local_irq_enable() reschedule?)

Quoted 3 lines

The last time I checked it was essentially a plain 'sti'.

Presumably there's a reschedule interrupt queued; I think if you set the reschedule bit you have to IPI the cpu running the task.

Quoted 3 lines

Presumably there's a reschedule interrupt queued; I think if you set the reschedule bit you have to IPI the cpu running the task.

Yeah. But as we preempt_disable first, that one might have been processed already.

Quoted 2 lines · Jan Kiszka

Release and re-acquire preemption and IRQ lock in the same order as vcpu_enter_guest does.

This should happen in vcpu_enter_guest, before it decides to disable preemption/irqs (so you consolidate the control there).

Maybe add a new member to x86_ops?

Quoted 5 lines · Marcelo Tosatti

Quoted 3 lines · Jan Kiszka

Release and re-acquire preemption and IRQ lock in the same order as vcpu_enter_guest does.

Quoted 5 lines

This should happen in vcpu_enter_guest, before it decides to disable preemption/irqs (so you consolidate the control there).

Maybe add a new member to x86_ops?

Why don't do something like this ?

Quoted 13 lines · Marcelo Tosatti

Quoted 10 lines · Marcelo Tosatti

Quoted 3 lines · Jan Kiszka

Release and re-acquire preemption and IRQ lock in the same order as vcpu_enter_guest does.

This should happen in vcpu_enter_guest, before it decides to disable preemption/irqs (so you consolidate the control there).

Maybe add a new member to x86_ops?

Quoted 2 lines

Why don't do something like this ?

The downside is that we're moving a vmx specific hack to common code.

I think this could be simplified if interrupt injection happened outside the critical section. This is needed anyway because emulated interrupt injection needs to access guest memory (IVT and the stack).

Something else I noticed, handle_invalid_guest_state() doesn't check vcpu->requests; normal execution will exit due to the interrupt while emulated execution will not.

Signature · 1 line

error compiling committee.c: too many arguments to function

Quoted 17 lines · Avi Kivity

Quoted 15 lines · Marcelo Tosatti

Quoted 10 lines · Marcelo Tosatti

Quoted 3 lines · Jan Kiszka

Release and re-acquire preemption and IRQ lock in the same order as vcpu_enter_guest does.

This should happen in vcpu_enter_guest, before it decides to disable preemption/irqs (so you consolidate the control there).

Maybe add a new member to x86_ops?

Why don't do something like this ?

Quoted 5 lines

The downside is that we're moving a vmx specific hack to common code.

I think this could be simplified if interrupt injection happened outside the critical section. This is needed anyway because emulated interrupt injection needs to access guest memory (IVT and the stack).

Why can't it happen now (outside of the critical section), other than the kvm_vcpu_kick thing?

Quoted 6 lines

The downside is that we're moving a vmx specific hack to common code.

I think this could be simplified if interrupt injection happened outside the critical section. This is needed anyway because emulated interrupt injection needs to access guest memory (IVT and the stack).

Why can't it happen now (outside of the critical section), other than the kvm_vcpu_kick thing?

Quoted 7 lines · Marcelo Tosatti

Quoted 6 lines

The downside is that we're moving a vmx specific hack to common code.

I think this could be simplified if interrupt injection happened outside the critical section. This is needed anyway because emulated interrupt injection needs to access guest memory (IVT and the stack).

Quoted 3 lines

Why can't it happen now (outside of the critical section), other than the kvm_vcpu_kick thing?

I think there's little reason now. One thing we need to do is make it possible to call the injection code twice without entering the guest. I think right now it assumes nothing has been injected.

We need either to cancel a previous injection (a variant of vmx_complete_interrupts()), or avoid reinjecting if we already did.

Signature · 1 line

error compiling committee.c: too many arguments to function

Quoted 5 lines

I think there's little reason now. One thing we need to do is make it possible to call the injection code twice without entering the guest. I think right now it assumes nothing has been injected.

I Looked at this and it seems the current code handle this case. Injection puts an event on a queue and if we haven't entered a guest after this point on the next entry event is injected from the queue, just like if injection failed due to IDT access.

What may happen is that at the time of irq injection there will be irq with higher irr pending in APIC, But it looks like this already may happen if injection failed on IDT access, but this is rare.

Quoted 11 lines · Gleb Natapov

Quoted 5 lines

I think there's little reason now. One thing we need to do is make it possible to call the injection code twice without entering the guest. I think right now it assumes nothing has been injected.

I Looked at this and it seems the current code handle this case. Injection puts an event on a queue and if we haven't entered a guest after this point on the next entry event is injected from the queue, just like if injection failed due to IDT access.

Good (it was one of the goals of the original interrupt rework, ~2 years ago)

Quoted 4 lines

Good (it was one of the goals of the original interrupt rework, ~2 years ago)

But if we emulate an injection by playing with guest memory and registers we have to be sure we do it only once.

Quoted 4 lines · Gleb Natapov

Quoted 4 lines

Good (it was one of the goals of the original interrupt rework, ~2 years ago)

Quoted 3 lines

But if we emulate an injection by playing with guest memory and registers we have to be sure we do it only once.

Once we've successfully updated the stack, inject is complete and we can remove the interrupt from the queue.

Quoted 9 lines · Avi Kivity

Quoted 7 lines · Gleb Natapov

Quoted 4 lines

Good (it was one of the goals of the original interrupt rework, ~2 years ago)

But if we emulate an injection by playing with guest memory and registers we have to be sure we do it only once.

Quoted 3 lines

Once we've successfully updated the stack, inject is complete and we can remove the interrupt from the queue.

Then, it we skip actual guest entry for some reason, we will not be able to tell if we can inject next irq or not.