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

[George Dunlap <george.dunlap@eu.citrix.com>]

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@globallogic.com>
>> Acked-by: Julien Grall <julien.grall@linaro.org>
>>
>> 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@eu.citrix.com>

>>
>>> ---
>>>   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,
>>>
>>
>