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

[Oliver Upton <oupton@kernel.org>]

Hey,

On Wed, Sep 23, 2026 at 12:21:15AM +0100, Marc Zyngier wrote:
> 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
> 

Yeah, this looks reasonable to me.

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

No issues with this approach but it is worth noting that all other
callers of kvm_arm_halt_guest() do so behind the config_lock today. You
could probably do the same with the LPI disable path.

Thanks,
Oliver