KVM: Take missing slots_lock for kvm_io_bus_unregister_dev()

PATCH Applied Takuya Yoshikawa 3 participants 11 messages Dec 15, 2010 – Dec 16, 2010

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Takuya Yoshikawa

author

2010-12-15 16:39 UTC #0 lore raw

v2 cover KVM: Take missing slots_lock for kvm_io_bus_unregister_dev()

This patchset consists of three patches:

[1/3] KVM: Take missing slots_lock for kvm_io_bus_unregister_dev() [2/3] KVM: Centralize slots_lock aquisition during KVM_CREATE_IRQCHIP [3/3] KVM: Centralize irq_lock aquisition during KVM_CREATE_IRQCHIP

Though patch 1 is a fix, this is only for an error handling path. Patches 2,3 are just for coding style consistency. So pick up those whitch match your policy.

Changelog:
moved slots_lock and irq_lock aquisition to their caller following Marcelo's advice.

Thanks,
Takuya

Takuya Yoshikawa

author

2010-12-15 16:41 UTC #1 lore raw

PATCH v2 1/3 KVM: Take missing slots_lock for kvm_io_bus_unregister_dev()

From: Takuya Yoshikawa <[email protected]>

In KVM_CREATE_IRQCHIP, kvm_io_bus_unregister_dev() is called without taking slots_lock in the error handling path.

Signed-off-by Takuya Yoshikawa <[email protected]>

---
 arch/ia64/kvm/kvm-ia64.c |    2 ++
 arch/x86/kvm/x86.c       |    4 ++++
 2 files changed, 6 insertions(+), 0 deletions(-)

arch/ia64/kvm/kvm-ia64.c +2 -0

index 48a48bd..70d224d 100644
@@ -951,7 +951,9 @@ long kvm_arch_vm_ioctl(struct file *filp,
 			goto out;
 		r = kvm_setup_default_irq_routing(kvm);
 		if (r) {
+			mutex_lock(&kvm->slots_lock);
 			kvm_ioapic_destroy(kvm);
+			mutex_unlock(&kvm->slots_lock);
 			goto out;
 		}
 		break;

arch/x86/kvm/x86.c +4 -0

index 8d76150..3113aaf 100644
@@ -3308,8 +3308,10 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		if (vpic) {
 			r = kvm_ioapic_init(kvm);
 			if (r) {
+				mutex_lock(&kvm->slots_lock);
 				kvm_io_bus_unregister_dev(kvm, KVM_PIO_BUS,
 							  &vpic->dev);
+				mutex_unlock(&kvm->slots_lock);
 				kfree(vpic);
 				goto create_irqchip_unlock;
 			}

@@ -3320,10 +3322,12 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		smp_wmb();
 		r = kvm_setup_default_irq_routing(kvm);
 		if (r) {
+			mutex_lock(&kvm->slots_lock);
 			mutex_lock(&kvm->irq_lock);
 			kvm_ioapic_destroy(kvm);
 			kvm_destroy_pic(kvm);
 			mutex_unlock(&kvm->irq_lock);
+			mutex_unlock(&kvm->slots_lock);
 		}
 	create_irqchip_unlock:
 		mutex_unlock(&kvm->lock);

Signature · 1 line

1.7.1

Avi Kivity

Applied

2010-12-16 09:12 UTC #2 lore raw

Quoted 4 lines · Takuya Yoshikawa

From: Takuya Yoshikawa<[email protected]>
In KVM_CREATE_IRQCHIP, kvm_io_bus_unregister_dev() is called without taking slots_lock in the error handling path.

Thanks, applied.

Signature · 1 line

error compiling committee.c: too many arguments to function

Takuya Yoshikawa

author

2010-12-15 16:43 UTC #3 lore raw

PATCH v2 2/3 KVM: Centralize slots_lock aquisition during KVM_CREATE_IRQCHIP

In reply to Takuya Yoshikawa

From: Takuya Yoshikawa <[email protected]>

Move slots_lock aquisition from kvm_ioapic_init() and kvm_create_pic() to their caller.

As a result, x86's KVM_CREATE_IRQCHIP is now covered by a unified slots_lock section, including kvm_setup_default_irq_routing().

Signed-off-by Takuya Yoshikawa <[email protected]>

---
 arch/ia64/kvm/kvm-ia64.c |    2 ++
 arch/x86/kvm/i8259.c     |    2 --
 arch/x86/kvm/x86.c       |    6 ++----
 virt/kvm/ioapic.c        |    2 --
 4 files changed, 4 insertions(+), 8 deletions(-)

arch/ia64/kvm/kvm-ia64.c +2 -0

index 70d224d..060c594 100644
@@ -946,7 +946,9 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		}
 	case KVM_CREATE_IRQCHIP:
 		r = -EFAULT;
+		mutex_lock(&kvm->slots_lock);
 		r = kvm_ioapic_init(kvm);
+		mutex_unlock(&kvm->slots_lock);
 		if (r)
 			goto out;
 		r = kvm_setup_default_irq_routing(kvm);

arch/x86/kvm/i8259.c +0 -2

index f628234..37d24bc 100644
@@ -580,9 +580,7 @@ struct kvm_pic *kvm_create_pic(struct kvm *kvm)
 	 * Initialize PIO device
 	 */
 	kvm_iodevice_init(&s->dev, &picdev_ops);
-	mutex_lock(&kvm->slots_lock);
 	ret = kvm_io_bus_register_dev(kvm, KVM_PIO_BUS, &s->dev);
-	mutex_unlock(&kvm->slots_lock);
 	if (ret < 0) {
 		kfree(s);
 		return NULL;

arch/x86/kvm/x86.c +2 -4

index 3113aaf..736ab93 100644
@@ -3300,6 +3300,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		struct kvm_pic *vpic;
 
 		mutex_lock(&kvm->lock);
+		mutex_lock(&kvm->slots_lock);
 		r = -EEXIST;
 		if (kvm->arch.vpic)
 			goto create_irqchip_unlock;

@@ -3308,10 +3309,8 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		if (vpic) {
 			r = kvm_ioapic_init(kvm);
 			if (r) {
-				mutex_lock(&kvm->slots_lock);
 				kvm_io_bus_unregister_dev(kvm, KVM_PIO_BUS,
 							  &vpic->dev);
-				mutex_unlock(&kvm->slots_lock);
 				kfree(vpic);
 				goto create_irqchip_unlock;
 			}

@@ -3322,14 +3321,13 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		smp_wmb();
 		r = kvm_setup_default_irq_routing(kvm);
 		if (r) {
-			mutex_lock(&kvm->slots_lock);
 			mutex_lock(&kvm->irq_lock);
 			kvm_ioapic_destroy(kvm);
 			kvm_destroy_pic(kvm);
 			mutex_unlock(&kvm->irq_lock);
-			mutex_unlock(&kvm->slots_lock);
 		}
 	create_irqchip_unlock:
+		mutex_unlock(&kvm->slots_lock);
 		mutex_unlock(&kvm->lock);
 		break;
 	}

virt/kvm/ioapic.c +0 -2

index 0b9df83..532bffc 100644
@@ -393,9 +393,7 @@ int kvm_ioapic_init(struct kvm *kvm)
 	kvm_ioapic_reset(ioapic);
 	kvm_iodevice_init(&ioapic->dev, &ioapic_mmio_ops);
 	ioapic->kvm = kvm;
-	mutex_lock(&kvm->slots_lock);
 	ret = kvm_io_bus_register_dev(kvm, KVM_MMIO_BUS, &ioapic->dev);
-	mutex_unlock(&kvm->slots_lock);
 	if (ret < 0) {
 		kvm->arch.vioapic = NULL;
 		kfree(ioapic);

Signature · 1 line

1.7.1

Avi Kivity

2010-12-16 09:11 UTC #4 lore raw

Re: [PATCH 2/3] KVM: Centralize slots_lock aquisition during KVM_CREATE_IRQCHIP

Quoted 9 lines · Takuya Yoshikawa

From: Takuya Yoshikawa<[email protected]>
Move slots_lock aquisition from kvm_ioapic_init() and kvm_create_pic() to their caller.
As a result, x86's KVM_CREATE_IRQCHIP is now covered by a unified slots_lock section, including kvm_setup_default_irq_routing().

I'm not sure about this...

arch/x86/kvm/x86.c long kvm_arch_vm_ioctl(struct file *filp, · line 3303

Quoted 7 lines

arch/x86/kvm/x86.c +1 -0

@@ -3300,6 +3300,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
  		struct kvm_pic *vpic;
 
  		mutex_lock(&kvm->lock);
+		mutex_lock(&kvm->slots_lock);

here, the reader might wonder why we take slots_lock, since we aren't manipulating anything covered by the lock directly.

arch/x86/kvm/x86.c long kvm_arch_vm_ioctl(struct file *filp, · line 3311

Quoted 8 lines

arch/x86/kvm/x86.c +0 -1

  		r = -EEXIST;
  		if (kvm->arch.vpic)
  			goto create_irqchip_unlock;

@@ -3308,10 +3309,8 @@ long kvm_arch_vm_ioctl(struct file *filp,
  		if (vpic) {
  			r = kvm_ioapic_init(kvm);
  			if (r) {
-				mutex_lock(&kvm->slots_lock);

and here, the reader might wonder why we don't take slots_lock, which protects io_bus.

Maybe we ought to move slots_lock acquisition to kvm_io_bus_register() and friends.

Signature · 1 line

error compiling committee.c: too many arguments to function

Takuya Yoshikawa

2010-12-16 09:26 UTC #5 lore raw

Re: [PATCH 2/3] KVM: Centralize slots_lock aquisition during KVM_CREATE_IRQCHIP

Quoted 34 lines · Avi Kivity

Quoted 9 lines · Takuya Yoshikawa
From: Takuya Yoshikawa<[email protected]>
Move slots_lock aquisition from kvm_ioapic_init() and kvm_create_pic() to their caller.
As a result, x86's KVM_CREATE_IRQCHIP is now covered by a unified slots_lock section, including kvm_setup_default_irq_routing().
I'm not sure about this...
arch/x86/kvm/x86.c long kvm_arch_vm_ioctl(struct file *filp, · line 3303
Quoted 7 lines
arch/x86/kvm/x86.c +1 -0
@@ -3300,6 +3300,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
 struct kvm_pic *vpic;
 
 mutex_lock(&kvm->lock);
+ mutex_lock(&kvm->slots_lock);
here, the reader might wonder why we take slots_lock, since we aren't manipulating anything covered by the lock directly.
arch/x86/kvm/x86.c long kvm_arch_vm_ioctl(struct file *filp, · line 3311
Quoted 8 lines
arch/x86/kvm/x86.c +0 -1
 r = -EEXIST;
 if (kvm->arch.vpic)
 goto create_irqchip_unlock;
@@ -3308,10 +3309,8 @@ long kvm_arch_vm_ioctl(struct file *filp,
 if (vpic) {
 r = kvm_ioapic_init(kvm);
 if (r) {
- mutex_lock(&kvm->slots_lock);
and here, the reader might wonder why we don't take slots_lock, which protects io_bus.

Quoted 2 lines

Maybe we ought to move slots_lock acquisition to kvm_io_bus_register() and friends.

So it will move the lock acquisition to the opposite ( callee ) side than mine.
At first, I tried to do that, but there are so many ...

Anyway, your suggestion seems to be the best way if possible.

One question: how about kvm_io_bus_[read|write] ?

These are called from the emulator but I could not find where slots_lock are held though I can see the comments

"kvm_io_bus_[read|write] - called under kvm->slots_lock"

Takuya

Avi Kivity

2010-12-16 09:34 UTC #6 lore raw

Re: [PATCH 2/3] KVM: Centralize slots_lock aquisition during KVM_CREATE_IRQCHIP

Quoted 15 lines · Takuya Yoshikawa

Quoted 6 lines
and here, the reader might wonder why we don't take slots_lock, which protects io_bus.
Maybe we ought to move slots_lock acquisition to
kvm_io_bus_register() and friends.
So it will move the lock acquisition to the opposite ( callee ) side than mine.
At first, I tried to do that, but there are so many ...
Anyway, your suggestion seems to be the best way if possible.

Quoted 7 lines

One question: how about kvm_io_bus_[read|write] ?
These are called from the emulator but I could not find where slots_lock are held though I can see the comments
"kvm_io_bus_[read|write] - called under kvm->slots_lock"

They're under srcu now, the comments are outdated.

We used to have slots_lock be a rwsem, taken for read or write as necessary. Now we use srcu for read, and the slots_lock mutex + synchronize_srcu for write.

Signature · 1 line

error compiling committee.c: too many arguments to function

Takuya Yoshikawa

2010-12-16 09:44 UTC #7 lore raw

Re: [PATCH 2/3] KVM: Centralize slots_lock aquisition during KVM_CREATE_IRQCHIP

Quoted 15 lines · Avi Kivity

Quoted 15 lines
Quoted 2 lines
Maybe we ought to move slots_lock acquisition to kvm_io_bus_register() and friends.
So it will move the lock acquisition to the opposite ( callee ) side than mine. At first, I tried to do that, but there are so many ...
Anyway, your suggestion seems to be the best way if possible.
One question: how about kvm_io_bus_[read|write] ?
These are called from the emulator but I could not find where slots_lock are held though I can see the comments

Quoted 7 lines

Quoted 2 lines
"kvm_io_bus_[read|write] - called under kvm->slots_lock"
They're under srcu now, the comments are outdated.
We used to have slots_lock be a rwsem, taken for read or write as necessary. Now we use srcu for read, and the slots_lock mutex + synchronize_srcu for write.

Ah, would you mind updating the comments?
- or just removing outdated ones?

I will reread these locking based on your answer!

Thanks,
Takuya

Avi Kivity

2010-12-16 09:55 UTC #8 lore raw

Re: [PATCH 2/3] KVM: Centralize slots_lock aquisition during KVM_CREATE_IRQCHIP

Quoted 12 lines · Takuya Yoshikawa

Quoted 6 lines
They're under srcu now, the comments are outdated.
We used to have slots_lock be a rwsem, taken for read or write as necessary. Now we use srcu for read, and the slots_lock mutex + synchronize_srcu for write.
Ah, would you mind updating the comments?
- or just removing outdated ones?

Fixing the comments would be good.

Quoted 2 lines

I will reread these locking based on your answer!

One day I'll write Documentation/kvm/locking.txt, so it could get outdated as well.

Signature · 1 line

error compiling committee.c: too many arguments to function

Takuya Yoshikawa

author

2010-12-16 12:51 UTC #9 lore raw

Re: [PATCH 2/3] KVM: Centralize slots_lock aquisition during KVM_CREATE_IRQCHIP

Quoted 4 lines · Avi Kivity

One day I'll write Documentation/kvm/locking.txt, so it could get outdated as well.

I seemed to be in EOY(End Of Year) mode.

I remember why I could not move slots_lock acquisition to kvm_io_bus_[un]register().

The locking order constraint forced me to take slots_lock outside of irq_lock.

So there won't be much things to be done anymore except your documentation!

Thanks,
Takuya

Takuya Yoshikawa

author

2010-12-15 16:45 UTC #10 lore raw

PATCH v2 3/3 KVM: Centralize irq_lock aquisition during KVM_CREATE_IRQCHIP

In reply to Takuya Yoshikawa

From: Takuya Yoshikawa <[email protected]>

Move irq_lock aquisition from kvm_setup_default_irq_routing(), inside of kvm_set_irq_routing(), to its caller.

This makes the lock management clearer, with a bit longer irq_lock section.

Signed-off-by Takuya Yoshikawa <[email protected]>

---
 arch/ia64/kvm/kvm-ia64.c |    2 ++
 arch/x86/kvm/x86.c       |    4 ++--
 virt/kvm/irq_comm.c      |    2 --
 3 files changed, 4 insertions(+), 4 deletions(-)

arch/ia64/kvm/kvm-ia64.c +2 -0

index 060c594..34a1699 100644
@@ -951,7 +951,9 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		mutex_unlock(&kvm->slots_lock);
 		if (r)
 			goto out;
+		mutex_lock(&kvm->irq_lock);
 		r = kvm_setup_default_irq_routing(kvm);
+		mutex_unlock(&kvm->irq_lock);
 		if (r) {
 			mutex_lock(&kvm->slots_lock);
 			kvm_ioapic_destroy(kvm);

arch/x86/kvm/x86.c +2 -2

index 736ab93..5c70869 100644
@@ -3319,13 +3319,13 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		smp_wmb();
 		kvm->arch.vpic = vpic;
 		smp_wmb();
+		mutex_lock(&kvm->irq_lock);
 		r = kvm_setup_default_irq_routing(kvm);
 		if (r) {
-			mutex_lock(&kvm->irq_lock);
 			kvm_ioapic_destroy(kvm);
 			kvm_destroy_pic(kvm);
-			mutex_unlock(&kvm->irq_lock);
 		}
+		mutex_unlock(&kvm->irq_lock);
 	create_irqchip_unlock:
 		mutex_unlock(&kvm->slots_lock);
 		mutex_unlock(&kvm->lock);

virt/kvm/irq_comm.c +0 -2

index 9f614b4..265fd2f 100644
@@ -407,10 +407,8 @@ int kvm_set_irq_routing(struct kvm *kvm,
 		++ue;
 	}
 
-	mutex_lock(&kvm->irq_lock);
 	old = kvm->irq_routing;
 	kvm_irq_routing_update(kvm, new);
-	mutex_unlock(&kvm->irq_lock);
 
 	synchronize_rcu();

Signature · 1 line

1.7.1