Re: kvm: deadlock in kvm_vgic_map_resources

[Marc Zyngier]

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?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...