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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 30 May 2017, Julien Grall wrote:
> 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@xxxxxxx>
> > ---
> >  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.

Right! That's why I wrote
alpine.DEB.2.10.1705231135130.18759@sstabellini-ThinkPad-X260


> >  }
> > 
> >  bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
> > 
> 
> Cheers,
> 
> -- 
> Julien Grall
> 

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