[RFC PATCH] KVM: Fix missing lock for kvm_io_bus_unregister_dev()

[RFC PATCH] KVM: Fix missing lock for kvm_io_bus_unregister_dev()

[Takuya Yoshikawa]

Memo:
 - kvm_io_bus_register_dev() was protected as far as I checked.

 - kvm_create_pit() was commented like "Caller must hold slots_lock"
   but kvm_free_pit() was not. So I don't know if I should protect
   the whole kvm_free_pit().

What is the best fix? -- or I misread something?

Thanks,
  Takuya

===
From: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>

The comment for kvm_io_bus_unregister_dev() says "Caller must hold
slots_lock" but some callers don't do so.

Though this patch fixes these, more consistent locking manner may be
needed in the future.

Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
---
 arch/x86/kvm/i8254.c |    2 ++
 arch/x86/kvm/i8259.c |    2 ++
 arch/x86/kvm/x86.c   |    2 ++
 virt/kvm/ioapic.c    |    2 ++
 4 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
index efad723..f64393c 100644
--- a/arch/x86/kvm/i8254.c
+++ b/arch/x86/kvm/i8254.c
@@ -744,9 +744,11 @@ void kvm_free_pit(struct kvm *kvm)
 	struct hrtimer *timer;
 
 	if (kvm->arch.vpit) {
+		mutex_lock(&kvm->slots_lock);
 		kvm_io_bus_unregister_dev(kvm, KVM_PIO_BUS, &kvm->arch.vpit->dev);
 		kvm_io_bus_unregister_dev(kvm, KVM_PIO_BUS,
 					      &kvm->arch.vpit->speaker_dev);
+		mutex_unlock(&kvm->slots_lock);
 		kvm_unregister_irq_mask_notifier(kvm, 0,
 					       &kvm->arch.vpit->mask_notifier);
 		kvm_unregister_irq_ack_notifier(kvm,
diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
index f628234..d9fe35d 100644
--- a/arch/x86/kvm/i8259.c
+++ b/arch/x86/kvm/i8259.c
@@ -596,7 +596,9 @@ void kvm_destroy_pic(struct kvm *kvm)
 	struct kvm_pic *vpic = kvm->arch.vpic;
 
 	if (vpic) {
+		mutex_lock(&kvm->slots_lock);
 		kvm_io_bus_unregister_dev(kvm, KVM_PIO_BUS, &vpic->dev);
+		mutex_unlock(&kvm->slots_lock);
 		kvm->arch.vpic = NULL;
 		kfree(vpic);
 	}
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index ed373ba..48e59d1 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3313,8 +3313,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;
 			}
diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
index 0b9df83..fa76380 100644
--- a/virt/kvm/ioapic.c
+++ b/virt/kvm/ioapic.c
@@ -409,7 +409,9 @@ void kvm_ioapic_destroy(struct kvm *kvm)
 	struct kvm_ioapic *ioapic = kvm->arch.vioapic;
 
 	if (ioapic) {
+		mutex_lock(&kvm->slots_lock);
 		kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS, &ioapic->dev);
+		mutex_unlock(&kvm->slots_lock);
 		kvm->arch.vioapic = NULL;
 		kfree(ioapic);
 	}
-- 
1.7.1

Re: [RFC PATCH] KVM: Fix missing lock for kvm_io_bus_unregister_dev()

[Takuya Yoshikawa]

(2010/12/08 1:45), Takuya Yoshikawa wrote:
> diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
> index f628234..d9fe35d 100644
> --- a/arch/x86/kvm/i8259.c
> +++ b/arch/x86/kvm/i8259.c
> @@ -596,7 +596,9 @@ void kvm_destroy_pic(struct kvm *kvm)
>   	struct kvm_pic *vpic = kvm->arch.vpic;
>
>   	if (vpic) {
> +		mutex_lock(&kvm->slots_lock);
>   		kvm_io_bus_unregister_dev(kvm, KVM_PIO_BUS,&vpic->dev);
> +		mutex_unlock(&kvm->slots_lock);
>   		kvm->arch.vpic = NULL;
>   		kfree(vpic);
>   	}

Ah, this seems to break the lock ordering

   kvm->lock --> kvm->slots_lock --> kvm->irq_lock

I'll recheck later.

   Takuya

Re: [RFC PATCH] KVM: Fix missing lock for kvm_io_bus_unregister_dev()

[Marcelo Tosatti]

On Wed, Dec 08, 2010 at 01:45:06AM +0900, Takuya Yoshikawa wrote:
> Memo:
>  - kvm_io_bus_register_dev() was protected as far as I checked.
> 
>  - kvm_create_pit() was commented like "Caller must hold slots_lock"
>    but kvm_free_pit() was not. So I don't know if I should protect
>    the whole kvm_free_pit().
> 
> What is the best fix? -- or I misread something?
> 
> Thanks,
>   Takuya
> 
> ===
> From: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
> 
> The comment for kvm_io_bus_unregister_dev() says "Caller must hold
> slots_lock" but some callers don't do so.
> 
> Though this patch fixes these, more consistent locking manner may be
> needed in the future.
> 
> Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
> ---
>  arch/x86/kvm/i8254.c |    2 ++
>  arch/x86/kvm/i8259.c |    2 ++
>  arch/x86/kvm/x86.c   |    2 ++
>  virt/kvm/ioapic.c    |    2 ++
>  4 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
> index efad723..f64393c 100644
> --- a/arch/x86/kvm/i8254.c
> +++ b/arch/x86/kvm/i8254.c
> @@ -744,9 +744,11 @@ void kvm_free_pit(struct kvm *kvm)
>  	struct hrtimer *timer;
>  
>  	if (kvm->arch.vpit) {
> +		mutex_lock(&kvm->slots_lock);
>  		kvm_io_bus_unregister_dev(kvm, KVM_PIO_BUS, &kvm->arch.vpit->dev);
>  		kvm_io_bus_unregister_dev(kvm, KVM_PIO_BUS,
>  					      &kvm->arch.vpit->speaker_dev);
> +		mutex_unlock(&kvm->slots_lock);
>  		kvm_unregister_irq_mask_notifier(kvm, 0,
>  					       &kvm->arch.vpit->mask_notifier);
>  		kvm_unregister_irq_ack_notifier(kvm,

This is supposedly safe because this is only called from vm destroy
context, when dropping the last reference.

> diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
> index f628234..d9fe35d 100644
> --- a/arch/x86/kvm/i8259.c
> +++ b/arch/x86/kvm/i8259.c
> @@ -596,7 +596,9 @@ void kvm_destroy_pic(struct kvm *kvm)
>  	struct kvm_pic *vpic = kvm->arch.vpic;
>  
>  	if (vpic) {
> +		mutex_lock(&kvm->slots_lock);
>  		kvm_io_bus_unregister_dev(kvm, KVM_PIO_BUS, &vpic->dev);
> +		mutex_unlock(&kvm->slots_lock);
>  		kvm->arch.vpic = NULL;
>  		kfree(vpic);
>  	}
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index ed373ba..48e59d1 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3313,8 +3313,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;
>  			}
> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
> index 0b9df83..fa76380 100644
> --- a/virt/kvm/ioapic.c
> +++ b/virt/kvm/ioapic.c
> @@ -409,7 +409,9 @@ void kvm_ioapic_destroy(struct kvm *kvm)
>  	struct kvm_ioapic *ioapic = kvm->arch.vioapic;
>  
>  	if (ioapic) {
> +		mutex_lock(&kvm->slots_lock);
>  		kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS, &ioapic->dev);
> +		mutex_unlock(&kvm->slots_lock);
>  		kvm->arch.vioapic = NULL;
>  		kfree(ioapic);
>  	}
> -- 
> 1.7.1

It seems the best way to fix is to move irq_lock and slots_lock
acquision from kvm_set_irq_routing/kvm_ioapic_destroy/kvm_destroy_pic to
their callers.

Re: [RFC PATCH] KVM: Fix missing lock for kvm_io_bus_unregister_dev()

[Takuya Yoshikawa]

Marcelo Tosatti <mtosatti@redhat.com> wrote:
> > --- a/arch/x86/kvm/i8254.c
> > +++ b/arch/x86/kvm/i8254.c
> > @@ -744,9 +744,11 @@ void kvm_free_pit(struct kvm *kvm)
> >  	struct hrtimer *timer;
> >  
> >  	if (kvm->arch.vpit) {
> > +		mutex_lock(&kvm->slots_lock);
> >  		kvm_io_bus_unregister_dev(kvm, KVM_PIO_BUS, &kvm->arch.vpit->dev);
> >  		kvm_io_bus_unregister_dev(kvm, KVM_PIO_BUS,
> >  					      &kvm->arch.vpit->speaker_dev);
> > +		mutex_unlock(&kvm->slots_lock);
> >  		kvm_unregister_irq_mask_notifier(kvm, 0,
> >  					       &kvm->arch.vpit->mask_notifier);
> >  		kvm_unregister_irq_ack_notifier(kvm,
> 
> This is supposedly safe because this is only called from vm destroy
> context, when dropping the last reference.
> 

I see. Drop this from the next version.

> 
> It seems the best way to fix is to move irq_lock and slots_lock
> acquision from kvm_set_irq_routing/kvm_ioapic_destroy/kvm_destroy_pic to
> their callers.
> 

Then, the lock acquisition for registeration and unregisteration may be
centralized to vm_ioctl.

I'm now checking these to reformat the patch.

Thanks,
  Takuya