Re: [PATCH v2 2/4] KVM: arm64: Avoid lock inversion when setting the VM register width

[Marc Zyngier <maz@kernel.org>]

On Thu, 16 Mar 2023 21:14:10 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> kvm->lock must be taken outside of the vcpu->mutex. Of course, the
> locking documentation for KVM makes this abundantly clear. Nonetheless,
> the locking order in KVM/arm64 has been wrong for quite a while; we
> acquire the kvm->lock while holding the vcpu->mutex all over the shop.
> 
> All was seemingly fine until commit 42a90008f890 ("KVM: Ensure lockdep
> knows about kvm->lock vs. vcpu->mutex ordering rule") caught us with our
> pants down, leading to lockdep barfing:
> 
>  ======================================================
>  WARNING: possible circular locking dependency detected
>  6.2.0-rc7+ #19 Not tainted
>  ------------------------------------------------------
>  qemu-system-aar/859 is trying to acquire lock:
>  ffff5aa69269eba0 (&host_kvm->lock){+.+.}-{3:3}, at: kvm_reset_vcpu+0x34/0x274
> 
>  but task is already holding lock:
>  ffff5aa68768c0b8 (&vcpu->mutex){+.+.}-{3:3}, at: kvm_vcpu_ioctl+0x8c/0xba0
> 
>  which lock already depends on the new lock.
> 
> Add a dedicated lock to serialize writes to VM-scoped configuration from
> the context of a vCPU. Protect the register width flags with the new
> lock, thus avoiding the need to grab the kvm->lock while holding
> vcpu->mutex in kvm_reset_vcpu().
> 
> Reported-by: Jeremy Linton <jeremy.linton@arm.com>
> Link: https://lore.kernel.org/kvmarm/f6452cdd-65ff-34b8-bab0-5c06416da5f6@arm.com/
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/include/asm/kvm_host.h |  3 +++
>  arch/arm64/kvm/arm.c              | 18 ++++++++++++++++++
>  arch/arm64/kvm/reset.c            |  6 +++---
>  3 files changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 917586237a4d..1f4b9708a775 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -185,6 +185,9 @@ struct kvm_protected_vm {
>  };
>  
>  struct kvm_arch {
> +	/* Protects VM-scoped configuration data */
> +	struct mutex config_lock;
> +

nit: can we move this down into the structure and keep the MM stuff on
its own at the top? Placing it next to the flags would make some
sense, as these flags are definitely related to configuration matters.

>  	struct kvm_s2_mmu mmu;
>  
>  	/* VTCR_EL2 value for this VM */
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 731a78f85915..1478bec52767 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -128,6 +128,16 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
>  {
>  	int ret;
>  
> +	mutex_init(&kvm->arch.config_lock);
> +
> +#ifdef CONFIG_LOCKDEP
> +	/* Clue in lockdep that the config_lock must be taken inside kvm->lock */
> +	mutex_lock(&kvm->lock);
> +	mutex_lock(&kvm->arch.config_lock);
> +	mutex_unlock(&kvm->arch.config_lock);
> +	mutex_unlock(&kvm->lock);
> +#endif
> +
>  	ret = kvm_share_hyp(kvm, kvm + 1);
>  	if (ret)
>  		return ret;
> @@ -328,6 +338,14 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>  
>  	spin_lock_init(&vcpu->arch.mp_state_lock);
>  
> +#ifdef CONFIG_LOCKDEP
> +	/* Inform lockdep that the config_lock is acquired after vcpu->mutex */
> +	mutex_lock(&vcpu->mutex);
> +	mutex_lock(&vcpu->kvm->arch.config_lock);
> +	mutex_unlock(&vcpu->kvm->arch.config_lock);
> +	mutex_unlock(&vcpu->mutex);
> +#endif

Shouldn't this hunk be moved to the previous patch?

> +
>  	/* Force users to call KVM_ARM_VCPU_INIT */
>  	vcpu->arch.target = -1;
>  	bitmap_zero(vcpu->arch.features, KVM_VCPU_MAX_FEATURES);
> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> index b874ec6a37c1..ee445a7a1ef8 100644
> --- a/arch/arm64/kvm/reset.c
> +++ b/arch/arm64/kvm/reset.c
> @@ -205,7 +205,7 @@ static int kvm_set_vm_width(struct kvm_vcpu *vcpu)
>  
>  	is32bit = vcpu_has_feature(vcpu, KVM_ARM_VCPU_EL1_32BIT);
>  
> -	lockdep_assert_held(&kvm->lock);
> +	lockdep_assert_held(&kvm->arch.config_lock);
>  
>  	if (test_bit(KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED, &kvm->arch.flags)) {
>  		/*
> @@ -262,9 +262,9 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
>  	bool loaded;
>  	u32 pstate;
>  
> -	mutex_lock(&vcpu->kvm->lock);
> +	mutex_lock(&vcpu->kvm->arch.config_lock);
>  	ret = kvm_set_vm_width(vcpu);
> -	mutex_unlock(&vcpu->kvm->lock);
> +	mutex_unlock(&vcpu->kvm->arch.config_lock);
>  
>  	if (ret)
>  		return ret;

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel