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: Tue, 30 Dec 2008 10:14:09 +0800 Message-ID: <200812301014.10487.sheng@linux.intel.com> References: <20081225115609.GA10087@syang10-desktop> <200812292023.29565.sheng@linux.intel.com> <20081229152057.GB3823@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 mga09.intel.com ([134.134.136.24]:12875 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751388AbYL3COO (ORCPT ); Mon, 29 Dec 2008 21:14:14 -0500 In-Reply-To: <20081229152057.GB3823@amt.cnet> Content-Disposition: inline Sender: kvm-owner@vger.kernel.org List-ID: On Monday 29 December 2008 23:20:57 Marcelo Tosatti wrote: > On Mon, Dec 29, 2008 at 08:23:28PM +0800, Sheng Yang wrote: > > On Monday 29 December 2008 13:42:22 Amit Shah wrote: > > > On Sun, Dec 28, 2008 at 07:24:02PM +0800, Sheng Yang wrote: > > > > On Sat, Dec 27, 2008 at 06:06:26PM -0200, Marcelo Tosatti wrote: > > > > > On Fri, Dec 26, 2008 at 10:30:07AM +0800, Sheng Yang wrote: > > > > > > Thanks to Marcelo's observation, The following code have > > > > > > potential issue: > > > > > > > > > > > > if (cancel_work_sync(&assigned_dev->interrupt_work)) > > > > > > kvm_put_kvm(kvm); > > > > > > > > > > > > In fact, cancel_work_sync() would return true either work struct > > > > > > is only scheduled or the callback of work struct is executed. > > > > > > This code only consider the former situation. > > > > > > > > > > Why not simply drop the reference inc / dec from irq handler/work > > > > > function? > > > > > > > > Sorry, I don't know the answer. After checking the code, I also think > > > > it's a little strange to increase refernce count here, and I think we > > > > won't suppose work_handler can release the kvm struct. > > > > > > At the time of developing that code, this was my observation: > > > > > > I see from the call chain kvm_put_kvm->...->kvm_arch_destroy_vm, no > > > locks are taken to actually destroy the vm. We can't be in ioctls, > > > sure. But shouldn't the mutex be taken to ensure there's nothing else > > > going on while destroying? > > > > > > At least with the workqueue model, we could be called asynchronously in > > > kernel context and I would have held the mutex and about to inject > > > interrupts while everything is being wiped off underneath. However, the > > > workqueue model tries its best to schedule the work on the same CPU, > > > though we can't use that guarantee to ensure things will be fine. > > > > > > --- > > > So I had to get a ref to the current vm till we had any pending work > > > scheduled. I think I put in comments in the code, but sadly most of my > > > comments we stripped out before the merge. > > > > Not quite understand... > > > > The free assigned device in the destroy path of VM, so as free irq. And > > we got cancel_work_sync() in free irq which can sync with the execution > > of scheduled work. And now before cancel_work_sync(), we disable the > > interrupt so that no more schedule work happen again. So after > > cancel_work_sync(), everything(I think it's irq handler and schedule work > > here) asynchronously should quiet down. > > > > Or I miss something? > > Thats right. As long as you disable the irq and cancel pending work > before freeing the data structures those paths use. > > 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. -- regards Yang, Sheng