* [PATCH 0/2] KVM: arm/arm64: vgic-its: Guest memory access fixes
@ 2019-03-19 13:30 Marc Zyngier
2019-03-19 13:30 ` [PATCH 1/2] KVM: arm/arm64: vgic-its: Take the srcu lock when writing to guest memory Marc Zyngier
2019-03-19 13:30 ` [PATCH 2/2] KVM: arm/arm64: vgic-its: Take the srcu lock when parsing the memslots Marc Zyngier
0 siblings, 2 replies; 6+ messages in thread
From: Marc Zyngier @ 2019-03-19 13:30 UTC (permalink / raw)
To: linux-arm-kernel, kvmarm, kvm; +Cc: Andre Przywara
While messing with something completely different, I managed to
trigger a number of RCU warnings, due to the lack of srcu locking
in the ITS code.
Andre partially addressed those last year[1], but there was some
more left.
[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2018-May/577056.html
Marc Zyngier (2):
KVM: arm/arm64: vgic-its: Take the srcu lock when writing to guest
memory
KVM: arm/arm64: vgic-its: Take the srcu lock when parsing the memslots
arch/arm/include/asm/kvm_mmu.h | 11 +++++++++++
arch/arm64/include/asm/kvm_mmu.h | 11 +++++++++++
virt/kvm/arm/vgic/vgic-its.c | 21 ++++++++++++++-------
3 files changed, 36 insertions(+), 7 deletions(-)
--
2.20.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] KVM: arm/arm64: vgic-its: Take the srcu lock when writing to guest memory
2019-03-19 13:30 [PATCH 0/2] KVM: arm/arm64: vgic-its: Guest memory access fixes Marc Zyngier
@ 2019-03-19 13:30 ` Marc Zyngier
2019-03-19 15:16 ` Auger Eric
2019-03-19 13:30 ` [PATCH 2/2] KVM: arm/arm64: vgic-its: Take the srcu lock when parsing the memslots Marc Zyngier
1 sibling, 1 reply; 6+ messages in thread
From: Marc Zyngier @ 2019-03-19 13:30 UTC (permalink / raw)
To: linux-arm-kernel, kvmarm, kvm; +Cc: Andre Przywara
When halting a guest, QEMU flushes the virtual ITS caches, which
amounts to writing to the various tables that the guest has allocated.
When doing this, we fail to take the srcu lock, and the kernel
shouts loudly if running a lockdep kernel:
[ 69.680416] =============================
[ 69.680819] WARNING: suspicious RCU usage
[ 69.681526] 5.1.0-rc1-00008-g600025238f51-dirty #18 Not tainted
[ 69.682096] -----------------------------
[ 69.682501] ./include/linux/kvm_host.h:605 suspicious rcu_dereference_check() usage!
[ 69.683225]
[ 69.683225] other info that might help us debug this:
[ 69.683225]
[ 69.683975]
[ 69.683975] rcu_scheduler_active = 2, debug_locks = 1
[ 69.684598] 6 locks held by qemu-system-aar/4097:
[ 69.685059] #0: 0000000034196013 (&kvm->lock){+.+.}, at: vgic_its_set_attr+0x244/0x3a0
[ 69.686087] #1: 00000000f2ed935e (&its->its_lock){+.+.}, at: vgic_its_set_attr+0x250/0x3a0
[ 69.686919] #2: 000000005e71ea54 (&vcpu->mutex){+.+.}, at: lock_all_vcpus+0x64/0xd0
[ 69.687698] #3: 00000000c17e548d (&vcpu->mutex){+.+.}, at: lock_all_vcpus+0x64/0xd0
[ 69.688475] #4: 00000000ba386017 (&vcpu->mutex){+.+.}, at: lock_all_vcpus+0x64/0xd0
[ 69.689978] #5: 00000000c2c3c335 (&vcpu->mutex){+.+.}, at: lock_all_vcpus+0x64/0xd0
[ 69.690729]
[ 69.690729] stack backtrace:
[ 69.691151] CPU: 2 PID: 4097 Comm: qemu-system-aar Not tainted 5.1.0-rc1-00008-g600025238f51-dirty #18
[ 69.691984] Hardware name: rockchip evb_rk3399/evb_rk3399, BIOS 2019.04-rc3-00124-g2feec69fb1 03/15/2019
[ 69.692831] Call trace:
[ 69.694072] lockdep_rcu_suspicious+0xcc/0x110
[ 69.694490] gfn_to_memslot+0x174/0x190
[ 69.694853] kvm_write_guest+0x50/0xb0
[ 69.695209] vgic_its_save_tables_v0+0x248/0x330
[ 69.695639] vgic_its_set_attr+0x298/0x3a0
[ 69.696024] kvm_device_ioctl_attr+0x9c/0xd8
[ 69.696424] kvm_device_ioctl+0x8c/0xf8
[ 69.696788] do_vfs_ioctl+0xc8/0x960
[ 69.697128] ksys_ioctl+0x8c/0xa0
[ 69.697445] __arm64_sys_ioctl+0x28/0x38
[ 69.697817] el0_svc_common+0xd8/0x138
[ 69.698173] el0_svc_handler+0x38/0x78
[ 69.698528] el0_svc+0x8/0xc
The fix is to obviously take the srcu lock, just like we do on the
read side of things since bf308242ab98. One wonders why this wasn't
fixed at the same time, but hey...
Fixes: bf308242ab98 ("KVM: arm/arm64: VGIC/ITS: protect kvm_read_guest() calls with SRCU lock")
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
arch/arm/include/asm/kvm_mmu.h | 11 +++++++++++
arch/arm64/include/asm/kvm_mmu.h | 11 +++++++++++
virt/kvm/arm/vgic/vgic-its.c | 8 ++++----
3 files changed, 26 insertions(+), 4 deletions(-)
diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
index 2de96a180166..31de4ab93005 100644
--- a/arch/arm/include/asm/kvm_mmu.h
+++ b/arch/arm/include/asm/kvm_mmu.h
@@ -381,6 +381,17 @@ static inline int kvm_read_guest_lock(struct kvm *kvm,
return ret;
}
+static inline int kvm_write_guest_lock(struct kvm *kvm, gpa_t gpa,
+ const void *data, unsigned long len)
+{
+ int srcu_idx = srcu_read_lock(&kvm->srcu);
+ int ret = kvm_write_guest(kvm, gpa, data, len);
+
+ srcu_read_unlock(&kvm->srcu, srcu_idx);
+
+ return ret;
+}
+
static inline void *kvm_get_hyp_vector(void)
{
switch(read_cpuid_part()) {
diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
index b0742a16c6c9..ebeefcf835e8 100644
--- a/arch/arm64/include/asm/kvm_mmu.h
+++ b/arch/arm64/include/asm/kvm_mmu.h
@@ -445,6 +445,17 @@ static inline int kvm_read_guest_lock(struct kvm *kvm,
return ret;
}
+static inline int kvm_write_guest_lock(struct kvm *kvm, gpa_t gpa,
+ const void *data, unsigned long len)
+{
+ int srcu_idx = srcu_read_lock(&kvm->srcu);
+ int ret = kvm_write_guest(kvm, gpa, data, len);
+
+ srcu_read_unlock(&kvm->srcu, srcu_idx);
+
+ return ret;
+}
+
#ifdef CONFIG_KVM_INDIRECT_VECTORS
/*
* EL2 vectors can be mapped and rerouted in a number of ways,
diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index a60a768d9258..bb76220f2f30 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -2058,7 +2058,7 @@ static int vgic_its_save_ite(struct vgic_its *its, struct its_device *dev,
((u64)ite->irq->intid << KVM_ITS_ITE_PINTID_SHIFT) |
ite->collection->collection_id;
val = cpu_to_le64(val);
- return kvm_write_guest(kvm, gpa, &val, ite_esz);
+ return kvm_write_guest_lock(kvm, gpa, &val, ite_esz);
}
/**
@@ -2205,7 +2205,7 @@ static int vgic_its_save_dte(struct vgic_its *its, struct its_device *dev,
(itt_addr_field << KVM_ITS_DTE_ITTADDR_SHIFT) |
(dev->num_eventid_bits - 1));
val = cpu_to_le64(val);
- return kvm_write_guest(kvm, ptr, &val, dte_esz);
+ return kvm_write_guest_lock(kvm, ptr, &val, dte_esz);
}
/**
@@ -2385,7 +2385,7 @@ static int vgic_its_save_cte(struct vgic_its *its,
((u64)collection->target_addr << KVM_ITS_CTE_RDBASE_SHIFT) |
collection->collection_id);
val = cpu_to_le64(val);
- return kvm_write_guest(its->dev->kvm, gpa, &val, esz);
+ return kvm_write_guest_lock(its->dev->kvm, gpa, &val, esz);
}
static int vgic_its_restore_cte(struct vgic_its *its, gpa_t gpa, int esz)
@@ -2456,7 +2456,7 @@ static int vgic_its_save_collection_table(struct vgic_its *its)
*/
val = 0;
BUG_ON(cte_esz > sizeof(val));
- ret = kvm_write_guest(its->dev->kvm, gpa, &val, cte_esz);
+ ret = kvm_write_guest_lock(its->dev->kvm, gpa, &val, cte_esz);
return ret;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] KVM: arm/arm64: vgic-its: Take the srcu lock when parsing the memslots
2019-03-19 13:30 [PATCH 0/2] KVM: arm/arm64: vgic-its: Guest memory access fixes Marc Zyngier
2019-03-19 13:30 ` [PATCH 1/2] KVM: arm/arm64: vgic-its: Take the srcu lock when writing to guest memory Marc Zyngier
@ 2019-03-19 13:30 ` Marc Zyngier
2019-03-19 15:17 ` Auger Eric
1 sibling, 1 reply; 6+ messages in thread
From: Marc Zyngier @ 2019-03-19 13:30 UTC (permalink / raw)
To: linux-arm-kernel, kvmarm, kvm; +Cc: Andre Przywara
Calling kvm_is_visible_gfn() implies that we're parsing the memslots,
and doing this without the srcu lock is frown upon:
[12704.164532] =============================
[12704.164544] WARNING: suspicious RCU usage
[12704.164560] 5.1.0-rc1-00008-g600025238f51-dirty #16 Tainted: G W
[12704.164573] -----------------------------
[12704.164589] ./include/linux/kvm_host.h:605 suspicious rcu_dereference_check() usage!
[12704.164602] other info that might help us debug this:
[12704.164616] rcu_scheduler_active = 2, debug_locks = 1
[12704.164631] 6 locks held by qemu-system-aar/13968:
[12704.164644] #0: 000000007ebdae4f (&kvm->lock){+.+.}, at: vgic_its_set_attr+0x244/0x3a0
[12704.164691] #1: 000000007d751022 (&its->its_lock){+.+.}, at: vgic_its_set_attr+0x250/0x3a0
[12704.164726] #2: 00000000219d2706 (&vcpu->mutex){+.+.}, at: lock_all_vcpus+0x64/0xd0
[12704.164761] #3: 00000000a760aecd (&vcpu->mutex){+.+.}, at: lock_all_vcpus+0x64/0xd0
[12704.164794] #4: 000000000ef8e31d (&vcpu->mutex){+.+.}, at: lock_all_vcpus+0x64/0xd0
[12704.164827] #5: 000000007a872093 (&vcpu->mutex){+.+.}, at: lock_all_vcpus+0x64/0xd0
[12704.164861] stack backtrace:
[12704.164878] CPU: 2 PID: 13968 Comm: qemu-system-aar Tainted: G W 5.1.0-rc1-00008-g600025238f51-dirty #16
[12704.164887] Hardware name: rockchip evb_rk3399/evb_rk3399, BIOS 2019.04-rc3-00124-g2feec69fb1 03/15/2019
[12704.164896] Call trace:
[12704.164910] dump_backtrace+0x0/0x138
[12704.164920] show_stack+0x24/0x30
[12704.164934] dump_stack+0xbc/0x104
[12704.164946] lockdep_rcu_suspicious+0xcc/0x110
[12704.164958] gfn_to_memslot+0x174/0x190
[12704.164969] kvm_is_visible_gfn+0x28/0x70
[12704.164980] vgic_its_check_id.isra.0+0xec/0x1e8
[12704.164991] vgic_its_save_tables_v0+0x1ac/0x330
[12704.165001] vgic_its_set_attr+0x298/0x3a0
[12704.165012] kvm_device_ioctl_attr+0x9c/0xd8
[12704.165022] kvm_device_ioctl+0x8c/0xf8
[12704.165035] do_vfs_ioctl+0xc8/0x960
[12704.165045] ksys_ioctl+0x8c/0xa0
[12704.165055] __arm64_sys_ioctl+0x28/0x38
[12704.165067] el0_svc_common+0xd8/0x138
[12704.165078] el0_svc_handler+0x38/0x78
[12704.165089] el0_svc+0x8/0xc
Make sure the lock is taken when doing this.
Fixes: bf308242ab98 ("KVM: arm/arm64: VGIC/ITS: protect kvm_read_guest() calls with SRCU lock")
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
virt/kvm/arm/vgic/vgic-its.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index bb76220f2f30..7543f166943d 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -880,8 +880,9 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
u64 indirect_ptr, type = GITS_BASER_TYPE(baser);
phys_addr_t base = GITS_BASER_ADDR_48_to_52(baser);
int esz = GITS_BASER_ENTRY_SIZE(baser);
- int index;
+ int index, idx;
gfn_t gfn;
+ bool ret;
switch (type) {
case GITS_BASER_TYPE_DEVICE:
@@ -908,7 +909,8 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
if (eaddr)
*eaddr = addr;
- return kvm_is_visible_gfn(its->dev->kvm, gfn);
+
+ goto out;
}
/* calculate and check the index into the 1st level */
@@ -938,7 +940,12 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
if (eaddr)
*eaddr = indirect_ptr;
- return kvm_is_visible_gfn(its->dev->kvm, gfn);
+
+out:
+ idx = srcu_read_lock(&its->dev->kvm->srcu);
+ ret = kvm_is_visible_gfn(its->dev->kvm, gfn);
+ srcu_read_unlock(&its->dev->kvm->srcu, idx);
+ return ret;
}
static int vgic_its_alloc_collection(struct vgic_its *its,
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] KVM: arm/arm64: vgic-its: Take the srcu lock when writing to guest memory
2019-03-19 13:30 ` [PATCH 1/2] KVM: arm/arm64: vgic-its: Take the srcu lock when writing to guest memory Marc Zyngier
@ 2019-03-19 15:16 ` Auger Eric
2019-03-19 15:47 ` Marc Zyngier
0 siblings, 1 reply; 6+ messages in thread
From: Auger Eric @ 2019-03-19 15:16 UTC (permalink / raw)
To: Marc Zyngier, linux-arm-kernel, kvmarm, kvm; +Cc: Andre Przywara
Hi Marc,
On 3/19/19 2:30 PM, Marc Zyngier wrote:
> When halting a guest, QEMU flushes the virtual ITS caches, which
> amounts to writing to the various tables that the guest has allocated.
>
> When doing this, we fail to take the srcu lock, and the kernel
> shouts loudly if running a lockdep kernel:
>
> [ 69.680416] =============================
> [ 69.680819] WARNING: suspicious RCU usage
> [ 69.681526] 5.1.0-rc1-00008-g600025238f51-dirty #18 Not tainted
> [ 69.682096] -----------------------------
> [ 69.682501] ./include/linux/kvm_host.h:605 suspicious rcu_dereference_check() usage!
> [ 69.683225]
> [ 69.683225] other info that might help us debug this:
> [ 69.683225]
> [ 69.683975]
> [ 69.683975] rcu_scheduler_active = 2, debug_locks = 1
> [ 69.684598] 6 locks held by qemu-system-aar/4097:
> [ 69.685059] #0: 0000000034196013 (&kvm->lock){+.+.}, at: vgic_its_set_attr+0x244/0x3a0
> [ 69.686087] #1: 00000000f2ed935e (&its->its_lock){+.+.}, at: vgic_its_set_attr+0x250/0x3a0
> [ 69.686919] #2: 000000005e71ea54 (&vcpu->mutex){+.+.}, at: lock_all_vcpus+0x64/0xd0
> [ 69.687698] #3: 00000000c17e548d (&vcpu->mutex){+.+.}, at: lock_all_vcpus+0x64/0xd0
> [ 69.688475] #4: 00000000ba386017 (&vcpu->mutex){+.+.}, at: lock_all_vcpus+0x64/0xd0
> [ 69.689978] #5: 00000000c2c3c335 (&vcpu->mutex){+.+.}, at: lock_all_vcpus+0x64/0xd0
> [ 69.690729]
> [ 69.690729] stack backtrace:
> [ 69.691151] CPU: 2 PID: 4097 Comm: qemu-system-aar Not tainted 5.1.0-rc1-00008-g600025238f51-dirty #18
> [ 69.691984] Hardware name: rockchip evb_rk3399/evb_rk3399, BIOS 2019.04-rc3-00124-g2feec69fb1 03/15/2019
> [ 69.692831] Call trace:
> [ 69.694072] lockdep_rcu_suspicious+0xcc/0x110
> [ 69.694490] gfn_to_memslot+0x174/0x190
> [ 69.694853] kvm_write_guest+0x50/0xb0
> [ 69.695209] vgic_its_save_tables_v0+0x248/0x330
> [ 69.695639] vgic_its_set_attr+0x298/0x3a0
> [ 69.696024] kvm_device_ioctl_attr+0x9c/0xd8
> [ 69.696424] kvm_device_ioctl+0x8c/0xf8
> [ 69.696788] do_vfs_ioctl+0xc8/0x960
> [ 69.697128] ksys_ioctl+0x8c/0xa0
> [ 69.697445] __arm64_sys_ioctl+0x28/0x38
> [ 69.697817] el0_svc_common+0xd8/0x138
> [ 69.698173] el0_svc_handler+0x38/0x78
> [ 69.698528] el0_svc+0x8/0xc
>
> The fix is to obviously take the srcu lock, just like we do on the
> read side of things since bf308242ab98. One wonders why this wasn't
> fixed at the same time, but hey...
>
> Fixes: bf308242ab98 ("KVM: arm/arm64: VGIC/ITS: protect kvm_read_guest() calls with SRCU lock")
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
> arch/arm/include/asm/kvm_mmu.h | 11 +++++++++++
> arch/arm64/include/asm/kvm_mmu.h | 11 +++++++++++
> virt/kvm/arm/vgic/vgic-its.c | 8 ++++----
> 3 files changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
> index 2de96a180166..31de4ab93005 100644
> --- a/arch/arm/include/asm/kvm_mmu.h
> +++ b/arch/arm/include/asm/kvm_mmu.h
> @@ -381,6 +381,17 @@ static inline int kvm_read_guest_lock(struct kvm *kvm,
> return ret;
> }
>
> +static inline int kvm_write_guest_lock(struct kvm *kvm, gpa_t gpa,
> + const void *data, unsigned long len)
> +{
> + int srcu_idx = srcu_read_lock(&kvm->srcu);
> + int ret = kvm_write_guest(kvm, gpa, data, len);
> +
> + srcu_read_unlock(&kvm->srcu, srcu_idx);
> +
> + return ret;
> +}
> +
> static inline void *kvm_get_hyp_vector(void)
> {
> switch(read_cpuid_part()) {
> diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
> index b0742a16c6c9..ebeefcf835e8 100644
> --- a/arch/arm64/include/asm/kvm_mmu.h
> +++ b/arch/arm64/include/asm/kvm_mmu.h
> @@ -445,6 +445,17 @@ static inline int kvm_read_guest_lock(struct kvm *kvm,
> return ret;
> }
>
> +static inline int kvm_write_guest_lock(struct kvm *kvm, gpa_t gpa,
> + const void *data, unsigned long len)
> +{
> + int srcu_idx = srcu_read_lock(&kvm->srcu);
> + int ret = kvm_write_guest(kvm, gpa, data, len);
> +
> + srcu_read_unlock(&kvm->srcu, srcu_idx);
> +
> + return ret;
> +}
> +
> #ifdef CONFIG_KVM_INDIRECT_VECTORS
> /*
> * EL2 vectors can be mapped and rerouted in a number of ways,
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index a60a768d9258..bb76220f2f30 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -2058,7 +2058,7 @@ static int vgic_its_save_ite(struct vgic_its *its, struct its_device *dev,
> ((u64)ite->irq->intid << KVM_ITS_ITE_PINTID_SHIFT) |
> ite->collection->collection_id;
> val = cpu_to_le64(val);
> - return kvm_write_guest(kvm, gpa, &val, ite_esz);
> + return kvm_write_guest_lock(kvm, gpa, &val, ite_esz);
> }
>
> /**
> @@ -2205,7 +2205,7 @@ static int vgic_its_save_dte(struct vgic_its *its, struct its_device *dev,
> (itt_addr_field << KVM_ITS_DTE_ITTADDR_SHIFT) |
> (dev->num_eventid_bits - 1));
> val = cpu_to_le64(val);
> - return kvm_write_guest(kvm, ptr, &val, dte_esz);
> + return kvm_write_guest_lock(kvm, ptr, &val, dte_esz);
> }
>
> /**
> @@ -2385,7 +2385,7 @@ static int vgic_its_save_cte(struct vgic_its *its,
> ((u64)collection->target_addr << KVM_ITS_CTE_RDBASE_SHIFT) |
> collection->collection_id);
> val = cpu_to_le64(val);
> - return kvm_write_guest(its->dev->kvm, gpa, &val, esz);
> + return kvm_write_guest_lock(its->dev->kvm, gpa, &val, esz);
> }
>
> static int vgic_its_restore_cte(struct vgic_its *its, gpa_t gpa, int esz)
> @@ -2456,7 +2456,7 @@ static int vgic_its_save_collection_table(struct vgic_its *its)
> */
> val = 0;
> BUG_ON(cte_esz > sizeof(val));
> - ret = kvm_write_guest(its->dev->kvm, gpa, &val, cte_esz);
> + ret = kvm_write_guest_lock(its->dev->kvm, gpa, &val, cte_esz);
> return ret;
> }
Don't we need to use kvm_write_guest_lock() as well also in
vgic_v3_lpi_sync_pending_status and vgic_v3_save_pending_tables?
Thanks
Eric
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] KVM: arm/arm64: vgic-its: Take the srcu lock when parsing the memslots
2019-03-19 13:30 ` [PATCH 2/2] KVM: arm/arm64: vgic-its: Take the srcu lock when parsing the memslots Marc Zyngier
@ 2019-03-19 15:17 ` Auger Eric
0 siblings, 0 replies; 6+ messages in thread
From: Auger Eric @ 2019-03-19 15:17 UTC (permalink / raw)
To: Marc Zyngier, linux-arm-kernel, kvmarm, kvm; +Cc: Andre Przywara
Hi Marc,
On 3/19/19 2:30 PM, Marc Zyngier wrote:
> Calling kvm_is_visible_gfn() implies that we're parsing the memslots,
> and doing this without the srcu lock is frown upon:
>
> [12704.164532] =============================
> [12704.164544] WARNING: suspicious RCU usage
> [12704.164560] 5.1.0-rc1-00008-g600025238f51-dirty #16 Tainted: G W
> [12704.164573] -----------------------------
> [12704.164589] ./include/linux/kvm_host.h:605 suspicious rcu_dereference_check() usage!
> [12704.164602] other info that might help us debug this:
> [12704.164616] rcu_scheduler_active = 2, debug_locks = 1
> [12704.164631] 6 locks held by qemu-system-aar/13968:
> [12704.164644] #0: 000000007ebdae4f (&kvm->lock){+.+.}, at: vgic_its_set_attr+0x244/0x3a0
> [12704.164691] #1: 000000007d751022 (&its->its_lock){+.+.}, at: vgic_its_set_attr+0x250/0x3a0
> [12704.164726] #2: 00000000219d2706 (&vcpu->mutex){+.+.}, at: lock_all_vcpus+0x64/0xd0
> [12704.164761] #3: 00000000a760aecd (&vcpu->mutex){+.+.}, at: lock_all_vcpus+0x64/0xd0
> [12704.164794] #4: 000000000ef8e31d (&vcpu->mutex){+.+.}, at: lock_all_vcpus+0x64/0xd0
> [12704.164827] #5: 000000007a872093 (&vcpu->mutex){+.+.}, at: lock_all_vcpus+0x64/0xd0
> [12704.164861] stack backtrace:
> [12704.164878] CPU: 2 PID: 13968 Comm: qemu-system-aar Tainted: G W 5.1.0-rc1-00008-g600025238f51-dirty #16
> [12704.164887] Hardware name: rockchip evb_rk3399/evb_rk3399, BIOS 2019.04-rc3-00124-g2feec69fb1 03/15/2019
> [12704.164896] Call trace:
> [12704.164910] dump_backtrace+0x0/0x138
> [12704.164920] show_stack+0x24/0x30
> [12704.164934] dump_stack+0xbc/0x104
> [12704.164946] lockdep_rcu_suspicious+0xcc/0x110
> [12704.164958] gfn_to_memslot+0x174/0x190
> [12704.164969] kvm_is_visible_gfn+0x28/0x70
> [12704.164980] vgic_its_check_id.isra.0+0xec/0x1e8
> [12704.164991] vgic_its_save_tables_v0+0x1ac/0x330
> [12704.165001] vgic_its_set_attr+0x298/0x3a0
> [12704.165012] kvm_device_ioctl_attr+0x9c/0xd8
> [12704.165022] kvm_device_ioctl+0x8c/0xf8
> [12704.165035] do_vfs_ioctl+0xc8/0x960
> [12704.165045] ksys_ioctl+0x8c/0xa0
> [12704.165055] __arm64_sys_ioctl+0x28/0x38
> [12704.165067] el0_svc_common+0xd8/0x138
> [12704.165078] el0_svc_handler+0x38/0x78
> [12704.165089] el0_svc+0x8/0xc
>
> Make sure the lock is taken when doing this.
>
> Fixes: bf308242ab98 ("KVM: arm/arm64: VGIC/ITS: protect kvm_read_guest() calls with SRCU lock")
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Thanks
Eric
> ---
> virt/kvm/arm/vgic/vgic-its.c | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index bb76220f2f30..7543f166943d 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -880,8 +880,9 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
> u64 indirect_ptr, type = GITS_BASER_TYPE(baser);
> phys_addr_t base = GITS_BASER_ADDR_48_to_52(baser);
> int esz = GITS_BASER_ENTRY_SIZE(baser);
> - int index;
> + int index, idx;
> gfn_t gfn;
> + bool ret;
>
> switch (type) {
> case GITS_BASER_TYPE_DEVICE:
> @@ -908,7 +909,8 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
>
> if (eaddr)
> *eaddr = addr;
> - return kvm_is_visible_gfn(its->dev->kvm, gfn);
> +
> + goto out;
> }
>
> /* calculate and check the index into the 1st level */
> @@ -938,7 +940,12 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
>
> if (eaddr)
> *eaddr = indirect_ptr;
> - return kvm_is_visible_gfn(its->dev->kvm, gfn);
> +
> +out:
> + idx = srcu_read_lock(&its->dev->kvm->srcu);
> + ret = kvm_is_visible_gfn(its->dev->kvm, gfn);
> + srcu_read_unlock(&its->dev->kvm->srcu, idx);
> + return ret;
> }
>
> static int vgic_its_alloc_collection(struct vgic_its *its,
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] KVM: arm/arm64: vgic-its: Take the srcu lock when writing to guest memory
2019-03-19 15:16 ` Auger Eric
@ 2019-03-19 15:47 ` Marc Zyngier
0 siblings, 0 replies; 6+ messages in thread
From: Marc Zyngier @ 2019-03-19 15:47 UTC (permalink / raw)
To: Auger Eric; +Cc: kvm, Andre Przywara, kvmarm, linux-arm-kernel
Hi Eric,
On Tue, 19 Mar 2019 15:16:38 +0000,
Auger Eric <eric.auger@redhat.com> wrote:
>
> Hi Marc,
>
> On 3/19/19 2:30 PM, Marc Zyngier wrote:
> > When halting a guest, QEMU flushes the virtual ITS caches, which
> > amounts to writing to the various tables that the guest has allocated.
> >
> > When doing this, we fail to take the srcu lock, and the kernel
> > shouts loudly if running a lockdep kernel:
> >
> > [ 69.680416] =============================
> > [ 69.680819] WARNING: suspicious RCU usage
> > [ 69.681526] 5.1.0-rc1-00008-g600025238f51-dirty #18 Not tainted
> > [ 69.682096] -----------------------------
> > [ 69.682501] ./include/linux/kvm_host.h:605 suspicious rcu_dereference_check() usage!
> > [ 69.683225]
> > [ 69.683225] other info that might help us debug this:
> > [ 69.683225]
> > [ 69.683975]
> > [ 69.683975] rcu_scheduler_active = 2, debug_locks = 1
> > [ 69.684598] 6 locks held by qemu-system-aar/4097:
> > [ 69.685059] #0: 0000000034196013 (&kvm->lock){+.+.}, at: vgic_its_set_attr+0x244/0x3a0
> > [ 69.686087] #1: 00000000f2ed935e (&its->its_lock){+.+.}, at: vgic_its_set_attr+0x250/0x3a0
> > [ 69.686919] #2: 000000005e71ea54 (&vcpu->mutex){+.+.}, at: lock_all_vcpus+0x64/0xd0
> > [ 69.687698] #3: 00000000c17e548d (&vcpu->mutex){+.+.}, at: lock_all_vcpus+0x64/0xd0
> > [ 69.688475] #4: 00000000ba386017 (&vcpu->mutex){+.+.}, at: lock_all_vcpus+0x64/0xd0
> > [ 69.689978] #5: 00000000c2c3c335 (&vcpu->mutex){+.+.}, at: lock_all_vcpus+0x64/0xd0
> > [ 69.690729]
> > [ 69.690729] stack backtrace:
> > [ 69.691151] CPU: 2 PID: 4097 Comm: qemu-system-aar Not tainted 5.1.0-rc1-00008-g600025238f51-dirty #18
> > [ 69.691984] Hardware name: rockchip evb_rk3399/evb_rk3399, BIOS 2019.04-rc3-00124-g2feec69fb1 03/15/2019
> > [ 69.692831] Call trace:
> > [ 69.694072] lockdep_rcu_suspicious+0xcc/0x110
> > [ 69.694490] gfn_to_memslot+0x174/0x190
> > [ 69.694853] kvm_write_guest+0x50/0xb0
> > [ 69.695209] vgic_its_save_tables_v0+0x248/0x330
> > [ 69.695639] vgic_its_set_attr+0x298/0x3a0
> > [ 69.696024] kvm_device_ioctl_attr+0x9c/0xd8
> > [ 69.696424] kvm_device_ioctl+0x8c/0xf8
> > [ 69.696788] do_vfs_ioctl+0xc8/0x960
> > [ 69.697128] ksys_ioctl+0x8c/0xa0
> > [ 69.697445] __arm64_sys_ioctl+0x28/0x38
> > [ 69.697817] el0_svc_common+0xd8/0x138
> > [ 69.698173] el0_svc_handler+0x38/0x78
> > [ 69.698528] el0_svc+0x8/0xc
> >
> > The fix is to obviously take the srcu lock, just like we do on the
> > read side of things since bf308242ab98. One wonders why this wasn't
> > fixed at the same time, but hey...
> >
> > Fixes: bf308242ab98 ("KVM: arm/arm64: VGIC/ITS: protect kvm_read_guest() calls with SRCU lock")
> > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> > ---
> > arch/arm/include/asm/kvm_mmu.h | 11 +++++++++++
> > arch/arm64/include/asm/kvm_mmu.h | 11 +++++++++++
> > virt/kvm/arm/vgic/vgic-its.c | 8 ++++----
> > 3 files changed, 26 insertions(+), 4 deletions(-)
[...]
> Don't we need to use kvm_write_guest_lock() as well also in
> vgic_v3_lpi_sync_pending_status and vgic_v3_save_pending_tables?
Ah, well spotted. Yes, that's needed too. I'll fix that.
Thanks,
M.
--
Jazz is not dead, it just smell funny.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-03-19 15:47 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-19 13:30 [PATCH 0/2] KVM: arm/arm64: vgic-its: Guest memory access fixes Marc Zyngier
2019-03-19 13:30 ` [PATCH 1/2] KVM: arm/arm64: vgic-its: Take the srcu lock when writing to guest memory Marc Zyngier
2019-03-19 15:16 ` Auger Eric
2019-03-19 15:47 ` Marc Zyngier
2019-03-19 13:30 ` [PATCH 2/2] KVM: arm/arm64: vgic-its: Take the srcu lock when parsing the memslots Marc Zyngier
2019-03-19 15:17 ` Auger Eric
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).