[PATCH 1/2] KVM: arm/arm64: vgic-its: Take the srcu lock when writing to guest memory

Auger Eric eric.auger at redhat.com
Tue Mar 19 08:16:38 PDT 2019


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 at 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
>  
> 



More information about the linux-arm-kernel mailing list