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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 19 Jun 2017, Julien Grall wrote:
> Hi,
> 
> On 14/06/17 18:55, Julien Grall wrote:
> > Hi Andre,
> > 
> > On 06/14/2017 05:51 PM, 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 is safe to just prevent the compiler from reading from the
> > > struct more than once.
> > > 
> > > Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> > 
> > Reviewed-by: Julien Grall <julien.grall@xxxxxxx>
> 
> As you mentioned in the cover letter, I think it would be good to get this
> fixed in Xen 4.9. Stefano what do you think?

Yes, I think it makes sense. Do you release-ack it?

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