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

[sashiko-bot@kernel.org]

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?

>  		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?

>  	} else {
>  		ctlr = atomic_cmpxchg_acquire(&vgic_cpu->ctlr, 0,
>  					      GICR_CTLR_ENABLE_LPIS);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260922214212.3327146-1-maz@kernel.org?part=3