From mboxrd@z Thu Jan 1 00:00:00 1970 From: Amit Shah Subject: Re: [PATCH 15/15] KVM: Fix racy in kvm_free_assigned_irq Date: Mon, 29 Dec 2008 00:42:22 -0500 Message-ID: <20081229054222.GB13166@shell.devel.redhat.com> References: <20081225115609.GA10087@syang10-desktop> <1230258607-15208-1-git-send-email-sheng@linux.intel.com> <20081227200626.GA4095@amt.cnet> <20081228112402.GB2610@syang10-desktop> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Marcelo Tosatti , Avi Kivity , kvm@vger.kernel.org, Amit Shah , "Han, Weidong" To: Sheng Yang Return-path: Received: from mx1.redhat.com ([66.187.233.31]:46971 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750801AbYL2Fmg (ORCPT ); Mon, 29 Dec 2008 00:42:36 -0500 Content-Disposition: inline In-Reply-To: <20081228112402.GB2610@syang10-desktop> Sender: kvm-owner@vger.kernel.org List-ID: 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. Amit