Re: [PATCH v10 01/32] ARM: vGIC: avoid rank lock when reading priority

[Julien Grall <julien.grall@arm.com>]

Hi Andre,

On 26/05/17 18:35, Andre Przywara wrote:
> When reading the priority value of a virtual interrupt, we were taking
> the respective rank lock so far.
> However for forwarded interrupts (Dom0 only so far) this may lead to a
> deadlock with the following call chain:
> - MMIO access to change the IRQ affinity, calling the ITARGETSR handler
> - this handler takes the appropriate rank lock and calls vgic_store_itargetsr()
> - vgic_store_itargetsr() will eventually call vgic_migrate_irq()
> - if this IRQ is already in-flight, it will remove it from the old
>   VCPU and inject it into the new one, by calling vgic_vcpu_inject_irq()
> - vgic_vcpu_inject_irq will call vgic_get_virq_priority()
> - vgic_get_virq_priority() tries to take the rank lock - again!
> It seems like this code path has never been exercised before.
>
> Fix this by avoiding taking the lock in vgic_get_virq_priority() (like we
> do in vgic_get_target_vcpu()).
> Actually we are just reading one byte, and priority changes while
> interrupts are handled are a benign race that can happen on real hardware
> too. So it looks safe to just use read_atomic() instead.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  xen/arch/arm/vgic.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 83569b0..54b2aad 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -227,14 +227,8 @@ struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int virq)
>  static int vgic_get_virq_priority(struct vcpu *v, unsigned int virq)
>  {
>      struct vgic_irq_rank *rank = vgic_rank_irq(v, virq);
> -    unsigned long flags;
> -    int priority;
> -
> -    vgic_lock_rank(v, rank, flags);
> -    priority = rank->priority[virq & INTERRUPT_RANK_MASK];
> -    vgic_unlock_rank(v, rank, flags);
>
> -    return priority;
> +    return read_atomic(&rank->priority[virq & INTERRUPT_RANK_MASK]);

The write in rank->priority will not be atomic (see vgic_reg_update 
implementation): the register is first masked, the the priority set.

So you may end up to read 0 (which is the higher priority) by mistake.

We should probably think to make vgic_reg_* helper atomic.

>  }
>
>  bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
>

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel