| Message ID | 20170112104253.GB13547@cbox (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12/01/17 10:42, Christoffer Dall wrote: > 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); > +} > + I initially wrote that exactly, but ended up deciding against as it changes the locking more than strictly necessary. On the other hand, I think this looks better, so if everyone agrees I'll take that. > /** > * 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); I'm still keen on factoring the destroy calls in the calling function. Is there any reason why we wouldn't do it? Thanks, M.
Hi, On 12/01/17 10:42, Christoffer Dall wrote: > 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): Yes, I like that version more. And if we now move the calls to __kvm_vgic_destroy() into vgic-init.c (as in Marc's first patch, but just before dropping the lock), we don't even need to export __kvm_vgic_destroy(), right? Cheers, Andre. > 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 >
On Thu, Jan 12, 2017 at 10:50:04AM +0000, Marc Zyngier wrote: > On 12/01/17 10:42, Christoffer Dall wrote: > > 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); > > +} > > + > > I initially wrote that exactly, but ended up deciding against as it > changes the locking more than strictly necessary. On the other hand, I > think this looks better, so if everyone agrees I'll take that. > > > /** > > * 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); > > I'm still keen on factoring the destroy calls in the calling function. > Is there any reason why we wouldn't do it? > I was very slightly biased to not do it, because I feel like it was clear that the scary function that does a lot of work cleans up nicely after itself in case of failure with the current code, but I'm not married to either approach, so whatever you prefer. Thanks, -Christoffer
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);