lkml.org 
[lkml]   [2017]   [Jan]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: kvm: deadlock in kvm_vgic_map_resources
    On Thu, Jan 12, 2017 at 10:30:39AM +0000, Marc Zyngier wrote:
    > On 12/01/17 09:55, Andre Przywara wrote:
    > > Hi,
    > >
    > > On 12/01/17 09:32, Marc Zyngier wrote:
    > >> Hi Dmitry,
    > >>
    > >> On 11/01/17 19:01, Dmitry Vyukov wrote:
    > >>> Hello,
    > >>>
    > >>> While running syzkaller fuzzer I've got the following deadlock.
    > >>> On commit 9c763584b7c8911106bb77af7e648bef09af9d80.
    > >>>
    > >>>
    > >>> =============================================
    > >>> [ INFO: possible recursive locking detected ]
    > >>> 4.9.0-rc6-xc2-00056-g08372dd4b91d-dirty #50 Not tainted
    > >>> ---------------------------------------------
    > >>> syz-executor/20805 is trying to acquire lock:
    > >>> (
    > >>> &kvm->lock
    > >>> ){+.+.+.}
    > >>> , at:
    > >>> [< inline >] kvm_vgic_dist_destroy
    > >>> arch/arm64/kvm/../../../virt/kvm/arm/vgic/vgic-init.c:271
    > >>> [<ffff2000080ea4bc>] kvm_vgic_destroy+0x34/0x250
    > >>> arch/arm64/kvm/../../../virt/kvm/arm/vgic/vgic-init.c:294
    > >>> but task is already holding lock:
    > >>> (&kvm->lock){+.+.+.}, at:
    > >>> [<ffff2000080ea7e4>] kvm_vgic_map_resources+0x2c/0x108
    > >>> arch/arm64/kvm/../../../virt/kvm/arm/vgic/vgic-init.c:343
    > >>> other info that might help us debug this:
    > >>> Possible unsafe locking scenario:
    > >>> CPU0
    > >>> ----
    > >>> lock(&kvm->lock);
    > >>> lock(&kvm->lock);
    > >>> *** DEADLOCK ***
    > >>> May be due to missing lock nesting notation
    > >>> 2 locks held by syz-executor/20805:
    > >>> #0:(&vcpu->mutex){+.+.+.}, at:
    > >>> [<ffff2000080bcc30>] vcpu_load+0x28/0x1d0
    > >>> arch/arm64/kvm/../../../virt/kvm/kvm_main.c:143
    > >>> #1:(&kvm->lock){+.+.+.}, at:
    > >>> [<ffff2000080ea7e4>] kvm_vgic_map_resources+0x2c/0x108
    > >>> arch/arm64/kvm/../../../virt/kvm/arm/vgic/vgic-init.c:343
    > >>> stack backtrace:
    > >>> CPU: 2 PID: 20805 Comm: syz-executor Not tainted
    > >>> 4.9.0-rc6-xc2-00056-g08372dd4b91d-dirty #50
    > >>> Hardware name: Hardkernel ODROID-C2 (DT)
    > >>> Call trace:
    > >>> [<ffff200008090560>] dump_backtrace+0x0/0x3c8 arch/arm64/kernel/traps.c:69
    > >>> [<ffff200008090948>] show_stack+0x20/0x30 arch/arm64/kernel/traps.c:219
    > >>> [< inline >] __dump_stack lib/dump_stack.c:15
    > >>> [<ffff200008895840>] dump_stack+0x100/0x150 lib/dump_stack.c:51
    > >>> [< inline >] print_deadlock_bug kernel/locking/lockdep.c:1728
    > >>> [< inline >] check_deadlock kernel/locking/lockdep.c:1772
    > >>> [< inline >] validate_chain kernel/locking/lockdep.c:2250
    > >>> [<ffff2000081c8718>] __lock_acquire+0x1938/0x3440 kernel/locking/lockdep.c:3335
    > >>> [<ffff2000081caa84>] lock_acquire+0xdc/0x1d8 kernel/locking/lockdep.c:3746
    > >>> [< inline >] __mutex_lock_common kernel/locking/mutex.c:521
    > >>> [<ffff200009700004>] mutex_lock_nested+0xdc/0x7b8 kernel/locking/mutex.c:621
    > >>> [< inline >] kvm_vgic_dist_destroy
    > >>> arch/arm64/kvm/../../../virt/kvm/arm/vgic/vgic-init.c:271
    > >>> [<ffff2000080ea4bc>] kvm_vgic_destroy+0x34/0x250
    > >>> arch/arm64/kvm/../../../virt/kvm/arm/vgic/vgic-init.c:294
    > >>> [<ffff2000080ec290>] vgic_v2_map_resources+0x218/0x430
    > >>> arch/arm64/kvm/../../../virt/kvm/arm/vgic/vgic-v2.c:295
    > >>> [<ffff2000080ea884>] kvm_vgic_map_resources+0xcc/0x108
    > >>> arch/arm64/kvm/../../../virt/kvm/arm/vgic/vgic-init.c:348
    > >>> [< inline >] kvm_vcpu_first_run_init
    > >>> arch/arm64/kvm/../../../arch/arm/kvm/arm.c:505
    > >>> [<ffff2000080d2768>] kvm_arch_vcpu_ioctl_run+0xab8/0xce0
    > >>> arch/arm64/kvm/../../../arch/arm/kvm/arm.c:591
    > >>> [<ffff2000080c1fec>] kvm_vcpu_ioctl+0x434/0xc08
    > >>> arch/arm64/kvm/../../../virt/kvm/kvm_main.c:2557
    > >>> [< inline >] vfs_ioctl fs/ioctl.c:43
    > >>> [<ffff200008450c38>] do_vfs_ioctl+0x128/0xfc0 fs/ioctl.c:679
    > >>> [< inline >] SYSC_ioctl fs/ioctl.c:694
    > >>> [<ffff200008451b78>] SyS_ioctl+0xa8/0xb8 fs/ioctl.c:685
    > >>> [<ffff200008083ef0>] el0_svc_naked+0x24/0x28 arch/arm64/kernel/entry.S:755
    > >>
    > >> Nice catch, and many thanks for reporting this.
    > >>
    > >> The bug is fairly obvious. Christoffer, what do you think? I don't think
    > >> we need to hold the kvm->lock all the way, but I'd like another pair of
    > >> eyes (the coffee machine is out of order again, and tea doesn't cut it).
    > >>
    > >> Thanks,
    > >>
    > >> M.
    > >>
    > >> From 93f80b20fb9351a49ee8b74eed3fc59c84651371 Mon Sep 17 00:00:00 2001
    > >> From: Marc Zyngier <marc.zyngier@arm.com>
    > >> Date: Thu, 12 Jan 2017 09:21:56 +0000
    > >> Subject: [PATCH] KVM: arm/arm64: vgic: Fix deadlock on error handling
    > >>
    > >> Dmitry Vyukov reported that the syzkaller fuzzer triggered a
    > >> deadlock in the vgic setup code when an error was detected, as
    > >> the cleanup code tries to take a lock that is already held by
    > >> the setup code.
    > >>
    > >> The fix is pretty obvious: move the cleaup call after having
    > >> dropped the lock, since not much can happen at that point.
    > > ^^^^^^^^
    > > Is that really true? If for instance the calls to
    > > vgic_register_dist_iodev() or kvm_phys_addr_ioremap() in
    > > vgic_v2_map_resources() fail, we leave the function with a half
    > > initialized VGIC (because vgic_init() succeeded).
    >
    > But we only set dist->ready to true when everything went OK. How is
    > that an issue?
    >
    > > Dropping the lock at
    > > this point without having the GIC cleaned up before sounds a bit
    > > suspicious (I may be wrong on this, though).
    >
    > Thinking of it, that may open a race with vgic init call, leading to
    > leaking distributor memory.
    >
    > >
    > > Can't we just document that kvm_vgic_destroy() needs to be called with
    > > the kvm->lock held and take the lock around the only other caller
    > > (kvm_arch_destroy_vm() in arch/arm/kvm/arm.c)?
    > > We can then keep holding the lock in the map_resources calls.
    > > Though we might still move the calls to kvm_vgic_destroy() into the
    > > wrapper function as a cleanup (as shown below), just before dropping the
    > > lock.
    >
    > I'd rather keep the changes limited to the vgic code, and save myself
    > having to document more locking (we already have our fair share here).
    > How about this (untested):
    >
    > From 24dc3f5750da20d89e0ce9b7855d125d0100bee8 Mon Sep 17 00:00:00 2001
    > From: Marc Zyngier <marc.zyngier@arm.com>
    > Date: Thu, 12 Jan 2017 09:21:56 +0000
    > Subject: [PATCH] KVM: arm/arm64: vgic: Fix deadlock on error handling
    >
    > Dmitry Vyukov reported that the syzkaller fuzzer triggered a
    > deadlock in the vgic setup code when an error was detected, as
    > the cleanup code tries to take a lock that is already held by
    > the setup code.
    >
    > The fix is to avoid retaking the lock when cleaning up, by
    > telling the cleanup function that we already hold it.
    >
    > Cc: stable@vger.kernel.org
    > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
    > ---
    > virt/kvm/arm/vgic/vgic-init.c | 21 ++++++++++++++++-----
    > virt/kvm/arm/vgic/vgic-v2.c | 2 --
    > virt/kvm/arm/vgic/vgic-v3.c | 2 --
    > 3 files changed, 16 insertions(+), 9 deletions(-)
    >
    > diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
    > index 5114391..30d74e2 100644
    > --- a/virt/kvm/arm/vgic/vgic-init.c
    > +++ b/virt/kvm/arm/vgic/vgic-init.c
    > @@ -264,11 +264,12 @@ int vgic_init(struct kvm *kvm)
    > return ret;
    > }
    >
    > -static void kvm_vgic_dist_destroy(struct kvm *kvm)
    > +static void kvm_vgic_dist_destroy(struct kvm *kvm, bool locked)
    > {
    > struct vgic_dist *dist = &kvm->arch.vgic;
    >
    > - mutex_lock(&kvm->lock);
    > + if (!locked)
    > + mutex_lock(&kvm->lock);

    Hmm, not a fan of passing this variable around. How about this instead
    then (untested):

    diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
    index 5114391..a25806b 100644
    --- a/virt/kvm/arm/vgic/vgic-init.c
    +++ b/virt/kvm/arm/vgic/vgic-init.c
    @@ -264,19 +264,16 @@ int vgic_init(struct kvm *kvm)
    return ret;
    }

    +/* Must be called with the kvm->lock held */
    static void kvm_vgic_dist_destroy(struct kvm *kvm)
    {
    struct vgic_dist *dist = &kvm->arch.vgic;

    - mutex_lock(&kvm->lock);
    -
    dist->ready = false;
    dist->initialized = false;

    kfree(dist->spis);
    dist->nr_spis = 0;
    -
    - mutex_unlock(&kvm->lock);
    }

    void kvm_vgic_vcpu_destroy(struct kvm_vcpu *vcpu)
    @@ -286,7 +283,7 @@ void kvm_vgic_vcpu_destroy(struct kvm_vcpu *vcpu)
    INIT_LIST_HEAD(&vgic_cpu->ap_list_head);
    }

    -void kvm_vgic_destroy(struct kvm *kvm)
    +void __kvm_vgic_destroy(struct kvm *kvm)
    {
    struct kvm_vcpu *vcpu;
    int i;
    @@ -297,6 +294,13 @@ void kvm_vgic_destroy(struct kvm *kvm)
    kvm_vgic_vcpu_destroy(vcpu);
    }

    +void kvm_vgic_destroy(struct kvm *kvm)
    +{
    + mutex_lock(&kvm->lock);
    + __kvm_vgic_destroy(kvm);
    + mutex_unlock(&kvm->lock);
    +}
    +
    /**
    * vgic_lazy_init: Lazy init is only allowed if the GIC exposed to the guest
    * is a GICv2. A GICv3 must be explicitly initialized by the guest using the
    diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c
    index 9bab867..c6f7ec7 100644
    --- a/virt/kvm/arm/vgic/vgic-v2.c
    +++ b/virt/kvm/arm/vgic/vgic-v2.c
    @@ -294,7 +294,7 @@ int vgic_v2_map_resources(struct kvm *kvm)

    out:
    if (ret)
    - kvm_vgic_destroy(kvm);
    + __kvm_vgic_destroy(kvm);
    return ret;
    }

    diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
    index 5c9f974..f1c7819 100644
    --- a/virt/kvm/arm/vgic/vgic-v3.c
    +++ b/virt/kvm/arm/vgic/vgic-v3.c
    @@ -303,7 +303,7 @@ int vgic_v3_map_resources(struct kvm *kvm)

    out:
    if (ret)
    - kvm_vgic_destroy(kvm);
    + __kvm_vgic_destroy(kvm);
    return ret;
    }

    diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
    index 859f65c..74a0bbb 100644
    --- a/virt/kvm/arm/vgic/vgic.h
    +++ b/virt/kvm/arm/vgic/vgic.h
    @@ -37,6 +37,8 @@ struct vgic_vmcr {
    u32 pmr;
    };

    +void __kvm_vgic_destroy(struct kvm *kvm);
    +
    struct vgic_irq *vgic_get_irq(struct kvm *kvm, struct kvm_vcpu *vcpu,
    u32 intid);
    void vgic_put_irq(struct kvm *kvm, struct vgic_irq *irq);

    Thanks,
    -Christoffer

    \
     
     \ /
      Last update: 2017-01-12 11:43    [W:0.040 / U:40.772 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site