Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932534Ab2KNDE7 (ORCPT ); Tue, 13 Nov 2012 22:04:59 -0500 Received: from hrndva-omtalb.mail.rr.com ([71.74.56.122]:31247 "EHLO hrndva-omtalb.mail.rr.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932406Ab2KNDE5 (ORCPT ); Tue, 13 Nov 2012 22:04:57 -0500 X-Authority-Analysis: v=2.0 cv=RoZH3VaK c=1 sm=0 a=rXTBtCOcEpjy1lPqhTCpEQ==:17 a=mNMOxpOpBa8A:10 a=ueNZMrm-JmMA:10 a=5SG0PmZfjMsA:10 a=Q9fys5e9bTEA:10 a=meVymXHHAAAA:8 a=ZtitcL5Qm7IA:10 a=WTJdmG3rAAAA:8 a=QyXUC8HyAAAA:8 a=d8lIWgEgSppnsl-WOP8A:9 a=PUjeQqilurYA:10 a=nsF78Xlgl7EA:10 a=dGJ0OcVc7YAA:10 a=rXTBtCOcEpjy1lPqhTCpEQ==:117 X-Cloudmark-Score: 0 X-Originating-IP: 74.67.115.198 Message-ID: <1352862293.18025.52.camel@gandalf.local.home> Subject: Re: [PATCH 03/11] intel-iommu: Fix AB-BA lockdep report From: Steven Rostedt To: shuah.khan@hp.com, Greg Kroah-Hartman , stable Cc: roland@purestorage.com, linux-kernel@vger.kernel.org, linux-rt-users , Thomas Gleixner , Carsten Emde , John Kacur , David Woodhouse , shuahkhan@gmail.com Date: Tue, 13 Nov 2012 22:04:53 -0500 In-Reply-To: <1352859923.7889.2.camel@lorien2> References: <20111204185444.411298317@goodmis.org> <20111204190008.804987080@goodmis.org> <1352859923.7889.2.camel@lorien2> Content-Type: text/plain; charset="ISO-8859-15" X-Mailer: Evolution 3.4.3-1 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8592 Lines: 188 On Tue, 2012-11-13 at 19:25 -0700, Shuah Khan wrote: > On Sun, 2011-12-04 at 13:54 -0500, Steven Rostedt wrote: > > From: Roland Dreier > > > > When unbinding a device so that I could pass it through to a KVM VM, I > > got the lockdep report below. It looks like a legitimate lock > > ordering problem: > > Did this patch not make it into stable releases other than 3.1. I > couldn't find it in any other stable tress prior to 3.1. Ah, this was done in the early stable-rt releases. Where we took mainline fixes as long as they were in tglx's tree. But today the stable-rt tree waits for mainline fixes to come in via the stable tree so we don't do something like this. That is, miss getting a fix into stable. Yeah, this should be added to 3.0. (looks to be already in 3.2 and 3.4). Greg, Can you add the following commit to the 3.0 stable tree. We've had this in v3.0-rt for some time now. :-/ commit 3e7abe2556b583e87dabda3e0e6178a67b20d06f Author: Roland Dreier Date: Wed Jul 20 06:22:21 2011 -0700 intel-iommu: Fix AB-BA lockdep report Thanks, -- Steve > > -- Shuah > > > > - domain_context_mapping_one() takes iommu->lock and calls > > iommu_support_dev_iotlb(), which takes device_domain_lock (inside > > iommu->lock). > > > > - domain_remove_one_dev_info() starts by taking device_domain_lock > > then takes iommu->lock inside it (near the end of the function). > > > > So this is the classic AB-BA deadlock. It looks like a safe fix is to > > simply release device_domain_lock a bit earlier, since as far as I can > > tell, it doesn't protect any of the stuff accessed at the end of > > domain_remove_one_dev_info() anyway. > > > > BTW, the use of device_domain_lock looks a bit unsafe to me... it's > > at least not obvious to me why we aren't vulnerable to the race below: > > > > iommu_support_dev_iotlb() > > domain_remove_dev_info() > > > > lock device_domain_lock > > find info > > unlock device_domain_lock > > > > lock device_domain_lock > > find same info > > unlock device_domain_lock > > > > free_devinfo_mem(info) > > > > do stuff with info after it's free > > > > However I don't understand the locking here well enough to know if > > this is a real problem, let alone what the best fix is. > > > > Anyway here's the full lockdep output that prompted all of this: > > > > ======================================================= > > [ INFO: possible circular locking dependency detected ] > > 2.6.39.1+ #1 > > ------------------------------------------------------- > > bash/13954 is trying to acquire lock: > > (&(&iommu->lock)->rlock){......}, at: [] domain_remove_one_dev_info+0x121/0x230 > > > > but task is already holding lock: > > (device_domain_lock){-.-...}, at: [] domain_remove_one_dev_info+0x208/0x230 > > > > which lock already depends on the new lock. > > > > the existing dependency chain (in reverse order) is: > > > > -> #1 (device_domain_lock){-.-...}: > > [] lock_acquire+0x9d/0x130 > > [] _raw_spin_lock_irqsave+0x55/0xa0 > > [] domain_context_mapping_one+0x600/0x750 > > [] domain_context_mapping+0x3f/0x120 > > [] iommu_prepare_identity_map+0x1c5/0x1e0 > > [] intel_iommu_init+0x88e/0xb5e > > [] pci_iommu_init+0x16/0x41 > > [] do_one_initcall+0x45/0x190 > > [] kernel_init+0xe3/0x168 > > [] kernel_thread_helper+0x4/0x10 > > > > -> #0 (&(&iommu->lock)->rlock){......}: > > [] __lock_acquire+0x195e/0x1e10 > > [] lock_acquire+0x9d/0x130 > > [] _raw_spin_lock_irqsave+0x55/0xa0 > > [] domain_remove_one_dev_info+0x121/0x230 > > [] device_notifier+0x72/0x90 > > [] notifier_call_chain+0x8c/0xc0 > > [] __blocking_notifier_call_chain+0x78/0xb0 > > [] blocking_notifier_call_chain+0x16/0x20 > > [] __device_release_driver+0xbc/0xe0 > > [] device_release_driver+0x2f/0x50 > > [] driver_unbind+0xa3/0xc0 > > [] drv_attr_store+0x2c/0x30 > > [] sysfs_write_file+0xe6/0x170 > > [] vfs_write+0xce/0x190 > > [] sys_write+0x54/0xa0 > > [] system_call_fastpath+0x16/0x1b > > > > other info that might help us debug this: > > > > 6 locks held by bash/13954: > > #0: (&buffer->mutex){+.+.+.}, at: [] sysfs_write_file+0x44/0x170 > > #1: (s_active#3){++++.+}, at: [] sysfs_write_file+0xcd/0x170 > > #2: (&__lockdep_no_validate__){+.+.+.}, at: [] driver_unbind+0x9b/0xc0 > > #3: (&__lockdep_no_validate__){+.+.+.}, at: [] device_release_driver+0x27/0x50 > > #4: (&(&priv->bus_notifier)->rwsem){.+.+.+}, at: [] __blocking_notifier_call_chain+0x5f/0xb0 > > #5: (device_domain_lock){-.-...}, at: [] domain_remove_one_dev_info+0x208/0x230 > > > > stack backtrace: > > Pid: 13954, comm: bash Not tainted 2.6.39.1+ #1 > > Call Trace: > > [] print_circular_bug+0xf7/0x100 > > [] __lock_acquire+0x195e/0x1e10 > > [] ? trace_hardirqs_off+0xd/0x10 > > [] ? trace_hardirqs_on_caller+0x13d/0x180 > > [] lock_acquire+0x9d/0x130 > > [] ? domain_remove_one_dev_info+0x121/0x230 > > [] _raw_spin_lock_irqsave+0x55/0xa0 > > [] ? domain_remove_one_dev_info+0x121/0x230 > > [] ? trace_hardirqs_off+0xd/0x10 > > [] domain_remove_one_dev_info+0x121/0x230 > > [] device_notifier+0x72/0x90 > > [] notifier_call_chain+0x8c/0xc0 > > [] __blocking_notifier_call_chain+0x78/0xb0 > > [] blocking_notifier_call_chain+0x16/0x20 > > [] __device_release_driver+0xbc/0xe0 > > [] device_release_driver+0x2f/0x50 > > [] driver_unbind+0xa3/0xc0 > > [] drv_attr_store+0x2c/0x30 > > [] sysfs_write_file+0xe6/0x170 > > [] vfs_write+0xce/0x190 > > [] sys_write+0x54/0xa0 > > [] system_call_fastpath+0x16/0x1b > > > > Signed-off-by: Roland Dreier > > Signed-off-by: David Woodhouse > > --- > > drivers/pci/intel-iommu.c | 4 ++-- > > 1 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/pci/intel-iommu.c b/drivers/pci/intel-iommu.c > > index 8c2564d..bc05a51 100644 > > --- a/drivers/pci/intel-iommu.c > > +++ b/drivers/pci/intel-iommu.c > > @@ -3569,6 +3569,8 @@ static void domain_remove_one_dev_info(struct dmar_domain *domain, > > found = 1; > > } > > > > + spin_unlock_irqrestore(&device_domain_lock, flags); > > + > > if (found == 0) { > > unsigned long tmp_flags; > > spin_lock_irqsave(&domain->iommu_lock, tmp_flags); > > @@ -3585,8 +3587,6 @@ static void domain_remove_one_dev_info(struct dmar_domain *domain, > > spin_unlock_irqrestore(&iommu->lock, tmp_flags); > > } > > } > > - > > - spin_unlock_irqrestore(&device_domain_lock, flags); > > } > > > > static void vm_domain_remove_all_dev_info(struct dmar_domain *domain) > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/