[PATCH 0/2] KVM: arm64: Ensure VGIC is fully initialized before entering guest

[PATCH 0/2] KVM: arm64: Ensure VGIC is fully initialized before entering guest

[Oliver Upton]

Fix for the race [*] that I had mentioned last week. The first patch
isn't entirely necessary, but it gets rid of a bogus check that could've
potentially raced with kvm_vgic_map_resources() as vgic_dist::ready is
no longer protected with the config_lock.

Applies to kvmarm/fixes.

Oliver Upton (2):
  KVM: arm64: vgic: Don't check for vgic_ready() when setting NR_IRQS
  KVM: arm64: Ensure vgic_ready() is ordered against MMIO registration

 arch/arm64/kvm/vgic/vgic-init.c       | 13 +++++++++++--
 arch/arm64/kvm/vgic/vgic-kvm-device.c |  7 ++++++-
 2 files changed, 17 insertions(+), 3 deletions(-)


base-commit: df5fd75ee305cb5927e0b1a0b46cc988ad8db2b1
-- 
2.47.0.rc1.288.g06298d1525-goog

[PATCH 1/2] KVM: arm64: vgic: Don't check for vgic_ready() when setting NR_IRQS

[Oliver Upton]

KVM commits to a particular sizing of SPIs when the vgic is initialized,
which is before the point a vgic becomes ready. On top of that, KVM
supplies a default amount of SPIs should userspace not explicitly
configure this.

As such, the check for vgic_ready() in the handling of
KVM_DEV_ARM_VGIC_GRP_NR_IRQS is completely wrong, and testing if nr_spis
is nonzero is sufficient for preventing userspace from playing games
with us.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/kvm/vgic/vgic-kvm-device.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/vgic/vgic-kvm-device.c b/arch/arm64/kvm/vgic/vgic-kvm-device.c
index 1d26bb5b02f4..5f4f57aaa23e 100644
--- a/arch/arm64/kvm/vgic/vgic-kvm-device.c
+++ b/arch/arm64/kvm/vgic/vgic-kvm-device.c
@@ -236,7 +236,12 @@ static int vgic_set_common_attr(struct kvm_device *dev,
 
 		mutex_lock(&dev->kvm->arch.config_lock);
 
-		if (vgic_ready(dev->kvm) || dev->kvm->arch.vgic.nr_spis)
+		/*
+		 * Either userspace has already configured NR_IRQS or
+		 * the vgic has already been initialized and vgic_init()
+		 * supplied a default amount of SPIs.
+		 */
+		if (dev->kvm->arch.vgic.nr_spis)
 			ret = -EBUSY;
 		else
 			dev->kvm->arch.vgic.nr_spis =
-- 
2.47.0.rc1.288.g06298d1525-goog

[PATCH 2/2] KVM: arm64: Ensure vgic_ready() is ordered against MMIO registration

[Oliver Upton]

kvm_vgic_map_resources() prematurely marks the distributor as 'ready',
potentially allowing vCPUs to enter the guest before the distributor's
MMIO registration has been made visible.

Plug the race by marking the distributor as ready only after MMIO
registration is completed. Rely on the implied ordering of
synchronize_srcu() to ensure the MMIO registration is visible before
vgic_dist::ready. This also means that writers to vgic_dist::ready are
now serialized by the slots_lock, which was effectively the case already
as all writers held the slots_lock in addition to the config_lock.

Fixes: 59112e9c390b ("KVM: arm64: vgic: Fix a circular locking issue")
Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/kvm/vgic/vgic-init.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
index d88fdeaf6144..48c952563e85 100644
--- a/arch/arm64/kvm/vgic/vgic-init.c
+++ b/arch/arm64/kvm/vgic/vgic-init.c
@@ -544,14 +544,23 @@ int kvm_vgic_map_resources(struct kvm *kvm)
 	if (ret)
 		goto out;
 
-	dist->ready = true;
 	dist_base = dist->vgic_dist_base;
 	mutex_unlock(&kvm->arch.config_lock);
 
 	ret = vgic_register_dist_iodev(kvm, dist_base, type);
-	if (ret)
+	if (ret) {
 		kvm_err("Unable to register VGIC dist MMIO regions\n");
+		goto out_slots;
+	}
 
+	/*
+	 * kvm_io_bus_register_dev() guarantees all readers see the new MMIO
+	 * registration before returning through synchronize_srcu(), which also
+	 * implies a full memory barrier. As such, marking the distributor as
+	 * 'ready' here is guaranteed to be ordered after all vCPUs having seen
+	 * a completely configured distributor.
+	 */
+	dist->ready = true;
 	goto out_slots;
 out:
 	mutex_unlock(&kvm->arch.config_lock);
-- 
2.47.0.rc1.288.g06298d1525-goog

Re: [PATCH 0/2] KVM: arm64: Ensure VGIC is fully initialized before entering guest

[Oliver Upton]

On Thu, Oct 17, 2024 at 12:19:45AM +0000, Oliver Upton wrote:
> Fix for the race [*] that I had mentioned last week. The first patch
> isn't entirely necessary, but it gets rid of a bogus check that could've
> potentially raced with kvm_vgic_map_resources() as vgic_dist::ready is
> no longer protected with the config_lock.
> 
> Applies to kvmarm/fixes.

-ENEEDBEER

Here's the link I meant to add.

[*]: https://lore.kernel.org/kvmarm/ZweUiHMC3RNwNXzY@linux.dev/

> Oliver Upton (2):
>   KVM: arm64: vgic: Don't check for vgic_ready() when setting NR_IRQS
>   KVM: arm64: Ensure vgic_ready() is ordered against MMIO registration
> 
>  arch/arm64/kvm/vgic/vgic-init.c       | 13 +++++++++++--
>  arch/arm64/kvm/vgic/vgic-kvm-device.c |  7 ++++++-
>  2 files changed, 17 insertions(+), 3 deletions(-)
> 
> 
> base-commit: df5fd75ee305cb5927e0b1a0b46cc988ad8db2b1
> -- 
> 2.47.0.rc1.288.g06298d1525-goog
> 

-- 
Thanks,
Oliver

Re: [PATCH 2/2] KVM: arm64: Ensure vgic_ready() is ordered against MMIO registration

[Marc Zyngier]

On Thu, 17 Oct 2024 01:19:47 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> kvm_vgic_map_resources() prematurely marks the distributor as 'ready',
> potentially allowing vCPUs to enter the guest before the distributor's
> MMIO registration has been made visible.
> 
> Plug the race by marking the distributor as ready only after MMIO
> registration is completed. Rely on the implied ordering of
> synchronize_srcu() to ensure the MMIO registration is visible before
> vgic_dist::ready. This also means that writers to vgic_dist::ready are
> now serialized by the slots_lock, which was effectively the case already
> as all writers held the slots_lock in addition to the config_lock.
> 
> Fixes: 59112e9c390b ("KVM: arm64: vgic: Fix a circular locking issue")
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/kvm/vgic/vgic-init.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
> index d88fdeaf6144..48c952563e85 100644
> --- a/arch/arm64/kvm/vgic/vgic-init.c
> +++ b/arch/arm64/kvm/vgic/vgic-init.c
> @@ -544,14 +544,23 @@ int kvm_vgic_map_resources(struct kvm *kvm)
>  	if (ret)
>  		goto out;
>  
> -	dist->ready = true;
>  	dist_base = dist->vgic_dist_base;
>  	mutex_unlock(&kvm->arch.config_lock);
>  
>  	ret = vgic_register_dist_iodev(kvm, dist_base, type);
> -	if (ret)
> +	if (ret) {
>  		kvm_err("Unable to register VGIC dist MMIO regions\n");
> +		goto out_slots;
> +	}
>  
> +	/*
> +	 * kvm_io_bus_register_dev() guarantees all readers see the new MMIO
> +	 * registration before returning through synchronize_srcu(), which also
> +	 * implies a full memory barrier. As such, marking the distributor as
> +	 * 'ready' here is guaranteed to be ordered after all vCPUs having seen
> +	 * a completely configured distributor.
> +	 */
> +	dist->ready = true;
>  	goto out_slots;
>  out:
>  	mutex_unlock(&kvm->arch.config_lock);

I thought we would benefit from the removal of the early-out at the
beginning of the function, specially given that this is only called on
the first run of any vcpu -- and we can probably afford to take the
locks on such a rare event. My impression is that we'd gain in
robustness and maintainability.

This can come as a later patch if we agree that this is valuable.

Thanks,

	M.

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

Re: [PATCH 0/2] KVM: arm64: Ensure VGIC is fully initialized before entering guest

[Marc Zyngier]

On Thu, 17 Oct 2024 00:19:45 +0000, Oliver Upton wrote:
> Fix for the race [*] that I had mentioned last week. The first patch
> isn't entirely necessary, but it gets rid of a bogus check that could've
> potentially raced with kvm_vgic_map_resources() as vgic_dist::ready is
> no longer protected with the config_lock.
> 
> Applies to kvmarm/fixes.
> 
> [...]

Applied to fixes, thanks!

[1/2] KVM: arm64: vgic: Don't check for vgic_ready() when setting NR_IRQS
      commit: 5978d4ec7e82ffc472ac2645601dd10b09e61b0f
[2/2] KVM: arm64: Ensure vgic_ready() is ordered against MMIO registration
      commit: 78a00555550042ed77b33ace7423aced228b3b4e

Cheers,

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

Re: [PATCH 2/2] KVM: arm64: Ensure vgic_ready() is ordered against MMIO registration

[Oliver Upton]

On Thu, Oct 17, 2024 at 08:58:30AM +0100, Marc Zyngier wrote:
> On Thu, 17 Oct 2024 01:19:47 +0100, Oliver Upton <oliver.upton@linux.dev> wrote:
> I thought we would benefit from the removal of the early-out at the
> beginning of the function, specially given that this is only called on
> the first run of any vcpu -- and we can probably afford to take the
> locks on such a rare event. My impression is that we'd gain in
> robustness and maintainability.
> 
> This can come as a later patch if we agree that this is valuable.

I should've mentioned my rationale a bit more here, thanks for queuing
the patch anyway. Prolonged blackout for live migration is a problem for
larger VMs, and I was too chicken to have all vCPUs pile up on slots_lock
and config_lock.

Too lazy to prove this is/isn't an issue, so I figured I'd preserve the
existing flow as much as possible. If we get this wrong again (which
wouldn't be surprising) then let's drop the early out and just take the
locks.

Seem reasonable?

-- 
Thanks,
Oliver