Re: [PATCH] KVM: arm64: vgic: Hold config_lock while tearing down a CPU interface

[Marc Zyngier <maz@kernel.org>]

On Mon, 19 Aug 2024 10:39:50 +0100,
Zenghui Yu <yuzenghui@huawei.com> wrote:
> 
> Hi Marc,
> 
> On 2024/8/8 17:15, Marc Zyngier wrote:
> > Tearing down a vcpu CPU interface involves freeing the private interrupt
> > array. If we don't hold the lock, we may race against another thread
> > trying to configure it. Yeah, fuzzers do wonderful things...
> > 
> > Taking the lock early solves this particular problem.
> > 
> > Fixes: 03b3d00a70b5 ("KVM: arm64: vgic: Allocate private interrupts on demand")
> > Reported-by: Alexander Potapenko <glider@google.com>
> > Tested-by: Alexander Potapenko <glider@google.com>
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  arch/arm64/kvm/vgic/vgic-init.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
> > index 7f68cf58b978..41feb858ff9a 100644
> > --- a/arch/arm64/kvm/vgic/vgic-init.c
> > +++ b/arch/arm64/kvm/vgic/vgic-init.c
> > @@ -438,14 +438,13 @@ void kvm_vgic_destroy(struct kvm *kvm)
> >  	unsigned long i;
> >  
> >  	mutex_lock(&kvm->slots_lock);
> > +	mutex_lock(&kvm->arch.config_lock);
> >  
> >  	vgic_debug_destroy(kvm);
> >  
> >  	kvm_for_each_vcpu(i, vcpu, kvm)
> >  		__kvm_vgic_vcpu_destroy(vcpu);
> >  
> > -	mutex_lock(&kvm->arch.config_lock);
> > -
> >  	kvm_vgic_dist_destroy(kvm);
> >  
> >  	mutex_unlock(&kvm->arch.config_lock);
> 
> The following splat was triggered when running the vgic_init selftest on
> a lockdep kernel (I use rc4, with defconfig + PROVE_LOCKING).
> 
> I'm not entirely sure that the splat is related to this change. Just
> haven't got time to dig further so post it out early for record.

Arghh. Thanks for reporting this. Can you try the following patch? It
does the trick for me, but I don't trust myself anymore...

	M.

diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
index 41feb858ff9a..e7c53e8af3d1 100644
--- a/arch/arm64/kvm/vgic/vgic-init.c
+++ b/arch/arm64/kvm/vgic/vgic-init.c
@@ -417,10 +417,8 @@ static void __kvm_vgic_vcpu_destroy(struct kvm_vcpu *vcpu)
 	kfree(vgic_cpu->private_irqs);
 	vgic_cpu->private_irqs = NULL;
 
-	if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) {
-		vgic_unregister_redist_iodev(vcpu);
+	if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3)
 		vgic_cpu->rd_iodev.base_addr = VGIC_ADDR_UNDEF;
-	}
 }
 
 void kvm_vgic_vcpu_destroy(struct kvm_vcpu *vcpu)
@@ -448,6 +446,11 @@ void kvm_vgic_destroy(struct kvm *kvm)
 	kvm_vgic_dist_destroy(kvm);
 
 	mutex_unlock(&kvm->arch.config_lock);
+
+	if (kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3)
+		kvm_for_each_vcpu(i, vcpu, kvm)
+			vgic_unregister_redist_iodev(vcpu);
+
 	mutex_unlock(&kvm->slots_lock);
 }
 
diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c
index 2caa64415ff3..f50274fd5581 100644
--- a/arch/arm64/kvm/vgic/vgic.c
+++ b/arch/arm64/kvm/vgic/vgic.c
@@ -36,6 +36,11 @@ struct vgic_global kvm_vgic_global_state __ro_after_init = {
  * we have to disable IRQs before taking this lock and everything lower
  * than it.
  *
+ * The config_lock has additional ordering requirements:
+ * kvm->slots_lock
+ *   kvm->srcu
+ *     kvm->arch.config_lock
+ *
  * If you need to take multiple locks, always take the upper lock first,
  * then the lower ones, e.g. first take the its_lock, then the irq_lock.
  * If you are already holding a lock and need to take a higher one, you

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