Re: [PATCH] arm/acpi: Fix the deadlock in function vgic_lock_rank()

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

Hi Stefano,

On 31/05/16 10:40, Stefano Stabellini wrote:
> On Mon, 30 May 2016, Julien Grall wrote:
>> ACPI can only be enabled in expert mode and will be a tech-preview for Xen
>> 4.7. So I would revert the patch.  SPIs will not be routed, but it is better
>> than a deadlock.
>>
>> I would also replace the patch with a warning until the issue will be fixed in
>> Xen 4.8.
>>
>> Any opinions?
>>
>>> +int gic_route_irq_to_guest(struct domain *d, unsigned int virq,
>>> +                           struct irq_desc *desc, unsigned int priority)
>>> +{
>>> +    unsigned long flags;
>>> +    int lock = 0, retval;
>>> +    struct vgic_irq_rank *rank;
>>> +
>>> +    /* Use vcpu0 to retrieve the pending_irq struct. Given that we only
>>> +     * route SPIs to guests, it doesn't make any difference. */
>>> +    rank = vgic_rank_irq(d->vcpu[0], virq);
>>> +
>>> +    /* Take the rank spinlock unless it has already been taken by the
>>> +     * caller. */
>>> +    if ( !spin_is_locked(&rank->lock) ) {
>>
>> AFAICT, spin_is_locked only tell us that someone has locked the rank. So this
>> would be unsafe.
>
> The code is checking if the lock is already taken, and if it is not
> taken, it will take the lock. The purpose of this code is to
> allow gic_route_irq_to_guest to be called by both functions which
> already have the lock held and functions that do not. The same goal
> could be achieved by duplicating gic_route_irq_to_guest into two
> identical functions except for the lock taking. That would be
> admittedly a more obvious fix but also a particularly ugly one.

spin_is_locked does not work as you expect. The function will not tell 
you if the lock was taken by the current CPU, but if the lock was taken 
by *a* CPU.

It would be possible to have CPU A calling this function and have CPU B 
with the lock taken. So the data structure would be accessed by 2 CPUs 
concurrently, which is unsafe.

Regards,

-- 
Julien Grall

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