Re: [PATCH 1/3] irqchip/gic-v4: Clear the domain and fwnode pointers after freeing them

On Thu, 20 Aug 2026 15:11:03 +0100,
Fuad Tabba <[email protected]> wrote:
> 
> Oh Sashiko, here we ago again...
> 
> On Thu, 20 Aug 2026 at 14:06, <[email protected]> wrote:
> ...
> > [Severity: High]
> > This is a pre-existing issue, but does this function permanently leak IRQs
> > and SGI domains?
> >
> > In arch/arm64/kvm/vgic/vgic-v4.c:vgic_v4_init(), KVM intentionally truncates
> > dist->its_vm.nr_vpes upon failure to allocate VPE IRQs:
> >
> >     dist->its_vm.nr_vpes = i;
> >
> > This limits the teardown loop in vgic_v4_teardown(), which later calls
> > its_free_vcpu_irqs() with the fatally truncated nr_vpes value.
> >
> > Because its_free_vcpu_irqs() relies on vm->nr_vpes to free the IRQ hierarchy
> > and SGI domains originally allocated for all VCPUs, the truncation causes a
> > partial free.
> 
> Looks like a bug indeed...
> 
> I think the problem is that vgic_v4_init() overloads its_vm.nr_vpes:
> it truncates the count to bound the free_irq() loop, but
> its_free_vcpu_irqs() reads the same count and under-frees what
> its_alloc_vcpu_irqs() allocated at and above the failure index.
> 
> The fix I have in mind keeps the two counts apart: teardown frees
> everything allocated, and free_irq() only touches the requested
> doorbells. Marc, what do you think? Something like this (not tested):
> 
> @@ vgic_v4_init(), on vgic_v4_request_vpe_irq() failure @@
>                  ret = vgic_v4_request_vpe_irq(vcpu, irq);
>                  if (ret) {
>                          kvm_err("failed to allocate vcpu IRQ%d\n", irq);
> -                        /*
> -                         * Trick: adjust the number of vpes so we know
> -                         * how many to nuke on teardown...
> -                         */
> -                        dist->its_vm.nr_vpes = i;
>                          break;
>                  }
>          }
> 
> -        if (ret)
> -                vgic_v4_teardown(kvm);
> +        if (ret) {
> +                /* Undo the doorbells we requested (0..i-1)... */
> +                while (--i >= 0) {
> +                        int db = dist->its_vm.vpes[i]->irq;
> +                        irq_clear_status_flags(db, DB_IRQ_FLAGS);
> +                        free_irq(db, kvm_get_vcpu(kvm, i));
> +                }
> +                /* ...but free everything its_alloc_vcpu_irqs() allocated. */
> +                its_free_vcpu_irqs(&dist->its_vm);
> +                kfree(dist->its_vm.vpes);
> +                dist->its_vm.nr_vpes = 0;
> +                dist->its_vm.vpes = NULL;
> +        }
> 
>          return ret;
> 
> The actual patch would factor the shared teardown tail into a helper
> rather than duplicate it.
> 
> If you (Marc) agree, I'll send this as a separate patch (after testing
> it), or as part of a possible respin in a couple of days.

Why isn't this as simple as:

diff --git a/arch/arm64/kvm/vgic/vgic-v4.c b/arch/arm64/kvm/vgic/vgic-v4.c
index ed236f083f0d7..57a16530801c0 100644
--- a/arch/arm64/kvm/vgic/vgic-v4.c
+++ b/arch/arm64/kvm/vgic/vgic-v4.c
@@ -333,6 +333,8 @@ void vgic_v4_teardown(struct kvm *kvm)
 		free_irq(irq, vcpu);
 	}
 
+	/* Make sure we free all VM-wide, per-CPU resources */
+	its_vm->nr_vpes = aromic_read(&kvm->online_vcpus);
 	its_free_vcpu_irqs(its_vm);
 	kfree(its_vm->vpes);
 	its_vm->nr_vpes = 0;

Thanks,

	M.

-- 
Jazz isn't dead. It just smells funny.