Skip to content

Commit

Permalink
KVM: x86/xen: Avoid deadlock by adding kvm->arch.xen.xen_lock leaf no…
Browse files Browse the repository at this point in the history
…de lock

In commit 14243b3 ("KVM: x86/xen: Add KVM_IRQ_ROUTING_XEN_EVTCHN
and event channel delivery") the clever version of me left some helpful
notes for those who would come after him:

       /*
        * For the irqfd workqueue, using the main kvm->lock mutex is
        * fine since this function is invoked from kvm_set_irq() with
        * no other lock held, no srcu. In future if it will be called
        * directly from a vCPU thread (e.g. on hypercall for an IPI)
        * then it may need to switch to using a leaf-node mutex for
        * serializing the shared_info mapping.
        */
       mutex_lock(&kvm->lock);

In commit 2fd6df2 ("KVM: x86/xen: intercept EVTCHNOP_send from guests")
the other version of me ran straight past that comment without reading it,
and introduced a potential deadlock by taking vcpu->mutex and kvm->lock
in the wrong order.

Solve this as originally suggested, by adding a leaf-node lock in the Xen
state rather than using kvm->lock for it.

Fixes: 2fd6df2 ("KVM: x86/xen: intercept EVTCHNOP_send from guests")
Signed-off-by: David Woodhouse <[email protected]>
Message-Id: <[email protected]>
[Rebase, add docs. - Paolo]
Signed-off-by: Paolo Bonzini <[email protected]>
  • Loading branch information
dwmw2 authored and bonzini committed Jan 11, 2023
1 parent 42a9000 commit 310bc39
Show file tree
Hide file tree
Showing 3 changed files with 32 additions and 38 deletions.
2 changes: 1 addition & 1 deletion Documentation/virt/kvm/locking.rst
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ For SRCU:

On x86:

- vcpu->mutex is taken outside kvm->arch.hyperv.hv_lock
- vcpu->mutex is taken outside kvm->arch.hyperv.hv_lock and kvm->arch.xen.xen_lock

- kvm->arch.mmu_lock is an rwlock. kvm->arch.tdp_mmu_pages_lock and
kvm->arch.mmu_unsync_pages_lock are taken inside kvm->arch.mmu_lock, and
Expand Down
1 change: 1 addition & 0 deletions arch/x86/include/asm/kvm_host.h
Original file line number Diff line number Diff line change
Expand Up @@ -1111,6 +1111,7 @@ struct msr_bitmap_range {

/* Xen emulation context */
struct kvm_xen {
struct mutex xen_lock;
u32 xen_version;
bool long_mode;
bool runstate_update_flag;
Expand Down
67 changes: 30 additions & 37 deletions arch/x86/kvm/xen.c
Original file line number Diff line number Diff line change
Expand Up @@ -607,26 +607,26 @@ int kvm_xen_hvm_set_attr(struct kvm *kvm, struct kvm_xen_hvm_attr *data)
if (!IS_ENABLED(CONFIG_64BIT) && data->u.long_mode) {
r = -EINVAL;
} else {
mutex_lock(&kvm->lock);
mutex_lock(&kvm->arch.xen.xen_lock);
kvm->arch.xen.long_mode = !!data->u.long_mode;
mutex_unlock(&kvm->lock);
mutex_unlock(&kvm->arch.xen.xen_lock);
r = 0;
}
break;

case KVM_XEN_ATTR_TYPE_SHARED_INFO:
mutex_lock(&kvm->lock);
mutex_lock(&kvm->arch.xen.xen_lock);
r = kvm_xen_shared_info_init(kvm, data->u.shared_info.gfn);
mutex_unlock(&kvm->lock);
mutex_unlock(&kvm->arch.xen.xen_lock);
break;

case KVM_XEN_ATTR_TYPE_UPCALL_VECTOR:
if (data->u.vector && data->u.vector < 0x10)
r = -EINVAL;
else {
mutex_lock(&kvm->lock);
mutex_lock(&kvm->arch.xen.xen_lock);
kvm->arch.xen.upcall_vector = data->u.vector;
mutex_unlock(&kvm->lock);
mutex_unlock(&kvm->arch.xen.xen_lock);
r = 0;
}
break;
Expand All @@ -636,9 +636,9 @@ int kvm_xen_hvm_set_attr(struct kvm *kvm, struct kvm_xen_hvm_attr *data)
break;

case KVM_XEN_ATTR_TYPE_XEN_VERSION:
mutex_lock(&kvm->lock);
mutex_lock(&kvm->arch.xen.xen_lock);
kvm->arch.xen.xen_version = data->u.xen_version;
mutex_unlock(&kvm->lock);
mutex_unlock(&kvm->arch.xen.xen_lock);
r = 0;
break;

Expand All @@ -647,9 +647,9 @@ int kvm_xen_hvm_set_attr(struct kvm *kvm, struct kvm_xen_hvm_attr *data)
r = -EOPNOTSUPP;
break;
}
mutex_lock(&kvm->lock);
mutex_lock(&kvm->arch.xen.xen_lock);
kvm->arch.xen.runstate_update_flag = !!data->u.runstate_update_flag;
mutex_unlock(&kvm->lock);
mutex_unlock(&kvm->arch.xen.xen_lock);
r = 0;
break;

Expand All @@ -664,7 +664,7 @@ int kvm_xen_hvm_get_attr(struct kvm *kvm, struct kvm_xen_hvm_attr *data)
{
int r = -ENOENT;

mutex_lock(&kvm->lock);
mutex_lock(&kvm->arch.xen.xen_lock);

switch (data->type) {
case KVM_XEN_ATTR_TYPE_LONG_MODE:
Expand Down Expand Up @@ -703,15 +703,15 @@ int kvm_xen_hvm_get_attr(struct kvm *kvm, struct kvm_xen_hvm_attr *data)
break;
}

mutex_unlock(&kvm->lock);
mutex_unlock(&kvm->arch.xen.xen_lock);
return r;
}

int kvm_xen_vcpu_set_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data)
{
int idx, r = -ENOENT;

mutex_lock(&vcpu->kvm->lock);
mutex_lock(&vcpu->kvm->arch.xen.xen_lock);
idx = srcu_read_lock(&vcpu->kvm->srcu);

switch (data->type) {
Expand Down Expand Up @@ -939,15 +939,15 @@ int kvm_xen_vcpu_set_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data)
}

srcu_read_unlock(&vcpu->kvm->srcu, idx);
mutex_unlock(&vcpu->kvm->lock);
mutex_unlock(&vcpu->kvm->arch.xen.xen_lock);
return r;
}

int kvm_xen_vcpu_get_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data)
{
int r = -ENOENT;

mutex_lock(&vcpu->kvm->lock);
mutex_lock(&vcpu->kvm->arch.xen.xen_lock);

switch (data->type) {
case KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO:
Expand Down Expand Up @@ -1030,7 +1030,7 @@ int kvm_xen_vcpu_get_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data)
break;
}

mutex_unlock(&vcpu->kvm->lock);
mutex_unlock(&vcpu->kvm->arch.xen.xen_lock);
return r;
}

Expand Down Expand Up @@ -1123,7 +1123,7 @@ int kvm_xen_hvm_config(struct kvm *kvm, struct kvm_xen_hvm_config *xhc)
xhc->blob_size_32 || xhc->blob_size_64))
return -EINVAL;

mutex_lock(&kvm->lock);
mutex_lock(&kvm->arch.xen.xen_lock);

if (xhc->msr && !kvm->arch.xen_hvm_config.msr)
static_branch_inc(&kvm_xen_enabled.key);
Expand All @@ -1132,7 +1132,7 @@ int kvm_xen_hvm_config(struct kvm *kvm, struct kvm_xen_hvm_config *xhc)

memcpy(&kvm->arch.xen_hvm_config, xhc, sizeof(*xhc));

mutex_unlock(&kvm->lock);
mutex_unlock(&kvm->arch.xen.xen_lock);
return 0;
}

Expand Down Expand Up @@ -1675,15 +1675,7 @@ static int kvm_xen_set_evtchn(struct kvm_xen_evtchn *xe, struct kvm *kvm)
mm_borrowed = true;
}

/*
* For the irqfd workqueue, using the main kvm->lock mutex is
* fine since this function is invoked from kvm_set_irq() with
* no other lock held, no srcu. In future if it will be called
* directly from a vCPU thread (e.g. on hypercall for an IPI)
* then it may need to switch to using a leaf-node mutex for
* serializing the shared_info mapping.
*/
mutex_lock(&kvm->lock);
mutex_lock(&kvm->arch.xen.xen_lock);

/*
* It is theoretically possible for the page to be unmapped
Expand Down Expand Up @@ -1712,7 +1704,7 @@ static int kvm_xen_set_evtchn(struct kvm_xen_evtchn *xe, struct kvm *kvm)
srcu_read_unlock(&kvm->srcu, idx);
} while(!rc);

mutex_unlock(&kvm->lock);
mutex_unlock(&kvm->arch.xen.xen_lock);

if (mm_borrowed)
kthread_unuse_mm(kvm->mm);
Expand Down Expand Up @@ -1828,7 +1820,7 @@ static int kvm_xen_eventfd_update(struct kvm *kvm,
int ret;

/* Protect writes to evtchnfd as well as the idr lookup. */
mutex_lock(&kvm->lock);
mutex_lock(&kvm->arch.xen.xen_lock);
evtchnfd = idr_find(&kvm->arch.xen.evtchn_ports, port);

ret = -ENOENT;
Expand Down Expand Up @@ -1859,7 +1851,7 @@ static int kvm_xen_eventfd_update(struct kvm *kvm,
}
ret = 0;
out_unlock:
mutex_unlock(&kvm->lock);
mutex_unlock(&kvm->arch.xen.xen_lock);
return ret;
}

Expand Down Expand Up @@ -1922,10 +1914,10 @@ static int kvm_xen_eventfd_assign(struct kvm *kvm,
evtchnfd->deliver.port.priority = data->u.evtchn.deliver.port.priority;
}

mutex_lock(&kvm->lock);
mutex_lock(&kvm->arch.xen.xen_lock);
ret = idr_alloc(&kvm->arch.xen.evtchn_ports, evtchnfd, port, port + 1,
GFP_KERNEL);
mutex_unlock(&kvm->lock);
mutex_unlock(&kvm->arch.xen.xen_lock);
if (ret >= 0)
return 0;

Expand All @@ -1943,9 +1935,9 @@ static int kvm_xen_eventfd_deassign(struct kvm *kvm, u32 port)
{
struct evtchnfd *evtchnfd;

mutex_lock(&kvm->lock);
mutex_lock(&kvm->arch.xen.xen_lock);
evtchnfd = idr_remove(&kvm->arch.xen.evtchn_ports, port);
mutex_unlock(&kvm->lock);
mutex_unlock(&kvm->arch.xen.xen_lock);

if (!evtchnfd)
return -ENOENT;
Expand All @@ -1963,7 +1955,7 @@ static int kvm_xen_eventfd_reset(struct kvm *kvm)
int i;
int n = 0;

mutex_lock(&kvm->lock);
mutex_lock(&kvm->arch.xen.xen_lock);

/*
* Because synchronize_srcu() cannot be called inside the
Expand All @@ -1975,7 +1967,7 @@ static int kvm_xen_eventfd_reset(struct kvm *kvm)

all_evtchnfds = kmalloc_array(n, sizeof(struct evtchnfd *), GFP_KERNEL);
if (!all_evtchnfds) {
mutex_unlock(&kvm->lock);
mutex_unlock(&kvm->arch.xen.xen_lock);
return -ENOMEM;
}

Expand All @@ -1984,7 +1976,7 @@ static int kvm_xen_eventfd_reset(struct kvm *kvm)
all_evtchnfds[n++] = evtchnfd;
idr_remove(&kvm->arch.xen.evtchn_ports, evtchnfd->send_port);
}
mutex_unlock(&kvm->lock);
mutex_unlock(&kvm->arch.xen.xen_lock);

synchronize_srcu(&kvm->srcu);

Expand Down Expand Up @@ -2086,6 +2078,7 @@ void kvm_xen_destroy_vcpu(struct kvm_vcpu *vcpu)

void kvm_xen_init_vm(struct kvm *kvm)
{
mutex_init(&kvm->arch.xen.xen_lock);
idr_init(&kvm->arch.xen.evtchn_ports);
kvm_gpc_init(&kvm->arch.xen.shinfo_cache, kvm, NULL, KVM_HOST_USES_PFN);
}
Expand Down

0 comments on commit 310bc39

Please sign in to comment.