Re: [PATCH v2 2/2] KVM: arm64: vgic-its: Remove the pending state of a discarded LPI

[Oliver Upton <oupton@kernel.org>]

On Tue, Oct 13, 2026 at 08:50:51AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] kvm: arm64: vgic-its: TOCTOU race resurrects discarded LPI due to IPI skip
> - [High] kvm: arm64: vgic-its: Race on vcpu->arch.pause breaks mutual exclusion
> - [Medium] kvm: arm64: vgic-its: Global VM halt on DISCARD causes Denial of Service
> --
> 
> commit 4bb7971d12d2fa6f1083c9f2ddc383b8d63483fb
> Author: Fuad Tabba <fuad.tabba@linux.dev>
> KVM: arm64: vgic-its: Remove the pending state of a discarded LPI
> 
> This commit ensures the pending state of a queued LPI is cleared when handling
> an ITS DISCARD command. It halts the guest before clearing the pending state if
> the LPI is queued on a running vCPU.
> 
> > diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> > --- a/arch/arm64/kvm/vgic/vgic-its.c
> > +++ b/arch/arm64/kvm/vgic/vgic-its.c
> > @@ -829,12 +829,26 @@ static int vgic_its_cmd_handle_discard(struct kvm *kvm, struct vgic_its *its,
> >  		struct its_device *device = find_its_device(its, device_id);
> >  		int ite_esz = vgic_its_get_abi(its)->ite_esz;
> >  		gpa_t gpa = device->itt_addr + ite->event_id * ite_esz;
> > +		struct vgic_irq *irq = ite->irq;
> > +		bool queued;
> > +
> > +		vgic_its_invalidate_cache(its);
> > +
> >  		/*
> > -		 * Though the spec talks about removing the pending state, we
> > -		 * don't bother here since we clear the ITTE anyway and the
> > -		 * pending state is a property of the ITTE struct.
> > +		 * A queued LPI can be pending in a running vCPU's list register,
> > +		 * which the exit path folds back: halt the guest before clearing.
> >  		 */
> > -		vgic_its_invalidate_cache(its);
> > +		scoped_guard(raw_spinlock_irqsave, &irq->irq_lock)
> > +			queued = irq->vcpu;
> > +
> > +		if (queued)
> > +			kvm_arm_halt_guest(kvm);
> 
> [Severity: Medium]
> Does this introduce a denial of service if a guest spams DISCARD commands?
> 
> Because kvm_arm_halt_guest() forces all vCPUs in the VM to exit, a guest
> rapidly issuing DISCARD commands for a single vCPU's LPI could repeatedly
> lock up the entire VM and cause host starvation. Could we restrict the
> synchronization strictly to the target irq->vcpu where the LPI is queued?

A guest has chosen to shoot itself in the foot, why should we care? The
whole point of pausing the VM is to flush state and block the vCPU
threads...

> > +
> > +		scoped_guard(raw_spinlock_irqsave, &irq->irq_lock)
> > +			irq->pending_latch = false;
> 
> [Severity: High]
> Is there a window here where an exiting vCPU might overwrite the cleared
> latch and resurrect the LPI?
> 
> If the target vCPU is just exiting when kvm_arm_halt_guest() broadcasts
> KVM_REQ_SLEEP, the vCPU sets vcpu->mode = OUTSIDE_GUEST_MODE before folding
> its hardware state in kvm_arch_vcpu_ioctl_run().
> 
> Because kvm_request_needs_ipi() checks:
> 
> virt/kvm/kvm_main.c:kvm_request_needs_ipi() {
>     if (req & KVM_REQUEST_WAIT)
>         return mode != OUTSIDE_GUEST_MODE;
> }
> 
> The IPI is skipped and kvm_arm_halt_guest() returns immediately without
> waiting for the vCPU to sync. The DISCARD thread then clears
> irq->pending_latch = false.
> 
> Concurrently, the exiting vCPU executes kvm_vgic_sync_hwstate(), reads the
> still-pending state from the hardware LR, and unconditionally overwrites the
> cleared latch back to true:
> 
> arch/arm64/kvm/vgic/vgic-v3.c:vgic_v3_fold_lr() {
>     if (irq->config == VGIC_CONFIG_EDGE && (val & ICH_LR_PENDING_BIT))
>         irq->pending_latch = true;
> }
> 
> Could we coordinate with the vCPU's state folding sequence to ensure the
> discard isn't lost?

https://lore.kernel.org/kvmarm/20260929093548.3598547-1-maz@kernel.org/

> > +
> > +		if (queued)
> > +			kvm_arm_resume_guest(kvm);
> 
> [Severity: High]
> Can concurrent DISCARD commands on different ITS instances break the mutual
> exclusion of vcpu->arch.pause?
> 
> vgic_its_cmd_handle_discard() executes holding the per-ITS its->its_lock.
> If a VM has multiple ITS devices, two DISCARD commands could run concurrently
> and both call kvm_arm_halt_guest(), which sets vcpu->arch.pause to true
> across all vCPUs.
> 
> The first thread to finish will call kvm_arm_resume_guest(), which does:
> 
> arch/arm64/kvm/arm.c:kvm_arm_resume_guest() {
>     kvm_for_each_vcpu(i, vcpu, kvm) {
>         vcpu->arch.pause = false;
>         __kvm_vcpu_wake_up(vcpu);
>     }
> }
> 
> This unconditionally clears the pause flag and wakes up all vCPUs while the
> second thread is still modifying VGIC state assuming the guest is fully
> halted.
> 

https://lore.kernel.org/kvmarm/20260929093548.3598547-1-maz@kernel.org/

Thanks,
Oliver