Re: [PATCH] x86: fix ordering of operations in destroy_irq()

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

On 05/29/2013 07:58 AM, Jan Beulich wrote:
> The fix for XSA-36, switching the default of vector map management to
> be per-device, exposed more readily a problem with the cleanup of these
> vector maps: dynamic_irq_cleanup() clearing desc->arch.used_vectors
> keeps the subsequently invoked clear_irq_vector() from clearing the
> bits for both the in-use and a possibly still outstanding old vector.
>
> Fix this by folding dynamic_irq_cleanup() into destroy_irq(), which was
> its only caller, deferring the clearing of the vector map pointer until
> after clear_irq_vector().
>
> Once at it, also defer resetting of desc->handler until after the loop
> around smp_mb() checking for IRQ_INPROGRESS to be clear, fixing a
> (mostly theoretical) issue with the intercation with do_IRQ(): If we
> don't defer the pointer reset, do_IRQ() could, for non-guest IRQs, call
> ->ack() and ->end() with different ->handler pointers, potentially
> leading to an IRQ remaining un-acked. The issue is mostly theoretical
> because non-guest IRQs are subject to destroy_irq() only on (boot time)
> error paths.
>
> As to the changed locking: Invoking clear_irq_vector() with desc->lock
> held is okay because vector_lock already nests inside desc->lock (proven
> by set_desc_affinity(), which takes vector_lock and gets called from
> various desc->handler->ack implementations, getting invoked with
> desc->lock held).
>
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

How big of an impact is this bug?  How many people are actually affected 
by it?

It's a bit hard for me to tell from the description, but it looks like 
it's code motion, then some "theoretical" issues.

Remember our three goals:
- A bug-free release
- An awesome release
- An on-time release

Is the improvement this patch represents worth the potential risk of 
bugs at this point?

  -George