diff options
author | Jan Beulich <jbeulich@suse.com> | 2013-05-31 08:35:22 +0200 |
---|---|---|
committer | Jan Beulich <jbeulich@suse.com> | 2013-05-31 08:35:22 +0200 |
commit | 43427e65ccfea0c6dc0232f358287e6cc616b507 (patch) | |
tree | 47662a1de438c7787f19b3cdf813159dc5b2dd51 /xen | |
parent | 7476f4788250ef281e09b449753c7ddc19239c64 (diff) | |
download | xen-43427e65ccfea0c6dc0232f358287e6cc616b507.tar.gz xen-43427e65ccfea0c6dc0232f358287e6cc616b507.tar.bz2 xen-43427e65ccfea0c6dc0232f358287e6cc616b507.zip |
x86: fix ordering of operations in destroy_irq()
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>
Acked-by: Keir Fraser <keir@xen.org>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: George Dunlap <george.dunlap@eu.citrix.com>
Diffstat (limited to 'xen')
-rw-r--r-- | xen/arch/x86/irq.c | 43 |
1 files changed, 20 insertions, 23 deletions
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c index b0b0c655bd..699221ae99 100644 --- a/xen/arch/x86/irq.c +++ b/xen/arch/x86/irq.c @@ -197,12 +197,24 @@ int create_irq(int node) return irq; } -static void dynamic_irq_cleanup(unsigned int irq) +void destroy_irq(unsigned int irq) { struct irq_desc *desc = irq_to_desc(irq); unsigned long flags; struct irqaction *action; + BUG_ON(!MSI_IRQ(irq)); + + if ( dom0 ) + { + int err = irq_deny_access(dom0, irq); + + if ( err ) + printk(XENLOG_G_ERR + "Could not revoke Dom0 access to IRQ%u (error %d)\n", + irq, err); + } + spin_lock_irqsave(&desc->lock, flags); desc->status |= IRQ_DISABLED; desc->status &= ~IRQ_GUEST; @@ -210,16 +222,19 @@ static void dynamic_irq_cleanup(unsigned int irq) action = desc->action; desc->action = NULL; desc->msi_desc = NULL; - desc->handler = &no_irq_type; - desc->arch.used_vectors = NULL; cpumask_setall(desc->affinity); spin_unlock_irqrestore(&desc->lock, flags); /* Wait to make sure it's not being used on another CPU */ do { smp_mb(); } while ( desc->status & IRQ_INPROGRESS ); - if (action) - xfree(action); + spin_lock_irqsave(&desc->lock, flags); + desc->handler = &no_irq_type; + clear_irq_vector(irq); + desc->arch.used_vectors = NULL; + spin_unlock_irqrestore(&desc->lock, flags); + + xfree(action); } static void __clear_irq_vector(int irq) @@ -286,24 +301,6 @@ void clear_irq_vector(int irq) spin_unlock_irqrestore(&vector_lock, flags); } -void destroy_irq(unsigned int irq) -{ - BUG_ON(!MSI_IRQ(irq)); - - if ( dom0 ) - { - int err = irq_deny_access(dom0, irq); - - if ( err ) - printk(XENLOG_G_ERR - "Could not revoke Dom0 access to IRQ%u (error %d)\n", - irq, err); - } - - dynamic_irq_cleanup(irq); - clear_irq_vector(irq); -} - int irq_to_vector(int irq) { int vector = -1; |