Re: [PATCH 3/3] KVM: arm64: vgic: Stop the VM when disabling LPIs

[Marc Zyngier <maz@kernel.org>]

On Tue, 22 Sep 2026 23:01:25 +0100,
sashiko-bot@kernel.org wrote:
> 
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] kvm: arm64: vgic: race in vcpu->arch.pause unpause due to missing VM lock
> - [High] kvm: arm64: vgic: stale LR state during vgic_flush_pending_lpis()
> --
> 
> commit 81f0f5ee375a1e011fabe86a70b81c8175c5fd88
> Author: Marc Zyngier <maz@kernel.org>
> 
> KVM: arm64: vgic: Stop the VM when disabling LPIs
> 
> Disabling LPIs modifies the AP list of the affected CPU, which could be
> running. This commit aims to safely manipulate the state by pausing all
> vCPUs so they are in a quiescent state when disabling LPIs.
> 
> > diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> > index 5913a20d83019..adefc42964732 100644
> > --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> > +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> > @@ -303,9 +303,19 @@ static void vgic_mmio_write_v3r_ctlr(struct kvm_vcpu *vcpu,
> >  		if (ctlr != GICR_CTLR_ENABLE_LPIS)
> >  			return;
> >  
> > +		/*
> > +		 * Yes, disabling LPIs is painful, since it can be done from
> > +		 * a *remote* vcpu! So let's not take any chance, and make
> > +		 * sure that everybody has written their LRs back to the irq
> > +		 * structures, and release any reference they would have.
> > +		 *
> > +		 * If it hurts, don't do it.
> > +		 */
> > +		kvm_arm_halt_guest(vcpu->kvm);
> >  		vgic_flush_pending_lpis(vcpu);
> 
> [Severity: High]
> Does the asynchronous halt ensure the target vCPU has fully synchronized its
> hardware state back to software state before proceeding to flush?
> 
> kvm_arm_halt_guest() uses kvm_make_all_cpus_request(kvm, KVM_REQ_SLEEP).
> If the target vCPU has exited guest mode and set vcpu->mode = OUTSIDE_GUEST_MODE
> but has not yet executed kvm_vgic_sync_hwstate() to fold its LRs,
> kvm_make_all_cpus_request() will skip waiting for it:
> 
> virt/kvm/kvm_main.c:kvm_request_needs_ipi() {
>     ...
> 	if (req & KVM_REQUEST_WAIT)
> 		return mode != OUTSIDE_GUEST_MODE;
>     ...
> }
> 
> If the wait is skipped, could the flushing vCPU clear the ap_list in
> vgic_flush_pending_lpis() just before the target vCPU folds its LRs? 
> Would this incorrectly resurrect the deleted LPI state and cause ghost
> interrupts?

I think there is a kernel of truth here, and I reckon this is a *very*
old bug. Something like the below should help. Oliver what do you
think?

diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 8b080804bc90b..5f5dd8bead4b9 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -1352,7 +1352,6 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
 
 		ret = kvm_arm_vcpu_enter_exit(vcpu);
 
-		vcpu->mode = OUTSIDE_GUEST_MODE;
 		vcpu->stat.exits++;
 		/*
 		 * Back from guest
@@ -1386,6 +1385,12 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
 
 		kvm_arch_vcpu_ctxsync_fp(vcpu);
 
+		/*
+		 * All the state has been synchronised, let advertise
+		 * we're outside of the guest.
+		 */
+		smp_store_release(&vcpu->mode, OUTSIDE_GUEST_MODE);
+
 		/*
 		 * We must ensure that any pending interrupts are taken before
 		 * we exit guest timing so that timer ticks are accounted as


> 
> >  		vgic_its_invalidate_all_caches(vcpu->kvm);
> >  		atomic_set_release(&vgic_cpu->ctlr, 0);
> > +		kvm_arm_resume_guest(vcpu->kvm);
> 
> [Severity: High]
> Does this block need to hold a VM-wide lock?
> 
> Since vgic_mmio_write_v3r_ctlr() only synchronizes locally using an atomic
> cmpxchg on the target vgic_cpu->ctlr, multiple vCPUs could concurrently
> write to different VGIC redistributors to disable their LPIs.
> 
> If this happens, multiple vCPUs will concurrently call kvm_arm_halt_guest()
> and race to set vcpu->arch.pause = true:
> 
> arch/arm64/kvm/arm.c:kvm_arm_halt_guest() {
>     ...
> 	kvm_for_each_vcpu(i, vcpu, kvm)
> 		vcpu->arch.pause = true;
>     ...
> }
> 
> The first vCPU to finish flushing will call kvm_arm_resume_guest() which
> unconditionally clears the pause boolean:
> 
> 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);
> 	}
>     ...
> }
> 
> Could this unpause the VM prematurely while another vCPU is still tearing
> down VGIC state in vgic_flush_pending_lpis(), allowing guests to run and
> modify their VGIC state concurrently?

Just as interesting. It appears that this doesn't nest correctly,
while we really rely on it to work. An obvious way to do this is to
actually turn pause into a refcount. Something like the (untested)
hack below.

	M.

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 27fe0cd5b2d7a..08dc8f816824c 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -889,11 +889,10 @@ struct kvm_vcpu_arch {
 	/*
 	 * Don't run the guest (internal implementation need).
 	 *
-	 * Contrary to the flags above, this is set/cleared outside of
-	 * a vcpu context, and thus cannot be mixed with the flags
-	 * themselves (or the flag accesses need to be made atomic).
+	 * Contrary to the flags above, this is updated outside of
+	 * a vcpu context, and thus cannot be mixed with the flags.
 	 */
-	bool pause;
+	refcount_t pause;
 
 	/*
 	 * We maintain more than a single set of debug registers to support
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 8b080804bc90b..62b683107ccae 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -561,6 +561,7 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
 	kvm_arm_pvtime_vcpu_init(&vcpu->arch);
 
 	vcpu->arch.hw_mmu = &vcpu->kvm->arch.mmu;
+	refcount_set(&vcpu->arch.pause, 0);
 
 	/*
 	 * This vCPU may have been created after mpidr_data was initialized.
@@ -835,6 +836,11 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
 	return ret;
 }
 
+static bool vcpu_can_run(struct kvm_vcpu *vcpu)
+{
+	return !kvm_arm_vcpu_stopped(vcpu) && !refcount_read(&vcpu->arch.pause);
+}
+
 /**
  * kvm_arch_vcpu_runnable - determine if the vcpu can be scheduled
  * @v:		The VCPU pointer
@@ -850,8 +856,7 @@ int kvm_arch_vcpu_runnable(struct kvm_vcpu *v)
 		      (kvm_timer_should_notify_user(v) ||
 		       kvm_pmu_should_notify_user(v)));
 
-	return ((irq_lines || kvm_vgic_vcpu_pending_irq(v))
-		&& !kvm_arm_vcpu_stopped(v) && !v->arch.pause);
+	return ((irq_lines || kvm_vgic_vcpu_pending_irq(v)) && !vcpu_can_run(v));
 }
 
 bool kvm_arch_vcpu_in_kernel(struct kvm_vcpu *vcpu)
@@ -1014,7 +1019,7 @@ void kvm_arm_halt_guest(struct kvm *kvm)
 	struct kvm_vcpu *vcpu;
 
 	kvm_for_each_vcpu(i, vcpu, kvm)
-		vcpu->arch.pause = true;
+		refcount_inc(&vcpu->arch.pause);
 	kvm_make_all_cpus_request(kvm, KVM_REQ_SLEEP);
 }
 
@@ -1024,8 +1029,8 @@ void kvm_arm_resume_guest(struct kvm *kvm)
 	struct kvm_vcpu *vcpu;
 
 	kvm_for_each_vcpu(i, vcpu, kvm) {
-		vcpu->arch.pause = false;
-		__kvm_vcpu_wake_up(vcpu);
+		if (!refcount_dec_and_test(&vcpu->arch.pause) == 0)
+			__kvm_vcpu_wake_up(vcpu);
 	}
 }
 
@@ -1034,10 +1039,10 @@ static void kvm_vcpu_sleep(struct kvm_vcpu *vcpu)
 	struct rcuwait *wait = kvm_arch_vcpu_get_wait(vcpu);
 
 	rcuwait_wait_event(wait,
-			   (!kvm_arm_vcpu_stopped(vcpu)) && (!vcpu->arch.pause),
+			   vcpu_can_run(vcpu),
 			   TASK_INTERRUPTIBLE);
 
-	if (kvm_arm_vcpu_stopped(vcpu) || vcpu->arch.pause) {
+	if (!vcpu_can_run(vcpu)) {
 		/* Awaken to handle a signal, request we sleep again later. */
 		kvm_make_request(KVM_REQ_SLEEP, vcpu);
 	}

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