From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sheng Yang Subject: Re: [PATCH 15/15] KVM: Fix racy in kvm_free_assigned_irq Date: Wed, 31 Dec 2008 13:43:54 +0800 Message-ID: <200812311343.55772.sheng@linux.intel.com> References: <20081225115609.GA10087@syang10-desktop> <200812301014.10487.sheng@linux.intel.com> <20081230164551.GA5826@amt.cnet> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Cc: Amit Shah , Avi Kivity , kvm@vger.kernel.org, Amit Shah , "Han, Weidong" To: Marcelo Tosatti Return-path: Received: from mga06.intel.com ([134.134.136.21]:30646 "EHLO orsmga101.jf.intel.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750883AbYLaFoB (ORCPT ); Wed, 31 Dec 2008 00:44:01 -0500 In-Reply-To: <20081230164551.GA5826@amt.cnet> Content-Disposition: inline Sender: kvm-owner@vger.kernel.org List-ID: On Wednesday 31 December 2008 00:45:51 Marcelo Tosatti wrote: > On Tue, Dec 30, 2008 at 10:14:09AM +0800, Sheng Yang wrote: > > > There is one remaining issue: kvm_assigned_dev_interrupt_work_handler > > > can re-enable the interrupt for KVM_ASSIGNED_DEV_GUEST_MSI case. > > > Perhaps you need a new flag to indicate shutdown (so the host IRQ won't > > > be reenabled). > > > > Is it already covered by disable_irq_no_sync() before cancel_work_sync()? > > I've noted this in my comment: the irq may be disabled nested(once for > > MSI and twice for INTx), but I think it's fine for we're going to free > > it. > > The problem is that the irq can be re-enabled in > kvm_assigned_dev_interrupt_work_handler: > > > context 1 | context 2 > > disable_irq_nosync > kvm_assigned_dev_interrupt_work_handler > enable_irq > cancel_work_sync > > free_irq > Um... My understanding is a little different... Before context1 execute disable_irq_nosync(), in irq handler, disable_irq_nosync() would also been executed. So only one enable_irq() is not really enable irq here, which can cover the window at all. That's what I means nested disable irq... -- regards Yang, Sheng > So between cancel_work_sync and free_irq kvm_assigned_dev_intr can run > and schedule work. > > I guess it is OK to take the kvm mutex in kvm_free_assigned_irq (but > better verify), so it can: > > mutex_lock > assigned_dev->irq_requested_type = 0; > mutex_unlock > > disable_irq_nosync > cancel_work_sync > free_irq > > So that the work handler won't re-enable the interrupt. > > Other than that the latest patchset looks good.