From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH] x86: fix ordering of operations in destroy_irq() Date: Thu, 30 May 2013 17:23:07 +0100 Message-ID: <51A77CEB.6030409@eu.citrix.com> References: <51A5C33A02000078000D974A@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <51A5C33A02000078000D974A@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Andrew Cooper , Keir Fraser , xen-devel List-Id: xen-devel@lists.xenproject.org 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 > Signed-off-by: Jan Beulich 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