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

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

[Marc Zyngier]

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);
-- 
2.39.2

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

[Oliver Upton]

On Thu, 8 Aug 2024 10:15:46 +0100, 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.
> 
> 
> [...]

Applied to kvmarm/fixes, thanks!

[1/1] KVM: arm64: vgic: Hold config_lock while tearing down a CPU interface
      https://git.kernel.org/kvmarm/kvmarm/c/9eb18136af9f

--
Best,
Oliver

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

[Zenghui Yu]

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.

 ======================================================
 WARNING: possible circular locking dependency detected
 6.11.0-rc4-dirty Not tainted
 ------------------------------------------------------
 vgic_init/358856 is trying to acquire lock:
 ffff2028021b53c8 (&kvm->srcu){.+.+}-{0:0}, at: __synchronize_srcu+0x0/0x170

 but task is already holding lock:
 ffff2028021b4f80 (&kvm->arch.config_lock){+.+.}-{3:3}, at:
kvm_vgic_destroy+0x50/0x1e0

 which lock already depends on the new lock.


 the existing dependency chain (in reverse order) is:

 -> #1 (&kvm->arch.config_lock){+.+.}-{3:3}:
        __mutex_lock+0x84/0x400
        mutex_lock_nested+0x24/0x30
        vgic_mmio_write_v3_misc+0x48/0x120
        dispatch_mmio_write+0x90/0x128
        __kvm_io_bus_write+0xb4/0xec
        kvm_io_bus_write+0x68/0x100
        io_mem_abort+0xf0/0x3f0
        kvm_handle_guest_abort+0x5d0/0x108c
        handle_exit+0x6c/0x1c4
        kvm_arch_vcpu_ioctl_run+0x41c/0xac4
        kvm_vcpu_ioctl+0x310/0xaf4
        __arm64_sys_ioctl+0xa8/0xec
        invoke_syscall+0x48/0x110
        el0_svc_common.constprop.0+0x40/0xe0
        do_el0_svc+0x1c/0x28
        el0_svc+0x4c/0x11c
        el0t_64_sync_handler+0xc0/0xc4
        el0t_64_sync+0x190/0x194

 -> #0 (&kvm->srcu){.+.+}-{0:0}:
        __lock_acquire+0x1304/0x2044
        lock_sync+0xd8/0x128
        __synchronize_srcu+0x50/0x170
        synchronize_srcu_expedited+0x24/0x34
        kvm_io_bus_unregister_dev+0x110/0x26c
        vgic_unregister_redist_iodev+0x20/0x2c
        kvm_vgic_destroy+0xe8/0x1e0
        kvm_vgic_map_resources+0xa8/0x138
        kvm_arch_vcpu_run_pid_change+0xc0/0x430
        kvm_vcpu_ioctl+0xa24/0xaf4
        __arm64_sys_ioctl+0xa8/0xec
        invoke_syscall+0x48/0x110
        el0_svc_common.constprop.0+0x40/0xe0
        do_el0_svc+0x1c/0x28
        el0_svc+0x4c/0x11c
        el0t_64_sync_handler+0xc0/0xc4
        el0t_64_sync+0x190/0x194

 other info that might help us debug this:

  Possible unsafe locking scenario:

        CPU0                    CPU1
        ----                    ----
   lock(&kvm->arch.config_lock);
                                lock(&kvm->srcu);
                                lock(&kvm->arch.config_lock);
   sync(&kvm->srcu);

  *** DEADLOCK ***

 3 locks held by vgic_init/358856:
  #0: ffff20280aa19b40 (&vcpu->mutex){+.+.}-{3:3}, at:
kvm_vcpu_ioctl+0x84/0xaf4
  #1: ffff2028021b40a8 (&kvm->slots_lock){+.+.}-{3:3}, at:
kvm_vgic_destroy+0x44/0x1e0
  #2: ffff2028021b4f80 (&kvm->arch.config_lock){+.+.}-{3:3}, at:
kvm_vgic_destroy+0x50/0x1e0

 stack backtrace:
 CPU: 74 UID: 0 PID: 358856 Comm: vgic_init Kdump: loaded Not tainted
6.11.0-rc4-dirty
 Call trace:
  dump_backtrace+0x98/0xf0
  show_stack+0x18/0x24
  dump_stack_lvl+0x90/0xd0
  dump_stack+0x18/0x24
  print_circular_bug+0x290/0x370
  check_noncircular+0x15c/0x170
  __lock_acquire+0x1304/0x2044
  lock_sync+0xd8/0x128
  __synchronize_srcu+0x50/0x170
  synchronize_srcu_expedited+0x24/0x34
  kvm_io_bus_unregister_dev+0x110/0x26c
  vgic_unregister_redist_iodev+0x20/0x2c
  kvm_vgic_destroy+0xe8/0x1e0
  kvm_vgic_map_resources+0xa8/0x138
  kvm_arch_vcpu_run_pid_change+0xc0/0x430
  kvm_vcpu_ioctl+0xa24/0xaf4
  __arm64_sys_ioctl+0xa8/0xec
  invoke_syscall+0x48/0x110
  el0_svc_common.constprop.0+0x40/0xe0
  do_el0_svc+0x1c/0x28
  el0_svc+0x4c/0x11c
  el0t_64_sync_handler+0xc0/0xc4
  el0t_64_sync+0x190/0x194

Thanks,
Zenghui

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

[Marc Zyngier]

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.

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

[Zenghui Yu]

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.

Thanks,
Zenghui

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

[Marc Zyngier]

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.