Re: kvm: deadlock in kvm_vgic_map_resources

[Christoffer Dall]

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@xxxxxxx>
> >>>> 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@xxxxxxx>
> >> 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@xxxxxxxxxxxxxxx
> >> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
> >> ---
> >>  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