[PATCH] KVM: arm64: vgic: Hold config_lock while tearing down a CPU interface
Marc Zyngier
maz at kernel.org
Mon Aug 19 03:53:59 PDT 2024
On Mon, 19 Aug 2024 10:39:50 +0100,
Zenghui Yu <yuzenghui at 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 at google.com>
> > Tested-by: Alexander Potapenko <glider at google.com>
> > Signed-off-by: Marc Zyngier <maz at 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.
More information about the linux-arm-kernel
mailing list