Re: [Xen-devel] [PATCH v1] xen/arm: Fix deadlock in gic_set_guest_irq()

[George Dunlap <george.dunlap@xxxxxxxxxxxxx>]

On 02/04/2014 03:52 PM, Julien Grall wrote:
> Forget to add George.
>
> On 02/03/2014 05:51 PM, Julien Grall wrote:
> > (+ Xen ARM maintainers)
> >
> > Hello Oleksandr,
> >
> > Thanks for the patch. For next time, can you add the Xen ARM maintainers
> > in cc? With the amount of mail in the mailing list, your mail could be
> > lost easily. :)
> >
> > On 02/03/2014 05:33 PM, Oleksandr Tyshchenko wrote:
> > > The possible deadlock scenario is explained below:
> > >
> > > non interrupt context:    interrupt contex        interrupt context
> > >                            (CPU0):                 (CPU1):
> > > vgic_distr_mmio_write()   do_trap_irq()           do_softirq()
> > >    |                         |                       |
> > >    vgic_disable_irqs()       ...                     ...
> > >      |                         |                       |
> > >      gic_remove_from_queues()  vgic_vcpu_inject_irq()  vgic_vcpu_inject_irq()
> > >      |  ...                      |                       |
> > >      |  spin_lock(...)           gic_set_guest_irq()     gic_set_guest_irq()
> > >      |  ...                        ...                     ...
> > >      |  ... <----------------.---- spin_lock_irqsave(...)  ...
> > >      |  ... 
> > > <----------------.-.---------------------------spin_lock_irqsave(...)
> > >      |  ...                  . .       Oops! The lock has already taken.
> > >      |  spin_unlock(...)     . .
> > >      |  ...                  . .
> > >      gic_irq_disable()       . .
> > >         ...                  . .
> > >         spin_lock(...)       . .
> > >         ...                  . .
> > >         ... <----------------. .
> > >         ... <------------------.
> > >         ...
> > >         spin_unlock(...)
> > >
> > > Since the gic_remove_from_queues() and gic_irq_disable() called from
> > > non interrupt context and they acquire the same lock as gic_set_guest_irq()
> > > which called from interrupt context we must disable interrupts in these
> > > functions to avoid possible deadlocks.
> > >
> > > Change-Id: Ia354d87bb44418956e30cd7e49cc76616c359cc9
> > > Signed-off-by: Oleksandr Tyshchenko <oleksandr.tyshchenko@xxxxxxxxxxxxxxx>
> > Acked-by: Julien Grall <julien.grall@xxxxxxxxxx>
> >
> > I think this patch should have a release exception for Xen 4.4. It's fix
> > a race condition in the interrupt management.
Release-acked-by: George Dunlap <george.dunlap@xxxxxxxxxxxxx>

> > > ---
> > >   xen/arch/arm/gic.c |   10 ++++++----
> > >   1 file changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > > index c44a4d0..7d83b0c 100644
> > > --- a/xen/arch/arm/gic.c
> > > +++ b/xen/arch/arm/gic.c
> > > @@ -147,14 +147,15 @@ static void gic_irq_enable(struct irq_desc *desc)
> > >   static void gic_irq_disable(struct irq_desc *desc)
> > >   {
> > >       int irq = desc->irq;
> > > +    unsigned long flags;
> > >   
> > > -    spin_lock(&desc->lock);
> > > +    spin_lock_irqsave(&desc->lock, flags);
> > >       spin_lock(&gic.lock);
> > >       /* Disable routing */
> > >       GICD[GICD_ICENABLER + irq / 32] = (1u << (irq % 32));
> > >       desc->status |= IRQ_DISABLED;
> > >       spin_unlock(&gic.lock);
> > > -    spin_unlock(&desc->lock);
> > > +    spin_unlock_irqrestore(&desc->lock, flags);
> > >   }
> > >   
> > >   static unsigned int gic_irq_startup(struct irq_desc *desc)
> > > @@ -658,11 +659,12 @@ static inline void gic_add_to_lr_pending(struct vcpu *v, 
> > > unsigned int irq,
> > >   void gic_remove_from_queues(struct vcpu *v, unsigned int virtual_irq)
> > >   {
> > >       struct pending_irq *p = irq_to_pending(v, virtual_irq);
> > > +    unsigned long flags;
> > >   
> > > -    spin_lock(&gic.lock);
> > > +    spin_lock_irqsave(&gic.lock, flags);
> > >       if ( !list_empty(&p->lr_queue) )
> > >           list_del_init(&p->lr_queue);
> > > -    spin_unlock(&gic.lock);
> > > +    spin_unlock_irqrestore(&gic.lock, flags);
> > >   }
> > >   
> > >   void gic_set_guest_irq(struct vcpu *v, unsigned int virtual_irq,

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