KVM: Fix possible circular locking in kvm_vm_ioctl_assign_device()

PATCH In review Sheng Yang 3 participants 6 messages Dec 7, 2009 – Dec 15, 2009 KVM

lore

Share & tools

lore

Sheng Yang

author

2009-12-07 08:58 UTC #0 lore raw

PATCH 1/1 KVM: Fix possible circular locking in kvm_vm_ioctl_assign_device()

One possible order is:

KVM_CREATE_IRQCHIP ioctl(took kvm->lock) -> kvm_iobus_register_dev() -> down_write(kvm->slots_lock).

The other one is in kvm_vm_ioctl_assign_device(), which take kvm->slots_lock first, then kvm->lock.

Observe it due to kernel locking debug warnings.

Signed-off-by Sheng Yang <[email protected]>

---
 virt/kvm/assigned-dev.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

virt/kvm/assigned-dev.c +3 -3

index fd9c097..f73de63 100644
@@ -508,8 +508,8 @@ static int kvm_vm_ioctl_assign_device(struct kvm *kvm,
 	struct kvm_assigned_dev_kernel *match;
 	struct pci_dev *dev;
 
-	down_read(&kvm->slots_lock);
 	mutex_lock(&kvm->lock);
+	down_read(&kvm->slots_lock);
 
 	match = kvm_find_assigned_dev(&kvm->arch.assigned_dev_head,
 				      assigned_dev->assigned_dev_id);

@@ -573,8 +573,8 @@ static int kvm_vm_ioctl_assign_device(struct kvm *kvm,
 	}
 
 out:
-	mutex_unlock(&kvm->lock);
 	up_read(&kvm->slots_lock);
+	mutex_unlock(&kvm->lock);
 	return r;
 out_list_del:
 	list_del(&match->list);

@@ -585,8 +585,8 @@ out_put:
 	pci_dev_put(dev);
 out_free:
 	kfree(match);
-	mutex_unlock(&kvm->lock);
 	up_read(&kvm->slots_lock);
+	mutex_unlock(&kvm->lock);
 	return r;
 }

Signature · 1 line

1.5.4.5

Sheng Yang

author

2009-12-14 07:14 UTC #1 lore raw

Quoted 11 lines · On Monday 07 December 2009 16:58:04 Sheng Yang wrote:

One possible order is:
KVM_CREATE_IRQCHIP ioctl(took kvm->lock) -> kvm_iobus_register_dev() -> down_write(kvm->slots_lock).
The other one is in kvm_vm_ioctl_assign_device(), which take
kvm->slots_lock first, then kvm->lock.
Observe it due to kernel locking debug warnings.
Signed-off-by Sheng Yang <[email protected]>

Avi? Marcelo?

Signature · 2 lines

regards
Yang, Sheng

Quoted 38 lines

---
 virt/kvm/assigned-dev.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

virt/kvm/assigned-dev.c +3 -3

index fd9c097..f73de63 100644
@@ -508,8 +508,8 @@ static int kvm_vm_ioctl_assign_device(struct kvm *kvm,
 	struct kvm_assigned_dev_kernel *match;
 	struct pci_dev *dev;
 
-	down_read(&kvm->slots_lock);
 	mutex_lock(&kvm->lock);
+	down_read(&kvm->slots_lock);
 
 	match = kvm_find_assigned_dev(&kvm->arch.assigned_dev_head,
 				      assigned_dev->assigned_dev_id);

@@ -573,8 +573,8 @@ static int kvm_vm_ioctl_assign_device(struct kvm *kvm,
 	}
 
 out:
-	mutex_unlock(&kvm->lock);
 	up_read(&kvm->slots_lock);
+	mutex_unlock(&kvm->lock);
 	return r;
 out_list_del:
 	list_del(&match->list);

@@ -585,8 +585,8 @@ out_put:
 	pci_dev_put(dev);
 out_free:
 	kfree(match);
-	mutex_unlock(&kvm->lock);
 	up_read(&kvm->slots_lock);
+	mutex_unlock(&kvm->lock);
 	return r;
 }

Avi Kivity

2009-12-14 09:36 UTC #2 lore raw

Quoted 7 lines · Sheng Yang

Quoted 5 lines · On Monday 07 December 2009 16:58:04 Sheng Yang wrote:
One possible order is:
KVM_CREATE_IRQCHIP ioctl(took kvm->lock) -> kvm_iobus_register_dev() -> down_write(kvm->slots_lock).

Quoted 10 lines

Quoted 7 lines
The other one is in kvm_vm_ioctl_assign_device(), which take
kvm->slots_lock first, then kvm->lock.
Observe it due to kernel locking debug warnings.
Signed-off-by Sheng Yang<[email protected]>
Avi? Marcelo?

Sorry, hunting a regression so the tree is frozen. Will review soon.

Signature · 1 line

error compiling committee.c: too many arguments to function

Marcelo Tosatti

2009-12-14 19:12 UTC #3 lore raw

In reply to Sheng Yang

Quoted 9 lines · Sheng Yang

One possible order is:
KVM_CREATE_IRQCHIP ioctl(took kvm->lock) -> kvm_iobus_register_dev() -> down_write(kvm->slots_lock).
The other one is in kvm_vm_ioctl_assign_device(), which take kvm->slots_lock first, then kvm->lock.
Observe it due to kernel locking debug warnings.

Sheng,

Looks good.

Can you also update the comment on top of kvm_main.c?

Sheng Yang

author

2009-12-15 02:28 UTC #4 lore raw

PATCH 1/1 KVM: Fix possible circular locking in kvm_vm_ioctl_assign_device()

One possible order is:

KVM_CREATE_IRQCHIP ioctl(took kvm->lock) -> kvm_iobus_register_dev() -> down_write(kvm->slots_lock).

The other one is in kvm_vm_ioctl_assign_device(), which take kvm->slots_lock first, then kvm->lock.

Update the comment of lock order as well.

Observe it due to kernel locking debug warnings.

Signed-off-by Sheng Yang <[email protected]>

---
 virt/kvm/assigned-dev.c |    6 +++---
 virt/kvm/kvm_main.c     |    2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

virt/kvm/assigned-dev.c +3 -3

index fd9c097..f73de63 100644
@@ -508,8 +508,8 @@ static int kvm_vm_ioctl_assign_device(struct kvm *kvm,
 	struct kvm_assigned_dev_kernel *match;
 	struct pci_dev *dev;
 
-	down_read(&kvm->slots_lock);
 	mutex_lock(&kvm->lock);
+	down_read(&kvm->slots_lock);
 
 	match = kvm_find_assigned_dev(&kvm->arch.assigned_dev_head,
 				      assigned_dev->assigned_dev_id);

@@ -573,8 +573,8 @@ static int kvm_vm_ioctl_assign_device(struct kvm *kvm,
 	}
 
 out:
-	mutex_unlock(&kvm->lock);
 	up_read(&kvm->slots_lock);
+	mutex_unlock(&kvm->lock);
 	return r;
 out_list_del:
 	list_del(&match->list);

@@ -585,8 +585,8 @@ out_put:
 	pci_dev_put(dev);
 out_free:
 	kfree(match);
-	mutex_unlock(&kvm->lock);
 	up_read(&kvm->slots_lock);
+	mutex_unlock(&kvm->lock);
 	return r;
 }

virt/kvm/kvm_main.c +1 -1

index f92ba13..91b84c9 100644
@@ -63,7 +63,7 @@ MODULE_LICENSE("GPL");
 /*
  * Ordering of locks:
  *
- * 		kvm->slots_lock --> kvm->lock --> kvm->irq_lock
+ * 		kvm->lock --> kvm->slots_lock --> kvm->irq_lock
  */
 
 DEFINE_SPINLOCK(kvm_lock);

Signature · 1 line

1.5.4.5

Avi Kivity

2009-12-15 09:35 UTC #5 lore raw

Quoted 5 lines · Sheng Yang

One possible order is:
KVM_CREATE_IRQCHIP ioctl(took kvm->lock) -> kvm_iobus_register_dev() -> down_write(kvm->slots_lock).

Quoted 7 lines

The other one is in kvm_vm_ioctl_assign_device(), which take kvm->slots_lock first, then kvm->lock.
Update the comment of lock order as well.
Observe it due to kernel locking debug warnings.

Applied and queued for 2.6.33, thanks.

Signature · 1 line

error compiling committee.c: too many arguments to function