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

[Marc Zyngier <maz@kernel.org>]

On Mon, 19 Aug 2024 13:31:59 +0100,
Zenghui Yu <yuzenghui@huawei.com> wrote:
> 
> On 2024/8/19 18:53, Marc Zyngier wrote:
> > 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...
> 
> I tested it with defconfig+PROVE_LOCKING and my own config, both worked
> for me (the WARNING disappeared). I think the locking order should be
> correct now.

Awesome. Let me post a proper patch right away.

Thanks again,

	M.

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