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@xxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxx>
> ---
> 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.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
