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

[Marc Zyngier <maz@kernel.org>]

On 2023-03-23 19:20, Oliver Upton wrote:
> On Wed, Mar 22, 2023 at 12:02:40PM +0000, Marc Zyngier wrote:
>> 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.
> 
> Sure thing!
> 
>> >  	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?
> 
> Uh, I don't believe so since this is the patch that actually introduces
> kvm_arch::config_lock. The last patch was aimed at a separate lock for
> mp state.

Nah, you're obviously right. I reviewed this at 4am being jet-lagged.
Not the brightest comment... :-/ Sorry for the noise.

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

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