Re: [PATCH 01/16] KVM: arm64: vgic: Split out common LPI allocation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] kvm: data race on irq->group due to unlocked assignment after publication
--

commit 0cabb3a5e012e80ff468a8654506c83304b15486
Author: Sascha Bischoff <sascha.bischoff@arm.com>

KVM: arm64: vgic: Split out common LPI allocation

The GICv3 ITS code currently owns vgic_add_lpi(), even though the
first half of the helper is not ITS specific. It allocates or finds
the VM-wide vgic_irq object in lpi_xa and initializes the common LPI
state before the GICv3 ITS code reads the property table and pending
state.

> diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> index e5657cb603567..8e7bee1bae95d 100644
> --- a/arch/arm64/kvm/vgic/vgic-its.c
> +++ b/arch/arm64/kvm/vgic/vgic-its.c
> @@ -66,77 +66,22 @@ static int update_lpi_config(struct kvm *kvm, struct vgic_irq *irq,
[ ... ]
> -	irq = kzalloc_obj(struct vgic_irq, GFP_KERNEL_ACCOUNT);
> -	if (!irq)
> -		return ERR_PTR(-ENOMEM);
[ ... ]
> +	irq = vgic_add_lpi(kvm, intid, vcpu);
> +	if (IS_ERR(irq))
> +		return irq;
>  
> -	irq->config = VGIC_CONFIG_EDGE;
> -	refcount_set(&irq->refcount, 1);
> -	irq->intid = intid;
> -	irq->target_vcpu = vcpu;
>  	irq->group = 1;

[Severity: High]
Does this code introduce a data race on irq->group in vgic_its_add_lpi()?

Since vgic_add_lpi() now publishes the object to the globally visible
lpi_xa xarray and drops the locks, a concurrent thread (e.g., another vCPU
writing to GICR_INVLPIR or mapping the same LPI on another ITS) can
retrieve the LPI and access irq->group under irq_lock before we set it here.

This can misroute or drop the LPI if it is evaluated by vgic_irq_cmp()
as Group 0 instead of Group 1. Should we set the group before publishing,
or hold irq->irq_lock when assigning it?

> diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c
> index c2726d4c06e6e..bc0171c479a95 100644
> --- a/arch/arm64/kvm/vgic/vgic.c
> +++ b/arch/arm64/kvm/vgic/vgic.c
> @@ -79,6 +79,78 @@ static struct vgic_irq *vgic_get_lpi(struct kvm *kvm, u32 intid)
[ ... ]
> +/*
> + * Creates a new (reference to a) struct vgic_irq for a given LPI.
> + * If this LPI already exists, we increase its refcount and return it.
> + * If this is a "new" LPI, we allocate and initialize a new struct vgic_irq.
> + * This function returns a pointer to the _unlocked_ structure.
> + */
> +struct vgic_irq *vgic_add_lpi(struct kvm *kvm, u32 intid,
> +			      struct kvm_vcpu *vcpu)
> +{
[ ... ]
> +		/*
> +		 * The entry is either empty or contains a dead LPI (refcount=0)
> +		 * from the deferred release path, pending cleanup by
> +		 * vgic_release_deleted_lpis(). Evict and free it if present.
> +		 */
> +		oldirq = __xa_store(&dist->lpi_xa, intid, irq,
> +				    GFP_NOWAIT | __GFP_ACCOUNT);

[Severity: High]
Is it safe to publish the new LPI here while its group field is still
uninitialized (0)?

As mentioned above, doing this in vgic_add_lpi() makes the LPI accessible to
concurrent lookups in vgic_get_irq() before vgic_its_add_lpi() has finished
setting up the group field.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016155741.979985-1-sascha.bischoff@arm.com?part=1