Re: [PATCH 3/4 V10] Add ioctl for KVMCLOCK_GUEST_STOPPED

On Mon, 30 Jan 2012, Avi Kivity wrote:

On 01/30/2012 05:07 PM, Avi Kivity wrote:

quoted

quoted

+Parameters: None
+Returns: 0 on success, -1 on error
+
+This signals to the host kernel that the specified guest is being paused by
+userspace.  The host will set a flag in the pvclock structure that is checked
+from the soft lockup watchdog.  This ioctl can be called during pause or
+unpause.
+
 5. The kvm_run structure
 
 
+/*
+ * kvm_set_guest_paused() indicates to the guest kernel that it has been
+ * stopped by the hypervisor.  This function will be called from the host only.
+ */
+static int kvm_set_guest_paused(struct kvm *kvm)
+{
+	struct kvm_vcpu *vcpu;
+	struct pvclock_vcpu_time_info *src;
+	int i;
+
+	kvm_for_each_vcpu(i, vcpu, kvm) {
+		if (!vcpu->arch.time_page)
+			continue;
+		src = &vcpu->arch.hv_clock;
+		src->flags |= PVCLOCK_GUEST_STOPPED;

This looks racy.  The vcpu can remove its kvmclock concurrently with
this access, and src will be NULL.

Can you point me to the discussion that moved this to be a vm ioctl?  In
general vm ioctls that do things for all vcpus are racy, like here. 
You're accessing variables that are protected by the vcpu mutex, and not
taking the mutex (nor can you, since it is held while the guest is
running, unlike most kernel mutexes).

Note, the standard way to fix this race is to
kvm_make_request(KVM_REQ_KVMCLOCK_STOP) and kvm_vcpu_kick(vcpu), and do
the update in vcpu_enter_guest().  But I think this is needless
complexity and want to understand what motivated the move to a vm ioctl.

I will hold off on fixing this race until we settle the vm or vcpu ioctl
question.

Eric