Re: [PATCH] KVM: arm/arm64: vgic-its: Add error handling in vgic_its_cache_translation

[Marc Zyngier <maz@kernel.org>]

On Thu, 28 Nov 2024 13:45:34 +0000,
Keisuke Nishimura <keisuke.nishimura@inria.fr> wrote:
> 
> The xa_store() may fail because there is no guarantee that the cache_key
> index is already used in its->translation_cache. This fix (1) resolves
> the kref inconsistency on failure and (2) returns the error code.

Please describe the failure mode. Under which circumstances can this
fail?

> 
> Fixes: 8201d1028caa ("KVM: arm64: vgic-its: Maintain a translation cache per ITS")
> Signed-off-by: Keisuke Nishimura <keisuke.nishimura@inria.fr>
> ---
>  arch/arm64/kvm/vgic/vgic-its.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> index 198296933e7e..8f423857b7d2 100644
> --- a/arch/arm64/kvm/vgic/vgic-its.c
> +++ b/arch/arm64/kvm/vgic/vgic-its.c
> @@ -555,7 +555,7 @@ static struct vgic_irq *vgic_its_check_cache(struct kvm *kvm, phys_addr_t db,
>  	return irq;
>  }
>  
> -static void vgic_its_cache_translation(struct kvm *kvm, struct vgic_its *its,
> +static int vgic_its_cache_translation(struct kvm *kvm, struct vgic_its *its,
>  				       u32 devid, u32 eventid,
>  				       struct vgic_irq *irq)
>  {
> @@ -564,7 +564,11 @@ static void vgic_its_cache_translation(struct kvm *kvm, struct vgic_its *its,
>  
>  	/* Do not cache a directly injected interrupt */
>  	if (irq->hw)
> -		return;
> +		return 0;
> +
> +	old = xa_store(&its->translation_cache, cache_key, irq, GFP_KERNEL_ACCOUNT);
> +	if (xa_is_err(old))
> +		return xa_err(old);

Accessing the translation cache before the assert that checks the ITS
lock is on its own pretty dodgy...

Thanks,

	M.

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