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

[Marc Zyngier <maz@kernel.org>]

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.