[PATCH] KVM: arm64: vgic: Check the interrupt is still ours before migrating it

[PATCH] KVM: arm64: vgic: Check the interrupt is still ours before migrating it

[Hyunwoo Kim]

vgic_prune_ap_list() drops both ap_list_lock and irq_lock while migrating
an interrupt to another vCPU. After reacquiring the locks it only checks
that the affinity is unchanged (target_vcpu == vgic_target_oracle(irq))
before moving the interrupt, which assumes that an interrupt whose affinity
is preserved is still queued on this vCPU's ap_list.

That assumption no longer holds if the interrupt is taken off the ap_list
while the locks are dropped. vgic_flush_pending_lpis() removes the
interrupt from the list and sets irq->vcpu to NULL, but leaves
enabled/pending/target_vcpu untouched. As the interrupt is still enabled
and pending, vgic_target_oracle() returns the same target_vcpu, so the
affinity check passes and list_del() is run a second time on an entry that
has already been removed.

Also check that the interrupt is still assigned to this vCPU
(irq->vcpu == vcpu) before moving it.

Fixes: 0919e84c0fc1 ("KVM: arm/arm64: vgic-new: Add IRQ sync/flush framework")
Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
---
 arch/arm64/kvm/vgic/vgic.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c
index 1e9fe8764584..18b280de9a29 100644
--- a/arch/arm64/kvm/vgic/vgic.c
+++ b/arch/arm64/kvm/vgic/vgic.c
@@ -818,15 +818,16 @@ static void vgic_prune_ap_list(struct kvm_vcpu *vcpu)
 		raw_spin_lock(&irq->irq_lock);
 
 		/*
-		 * If the affinity has been preserved, move the
-		 * interrupt around. Otherwise, it means things have
-		 * changed while the interrupt was unlocked, and we
-		 * need to replay this.
+		 * If the interrupt is still ours and its affinity has
+		 * been preserved, move it around. Otherwise, it means
+		 * things have changed while the interrupt was unlocked
+		 * (it may even have been taken off the list with its
+		 * affinity left untouched), and we need to replay this.
 		 *
 		 * In all cases, we cannot trust the list not to have
 		 * changed, so we restart from the beginning.
 		 */
-		if (target_vcpu == vgic_target_oracle(irq)) {
+		if (irq->vcpu == vcpu && target_vcpu == vgic_target_oracle(irq)) {
 			struct vgic_cpu *new_cpu = &target_vcpu->arch.vgic_cpu;
 
 			list_del(&irq->ap_list);
-- 
2.43.0

Re: [PATCH] KVM: arm64: vgic: Check the interrupt is still ours before migrating it

[Oliver Upton]

On Fri, Jun 05, 2026 at 05:59:15AM +0900, Hyunwoo Kim wrote:
> vgic_prune_ap_list() drops both ap_list_lock and irq_lock while migrating
> an interrupt to another vCPU. After reacquiring the locks it only checks
> that the affinity is unchanged (target_vcpu == vgic_target_oracle(irq))
> before moving the interrupt, which assumes that an interrupt whose affinity
> is preserved is still queued on this vCPU's ap_list.
> 
> That assumption no longer holds if the interrupt is taken off the ap_list
> while the locks are dropped. vgic_flush_pending_lpis() removes the
> interrupt from the list and sets irq->vcpu to NULL, but leaves
> enabled/pending/target_vcpu untouched. As the interrupt is still enabled
> and pending, vgic_target_oracle() returns the same target_vcpu, so the
> affinity check passes and list_del() is run a second time on an entry that
> has already been removed.
> 
> Also check that the interrupt is still assigned to this vCPU
> (irq->vcpu == vcpu) before moving it.
> 
> Fixes: 0919e84c0fc1 ("KVM: arm/arm64: vgic-new: Add IRQ sync/flush framework")
> Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>

Looking at this and the other VGIC patch you sent (which should've been
a combined series), are you trying to deal with a vCPU writing to
another vCPU's redistributor? I.e. vCPU B setting GICR_CTLR.EnableLPIs=0
behind the back of vCPU A?

That is extremely relevant information as the off-the-cuff reaction is
that no race exists. But since the GIC architecture is awesome and
allows for this sort of insanity, it obviously does....

Anyway, for LPIs resident on a particular RD, there's zero expectation
that the pending state is preserved when EnableLPIs=0. So I'd rather
vgic_flush_pending_lpis() just invalidate the pending state.

Beyond that, I see two other fixes for lifetime issues around the
vgic_irq in the middle of migration. I'd like to see explicit RCU
protection around the release && reacquire of the ap_list_lock rather
than depending on the precondition that IRQs are disabled.

Then vgic_flush_pending_lpis() should leave IRQs intact that are pending
a migration (e.g. irq->vcpu != vgic_target_oracle()) as the only expectation
we need to uphold is that LPIs resident on the RD have the pending state cleared.

Although I think we could benefit from the wetware implementation of the
GIC giving this a once over too. Any thoughts Marc?

Thanks,
Oliver

> ---
>  arch/arm64/kvm/vgic/vgic.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c
> index 1e9fe8764584..18b280de9a29 100644
> --- a/arch/arm64/kvm/vgic/vgic.c
> +++ b/arch/arm64/kvm/vgic/vgic.c
> @@ -818,15 +818,16 @@ static void vgic_prune_ap_list(struct kvm_vcpu *vcpu)
>  		raw_spin_lock(&irq->irq_lock);
>  
>  		/*
> -		 * If the affinity has been preserved, move the
> -		 * interrupt around. Otherwise, it means things have
> -		 * changed while the interrupt was unlocked, and we
> -		 * need to replay this.
> +		 * If the interrupt is still ours and its affinity has
> +		 * been preserved, move it around. Otherwise, it means
> +		 * things have changed while the interrupt was unlocked
> +		 * (it may even have been taken off the list with its
> +		 * affinity left untouched), and we need to replay this.
>  		 *
>  		 * In all cases, we cannot trust the list not to have
>  		 * changed, so we restart from the beginning.
>  		 */
> -		if (target_vcpu == vgic_target_oracle(irq)) {
> +		if (irq->vcpu == vcpu && target_vcpu == vgic_target_oracle(irq)) {
>  			struct vgic_cpu *new_cpu = &target_vcpu->arch.vgic_cpu;
>  
>  			list_del(&irq->ap_list);
> -- 
> 2.43.0
> 

Re: [PATCH] KVM: arm64: vgic: Check the interrupt is still ours before migrating it

[Marc Zyngier]

On Fri, 05 Jun 2026 07:00:37 +0100,
Oliver Upton <oupton@kernel.org> wrote:
> 
> On Fri, Jun 05, 2026 at 05:59:15AM +0900, Hyunwoo Kim wrote:
> > vgic_prune_ap_list() drops both ap_list_lock and irq_lock while migrating
> > an interrupt to another vCPU. After reacquiring the locks it only checks
> > that the affinity is unchanged (target_vcpu == vgic_target_oracle(irq))
> > before moving the interrupt, which assumes that an interrupt whose affinity
> > is preserved is still queued on this vCPU's ap_list.
> > 
> > That assumption no longer holds if the interrupt is taken off the ap_list
> > while the locks are dropped. vgic_flush_pending_lpis() removes the
> > interrupt from the list and sets irq->vcpu to NULL, but leaves
> > enabled/pending/target_vcpu untouched. As the interrupt is still enabled
> > and pending, vgic_target_oracle() returns the same target_vcpu, so the
> > affinity check passes and list_del() is run a second time on an entry that
> > has already been removed.
> > 
> > Also check that the interrupt is still assigned to this vCPU
> > (irq->vcpu == vcpu) before moving it.
> > 
> > Fixes: 0919e84c0fc1 ("KVM: arm/arm64: vgic-new: Add IRQ sync/flush framework")
> > Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
> 
> Looking at this and the other VGIC patch you sent (which should've been
> a combined series), are you trying to deal with a vCPU writing to
> another vCPU's redistributor? I.e. vCPU B setting GICR_CTLR.EnableLPIs=0
> behind the back of vCPU A?
> 
> That is extremely relevant information as the off-the-cuff reaction is
> that no race exists. But since the GIC architecture is awesome and
> allows for this sort of insanity, it obviously does....
> 
> Anyway, for LPIs resident on a particular RD, there's zero expectation
> that the pending state is preserved when EnableLPIs=0. So I'd rather
> vgic_flush_pending_lpis() just invalidate the pending state.

Just clearing the pending state introduces a potential problem as we
now have an interrupt that is neither active nor pending on the AP
list. It is not impossible to solve (we now have similar behaviours
with SPI deactivation from another vcpu), but that requires posting a
KVM_REQ_VGIC_PROCESS_UPDATE to the target vcpu.

> Beyond that, I see two other fixes for lifetime issues around the
> vgic_irq in the middle of migration. I'd like to see explicit RCU
> protection around the release && reacquire of the ap_list_lock rather
> than depending on the precondition that IRQs are disabled.

I'm not sure I follow. Are you suggesting turning the AP list into an
RCU protected list?

Thanks,

	M.

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

Re: [PATCH] KVM: arm64: vgic: Check the interrupt is still ours before migrating it

[Oliver Upton]

On Fri, Jun 05, 2026 at 08:42:52AM +0100, Marc Zyngier wrote:
> On Fri, 05 Jun 2026 07:00:37 +0100,
> Oliver Upton <oupton@kernel.org> wrote:
> > 
> > On Fri, Jun 05, 2026 at 05:59:15AM +0900, Hyunwoo Kim wrote:
> > > vgic_prune_ap_list() drops both ap_list_lock and irq_lock while migrating
> > > an interrupt to another vCPU. After reacquiring the locks it only checks
> > > that the affinity is unchanged (target_vcpu == vgic_target_oracle(irq))
> > > before moving the interrupt, which assumes that an interrupt whose affinity
> > > is preserved is still queued on this vCPU's ap_list.
> > > 
> > > That assumption no longer holds if the interrupt is taken off the ap_list
> > > while the locks are dropped. vgic_flush_pending_lpis() removes the
> > > interrupt from the list and sets irq->vcpu to NULL, but leaves
> > > enabled/pending/target_vcpu untouched. As the interrupt is still enabled
> > > and pending, vgic_target_oracle() returns the same target_vcpu, so the
> > > affinity check passes and list_del() is run a second time on an entry that
> > > has already been removed.
> > > 
> > > Also check that the interrupt is still assigned to this vCPU
> > > (irq->vcpu == vcpu) before moving it.
> > > 
> > > Fixes: 0919e84c0fc1 ("KVM: arm/arm64: vgic-new: Add IRQ sync/flush framework")
> > > Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
> > 
> > Looking at this and the other VGIC patch you sent (which should've been
> > a combined series), are you trying to deal with a vCPU writing to
> > another vCPU's redistributor? I.e. vCPU B setting GICR_CTLR.EnableLPIs=0
> > behind the back of vCPU A?
> > 
> > That is extremely relevant information as the off-the-cuff reaction is
> > that no race exists. But since the GIC architecture is awesome and
> > allows for this sort of insanity, it obviously does....
> > 
> > Anyway, for LPIs resident on a particular RD, there's zero expectation
> > that the pending state is preserved when EnableLPIs=0. So I'd rather
> > vgic_flush_pending_lpis() just invalidate the pending state.
> 
> Just clearing the pending state introduces a potential problem as we
> now have an interrupt that is neither active nor pending on the AP
> list. It is not impossible to solve (we now have similar behaviours
> with SPI deactivation from another vcpu), but that requires posting a
> KVM_REQ_VGIC_PROCESS_UPDATE to the target vcpu.

Right, I was suggesting that in addition to deleting the LPI from the AP
list we actually invalidate the pending state so that someone sitting on
a pointer to a to-be-freed LPI sees vgic_target_oracle() returning
NULL

> > Beyond that, I see two other fixes for lifetime issues around the
> > vgic_irq in the middle of migration. I'd like to see explicit RCU
> > protection around the release && reacquire of the ap_list_lock rather
> > than depending on the precondition that IRQs are disabled.
> 
> I'm not sure I follow. Are you suggesting turning the AP list into an
> RCU protected list?

No, sorry, I should expand a little.

We store a reference on the vgic_irq struct in the AP list, which is
stable so long as the ap_list_lock is held. It should be possible for
the refcount to drop to 0 between releasing the ap_list_lock and
reacquiring it.

So either vgic_prune_ap_list() takes an additional reference on the
vgic_irq before dropping the ap_list_lock or rely on RCU to protect
vgic_irq structs observed with a non-zero refcount.

Thanks,
Oliver