* [PATCH] Revert "iommu/vt-d: Fix lock inversion between iommu->lock and device_domain_lock"
@ 2019-06-21 2:32 Peter Xu
2019-06-21 13:19 ` Chris Wilson
2019-06-22 19:47 ` Joerg Roedel
0 siblings, 2 replies; 3+ messages in thread
From: Peter Xu @ 2019-06-21 2:32 UTC (permalink / raw)
To: iommu, linux-kernel; +Cc: dave.jiang
This reverts commit 7560cc3ca7d9d11555f80c830544e463fcdb28b8.
With 5.2.0-rc5 I can easily trigger this with lockdep and iommu=pt:
======================================================
WARNING: possible circular locking dependency detected
5.2.0-rc5 #78 Not tainted
------------------------------------------------------
swapper/0/1 is trying to acquire lock:
00000000ea2b3beb (&(&iommu->lock)->rlock){+.+.}, at: domain_context_mapping_one+0xa5/0x4e0
but task is already holding lock:
00000000a681907b (device_domain_lock){....}, at: domain_context_mapping_one+0x8d/0x4e0
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #1 (device_domain_lock){....}:
_raw_spin_lock_irqsave+0x3c/0x50
dmar_insert_one_dev_info+0xbb/0x510
domain_add_dev_info+0x50/0x90
dev_prepare_static_identity_mapping+0x30/0x68
intel_iommu_init+0xddd/0x1422
pci_iommu_init+0x16/0x3f
do_one_initcall+0x5d/0x2b4
kernel_init_freeable+0x218/0x2c1
kernel_init+0xa/0x100
ret_from_fork+0x3a/0x50
-> #0 (&(&iommu->lock)->rlock){+.+.}:
lock_acquire+0x9e/0x170
_raw_spin_lock+0x25/0x30
domain_context_mapping_one+0xa5/0x4e0
pci_for_each_dma_alias+0x30/0x140
dmar_insert_one_dev_info+0x3b2/0x510
domain_add_dev_info+0x50/0x90
dev_prepare_static_identity_mapping+0x30/0x68
intel_iommu_init+0xddd/0x1422
pci_iommu_init+0x16/0x3f
do_one_initcall+0x5d/0x2b4
kernel_init_freeable+0x218/0x2c1
kernel_init+0xa/0x100
ret_from_fork+0x3a/0x50
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(device_domain_lock);
lock(&(&iommu->lock)->rlock);
lock(device_domain_lock);
lock(&(&iommu->lock)->rlock);
*** DEADLOCK ***
2 locks held by swapper/0/1:
#0: 00000000033eb13d (dmar_global_lock){++++}, at: intel_iommu_init+0x1e0/0x1422
#1: 00000000a681907b (device_domain_lock){....}, at: domain_context_mapping_one+0x8d/0x4e0
stack backtrace:
CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.2.0-rc5 #78
Hardware name: LENOVO 20KGS35G01/20KGS35G01, BIOS N23ET50W (1.25 ) 06/25/2018
Call Trace:
dump_stack+0x85/0xc0
print_circular_bug.cold.57+0x15c/0x195
__lock_acquire+0x152a/0x1710
lock_acquire+0x9e/0x170
? domain_context_mapping_one+0xa5/0x4e0
_raw_spin_lock+0x25/0x30
? domain_context_mapping_one+0xa5/0x4e0
domain_context_mapping_one+0xa5/0x4e0
? domain_context_mapping_one+0x4e0/0x4e0
pci_for_each_dma_alias+0x30/0x140
dmar_insert_one_dev_info+0x3b2/0x510
domain_add_dev_info+0x50/0x90
dev_prepare_static_identity_mapping+0x30/0x68
intel_iommu_init+0xddd/0x1422
? printk+0x58/0x6f
? lockdep_hardirqs_on+0xf0/0x180
? do_early_param+0x8e/0x8e
? e820__memblock_setup+0x63/0x63
pci_iommu_init+0x16/0x3f
do_one_initcall+0x5d/0x2b4
? do_early_param+0x8e/0x8e
? rcu_read_lock_sched_held+0x55/0x60
? do_early_param+0x8e/0x8e
kernel_init_freeable+0x218/0x2c1
? rest_init+0x230/0x230
kernel_init+0xa/0x100
ret_from_fork+0x3a/0x50
domain_context_mapping_one() is taking device_domain_lock first then
iommu lock, while dmar_insert_one_dev_info() is doing the reverse.
That should be introduced by commit:
7560cc3ca7d9 ("iommu/vt-d: Fix lock inversion between iommu->lock and
device_domain_lock", 2019-05-27)
So far I still cannot figure out how the previous deadlock was
triggered (I cannot find iommu lock taken before calling of
iommu_flush_dev_iotlb()), however I'm pretty sure that that change
should be incomplete at least because it does not fix all the places
so we're still taking the locks in different orders, while reverting
that commit is very clean to me so far that we should always take
device_domain_lock first then the iommu lock.
We can continue to try to find the real culprit mentioned in
7560cc3ca7d9, but for now I think we should revert it to fix current
breakage.
CC: Joerg Roedel <joro@8bytes.org>
CC: Lu Baolu <baolu.lu@linux.intel.com>
CC: dave.jiang@intel.com
Signed-off-by: Peter Xu <peterx@redhat.com>
---
drivers/iommu/intel-iommu.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 56297298d6ee..162b3236e72c 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -2504,7 +2504,6 @@ static struct dmar_domain *dmar_insert_one_dev_info(struct intel_iommu *iommu,
}
}
- spin_lock(&iommu->lock);
spin_lock_irqsave(&device_domain_lock, flags);
if (dev)
found = find_domain(dev);
@@ -2520,16 +2519,17 @@ static struct dmar_domain *dmar_insert_one_dev_info(struct intel_iommu *iommu,
if (found) {
spin_unlock_irqrestore(&device_domain_lock, flags);
- spin_unlock(&iommu->lock);
free_devinfo_mem(info);
/* Caller must free the original domain */
return found;
}
+ spin_lock(&iommu->lock);
ret = domain_attach_iommu(domain, iommu);
+ spin_unlock(&iommu->lock);
+
if (ret) {
spin_unlock_irqrestore(&device_domain_lock, flags);
- spin_unlock(&iommu->lock);
free_devinfo_mem(info);
return NULL;
}
@@ -2539,7 +2539,6 @@ static struct dmar_domain *dmar_insert_one_dev_info(struct intel_iommu *iommu,
if (dev)
dev->archdata.iommu = info;
spin_unlock_irqrestore(&device_domain_lock, flags);
- spin_unlock(&iommu->lock);
/* PASID table is mandatory for a PCI device in scalable mode. */
if (dev && dev_is_pci(dev) && sm_supported(iommu)) {
--
2.21.0
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] Revert "iommu/vt-d: Fix lock inversion between iommu->lock and device_domain_lock"
2019-06-21 2:32 [PATCH] Revert "iommu/vt-d: Fix lock inversion between iommu->lock and device_domain_lock" Peter Xu
@ 2019-06-21 13:19 ` Chris Wilson
2019-06-22 19:47 ` Joerg Roedel
1 sibling, 0 replies; 3+ messages in thread
From: Chris Wilson @ 2019-06-21 13:19 UTC (permalink / raw)
To: Peter Xu, iommu, linux-kernel; +Cc: dave.jiang
Quoting Peter Xu (2019-06-21 03:32:05)
> This reverts commit 7560cc3ca7d9d11555f80c830544e463fcdb28b8.
>
> With 5.2.0-rc5 I can easily trigger this with lockdep and iommu=pt:
>
> ======================================================
> WARNING: possible circular locking dependency detected
> 5.2.0-rc5 #78 Not tainted
> ------------------------------------------------------
> swapper/0/1 is trying to acquire lock:
> 00000000ea2b3beb (&(&iommu->lock)->rlock){+.+.}, at: domain_context_mapping_one+0xa5/0x4e0
> but task is already holding lock:
> 00000000a681907b (device_domain_lock){....}, at: domain_context_mapping_one+0x8d/0x4e0
> which lock already depends on the new lock.
> the existing dependency chain (in reverse order) is:
> -> #1 (device_domain_lock){....}:
> _raw_spin_lock_irqsave+0x3c/0x50
> dmar_insert_one_dev_info+0xbb/0x510
> domain_add_dev_info+0x50/0x90
> dev_prepare_static_identity_mapping+0x30/0x68
> intel_iommu_init+0xddd/0x1422
> pci_iommu_init+0x16/0x3f
> do_one_initcall+0x5d/0x2b4
> kernel_init_freeable+0x218/0x2c1
> kernel_init+0xa/0x100
> ret_from_fork+0x3a/0x50
> -> #0 (&(&iommu->lock)->rlock){+.+.}:
> lock_acquire+0x9e/0x170
> _raw_spin_lock+0x25/0x30
> domain_context_mapping_one+0xa5/0x4e0
> pci_for_each_dma_alias+0x30/0x140
> dmar_insert_one_dev_info+0x3b2/0x510
> domain_add_dev_info+0x50/0x90
> dev_prepare_static_identity_mapping+0x30/0x68
> intel_iommu_init+0xddd/0x1422
> pci_iommu_init+0x16/0x3f
> do_one_initcall+0x5d/0x2b4
> kernel_init_freeable+0x218/0x2c1
> kernel_init+0xa/0x100
> ret_from_fork+0x3a/0x50
>
> other info that might help us debug this:
> Possible unsafe locking scenario:
> CPU0 CPU1
> ---- ----
> lock(device_domain_lock);
> lock(&(&iommu->lock)->rlock);
> lock(device_domain_lock);
> lock(&(&iommu->lock)->rlock);
>
> *** DEADLOCK ***
> 2 locks held by swapper/0/1:
> #0: 00000000033eb13d (dmar_global_lock){++++}, at: intel_iommu_init+0x1e0/0x1422
> #1: 00000000a681907b (device_domain_lock){....}, at: domain_context_mapping_one+0x8d/0x4e0
>
> stack backtrace:
> CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.2.0-rc5 #78
> Hardware name: LENOVO 20KGS35G01/20KGS35G01, BIOS N23ET50W (1.25 ) 06/25/2018
> Call Trace:
> dump_stack+0x85/0xc0
> print_circular_bug.cold.57+0x15c/0x195
> __lock_acquire+0x152a/0x1710
> lock_acquire+0x9e/0x170
> ? domain_context_mapping_one+0xa5/0x4e0
> _raw_spin_lock+0x25/0x30
> ? domain_context_mapping_one+0xa5/0x4e0
> domain_context_mapping_one+0xa5/0x4e0
> ? domain_context_mapping_one+0x4e0/0x4e0
> pci_for_each_dma_alias+0x30/0x140
> dmar_insert_one_dev_info+0x3b2/0x510
> domain_add_dev_info+0x50/0x90
> dev_prepare_static_identity_mapping+0x30/0x68
> intel_iommu_init+0xddd/0x1422
> ? printk+0x58/0x6f
> ? lockdep_hardirqs_on+0xf0/0x180
> ? do_early_param+0x8e/0x8e
> ? e820__memblock_setup+0x63/0x63
> pci_iommu_init+0x16/0x3f
> do_one_initcall+0x5d/0x2b4
> ? do_early_param+0x8e/0x8e
> ? rcu_read_lock_sched_held+0x55/0x60
> ? do_early_param+0x8e/0x8e
> kernel_init_freeable+0x218/0x2c1
> ? rest_init+0x230/0x230
> kernel_init+0xa/0x100
> ret_from_fork+0x3a/0x50
>
> domain_context_mapping_one() is taking device_domain_lock first then
> iommu lock, while dmar_insert_one_dev_info() is doing the reverse.
>
> That should be introduced by commit:
>
> 7560cc3ca7d9 ("iommu/vt-d: Fix lock inversion between iommu->lock and
> device_domain_lock", 2019-05-27)
>
> So far I still cannot figure out how the previous deadlock was
> triggered (I cannot find iommu lock taken before calling of
> iommu_flush_dev_iotlb()), however I'm pretty sure that that change
> should be incomplete at least because it does not fix all the places
> so we're still taking the locks in different orders, while reverting
> that commit is very clean to me so far that we should always take
> device_domain_lock first then the iommu lock.
>
> We can continue to try to find the real culprit mentioned in
> 7560cc3ca7d9, but for now I think we should revert it to fix current
> breakage.
>
> CC: Joerg Roedel <joro@8bytes.org>
> CC: Lu Baolu <baolu.lu@linux.intel.com>
> CC: dave.jiang@intel.com
> Signed-off-by: Peter Xu <peterx@redhat.com>
I've run this through our CI which was also reporting the inversion, so
Tested-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] Revert "iommu/vt-d: Fix lock inversion between iommu->lock and device_domain_lock"
2019-06-21 2:32 [PATCH] Revert "iommu/vt-d: Fix lock inversion between iommu->lock and device_domain_lock" Peter Xu
2019-06-21 13:19 ` Chris Wilson
@ 2019-06-22 19:47 ` Joerg Roedel
1 sibling, 0 replies; 3+ messages in thread
From: Joerg Roedel @ 2019-06-22 19:47 UTC (permalink / raw)
To: Peter Xu; +Cc: iommu, dave.jiang, linux-kernel
On Fri, Jun 21, 2019 at 10:32:05AM +0800, Peter Xu wrote:
> This reverts commit 7560cc3ca7d9d11555f80c830544e463fcdb28b8.
Applied and on its way upstream, thanks.
Joerg
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-06-22 19:47 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-21 2:32 [PATCH] Revert "iommu/vt-d: Fix lock inversion between iommu->lock and device_domain_lock" Peter Xu
2019-06-21 13:19 ` Chris Wilson
2019-06-22 19:47 ` Joerg Roedel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).