KVM: kvm_vcpu_block task state race

PATCH Marcelo Tosatti 2 participants 10 messages May 8, 2008 – May 14, 2008 KVM

Share & tools

Marcelo Tosatti

author

2008-05-08 22:47 UTC #0 lore raw

There's still a race in kvm_vcpu_block(), if a wake_up_interruptible() call happens before the task state is set to TASK_INTERRUPTIBLE:

CPU0 CPU1

kvm_vcpu_block

add_wait_queue

kvm_cpu_has_interrupt = 0
set interrupt
if (waitqueue_active())
wake_up_interruptible()

kvm_cpu_has_pending_timer
kvm_arch_vcpu_runnable
signal_pending

set_current_state(TASK_INTERRUPTIBLE)
schedule()

Can be fixed by using prepare_to_wait() which sets the task state before testing for the wait condition.

Unfortunately it can't use wait_event_interruptible() due to vcpu_put/vcpu_load.

Signed-off-by Marcelo Tosatti <[email protected]>

virt/kvm/kvm_main.c +16 ~~-15~~

index 0846d3d..fcc08c2 100644
@@ -783,25 +783,26 @@ void mark_page_dirty(struct kvm *kvm, gfn_t gfn)
  */
 void kvm_vcpu_block(struct kvm_vcpu *vcpu)
 {
-	DECLARE_WAITQUEUE(wait, current);
-
-	add_wait_queue(&vcpu->wq, &wait);
-
-	/*
-	 * We will block until either an interrupt or a signal wakes us up
-	 */
-	while (!kvm_cpu_has_interrupt(vcpu)
-	       && !kvm_cpu_has_pending_timer(vcpu)
-	       && !signal_pending(current)
-	       && !kvm_arch_vcpu_runnable(vcpu)) {
-		set_current_state(TASK_INTERRUPTIBLE);
+	DEFINE_WAIT(wait);
+
+	for (;;) {
+		prepare_to_wait(&vcpu->wq, &wait, TASK_INTERRUPTIBLE);
+
+		if (kvm_cpu_has_interrupt(vcpu))
+			break;
+		if (kvm_cpu_has_pending_timer(vcpu))
+			break;
+		if (kvm_arch_vcpu_runnable(vcpu))
+			break;
+		if (signal_pending(current))
+			break;
+
 		vcpu_put(vcpu);
 		schedule();
 		vcpu_load(vcpu);
 	}
-
-	__set_current_state(TASK_RUNNING);
-	remove_wait_queue(&vcpu->wq, &wait);
+		
+	finish_wait(&vcpu->wq, &wait);
 }
 
 void kvm_resched(struct kvm_vcpu *vcpu)

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference Don't miss this year's exciting event. There's still time to save $100. Use priority code J8TL2D2.
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Avi Kivity

2008-05-09 07:40 UTC #1 lore raw

Quoted 18 lines · Marcelo Tosatti

There's still a race in kvm_vcpu_block(), if a wake_up_interruptible() call happens before the task state is set to TASK_INTERRUPTIBLE:
CPU0 CPU1
kvm_vcpu_block
add_wait_queue
kvm_cpu_has_interrupt = 0
set interrupt
if (waitqueue_active())
wake_up_interruptible()
kvm_cpu_has_pending_timer
kvm_arch_vcpu_runnable
signal_pending

Quoted 10 lines

set_current_state(TASK_INTERRUPTIBLE)
schedule()
Can be fixed by using prepare_to_wait() which sets the task state before testing for the wait condition.
Unfortunately it can't use wait_event_interruptible() due to vcpu_put/vcpu_load.

schedule() will call vcpu_put()/vcpu_load() for us through preempt notifiers. I feel a little uneasy about it, but no concreate reason why not to rely on it.

Signature · 8 lines

Do not meddle in the internals of kernels, for they are subtle and quick to panic.


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Marcelo Tosatti

author

2008-05-09 14:21 UTC #2 lore raw

Quoted 8 lines · Avi Kivity

Quoted 4 lines
Unfortunately it can't use wait_event_interruptible() due to vcpu_put/vcpu_load.
schedule() will call vcpu_put()/vcpu_load() for us through preempt notifiers. I feel a little uneasy about it, but no concreate reason why not to rely on it.

The preempt notifiers hook call kvm_arch_vcpu_load / kvm_arch_vcpu_put, which won't unlock the vcpu mutex, right?

I worry about a possible deadlock where some other operation that requires the vcpu mutex happens but the vcpu thread itself is in hlt.

Avi Kivity

2008-05-09 15:09 UTC #3 lore raw

Quoted 8 lines · Marcelo Tosatti

Quoted 5 lines · Avi Kivity
Quoted 5 lines
Unfortunately it can't use wait_event_interruptible() due to vcpu_put/vcpu_load.

Quoted 9 lines

Quoted 4 lines
schedule() will call vcpu_put()/vcpu_load() for us through preempt notifiers. I feel a little uneasy about it, but no concreate reason why not to rely on it.
The preempt notifiers hook call kvm_arch_vcpu_load / kvm_arch_vcpu_put, which won't unlock the vcpu mutex, right?

Yes.

Quoted 3 lines

I worry about a possible deadlock where some other operation that requires the vcpu mutex happens but the vcpu thread itself is in hlt.

Suppose the guest executed a busy-spin waiting for an interrupt instead of a hlt? We need to be able to handle that too.

The best practice is to issue all vcpu ioctls from the thread that created the vcpu; this becomes mandatory if we ever switch to a syscall interface and remove the mutex.

Signature · 8 lines

Do not meddle in the internals of kernels, for they are subtle and quick to panic.


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Marcelo Tosatti

author

2008-05-09 19:22 UTC #4 lore raw

Quoted 19 lines · Avi Kivity

Quoted 8 lines · Marcelo Tosatti
Quoted 5 lines · Avi Kivity
Quoted 5 lines
Unfortunately it can't use wait_event_interruptible() due to vcpu_put/vcpu_load.
Quoted 9 lines
Quoted 4 lines
schedule() will call vcpu_put()/vcpu_load() for us through preempt notifiers. I feel a little uneasy about it, but no concreate reason why not to rely on it.
The preempt notifiers hook call kvm_arch_vcpu_load / kvm_arch_vcpu_put, which won't unlock the vcpu mutex, right?

Quoted 8 lines

Yes.
Quoted 3 lines
I worry about a possible deadlock where some other operation that requires the vcpu mutex happens but the vcpu thread itself is in hlt.
Suppose the guest executed a busy-spin waiting for an interrupt instead of a hlt? We need to be able to handle that too.

True.

Quoted 3 lines

The best practice is to issue all vcpu ioctls from the thread that created the vcpu; this becomes mandatory if we ever switch to a syscall interface and remove the mutex.

For things like register dumps I don't believe its worthwhile. Much simpler to stop the vcpu with SIG_IPI, retrieve registers, and run it again (now that you mention the busy-spin, it is broken right now, if a vcpu is spinning without exiting to userspace).

So do you want to give wait_event_interruptible() a try or wait for that change until userspace never issues vcpu ioctl's to a possibly busy vcpu (and go with the patch above)?

Marcelo Tosatti

author

2008-05-09 19:27 UTC #5 lore raw

Quoted 4 lines · Marcelo Tosatti

For things like register dumps I don't believe its worthwhile. Much simpler to stop the vcpu with SIG_IPI, retrieve registers, and run it again (now that you mention the busy-spin, it is broken right now, if a vcpu is spinning without exiting to userspace).

... which is what Jan's gdb/monitor patch does.

Avi Kivity

2008-05-11 14:24 UTC #6 lore raw

Quoted 10 lines · Marcelo Tosatti

Quoted 5 lines · Marcelo Tosatti
For things like register dumps I don't believe its worthwhile. Much simpler to stop the vcpu with SIG_IPI, retrieve registers, and run it again (now that you mention the busy-spin, it is broken right now, if a vcpu is spinning without exiting to userspace).
... which is what Jan's gdb/monitor patch does.

We'd better change it. Suppose your vcpu is spinning, and you want to find out why?

One nice thing would be to add an on_vcpu(void (*func)(void *data), void *data), so that people don't have to open-code it for things like this. I tried to do this once, but backed off due to the mess that qemu-kvm threading was at the time. I think it's much better off now.

Signature · 8 lines

error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Avi Kivity

2008-05-11 14:26 UTC #7 lore raw

In reply to Marcelo Tosatti

Quoted 5 lines · Marcelo Tosatti

Quoted 4 lines
The best practice is to issue all vcpu ioctls from the thread that created the vcpu; this becomes mandatory if we ever switch to a syscall interface and remove the mutex.

Quoted 9 lines

For things like register dumps I don't believe its worthwhile. Much simpler to stop the vcpu with SIG_IPI, retrieve registers, and run it again (now that you mention the busy-spin, it is broken right now, if a vcpu is spinning without exiting to userspace).
So do you want to give wait_event_interruptible() a try or wait for that change until userspace never issues vcpu ioctl's to a possibly busy vcpu (and go with the patch above)?

Do we have anything critical that issues vcpu ioctls outside its thread? While I much prefer wait_event_interruptible(), I don't want to break existing userspace.

Signature · 8 lines

error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Marcelo Tosatti

author

2008-05-14 05:21 UTC #8 lore raw

Quoted 8 lines · Avi Kivity

Quoted 4 lines
So do you want to give wait_event_interruptible() a try or wait for that change until userspace never issues vcpu ioctl's to a possibly busy vcpu (and go with the patch above)?
Do we have anything critical that issues vcpu ioctls outside its thread? While I much prefer wait_event_interruptible(), I don't want to break existing userspace.

Well debugging can be critical, so IMO better avoid wait_event_interruptible() for now.

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Avi Kivity

2008-05-14 07:55 UTC #9 lore raw

Quoted 7 lines · Marcelo Tosatti

Quoted 5 lines · Avi Kivity
Quoted 5 lines
So do you want to give wait_event_interruptible() a try or wait for that change until userspace never issues vcpu ioctl's to a possibly busy vcpu (and go with the patch above)?

Quoted 8 lines

Quoted 4 lines
Do we have anything critical that issues vcpu ioctls outside its thread? While I much prefer wait_event_interruptible(), I don't want to break existing userspace.
Well debugging can be critical, so IMO better avoid wait_event_interruptible() for now.

The vast majority of users don't care about debugging, and debugging will be broken anyway if a vcpu is spinning (which might be the reason for debugging in the first place).

But the w_e_i() conversion can be done later, so I'll apply the patch.

Signature · 7 lines

error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft 
Defy all challenges. Microsoft(R) Visual Studio 2008. 
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/