[PATCH 1/2] iommu: Take group lock before attaching device in iommu_deferred_attach()

[PATCH 1/2] iommu: Take group lock before attaching device in iommu_deferred_attach()

[Vasant Hegde]

Commit 3ab657291638 ("iommu: use the __iommu_attach_device() directly for deferred attach")
replaced iommu_attach_device() call with __iommu_attach_device(). But
missed to take group lock. Take lock before attaching device to domain.

Fixes: 3ab657291638 ("iommu: use the __iommu_attach_device() directly for deferred attach")
Cc: Lianbo Jiang <lijiang@redhat.com>
Cc: Robin Murphy <robin.murphy@arm.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/iommu.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 9df7cc75c1bc..1743dba023b6 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2114,10 +2114,19 @@ EXPORT_SYMBOL_GPL(iommu_attach_device);
 
 int iommu_deferred_attach(struct device *dev, struct iommu_domain *domain)
 {
-	if (dev->iommu && dev->iommu->attach_deferred)
-		return __iommu_attach_device(domain, dev);
+	int ret = 0;
+	struct iommu_group *group = dev->iommu_group;
 
-	return 0;
+	if (!group)
+		return -EINVAL;
+
+	if (dev->iommu && dev->iommu->attach_deferred) {
+		mutex_lock(&group->mutex);
+		ret = __iommu_attach_device(domain, dev);
+		mutex_unlock(&group->mutex);
+	}
+
+	return ret;
 }
 
 void iommu_detach_device(struct iommu_domain *domain, struct device *dev)
-- 
2.31.1

[PATCH 2/2] iommu/amd: Fix Invalid wait context issue

[Vasant Hegde]

With commit c4cb23111103 ("iommu/amd: Add support for enable/disable IOPF")
we are hitting below issue. This happens because in IOPF enablement path
it holds spin lock with irq disable and then tries to take mutex lock.

dmesg:
-----
[    0.938739] =============================
[    0.938740] [ BUG: Invalid wait context ]
[    0.938742] 6.10.0-rc1+ #1 Not tainted
[    0.938745] -----------------------------
[    0.938746] swapper/0/1 is trying to lock:
[    0.938748] ffffffff8c9f01d8 (&port_lock_key){....}-{3:3}, at: serial8250_console_write+0x78/0x4a0
[    0.938767] other info that might help us debug this:
[    0.938768] context-{5:5}
[    0.938769] 7 locks held by swapper/0/1:
[    0.938772]  #0: ffff888101a91310 (&group->mutex){+.+.}-{4:4}, at: bus_iommu_probe+0x70/0x160
[    0.938790]  #1: ffff888101d1f1b8 (&domain->lock){....}-{3:3}, at: amd_iommu_attach_device+0xa5/0x700
[    0.938799]  #2: ffff888101cc3d18 (&dev_data->lock){....}-{3:3}, at: amd_iommu_attach_device+0xc5/0x700
[    0.938806]  #3: ffff888100052830 (&iommu->lock){....}-{2:2}, at: amd_iommu_iopf_add_device+0x3f/0xa0
[    0.938813]  #4: ffffffff8945a340 (console_lock){+.+.}-{0:0}, at: _printk+0x48/0x50
[    0.938822]  #5: ffffffff8945a390 (console_srcu){....}-{0:0}, at: console_flush_all+0x58/0x4e0
[    0.938867]  #6: ffffffff82459f80 (console_owner){....}-{0:0}, at: console_flush_all+0x1f0/0x4e0
[    0.938872] stack backtrace:
[    0.938874] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 6.10.0-rc1+ #1
[    0.938877] Hardware name: HP HP EliteBook 745 G3/807E, BIOS N73 Ver. 01.39 04/16/2019

Fix above issue by re-arranging code in attach device path:
  - move device PASID/IOPF enablement outside lock in AMD IOMMU driver.
    This is safe as core layer holds group->mutex lock before calling
    iommu_ops->attach_dev.

Reported-by: Borislav Petkov <bp@alien8.de>
Reported-by: Mikhail Gavrilov <mikhail.v.gavrilov@gmail.com>
Reported-by: Chris Bainbridge <chris.bainbridge@gmail.com>
Fixes: c4cb23111103 ("iommu/amd: Add support for enable/disable IOPF")
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/iommu.c | 48 +++++++++++++++++++--------------------
 drivers/iommu/amd/ppr.c   | 22 ++++--------------
 2 files changed, 28 insertions(+), 42 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 52d83730a22a..c2703599bb16 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2032,7 +2032,6 @@ static int do_attach(struct iommu_dev_data *dev_data,
 		     struct protection_domain *domain)
 {
 	struct amd_iommu *iommu = get_amd_iommu_from_dev_data(dev_data);
-	struct pci_dev *pdev;
 	int ret = 0;
 
 	/* Update data structures */
@@ -2047,30 +2046,13 @@ static int do_attach(struct iommu_dev_data *dev_data,
 	domain->dev_iommu[iommu->index] += 1;
 	domain->dev_cnt                 += 1;
 
-	pdev = dev_is_pci(dev_data->dev) ? to_pci_dev(dev_data->dev) : NULL;
+	/* Setup GCR3 table */
 	if (pdom_is_sva_capable(domain)) {
 		ret = init_gcr3_table(dev_data, domain);
 		if (ret)
 			return ret;
-
-		if (pdev) {
-			pdev_enable_caps(pdev);
-
-			/*
-			 * Device can continue to function even if IOPF
-			 * enablement failed. Hence in error path just
-			 * disable device PRI support.
-			 */
-			if (amd_iommu_iopf_add_device(iommu, dev_data))
-				pdev_disable_cap_pri(pdev);
-		}
-	} else if (pdev) {
-		pdev_enable_cap_ats(pdev);
 	}
 
-	/* Update device table */
-	amd_iommu_dev_update_dte(dev_data, true);
-
 	return ret;
 }
 
@@ -2163,6 +2145,11 @@ static void detach_device(struct device *dev)
 
 	do_detach(dev_data);
 
+out:
+	spin_unlock(&dev_data->lock);
+
+	spin_unlock_irqrestore(&domain->lock, flags);
+
 	/* Remove IOPF handler */
 	if (ppr)
 		amd_iommu_iopf_remove_device(iommu, dev_data);
@@ -2170,10 +2157,6 @@ static void detach_device(struct device *dev)
 	if (dev_is_pci(dev))
 		pdev_disable_caps(to_pci_dev(dev));
 
-out:
-	spin_unlock(&dev_data->lock);
-
-	spin_unlock_irqrestore(&domain->lock, flags);
 }
 
 static struct iommu_device *amd_iommu_probe_device(struct device *dev)
@@ -2485,6 +2468,7 @@ static int amd_iommu_attach_device(struct iommu_domain *dom,
 	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
 	struct protection_domain *domain = to_pdomain(dom);
 	struct amd_iommu *iommu = get_amd_iommu_from_dev(dev);
+	struct pci_dev *pdev;
 	int ret;
 
 	/*
@@ -2517,7 +2501,23 @@ static int amd_iommu_attach_device(struct iommu_domain *dom,
 	}
 #endif
 
-	iommu_completion_wait(iommu);
+	pdev = dev_is_pci(dev_data->dev) ? to_pci_dev(dev_data->dev) : NULL;
+	if (pdev && pdom_is_sva_capable(domain)) {
+		pdev_enable_caps(pdev);
+
+		/*
+		 * Device can continue to function even if IOPF
+		 * enablement failed. Hence in error path just
+		 * disable device PRI support.
+		 */
+		if (amd_iommu_iopf_add_device(iommu, dev_data))
+			pdev_disable_cap_pri(pdev);
+	} else if (pdev) {
+		pdev_enable_cap_ats(pdev);
+	}
+
+	/* Update device table */
+	amd_iommu_dev_update_dte(dev_data, true);
 
 	return ret;
 }
diff --git a/drivers/iommu/amd/ppr.c b/drivers/iommu/amd/ppr.c
index 091423bb8aac..b3b690410f2a 100644
--- a/drivers/iommu/amd/ppr.c
+++ b/drivers/iommu/amd/ppr.c
@@ -249,40 +249,26 @@ void amd_iommu_page_response(struct device *dev, struct iopf_fault *evt,
 int amd_iommu_iopf_add_device(struct amd_iommu *iommu,
 			      struct iommu_dev_data *dev_data)
 {
-	unsigned long flags;
 	int ret = 0;
 
 	if (!dev_data->pri_enabled)
 		return ret;
 
-	raw_spin_lock_irqsave(&iommu->lock, flags);
-
-	if (!iommu->iopf_queue) {
-		ret = -EINVAL;
-		goto out_unlock;
-	}
+	if (!iommu->iopf_queue)
+		return -EINVAL;
 
 	ret = iopf_queue_add_device(iommu->iopf_queue, dev_data->dev);
 	if (ret)
-		goto out_unlock;
+		return ret;
 
 	dev_data->ppr = true;
-
-out_unlock:
-	raw_spin_unlock_irqrestore(&iommu->lock, flags);
-	return ret;
+	return 0;
 }
 
 /* Its assumed that caller has verified that device was added to iopf queue */
 void amd_iommu_iopf_remove_device(struct amd_iommu *iommu,
 				  struct iommu_dev_data *dev_data)
 {
-	unsigned long flags;
-
-	raw_spin_lock_irqsave(&iommu->lock, flags);
-
 	iopf_queue_remove_device(iommu->iopf_queue, dev_data->dev);
 	dev_data->ppr = false;
-
-	raw_spin_unlock_irqrestore(&iommu->lock, flags);
 }
-- 
2.31.1

Re: [PATCH 2/2] iommu/amd: Fix Invalid wait context issue

[Chris Bainbridge]

On Tue, 28 May 2024 at 17:40, Vasant Hegde <vasant.hegde@amd.com> wrote:
>
> With commit c4cb23111103 ("iommu/amd: Add support for enable/disable IOPF")
> we are hitting below issue. This happens because in IOPF enablement path
> it holds spin lock with irq disable and then tries to take mutex lock.
>
> dmesg:
> -----
> [    0.938739] =============================
> [    0.938740] [ BUG: Invalid wait context ]
> [    0.938742] 6.10.0-rc1+ #1 Not tainted
> [    0.938745] -----------------------------
> [    0.938746] swapper/0/1 is trying to lock:
> [    0.938748] ffffffff8c9f01d8 (&port_lock_key){....}-{3:3}, at: serial8250_console_write+0x78/0x4a0
> [    0.938767] other info that might help us debug this:
> [    0.938768] context-{5:5}
> [    0.938769] 7 locks held by swapper/0/1:
> [    0.938772]  #0: ffff888101a91310 (&group->mutex){+.+.}-{4:4}, at: bus_iommu_probe+0x70/0x160
> [    0.938790]  #1: ffff888101d1f1b8 (&domain->lock){....}-{3:3}, at: amd_iommu_attach_device+0xa5/0x700
> [    0.938799]  #2: ffff888101cc3d18 (&dev_data->lock){....}-{3:3}, at: amd_iommu_attach_device+0xc5/0x700
> [    0.938806]  #3: ffff888100052830 (&iommu->lock){....}-{2:2}, at: amd_iommu_iopf_add_device+0x3f/0xa0
> [    0.938813]  #4: ffffffff8945a340 (console_lock){+.+.}-{0:0}, at: _printk+0x48/0x50
> [    0.938822]  #5: ffffffff8945a390 (console_srcu){....}-{0:0}, at: console_flush_all+0x58/0x4e0
> [    0.938867]  #6: ffffffff82459f80 (console_owner){....}-{0:0}, at: console_flush_all+0x1f0/0x4e0
> [    0.938872] stack backtrace:
> [    0.938874] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 6.10.0-rc1+ #1
> [    0.938877] Hardware name: HP HP EliteBook 745 G3/807E, BIOS N73 Ver. 01.39 04/16/2019
>
> Fix above issue by re-arranging code in attach device path:
>   - move device PASID/IOPF enablement outside lock in AMD IOMMU driver.
>     This is safe as core layer holds group->mutex lock before calling
>     iommu_ops->attach_dev.
>
> Reported-by: Borislav Petkov <bp@alien8.de>
> Reported-by: Mikhail Gavrilov <mikhail.v.gavrilov@gmail.com>
> Reported-by: Chris Bainbridge <chris.bainbridge@gmail.com>
> Fixes: c4cb23111103 ("iommu/amd: Add support for enable/disable IOPF")
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>

Patch works for me.

Tested-by: Chris Bainbridge <chris.bainbridge@gmail.com>

Re: [PATCH 2/2] iommu/amd: Fix Invalid wait context issue

[Mikhail Gavrilov]

[-- Attachment #1: Type: text/plain, Size: 7888 bytes --]

On Tue, May 28, 2024 at 9:40 PM Vasant Hegde <vasant.hegde@amd.com> wrote:
>
> With commit c4cb23111103 ("iommu/amd: Add support for enable/disable IOPF")
> we are hitting below issue. This happens because in IOPF enablement path
> it holds spin lock with irq disable and then tries to take mutex lock.
>
> dmesg:
> -----
> [    0.938739] =============================
> [    0.938740] [ BUG: Invalid wait context ]
> [    0.938742] 6.10.0-rc1+ #1 Not tainted
> [    0.938745] -----------------------------
> [    0.938746] swapper/0/1 is trying to lock:
> [    0.938748] ffffffff8c9f01d8 (&port_lock_key){....}-{3:3}, at: serial8250_console_write+0x78/0x4a0
> [    0.938767] other info that might help us debug this:
> [    0.938768] context-{5:5}
> [    0.938769] 7 locks held by swapper/0/1:
> [    0.938772]  #0: ffff888101a91310 (&group->mutex){+.+.}-{4:4}, at: bus_iommu_probe+0x70/0x160
> [    0.938790]  #1: ffff888101d1f1b8 (&domain->lock){....}-{3:3}, at: amd_iommu_attach_device+0xa5/0x700
> [    0.938799]  #2: ffff888101cc3d18 (&dev_data->lock){....}-{3:3}, at: amd_iommu_attach_device+0xc5/0x700
> [    0.938806]  #3: ffff888100052830 (&iommu->lock){....}-{2:2}, at: amd_iommu_iopf_add_device+0x3f/0xa0
> [    0.938813]  #4: ffffffff8945a340 (console_lock){+.+.}-{0:0}, at: _printk+0x48/0x50
> [    0.938822]  #5: ffffffff8945a390 (console_srcu){....}-{0:0}, at: console_flush_all+0x58/0x4e0
> [    0.938867]  #6: ffffffff82459f80 (console_owner){....}-{0:0}, at: console_flush_all+0x1f0/0x4e0
> [    0.938872] stack backtrace:
> [    0.938874] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 6.10.0-rc1+ #1
> [    0.938877] Hardware name: HP HP EliteBook 745 G3/807E, BIOS N73 Ver. 01.39 04/16/2019
>
> Fix above issue by re-arranging code in attach device path:
>   - move device PASID/IOPF enablement outside lock in AMD IOMMU driver.
>     This is safe as core layer holds group->mutex lock before calling
>     iommu_ops->attach_dev.
>
> Reported-by: Borislav Petkov <bp@alien8.de>
> Reported-by: Mikhail Gavrilov <mikhail.v.gavrilov@gmail.com>
> Reported-by: Chris Bainbridge <chris.bainbridge@gmail.com>
> Fixes: c4cb23111103 ("iommu/amd: Add support for enable/disable IOPF")
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/iommu.c | 48 +++++++++++++++++++--------------------
>  drivers/iommu/amd/ppr.c   | 22 ++++--------------
>  2 files changed, 28 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index 52d83730a22a..c2703599bb16 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -2032,7 +2032,6 @@ static int do_attach(struct iommu_dev_data *dev_data,
>                      struct protection_domain *domain)
>  {
>         struct amd_iommu *iommu = get_amd_iommu_from_dev_data(dev_data);
> -       struct pci_dev *pdev;
>         int ret = 0;
>
>         /* Update data structures */
> @@ -2047,30 +2046,13 @@ static int do_attach(struct iommu_dev_data *dev_data,
>         domain->dev_iommu[iommu->index] += 1;
>         domain->dev_cnt                 += 1;
>
> -       pdev = dev_is_pci(dev_data->dev) ? to_pci_dev(dev_data->dev) : NULL;
> +       /* Setup GCR3 table */
>         if (pdom_is_sva_capable(domain)) {
>                 ret = init_gcr3_table(dev_data, domain);
>                 if (ret)
>                         return ret;
> -
> -               if (pdev) {
> -                       pdev_enable_caps(pdev);
> -
> -                       /*
> -                        * Device can continue to function even if IOPF
> -                        * enablement failed. Hence in error path just
> -                        * disable device PRI support.
> -                        */
> -                       if (amd_iommu_iopf_add_device(iommu, dev_data))
> -                               pdev_disable_cap_pri(pdev);
> -               }
> -       } else if (pdev) {
> -               pdev_enable_cap_ats(pdev);
>         }
>
> -       /* Update device table */
> -       amd_iommu_dev_update_dte(dev_data, true);
> -
>         return ret;
>  }
>
> @@ -2163,6 +2145,11 @@ static void detach_device(struct device *dev)
>
>         do_detach(dev_data);
>
> +out:
> +       spin_unlock(&dev_data->lock);
> +
> +       spin_unlock_irqrestore(&domain->lock, flags);
> +
>         /* Remove IOPF handler */
>         if (ppr)
>                 amd_iommu_iopf_remove_device(iommu, dev_data);
> @@ -2170,10 +2157,6 @@ static void detach_device(struct device *dev)
>         if (dev_is_pci(dev))
>                 pdev_disable_caps(to_pci_dev(dev));
>
> -out:
> -       spin_unlock(&dev_data->lock);
> -
> -       spin_unlock_irqrestore(&domain->lock, flags);
>  }
>
>  static struct iommu_device *amd_iommu_probe_device(struct device *dev)
> @@ -2485,6 +2468,7 @@ static int amd_iommu_attach_device(struct iommu_domain *dom,
>         struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
>         struct protection_domain *domain = to_pdomain(dom);
>         struct amd_iommu *iommu = get_amd_iommu_from_dev(dev);
> +       struct pci_dev *pdev;
>         int ret;
>
>         /*
> @@ -2517,7 +2501,23 @@ static int amd_iommu_attach_device(struct iommu_domain *dom,
>         }
>  #endif
>
> -       iommu_completion_wait(iommu);
> +       pdev = dev_is_pci(dev_data->dev) ? to_pci_dev(dev_data->dev) : NULL;
> +       if (pdev && pdom_is_sva_capable(domain)) {
> +               pdev_enable_caps(pdev);
> +
> +               /*
> +                * Device can continue to function even if IOPF
> +                * enablement failed. Hence in error path just
> +                * disable device PRI support.
> +                */
> +               if (amd_iommu_iopf_add_device(iommu, dev_data))
> +                       pdev_disable_cap_pri(pdev);
> +       } else if (pdev) {
> +               pdev_enable_cap_ats(pdev);
> +       }
> +
> +       /* Update device table */
> +       amd_iommu_dev_update_dte(dev_data, true);
>
>         return ret;
>  }
> diff --git a/drivers/iommu/amd/ppr.c b/drivers/iommu/amd/ppr.c
> index 091423bb8aac..b3b690410f2a 100644
> --- a/drivers/iommu/amd/ppr.c
> +++ b/drivers/iommu/amd/ppr.c
> @@ -249,40 +249,26 @@ void amd_iommu_page_response(struct device *dev, struct iopf_fault *evt,
>  int amd_iommu_iopf_add_device(struct amd_iommu *iommu,
>                               struct iommu_dev_data *dev_data)
>  {
> -       unsigned long flags;
>         int ret = 0;
>
>         if (!dev_data->pri_enabled)
>                 return ret;
>
> -       raw_spin_lock_irqsave(&iommu->lock, flags);
> -
> -       if (!iommu->iopf_queue) {
> -               ret = -EINVAL;
> -               goto out_unlock;
> -       }
> +       if (!iommu->iopf_queue)
> +               return -EINVAL;
>
>         ret = iopf_queue_add_device(iommu->iopf_queue, dev_data->dev);
>         if (ret)
> -               goto out_unlock;
> +               return ret;
>
>         dev_data->ppr = true;
> -
> -out_unlock:
> -       raw_spin_unlock_irqrestore(&iommu->lock, flags);
> -       return ret;
> +       return 0;
>  }
>
>  /* Its assumed that caller has verified that device was added to iopf queue */
>  void amd_iommu_iopf_remove_device(struct amd_iommu *iommu,
>                                   struct iommu_dev_data *dev_data)
>  {
> -       unsigned long flags;
> -
> -       raw_spin_lock_irqsave(&iommu->lock, flags);
> -
>         iopf_queue_remove_device(iommu->iopf_queue, dev_data->dev);
>         dev_data->ppr = false;
> -
> -       raw_spin_unlock_irqrestore(&iommu->lock, flags);
>  }
> --
> 2.31.1
>

Tested-by: Mikhail Gavrilov <mikhail.v.gavrilov@gmail.com>

-- 
Best Regards,
Mike Gavrilov.

[-- Attachment #2: dmesg.zip --]
[-- Type: application/zip, Size: 63104 bytes --]

Re: [PATCH 1/2] iommu: Take group lock before attaching device in iommu_deferred_attach()

[Robin Murphy]

On 2024-05-28 5:39 pm, Vasant Hegde wrote:
> Commit 3ab657291638 ("iommu: use the __iommu_attach_device() directly for deferred attach")
> replaced iommu_attach_device() call with __iommu_attach_device(). But
> missed to take group lock. Take lock before attaching device to domain.

Why? Nothing here is even touching the group. If we've reached a 
deferred attach then we know the domain is the default domain already 
initialised and set as the current of the device's group, and the device 
is already otherwise added and holding a reference to that group, and a 
driver is bound to the device in order to make the DMA API call we're 
inside, so nothing should be at risk of disappearing under our feet. 
Please clarify what purpose this locking actually serves.

But then there's also the fact that the initial DMA mapping operation 
which triggers deferred attach could legitimately be called under a 
spinlock or in an IRQ handler, so if anything it was a bug in 
795bbbb9b6f8 ("iommu/dma-iommu: Handle deferred devices") to ever use 
iommu_attach_device() (and thus involve the mutex) in the first place :/

Thanks,
Robin.

> Fixes: 3ab657291638 ("iommu: use the __iommu_attach_device() directly for deferred attach")
> Cc: Lianbo Jiang <lijiang@redhat.com>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>   drivers/iommu/iommu.c | 15 ++++++++++++---
>   1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 9df7cc75c1bc..1743dba023b6 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -2114,10 +2114,19 @@ EXPORT_SYMBOL_GPL(iommu_attach_device);
>   
>   int iommu_deferred_attach(struct device *dev, struct iommu_domain *domain)
>   {
> -	if (dev->iommu && dev->iommu->attach_deferred)
> -		return __iommu_attach_device(domain, dev);
> +	int ret = 0;
> +	struct iommu_group *group = dev->iommu_group;
>   
> -	return 0;
> +	if (!group)
> +		return -EINVAL;
> +
> +	if (dev->iommu && dev->iommu->attach_deferred) {
> +		mutex_lock(&group->mutex);
> +		ret = __iommu_attach_device(domain, dev);
> +		mutex_unlock(&group->mutex);
> +	}
> +
> +	return ret;
>   }
>   
>   void iommu_detach_device(struct iommu_domain *domain, struct device *dev)

Re: [PATCH 1/2] iommu: Take group lock before attaching device in iommu_deferred_attach()

[Vasant Hegde]

Hi Robin,


On 5/28/2024 11:36 PM, Robin Murphy wrote:
> On 2024-05-28 5:39 pm, Vasant Hegde wrote:
>> Commit 3ab657291638 ("iommu: use the __iommu_attach_device() directly for
>> deferred attach")
>> replaced iommu_attach_device() call with __iommu_attach_device(). But
>> missed to take group lock. Take lock before attaching device to domain.
> 
> Why? Nothing here is even touching the group. If we've reached a deferred attach
> then we know the domain is the default domain already initialised and set as the
> current of the device's group, and the device is already otherwise added and
> holding a reference to that group, and a driver is bound to the device in order
> to make the DMA API call we're inside, so nothing should be at risk of
> disappearing under our feet. Please clarify what purpose this locking actually
> serves.

I am sorry. I should have written better description. I was trying to see if its
safe to use  iommu_group_mutex_assert() in our driver. I did audit the iommu.c
code and saw this one place it was missed. I just looked into git history and
tried to fix it. As you explained below I should have looked into the caller of
this function in detail!

> 
> But then there's also the fact that the initial DMA mapping operation which
> triggers deferred attach could legitimately be called under a spinlock or in an
> IRQ handler, so if anything it was a bug in 795bbbb9b6f8 ("iommu/dma-iommu:
> Handle deferred devices") to ever use iommu_attach_device() (and thus involve
> the mutex) in the first place :/

Thanks for the detailed explanation. I will drop this patch in next version.

-Vasant


> 
> Thanks,
> Robin.
> 
>> Fixes: 3ab657291638 ("iommu: use the __iommu_attach_device() directly for
>> deferred attach")
>> Cc: Lianbo Jiang <lijiang@redhat.com>
>> Cc: Robin Murphy <robin.murphy@arm.com>
>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
>> ---
>>   drivers/iommu/iommu.c | 15 ++++++++++++---
>>   1 file changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 9df7cc75c1bc..1743dba023b6 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -2114,10 +2114,19 @@ EXPORT_SYMBOL_GPL(iommu_attach_device);
>>     int iommu_deferred_attach(struct device *dev, struct iommu_domain *domain)
>>   {
>> -    if (dev->iommu && dev->iommu->attach_deferred)
>> -        return __iommu_attach_device(domain, dev);
>> +    int ret = 0;
>> +    struct iommu_group *group = dev->iommu_group;
>>   -    return 0;
>> +    if (!group)
>> +        return -EINVAL;
>> +
>> +    if (dev->iommu && dev->iommu->attach_deferred) {
>> +        mutex_lock(&group->mutex);
>> +        ret = __iommu_attach_device(domain, dev);
>> +        mutex_unlock(&group->mutex);
>> +    }
>> +
>> +    return ret;
>>   }
>>     void iommu_detach_device(struct iommu_domain *domain, struct device *dev)

Re: [PATCH 1/2] iommu: Take group lock before attaching device in iommu_deferred_attach()

[Jason Gunthorpe]

On Wed, May 29, 2024 at 12:54:14PM +0530, Vasant Hegde wrote:
> Hi Robin,
> 
> 
> On 5/28/2024 11:36 PM, Robin Murphy wrote:
> > On 2024-05-28 5:39 pm, Vasant Hegde wrote:
> >> Commit 3ab657291638 ("iommu: use the __iommu_attach_device() directly for
> >> deferred attach")
> >> replaced iommu_attach_device() call with __iommu_attach_device(). But
> >> missed to take group lock. Take lock before attaching device to domain.
> > 
> > Why? Nothing here is even touching the group. If we've reached a deferred attach
> > then we know the domain is the default domain already initialised and set as the
> > current of the device's group, and the device is already otherwise added and
> > holding a reference to that group, and a driver is bound to the device in order
> > to make the DMA API call we're inside, so nothing should be at risk of
> > disappearing under our feet. Please clarify what purpose this locking actually
> > serves.
> 
> I am sorry. I should have written better description. I was trying to see if its
> safe to use  iommu_group_mutex_assert() in our driver. 

The core code guarentees that the dev will not have racing
attach_dev() ops, and we added the iommu_group_mutex_assert() as a way
for drivers to document they are making use of that assumption.

I'm guessing the purpose of this patch is to allow the AMD driver to
use iommu_group_mutex_assert() as it will fail in the deferred attach
path.

> > But then there's also the fact that the initial DMA mapping operation which
> > triggers deferred attach could legitimately be called under a spinlock or in an
> > IRQ handler, so if anything it was a bug in 795bbbb9b6f8 ("iommu/dma-iommu:
> > Handle deferred devices") to ever use iommu_attach_device() (and thus involve
> > the mutex) in the first place :/

ops->attach_dev() must be called in a sleepable context, even
the Intel driver will hit a GFP_KERNEL allocation in its attach_dev()
op. It seems to be an issue in 795bbbb9b6f8 that it did not consider
this.

I suppose in practice kdump using drivers are setting up DMA during
their probe functions and don't get into this problem.

IMHO adding the mutex here is an appropriate thing, it should not be
closing any race, but it is the right locking documentation to make
iommu_group_mutex_assert() work.

Jason

Re: [PATCH 1/2] iommu: Take group lock before attaching device in iommu_deferred_attach()

[Vasant Hegde]

Jason,


On 6/10/2024 11:14 PM, Jason Gunthorpe wrote:
> On Wed, May 29, 2024 at 12:54:14PM +0530, Vasant Hegde wrote:
>> Hi Robin,
>>
>>
>> On 5/28/2024 11:36 PM, Robin Murphy wrote:
>>> On 2024-05-28 5:39 pm, Vasant Hegde wrote:
>>>> Commit 3ab657291638 ("iommu: use the __iommu_attach_device() directly for
>>>> deferred attach")
>>>> replaced iommu_attach_device() call with __iommu_attach_device(). But
>>>> missed to take group lock. Take lock before attaching device to domain.
>>>
>>> Why? Nothing here is even touching the group. If we've reached a deferred attach
>>> then we know the domain is the default domain already initialised and set as the
>>> current of the device's group, and the device is already otherwise added and
>>> holding a reference to that group, and a driver is bound to the device in order
>>> to make the DMA API call we're inside, so nothing should be at risk of
>>> disappearing under our feet. Please clarify what purpose this locking actually
>>> serves.
>>
>> I am sorry. I should have written better description. I was trying to see if its
>> safe to use  iommu_group_mutex_assert() in our driver. 
> 
> The core code guarentees that the dev will not have racing
> attach_dev() ops, and we added the iommu_group_mutex_assert() as a way
> for drivers to document they are making use of that assumption.
> 
> I'm guessing the purpose of this patch is to allow the AMD driver to
> use iommu_group_mutex_assert() as it will fail in the deferred attach
> path.

Right. That's what I was thinking.

> 
>>> But then there's also the fact that the initial DMA mapping operation which
>>> triggers deferred attach could legitimately be called under a spinlock or in an
>>> IRQ handler, so if anything it was a bug in 795bbbb9b6f8 ("iommu/dma-iommu:
>>> Handle deferred devices") to ever use iommu_attach_device() (and thus involve
>>> the mutex) in the first place :/
> 
> ops->attach_dev() must be called in a sleepable context, even
> the Intel driver will hit a GFP_KERNEL allocation in its attach_dev()
> op. It seems to be an issue in 795bbbb9b6f8 that it did not consider
> this.
> 
> I suppose in practice kdump using drivers are setting up DMA during
> their probe functions and don't get into this problem.

Looking into code path and AMD driver code, it does look like in kdump path
deferred attach gets called.

> 
> IMHO adding the mutex here is an appropriate thing, it should not be
> closing any race, but it is the right locking documentation to make
> iommu_group_mutex_assert() work.

It may be good thing to add mutex here.. But if any of the caller of these
deferred functions hold spin lock again we will hit the similar issue that I hit
with AMD driver.


-Vasant

Re: [PATCH 1/2] iommu: Take group lock before attaching device in iommu_deferred_attach()

[Jason Gunthorpe]

On Mon, Jun 24, 2024 at 07:50:46PM +0530, Vasant Hegde wrote:

> It may be good thing to add mutex here.. But if any of the caller of these
> deferred functions hold spin lock again we will hit the similar issue that I hit
> with AMD driver.

My point was that Intel doesn't support non-sleeping attach, it uses
GFP_KERNEL, that support seems to be a uniquely AMD thing of the major
drivers.

So, I don't see an issue with formalizing the lowest common
denominator in the core code - the context must be sleepable.

Presumably things work because the kdump kernels only run drivers that
reliably trigger first dma from sleepable contexts. Unless there is
something AMD only that relies on this special behavior??

Jason

Re: [PATCH 1/2] iommu: Take group lock before attaching device in iommu_deferred_attach()

[Vasant Hegde]

Jason,


On 6/24/2024 8:55 PM, Jason Gunthorpe wrote:
> On Mon, Jun 24, 2024 at 07:50:46PM +0530, Vasant Hegde wrote:
> 
>> It may be good thing to add mutex here.. But if any of the caller of these
>> deferred functions hold spin lock again we will hit the similar issue that I hit
>> with AMD driver.
> 
> My point was that Intel doesn't support non-sleeping attach, it uses
> GFP_KERNEL, that support seems to be a uniquely AMD thing of the major
> drivers.

As I understand we don't have any hard requirement. We can sleep in attach
device path. I do have a patch to replace dev_data->spinlock to mutex in AMD
driver. I need to do some more testing before sending it out.

> 
> So, I don't see an issue with formalizing the lowest common
> denominator in the core code - the context must be sleepable.
> 
> Presumably things work because the kdump kernels only run drivers that
> reliably trigger first dma from sleepable contexts. Unless there is
> something AMD only that relies on this special behavior??

AFAIK AMD driver doesn't have any special behaviour in kdump path. When we jump
to crashing kernel (kexec) we don't disable IOMMU and try to init IOMMU with
existing info.

Having said that I have not done exhaustive kdump test with mutex lock. I will
run some more tests and get back to you.

-Vasant