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

[Marcelo Tosatti <mtosatti@redhat.com>]

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.