From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Tosatti Subject: Re: [PATCH 15/15] KVM: Fix racy in kvm_free_assigned_irq Date: Tue, 30 Dec 2008 14:45:51 -0200 Message-ID: <20081230164551.GA5826@amt.cnet> References: <20081225115609.GA10087@syang10-desktop> <200812292023.29565.sheng@linux.intel.com> <20081229152057.GB3823@amt.cnet> <200812301014.10487.sheng@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Amit Shah , Avi Kivity , kvm@vger.kernel.org, Amit Shah , "Han, Weidong" To: Sheng Yang Return-path: Received: from mx2.redhat.com ([66.187.237.31]:35673 "EHLO mx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751222AbYL3QqF (ORCPT ); Tue, 30 Dec 2008 11:46:05 -0500 Content-Disposition: inline In-Reply-To: <200812301014.10487.sheng@linux.intel.com> Sender: kvm-owner@vger.kernel.org List-ID: 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 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.