kvm: deadlock in kvm_vgic_map_resources

Message ID 20170112104253.GB13547@cbox
State New
Headers show

Commit Message

Christoffer Dall Jan. 12, 2017, 10:42 a.m.
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):



Thanks,
-Christoffer

Comments

Marc Zyngier Jan. 12, 2017, 10:50 a.m. | #1
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.
-- 
Jazz is not dead. It just smells funny...
Andre Przywara Jan. 12, 2017, 10:51 a.m. | #2
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

>
Christoffer Dall Jan. 12, 2017, 3:06 p.m. | #3
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

Patch hide | download patch | download mbox

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);