Re: [PATCH v2 2/5] KVM: arm64: vgic-v3: Reset assignments after failed region setup

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Naive decrement of `rdreg->free_index` during concurrent vCPU destruction corrupts the redistributor allocation state, causing overlapping MMIO base addresses.
--

--- Patch [2]: [PATCH v2 2/5] KVM: arm64: vgic-v3: Reset assignments after failed region setup ---
commit 00ec61ba92a90418f1516339ae4c96442b750b29
Author: Karl Mehltretter <[email protected]>

KVM: arm64: vgic-v3: Reset assignments after failed region setup

This commit makes redistributor unregistration clear the cached
assignment and return the region slot to address an issue where a
failed REDIST_REGION write leaves redistributors partially registered.
It also updates the rollback path in vgic_register_all_redist_iodevs().

> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> index c75caadbd1d3c..331d203acb430 100644
> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> @@ -852,6 +852,7 @@ void vgic_unregister_redist_iodev(struct kvm_vcpu *vcpu)
>  	struct vgic_io_device *rd_dev = &vcpu->arch.vgic_cpu.rd_iodev;
>  
>  	kvm_io_bus_unregister_dev(vcpu->kvm, KVM_MMIO_BUS, &rd_dev->dev);
> +	vgic_unassign_redist_iodev(vcpu);

[Severity: High]
Does calling vgic_unassign_redist_iodev() here corrupt the redistributor
allocation state during concurrent vCPU destruction?

Because vgic_unregister_redist_iodev() is also called from the vCPU error
path in __kvm_vgic_vcpu_destroy(), this could be triggered if a vCPU
creation fails.

If multiple vCPUs are created concurrently, and VCPU A and VCPU B acquire
their redistributor slots sequentially (e.g., slots X and X+1), and then
VCPU A's creation fails, the error path for VCPU A will call:

__kvm_vgic_vcpu_destroy()
  vgic_unregister_redist_iodev()
    vgic_unassign_redist_iodev()

This blindly decrements the free_index from X+2 to X+1, since
vgic_unassign_redist_iodev() assumes it is unassigning the most recently
allocated slot:

arch/arm64/kvm/vgic/vgic-mmio-v3.c:vgic_unassign_redist_iodev() {
	...
	if (vgic_cpu->rdreg) {
		vgic_cpu->rdreg->free_index--;
		vgic_cpu->rdreg = NULL;
	}
	...
}

When VCPU C is later created, will it be assigned slot X+1, leading to an
overlap with VCPU B's MMIO base address?

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2