kvmtool: fix VM exit race attempting to pthread_kill an exited thread

lkvm currently suffers from a Segmentation Fault when exiting, which can
also lead to the console not being cleaned up correctly after a VM exits.

The issue is that (the misnamed) kvm_cpu__reboot function sends a
SIGKVMEXIT to each vcpu thread, which causes those vcpu threads to exit
once their main loops (kvm_cpu__start) detect that cpu->is_running is
now false. The lack of synchronisation in this exit path means that a
concurrent pause event (due to the br_write_lock in ioport__unregister)
ends up sending SIGKVMPAUSE to an exited thread, resulting in a SEGV.

This patch fixes the issue by moving kvm_cpu__reboot into kvm.c
(renaming it in the process) where it can hold the pause_lock mutex
across the reboot operation. This in turn makes it safe for the pause
code to check the is_running field of each CPU before attempting to
send a SIGKVMPAUSE signal.

Signed-off-by: Will Deacon <will.deacon@arm.com>

9 files changed, 36 insertions, 26 deletions

diff --git a/hw/i8042.c b/hw/i8042.c
index 3801e20a..288b7d11 100644
--- a/hw/i8042.c
+++ b/hw/i8042.c
@@ -163,7 +163,7 @@ static void kbd_write_command(struct kvm *kvm, u8 val)
 		state.mode &= ~MODE_DISABLE_AUX;
 		break;
 	case I8042_CMD_SYSTEM_RESET:
-		kvm_cpu__reboot(kvm);
+		kvm__reboot(kvm);
 		break;
 	default:
 		break;
diff --git a/include/kvm/kvm-cpu.h b/include/kvm/kvm-cpu.h
index aa0cb543..c4c9cca4 100644
--- a/include/kvm/kvm-cpu.h
+++ b/include/kvm/kvm-cpu.h
@@ -12,7 +12,6 @@ void kvm_cpu__reset_vcpu(struct kvm_cpu *vcpu);
 void kvm_cpu__setup_cpuid(struct kvm_cpu *vcpu);
 void kvm_cpu__enable_singlestep(struct kvm_cpu *vcpu);
 void kvm_cpu__run(struct kvm_cpu *vcpu);
-void kvm_cpu__reboot(struct kvm *kvm);
 int kvm_cpu__start(struct kvm_cpu *cpu);
 bool kvm_cpu__handle_exit(struct kvm_cpu *vcpu);
 int kvm_cpu__get_endianness(struct kvm_cpu *vcpu);
diff --git a/include/kvm/kvm.h b/include/kvm/kvm.h
index 37155dbc..a474dae6 100644
--- a/include/kvm/kvm.h
+++ b/include/kvm/kvm.h
@@ -93,6 +93,7 @@ int kvm__register_mmio(struct kvm *kvm, u64 phys_addr, u64 phys_addr_len, bool c
 		       void (*mmio_fn)(struct kvm_cpu *vcpu, u64 addr, u8 *data, u32 len, u8 is_write, void *ptr),
 			void *ptr);
 bool kvm__deregister_mmio(struct kvm *kvm, u64 phys_addr);
+void kvm__reboot(struct kvm *kvm);
 void kvm__pause(struct kvm *kvm);
 void kvm__continue(struct kvm *kvm);
 void kvm__notify_paused(void);
diff --git a/kvm-cpu.c b/kvm-cpu.c
index ad4441b1..3e2037e3 100644
--- a/kvm-cpu.c
+++ b/kvm-cpu.c
@@ -45,10 +45,8 @@ void kvm_cpu__run(struct kvm_cpu *vcpu)
 static void kvm_cpu_signal_handler(int signum)
 {
 	if (signum == SIGKVMEXIT) {
-		if (current_kvm_cpu && current_kvm_cpu->is_running) {
+		if (current_kvm_cpu && current_kvm_cpu->is_running)
 			current_kvm_cpu->is_running = false;
-			kvm__continue(current_kvm_cpu->kvm);
-		}
 	} else if (signum == SIGKVMPAUSE) {
 		current_kvm_cpu->paused = 1;
 	}
@@ -70,19 +68,6 @@ static void kvm_cpu__handle_coalesced_mmio(struct kvm_cpu *cpu)
 	}
 }
 
-void kvm_cpu__reboot(struct kvm *kvm)
-{
-	int i;
-
-	/* The kvm->cpus array contains a null pointer in the last location */
-	for (i = 0; ; i++) {
-		if (kvm->cpus[i])
-			pthread_kill(kvm->cpus[i]->thread, SIGKVMEXIT);
-		else
-			break;
-	}
-}
-
 int kvm_cpu__start(struct kvm_cpu *cpu)
 {
 	sigset_t sigset;
@@ -177,7 +162,7 @@ int kvm_cpu__start(struct kvm_cpu *cpu)
 				 * Ensure that all VCPUs are torn down,
 				 * regardless of which CPU generated the event.
 				 */
-				kvm_cpu__reboot(cpu->kvm);
+				kvm__reboot(cpu->kvm);
 				goto exit_kvm;
 			};
 			break;
diff --git a/kvm-ipc.c b/kvm-ipc.c
index 857b0dc9..1ef3c3e4 100644
--- a/kvm-ipc.c
+++ b/kvm-ipc.c
@@ -341,7 +341,7 @@ static void handle_stop(struct kvm *kvm, int fd, u32 type, u32 len, u8 *msg)
 	if (WARN_ON(type != KVM_IPC_STOP || len))
 		return;
 
-	kvm_cpu__reboot(kvm);
+	kvm__reboot(kvm);
 }
 
 /* Pause/resume the guest using SIGUSR2 */
diff --git a/kvm.c b/kvm.c
index 10ed2300..fcc882ee 100644
--- a/kvm.c
+++ b/kvm.c
@@ -428,6 +428,27 @@ void kvm__dump_mem(struct kvm *kvm, unsigned long addr, unsigned long size, int
 	}
 }
 
+void kvm__reboot(struct kvm *kvm)
+{
+	int i;
+
+	/* Check if the guest is running */
+	if (!kvm->cpus[0] || kvm->cpus[0]->thread == 0)
+		return;
+
+	mutex_lock(&pause_lock);
+
+	/* The kvm->cpus array contains a null pointer in the last location */
+	for (i = 0; ; i++) {
+		if (kvm->cpus[i])
+			pthread_kill(kvm->cpus[i]->thread, SIGKVMEXIT);
+		else
+			break;
+	}
+
+	mutex_unlock(&pause_lock);
+}
+
 void kvm__pause(struct kvm *kvm)
 {
 	int i, paused_vcpus = 0;
@@ -441,8 +462,12 @@ void kvm__pause(struct kvm *kvm)
 	pause_event = eventfd(0, 0);
 	if (pause_event < 0)
 		die("Failed creating pause notification event");
-	for (i = 0; i < kvm->nrcpus; i++)
-		pthread_kill(kvm->cpus[i]->thread, SIGKVMPAUSE);
+	for (i = 0; i < kvm->nrcpus; i++) {
+		if (kvm->cpus[i]->is_running)
+			pthread_kill(kvm->cpus[i]->thread, SIGKVMPAUSE);
+		else
+			paused_vcpus++;
+	}
 
 	while (paused_vcpus < kvm->nrcpus) {
 		u64 cur_read;
diff --git a/powerpc/spapr_rtas.c b/powerpc/spapr_rtas.c
index 38d899c7..b898ff20 100644
--- a/powerpc/spapr_rtas.c
+++ b/powerpc/spapr_rtas.c
@@ -118,7 +118,7 @@ static void rtas_power_off(struct kvm_cpu *vcpu,
 		rtas_st(vcpu->kvm, rets, 0, -3);
 		return;
 	}
-	kvm_cpu__reboot(vcpu->kvm);
+	kvm__reboot(vcpu->kvm);
 }
 
 static void rtas_system_reboot(struct kvm_cpu *vcpu,
@@ -131,7 +131,7 @@ static void rtas_system_reboot(struct kvm_cpu *vcpu,
 	}
 
 	/* NB this actually halts the VM */
-	kvm_cpu__reboot(vcpu->kvm);
+	kvm__reboot(vcpu->kvm);
 }
 
 static void rtas_query_cpu_stopped_state(struct kvm_cpu *vcpu,
diff --git a/term.c b/term.c
index aebd1745..58f66a0c 100644
--- a/term.c
+++ b/term.c
@@ -37,7 +37,7 @@ int term_getc(struct kvm *kvm, int term)
 	if (term_got_escape) {
 		term_got_escape = false;
 		if (c == 'x')
-			kvm_cpu__reboot(kvm);
+			kvm__reboot(kvm);
 		if (c == term_escape_char)
 			return c;
 	}
diff --git a/ui/sdl.c b/ui/sdl.c
index f97a5112..5035405b 100644
--- a/ui/sdl.c
+++ b/ui/sdl.c
@@ -266,7 +266,7 @@ static void *sdl__thread(void *p)
 	}
 exit:
 	done = true;
-	kvm_cpu__reboot(fb->kvm);
+	kvm__reboot(fb->kvm);
 
 	return NULL;
 }