* [PATCH 1/3] iommu/amd: Prevent possible null pointer dereference and infinite loop
[not found] ` <20180504162233.11283-1-bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
@ 2018-05-04 16:22 ` Sebastian Andrzej Siewior
2018-05-04 16:22 ` [PATCH 2/3] iommu/amd: Cleanup locking in __attach/detach_device() Sebastian Andrzej Siewior
2018-05-04 16:22 ` [PATCH 3/3] iommu/amd: Do not flush when device is busy Sebastian Andrzej Siewior
2 siblings, 0 replies; 7+ messages in thread
From: Sebastian Andrzej Siewior @ 2018-05-04 16:22 UTC (permalink / raw)
To: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA
Cc: Anna-Maria Gleixner, tglx-hfZtesqFncYOwBW4kG4KsQ,
Sebastian Andrzej Siewior
From: Anna-Maria Gleixner <anna-maria-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
The check for !dev_data->domain in __detach_device() emits a warning and
returns. The calling code in detach_device() dereferences dev_data->domain
afterwards unconditionally, so in case that dev_data->domain is NULL the
warning will be immediately followed by a NULL pointer dereference.
The calling code in cleanup_domain() loops infinite when !dev_data->domain
and the check in __detach_device() returns immediately because dev_list is
not changed.
do_detach() duplicates this check without throwing a warning.
Move the check with the explanation of the do_detach() code into the caller
detach_device() and return immediately. Throw an error, when hitting the
condition in cleanup_domain().
Signed-off-by: Anna-Maria Gleixner <anna-maria-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
---
drivers/iommu/amd_iommu.c | 22 ++++++++++------------
1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 8fb8c737fffe..e484275a4c69 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -1911,15 +1911,6 @@ static void do_detach(struct iommu_dev_data *dev_data)
struct amd_iommu *iommu;
u16 alias;
- /*
- * First check if the device is still attached. It might already
- * be detached from its domain because the generic
- * iommu_detach_group code detached it and we try again here in
- * our alias handling.
- */
- if (!dev_data->domain)
- return;
-
iommu = amd_iommu_rlookup_table[dev_data->devid];
alias = dev_data->alias;
@@ -2124,9 +2115,6 @@ static void __detach_device(struct iommu_dev_data *dev_data)
*/
WARN_ON(!irqs_disabled());
- if (WARN_ON(!dev_data->domain))
- return;
-
domain = dev_data->domain;
spin_lock(&domain->lock);
@@ -2148,6 +2136,15 @@ static void detach_device(struct device *dev)
dev_data = get_dev_data(dev);
domain = dev_data->domain;
+ /*
+ * First check if the device is still attached. It might already
+ * be detached from its domain because the generic
+ * iommu_detach_group code detached it and we try again here in
+ * our alias handling.
+ */
+ if (WARN_ON(!dev_data->domain))
+ return;
+
/* lock device table */
spin_lock_irqsave(&amd_iommu_devtable_lock, flags);
__detach_device(dev_data);
@@ -2793,6 +2790,7 @@ static void cleanup_domain(struct protection_domain *domain)
while (!list_empty(&domain->dev_list)) {
entry = list_first_entry(&domain->dev_list,
struct iommu_dev_data, list);
+ BUG_ON(!entry->domain);
__detach_device(entry);
}
--
2.17.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/3] iommu/amd: Cleanup locking in __attach/detach_device()
[not found] ` <20180504162233.11283-1-bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
2018-05-04 16:22 ` [PATCH 1/3] iommu/amd: Prevent possible null pointer dereference and infinite loop Sebastian Andrzej Siewior
@ 2018-05-04 16:22 ` Sebastian Andrzej Siewior
2018-05-04 16:22 ` [PATCH 3/3] iommu/amd: Do not flush when device is busy Sebastian Andrzej Siewior
2 siblings, 0 replies; 7+ messages in thread
From: Sebastian Andrzej Siewior @ 2018-05-04 16:22 UTC (permalink / raw)
To: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA
Cc: Anna-Maria Gleixner, tglx-hfZtesqFncYOwBW4kG4KsQ,
Sebastian Andrzej Siewior
From: Anna-Maria Gleixner <anna-maria-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
Since introduction of the pd_bitmap_lock in commit 2bc001808904
("iommu/amd: Split domain id out of amd_iommu_devtable_lock")
amd_iommu_devtable_lock is only taken around __detach_device() and
__attach_device() calls.
The lock is not protecting anything as all operations are domain specific
and protected by domain->lock in __detach_device() and __attach_device(),
so amd_iommu_devtable_lock has no real purpose anymore.
Lock domain->lock before calling into __detach_device() and
__attach_device() and simplify the implementation of those functions. Add
lockdep checks where appropriate.
Signed-off-by: Anna-Maria Gleixner <anna-maria-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
---
drivers/iommu/amd_iommu.c | 70 +++++++++------------------------------
1 file changed, 15 insertions(+), 55 deletions(-)
diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index e484275a4c69..f66a5d0b7c62 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -81,7 +81,6 @@
*/
#define AMD_IOMMU_PGSIZES ((~0xFFFUL) & ~(2ULL << 38))
-static DEFINE_SPINLOCK(amd_iommu_devtable_lock);
static DEFINE_SPINLOCK(pd_bitmap_lock);
/* List of all available dev_data structures */
@@ -1886,6 +1885,8 @@ static void do_attach(struct iommu_dev_data *dev_data,
u16 alias;
bool ats;
+ lockdep_assert_held(&domain->lock);
+
iommu = amd_iommu_rlookup_table[dev_data->devid];
alias = dev_data->alias;
ats = dev_data->ats.enabled;
@@ -1906,11 +1907,13 @@ static void do_attach(struct iommu_dev_data *dev_data,
device_flush_dte(dev_data);
}
-static void do_detach(struct iommu_dev_data *dev_data)
+static void __detach_device(struct iommu_dev_data *dev_data)
{
struct amd_iommu *iommu;
u16 alias;
+ lockdep_assert_held(&dev_data->domain->lock);
+
iommu = amd_iommu_rlookup_table[dev_data->devid];
alias = dev_data->alias;
@@ -1936,32 +1939,13 @@ static void do_detach(struct iommu_dev_data *dev_data)
static int __attach_device(struct iommu_dev_data *dev_data,
struct protection_domain *domain)
{
- int ret;
-
- /*
- * Must be called with IRQs disabled. Warn here to detect early
- * when its not.
- */
- WARN_ON(!irqs_disabled());
-
- /* lock domain */
- spin_lock(&domain->lock);
-
- ret = -EBUSY;
if (dev_data->domain != NULL)
- goto out_unlock;
+ return -EBUSY;
/* Attach alias group root */
do_attach(dev_data, domain);
- ret = 0;
-
-out_unlock:
-
- /* ready */
- spin_unlock(&domain->lock);
-
- return ret;
+ return 0;
}
@@ -2088,9 +2072,10 @@ static int attach_device(struct device *dev,
}
skip_ats_check:
- spin_lock_irqsave(&amd_iommu_devtable_lock, flags);
+
+ spin_lock_irqsave(&domain->lock, flags);
ret = __attach_device(dev_data, domain);
- spin_unlock_irqrestore(&amd_iommu_devtable_lock, flags);
+ spin_unlock_irqrestore(&domain->lock, flags);
/*
* We might boot into a crash-kernel here. The crashed kernel
@@ -2103,29 +2088,7 @@ static int attach_device(struct device *dev,
}
/*
- * Removes a device from a protection domain (unlocked)
- */
-static void __detach_device(struct iommu_dev_data *dev_data)
-{
- struct protection_domain *domain;
-
- /*
- * Must be called with IRQs disabled. Warn here to detect early
- * when its not.
- */
- WARN_ON(!irqs_disabled());
-
- domain = dev_data->domain;
-
- spin_lock(&domain->lock);
-
- do_detach(dev_data);
-
- spin_unlock(&domain->lock);
-}
-
-/*
- * Removes a device from a protection domain (with devtable_lock held)
+ * Removes a device from a protection domain
*/
static void detach_device(struct device *dev)
{
@@ -2145,10 +2108,9 @@ static void detach_device(struct device *dev)
if (WARN_ON(!dev_data->domain))
return;
- /* lock device table */
- spin_lock_irqsave(&amd_iommu_devtable_lock, flags);
+ spin_lock_irqsave(&domain->lock, flags);
__detach_device(dev_data);
- spin_unlock_irqrestore(&amd_iommu_devtable_lock, flags);
+ spin_unlock_irqrestore(&domain->lock, flags);
if (!dev_is_pci(dev))
return;
@@ -2785,16 +2747,14 @@ static void cleanup_domain(struct protection_domain *domain)
struct iommu_dev_data *entry;
unsigned long flags;
- spin_lock_irqsave(&amd_iommu_devtable_lock, flags);
-
+ spin_lock_irqsave(&domain->lock, flags);
while (!list_empty(&domain->dev_list)) {
entry = list_first_entry(&domain->dev_list,
struct iommu_dev_data, list);
BUG_ON(!entry->domain);
__detach_device(entry);
}
-
- spin_unlock_irqrestore(&amd_iommu_devtable_lock, flags);
+ spin_unlock_irqrestore(&domain->lock, flags);
}
static void protection_domain_free(struct protection_domain *domain)
--
2.17.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 3/3] iommu/amd: Do not flush when device is busy
[not found] ` <20180504162233.11283-1-bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
2018-05-04 16:22 ` [PATCH 1/3] iommu/amd: Prevent possible null pointer dereference and infinite loop Sebastian Andrzej Siewior
2018-05-04 16:22 ` [PATCH 2/3] iommu/amd: Cleanup locking in __attach/detach_device() Sebastian Andrzej Siewior
@ 2018-05-04 16:22 ` Sebastian Andrzej Siewior
[not found] ` <20180504162233.11283-4-bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
2 siblings, 1 reply; 7+ messages in thread
From: Sebastian Andrzej Siewior @ 2018-05-04 16:22 UTC (permalink / raw)
To: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA
Cc: Anna-Maria Gleixner, tglx-hfZtesqFncYOwBW4kG4KsQ,
Sebastian Andrzej Siewior
From: Anna-Maria Gleixner <anna-maria-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
When device is already attached to a domain, there is no need to execute
the domain_flush_tlb_pde(). Therefore move the check if the domain is set
into attach_device().
Signed-off-by: Anna-Maria Gleixner <anna-maria-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
---
drivers/iommu/amd_iommu.c | 32 ++++++++++----------------------
1 file changed, 10 insertions(+), 22 deletions(-)
diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index f66a5d0b7c62..a801678ae1b4 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -1878,8 +1878,11 @@ static void clear_dte_entry(u16 devid)
amd_iommu_apply_erratum_63(devid);
}
-static void do_attach(struct iommu_dev_data *dev_data,
- struct protection_domain *domain)
+/*
+ * This function does assigns the device visible for the hardware
+ */
+static void __attach_device(struct iommu_dev_data *dev_data,
+ struct protection_domain *domain)
{
struct amd_iommu *iommu;
u16 alias;
@@ -1932,23 +1935,6 @@ static void __detach_device(struct iommu_dev_data *dev_data)
device_flush_dte(dev_data);
}
-/*
- * If a device is not yet associated with a domain, this function does
- * assigns it visible for the hardware
- */
-static int __attach_device(struct iommu_dev_data *dev_data,
- struct protection_domain *domain)
-{
- if (dev_data->domain != NULL)
- return -EBUSY;
-
- /* Attach alias group root */
- do_attach(dev_data, domain);
-
- return 0;
-}
-
-
static void pdev_iommuv2_disable(struct pci_dev *pdev)
{
pci_disable_ats(pdev);
@@ -2045,7 +2031,6 @@ static int attach_device(struct device *dev,
struct pci_dev *pdev;
struct iommu_dev_data *dev_data;
unsigned long flags;
- int ret;
dev_data = get_dev_data(dev);
@@ -2073,8 +2058,11 @@ static int attach_device(struct device *dev,
skip_ats_check:
+ if (dev_data->domain != NULL)
+ return -EBUSY;
+
spin_lock_irqsave(&domain->lock, flags);
- ret = __attach_device(dev_data, domain);
+ __attach_device(dev_data, domain);
spin_unlock_irqrestore(&domain->lock, flags);
/*
@@ -2084,7 +2072,7 @@ static int attach_device(struct device *dev,
*/
domain_flush_tlb_pde(domain);
- return ret;
+ return 0;
}
/*
--
2.17.0
^ permalink raw reply related [flat|nested] 7+ messages in thread