kvm: deadlock in kvm_vgic_map_resources

[Dmitry Vyukov]

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


INFO: task syz-executor:20805 blocked for more than 120 seconds.
Not tainted 4.9.0-rc6-xc2-00056-g08372dd4b91d-dirty #50
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
syz-executor D 0 20805 1 0x00000001
Call trace:
[<ffff2000080894f4>] __switch_to+0x184/0x258 arch/arm64/kernel/process.c:345
[< inline >] context_switch kernel/sched/core.c:2899
[<ffff2000096fcc64>] __schedule+0x42c/0x1298 kernel/sched/core.c:3402
[<ffff2000096fdb98>] schedule+0xc8/0x260 kernel/sched/core.c:3457
[<ffff2000096fe654>] schedule_preempt_disabled+0x74/0x110
kernel/sched/core.c:3490
[< inline >] __mutex_lock_common kernel/locking/mutex.c:582
[<ffff200009700240>] mutex_lock_nested+0x318/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

[Marc Zyngier]

Hi Dmitry,

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.z...@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.

Cc: sta...@vger.kernel.org
Signed-off-by: Marc Zyngier <marc.z...@arm.com>
---
virt/kvm/arm/vgic/vgic-init.c | 4 ++++
virt/kvm/arm/vgic/vgic-v2.c | 2 --
virt/kvm/arm/vgic/vgic-v3.c | 2 --
3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
index 5114391..0e0c295 100644
--- a/virt/kvm/arm/vgic/vgic-init.c
+++ b/virt/kvm/arm/vgic/vgic-init.c
@@ -350,6 +350,10 @@ int kvm_vgic_map_resources(struct kvm *kvm)
ret = vgic_v3_map_resources(kvm);
out:
mutex_unlock(&kvm->lock);
+
+ if (ret)
+ kvm_vgic_destroy(kvm);
+
return ret;
}

diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c
index 9bab867..834137e 100644
--- a/virt/kvm/arm/vgic/vgic-v2.c
+++ b/virt/kvm/arm/vgic/vgic-v2.c
@@ -293,8 +293,6 @@ int vgic_v2_map_resources(struct kvm *kvm)
dist->ready = true;

out:
- if (ret)
- 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 7df1b90..a4c7fff 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c
@@ -308,8 +308,6 @@ int vgic_v3_map_resources(struct kvm *kvm)
dist->ready = true;

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

--
2.1.4


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

[Andre Przywara]

Hi,

^^^^^^^^
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). Dropping the lock at
this point without having the GIC cleaned up before sounds a bit
suspicious (I may be wrong on this, though).

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.

Cheers,
Andre.

[Christoffer Dall]

I was going to suggest the same. I cannot really see through all the
possible interactions, but I feel safer doing that.

Thanks,
-Christoffer

[Marc Zyngier]

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.z...@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: sta...@vger.kernel.org
Signed-off-by: Marc Zyngier <marc.z...@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);

dist->ready = false;
dist->initialized = false;
@@ -276,7 +277,8 @@ static void kvm_vgic_dist_destroy(struct kvm *kvm)
kfree(dist->spis);
dist->nr_spis = 0;

- mutex_unlock(&kvm->lock);
+ if (!locked)
+ mutex_unlock(&kvm->lock);
}

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

-void kvm_vgic_destroy(struct kvm *kvm)
+static void kvm_vgic_destroy_locked(struct kvm *kvm, bool locked)
{
struct kvm_vcpu *vcpu;
int i;

- kvm_vgic_dist_destroy(kvm);
+ kvm_vgic_dist_destroy(kvm, locked);

kvm_for_each_vcpu(i, vcpu, kvm)
kvm_vgic_vcpu_destroy(vcpu);
}

+void kvm_vgic_destroy(struct kvm *kvm)
+{
+ kvm_vgic_destroy_locked(kvm, false);
+}
+
/**
* 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
@@ -348,6 +355,10 @@ int kvm_vgic_map_resources(struct kvm *kvm)
ret = vgic_v2_map_resources(kvm);
else
ret = vgic_v3_map_resources(kvm);
+
+ if (ret)
+ kvm_vgic_destroy_locked(kvm, true);
+
out:
mutex_unlock(&kvm->lock);

return ret;
diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c
index 9bab867..834137e 100644
--- a/virt/kvm/arm/vgic/vgic-v2.c
+++ b/virt/kvm/arm/vgic/vgic-v2.c
@@ -293,8 +293,6 @@ int vgic_v2_map_resources(struct kvm *kvm)
dist->ready = true;

out:
- if (ret)
- 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 7df1b90..a4c7fff 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c
@@ -308,8 +308,6 @@ int vgic_v3_map_resources(struct kvm *kvm)
dist->ready = true;

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

[Christoffer Dall]

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

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

+/* Must be called with the kvm->lock held */

static void kvm_vgic_dist_destroy(struct kvm *kvm)

{
struct vgic_dist *dist = &kvm->arch.vgic;

- mutex_lock(&kvm->lock);

-

dist->ready = false;
dist->initialized = false;

kfree(dist->spis);
dist->nr_spis = 0;
-

- mutex_unlock(&kvm->lock);
}

void kvm_vgic_vcpu_destroy(struct kvm_vcpu *vcpu)
@@ -286,7 +283,7 @@ void kvm_vgic_vcpu_destroy(struct kvm_vcpu *vcpu)

INIT_LIST_HEAD(&vgic_cpu->ap_list_head);
}

-void kvm_vgic_destroy(struct kvm *kvm)

+void __kvm_vgic_destroy(struct kvm *kvm)

{
struct kvm_vcpu *vcpu;
int i;

@@ -297,6 +294,13 @@ void kvm_vgic_destroy(struct kvm *kvm)

kvm_vgic_vcpu_destroy(vcpu);
}

+void kvm_vgic_destroy(struct kvm *kvm)
+{

+ mutex_lock(&kvm->lock);
+ __kvm_vgic_destroy(kvm);
+ mutex_unlock(&kvm->lock);

+}
+
/**
* vgic_lazy_init: Lazy init is only allowed if the GIC exposed to the guest
* is a GICv2. A GICv3 must be explicitly initialized by the guest using the

diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c
index 9bab867..c6f7ec7 100644
--- a/virt/kvm/arm/vgic/vgic-v2.c
+++ b/virt/kvm/arm/vgic/vgic-v2.c
@@ -294,7 +294,7 @@ int vgic_v2_map_resources(struct kvm *kvm)

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

return ret;
}

diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c

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

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

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

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


Thanks,
-Christoffer

[Andre Przywara]

Hi,

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.

[Marc Zyngier]

On 12/01/17 10:42, Christoffer Dall wrote:

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.

[Christoffer Dall]

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