Re: [Xen-devel] [PATCH v3 06/15] x86/IRQ: fix locking around vector management

[Jan Beulich <JBeulich@xxxxxxxx>]

On 03.07.2019 20:23, Andrew Cooper wrote:
> On 17/05/2019 11:47, Jan Beulich wrote:
>> All of __{assign,bind,clear}_irq_vector() manipulate struct irq_desc
>> fields, and hence ought to be called with the descriptor lock held in
>> addition to vector_lock. This is currently the case for only
>> set_desc_affinity() (in the common case) and destroy_irq(), which also
>> clarifies what the nesting behavior between the locks has to be.
>> Reflect the new expectation by having these functions all take a
>> descriptor as parameter instead of an interrupt number.
>>
>> Also take care of the two special cases of calls to set_desc_affinity():
>> set_ioapic_affinity_irq() and VT-d's dma_msi_set_affinity() get called
>> directly as well, and in these cases the descriptor locks hadn't got
>> acquired till now. For set_ioapic_affinity_irq() this means acquiring /
>> releasing of the IO-APIC lock can be plain spin_{,un}lock() then.
>>
>> Drop one of the two leading underscores from all three functions at
>> the same time.
>>
>> There's one case left where descriptors get manipulated with just
>> vector_lock held: setup_vector_irq() assumes its caller to acquire
>> vector_lock, and hence can't itself acquire the descriptor locks (wrong
>> lock order). I don't currently see how to address this.
> 
> In practice, the only mutation is setting a bit in cpu_mask for the
> shared high priority vectors, so looks to be safe in practice.

I had tried to convince myself that it's safe in practice, but I'm
afraid I couldn't (and hence wouldn't want to say so in the patch
description here). There's one important thing to pay attention to:
Not all manipulations of ->arch.cpu_mask are atomic (and there's
really no way for them to be, with our current cpumask
infrastructure) - all of them assume to be done under lock. And
other than when offlining a CPU we're not in a fully synchronized
state while onlining one.

> The callers use of the vector_lock looks like a bodge though.

Well, it's definitely not nice, but unavoidable (afaict).

> However,  this analysis needs to be added to the comment for
> setup_vector_irq(), because the behaviour is extremely fragile and
> mustn't change.

Will do.

>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx> [VT-d]
>> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> With some form of adjustment to the comment for setup_vector_irq(), and
> ideally to the commit message about safety in practice, Acked-by: Andrew
> Cooper <andrew.cooper3@xxxxxxxxxx>

Thanks.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel