[PATCH 0/3] Fix device_lock deadlock on two probe() paths

[PATCH 0/3] Fix device_lock deadlock on two probe() paths

[Jason Gunthorpe]

I missed two paths where __iommu_probe_device() can be called while
already holding the device_lock() for the device that is to be probed.

This causes a deadlock because __iommu_probe_device() will attempt to
re-acquire the lock.

Organize things so that these two paths can re-use the existing already
held device_lock by marking the call chains based on if the lock is held
or not.

This is an incremental on top of Joerg's next, but it could also be handled by
respinning the last patch in that series. Please let me know.

Jason Gunthorpe (3):
  iommu: Provide iommu_probe_device_locked()
  iommu: Pass in the iommu_device to probe for in bus_iommu_probe()
  iommu: Do not attempt to re-lock the iommu device when probing

 drivers/acpi/scan.c        |  5 ++--
 drivers/iommu/iommu.c      | 60 +++++++++++++++++++++++++++-----------
 drivers/iommu/of_iommu.c   |  2 +-
 drivers/iommu/omap-iommu.c | 12 ++++++--
 include/linux/iommu.h      |  6 +++-
 5 files changed, 61 insertions(+), 24 deletions(-)


base-commit: 8d3740021d5d461e1ec4c17fc5625b9b4237c2f8
-- 
2.41.0

[PATCH 1/3] iommu: Provide iommu_probe_device_locked()

[Jason Gunthorpe]

The two callers coming from bus_tye -> dma_configure() already have the
device_lock held for the device being probed. Mark this in the API so that
the core code doesn't attempt to re-acquire the same lock and deadlock.

  __iommu_probe_device from iommu_probe_device+0x10/0x40
  iommu_probe_device from of_iommu_configure+0xf8/0x1c8
  of_iommu_configure from of_dma_configure_id+0x188/0x450
  of_dma_configure_id from platform_dma_configure+0x24/0x60
  platform_dma_configure from really_probe+0xac/0x3d4
  really_probe from __driver_probe_device+0xa0/0x1e8
  __driver_probe_device from driver_probe_device+0x30/0xd0
  driver_probe_device from __driver_attach+0x10c/0x190
  __driver_attach from bus_for_each_dev+0x60/0xb4
  bus_for_each_dev from bus_add_driver+0xe0/0x208
  bus_add_driver from driver_register+0x7c/0x118
  driver_register from exynos_drm_init+0xe0/0x14c
  exynos_drm_init from do_one_initcall+0x6c/0x318
  do_one_initcall from kernel_init_freeable+0x1c4/0x214
  kernel_init_freeable from kernel_init+0x18/0x12c
  kernel_init from ret_from_fork+0x14/0x2c

Internally make __iommu_probe_device() require the caller to get the
device_lock().

The three remaining callers of iommu_probe_device() don't seem to hold the
device_lock():

 powerpc/kernel/iommu.c: iommu_add_device()
 iommu/iommu.c: iommu_bus_notifier()/BUS_NOTIFY_ADD_DEVICE/
 intel/iommu.c: probe_acpi_namespace_devices()

Fixes: a2dde836050f ("iommu: Complete the locking for dev->iommu_group")
Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
Closes: https://lore.kernel.org/linux-iommu/bc5d6aa8-e8ca-c896-2f39-af074d55e436@samsung.com
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/acpi/scan.c      |  5 +++--
 drivers/iommu/iommu.c    | 34 ++++++++++++++++++++++------------
 drivers/iommu/of_iommu.c |  2 +-
 include/linux/iommu.h    |  1 +
 4 files changed, 27 insertions(+), 15 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index daa64dd687524b..3771164af60279 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1579,10 +1579,11 @@ static const struct iommu_ops *acpi_iommu_configure_id(struct device *dev,
 
 	/*
 	 * If we have reason to believe the IOMMU driver missed the initial
-	 * iommu_probe_device() call for dev, replay it to get things in order.
+	 * iommu_probe_device_locked() call for dev, replay it to get things in
+	 * order.
 	 */
 	if (!err && dev->bus)
-		err = iommu_probe_device(dev);
+		err = iommu_probe_device_locked(dev);
 
 	/* Ignore all other errors apart from EPROBE_DEFER */
 	if (err == -EPROBE_DEFER) {
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 25d7327e8013e7..ecf61bd3cfb076 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -452,24 +452,23 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
 	struct group_device *gdev;
 	int ret;
 
-	if (!ops)
-		return -ENODEV;
 	/*
 	 * Allow __iommu_probe_device() to be safely called in parallel,
 	 * both dev->iommu_group and the initial setup of dev->iommu are
 	 * protected this way.
 	 */
-	device_lock(dev);
+	device_lock_assert(dev);
+
+	if (!ops)
+		return -ENODEV;
 
 	/* Device is probed already if in a group */
-	if (dev->iommu_group) {
-		ret = 0;
-		goto out_unlock;
-	}
+	if (dev->iommu_group)
+		return 0;
 
 	ret = iommu_init_device(dev, ops);
 	if (ret)
-		goto out_unlock;
+		return ret;
 
 	group = dev->iommu_group;
 	gdev = iommu_group_alloc_device(group, dev);
@@ -505,7 +504,6 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
 			list_add_tail(&group->entry, group_list);
 	}
 	mutex_unlock(&group->mutex);
-	device_unlock(dev);
 
 	if (dev_is_pci(dev))
 		iommu_dma_set_pci_32bit_workaround(dev);
@@ -519,16 +517,16 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
 	iommu_deinit_device(dev);
 	mutex_unlock(&group->mutex);
 	iommu_group_put(group);
-out_unlock:
-	device_unlock(dev);
 	return ret;
 }
 
-int iommu_probe_device(struct device *dev)
+int iommu_probe_device_locked(struct device *dev)
 {
 	const struct iommu_ops *ops;
 	int ret;
 
+	device_lock_assert(dev);
+
 	ret = __iommu_probe_device(dev, NULL);
 	if (ret)
 		return ret;
@@ -540,6 +538,16 @@ int iommu_probe_device(struct device *dev)
 	return 0;
 }
 
+int iommu_probe_device(struct device *dev)
+{
+	int ret;
+
+	device_lock(dev);
+	ret = iommu_probe_device_locked(dev);
+	device_unlock(dev);
+	return ret;
+}
+
 static void __iommu_group_free_device(struct iommu_group *group,
 				      struct group_device *grp_dev)
 {
@@ -1789,7 +1797,9 @@ static int probe_iommu_group(struct device *dev, void *data)
 	struct list_head *group_list = data;
 	int ret;
 
+	device_lock(dev);
 	ret = __iommu_probe_device(dev, group_list);
+	device_unlock(dev);
 	if (ret == -ENODEV)
 		ret = 0;
 
diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index 157b286e36bf3a..b5b7d4bd2cefb9 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -160,7 +160,7 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
 	 * probe for dev, replay it to get things in order.
 	 */
 	if (!err && dev->bus)
-		err = iommu_probe_device(dev);
+		err = iommu_probe_device_locked(dev);
 
 	/* Ignore all other errors apart from EPROBE_DEFER */
 	if (err == -EPROBE_DEFER) {
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index f1e18e81fca78b..cb4fc518797039 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -709,6 +709,7 @@ static inline void dev_iommu_priv_set(struct device *dev, void *priv)
 }
 
 int iommu_probe_device(struct device *dev);
+int iommu_probe_device_locked(struct device *dev);
 
 int iommu_dev_enable_feature(struct device *dev, enum iommu_dev_features f);
 int iommu_dev_disable_feature(struct device *dev, enum iommu_dev_features f);
-- 
2.41.0

RE: [PATCH 1/3] iommu: Provide iommu_probe_device_locked()

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Wednesday, August 9, 2023 1:27 AM
> 
> The two callers coming from bus_tye -> dma_configure() already have the

s/bus_tye/bus_type/

> device_lock held for the device being probed. Mark this in the API so that
> the core code doesn't attempt to re-acquire the same lock and deadlock.
> 
>   __iommu_probe_device from iommu_probe_device+0x10/0x40
>   iommu_probe_device from of_iommu_configure+0xf8/0x1c8
>   of_iommu_configure from of_dma_configure_id+0x188/0x450
>   of_dma_configure_id from platform_dma_configure+0x24/0x60
>   platform_dma_configure from really_probe+0xac/0x3d4
>   really_probe from __driver_probe_device+0xa0/0x1e8
>   __driver_probe_device from driver_probe_device+0x30/0xd0
>   driver_probe_device from __driver_attach+0x10c/0x190
>   __driver_attach from bus_for_each_dev+0x60/0xb4
>   bus_for_each_dev from bus_add_driver+0xe0/0x208
>   bus_add_driver from driver_register+0x7c/0x118
>   driver_register from exynos_drm_init+0xe0/0x14c
>   exynos_drm_init from do_one_initcall+0x6c/0x318
>   do_one_initcall from kernel_init_freeable+0x1c4/0x214
>   kernel_init_freeable from kernel_init+0x18/0x12c
>   kernel_init from ret_from_fork+0x14/0x2c
> 
> Internally make __iommu_probe_device() require the caller to get the
> device_lock().
> 
> The three remaining callers of iommu_probe_device() don't seem to hold the
> device_lock():
> 
>  powerpc/kernel/iommu.c: iommu_add_device()
>  iommu/iommu.c: iommu_bus_notifier()/BUS_NOTIFY_ADD_DEVICE/
>  intel/iommu.c: probe_acpi_namespace_devices()
> 
> Fixes: a2dde836050f ("iommu: Complete the locking for dev->iommu_group")
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Closes: https://lore.kernel.org/linux-iommu/bc5d6aa8-e8ca-c896-2f39-
> af074d55e436@samsung.com
> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

Re: [PATCH 1/3] iommu: Provide iommu_probe_device_locked()

[Joerg Roedel]

Hi Jason,

On Tue, Aug 08, 2023 at 02:27:05PM -0300, Jason Gunthorpe wrote:
> Fixes: a2dde836050f ("iommu: Complete the locking for dev->iommu_group")

The right commit-id here is: a16b5d1681ab

Please update the fixes tags and re-send after addressing Kevin's
concerns.

Regards,

	Joerg

[PATCH 2/3] iommu: Pass in the iommu_device to probe for in bus_iommu_probe()

[Jason Gunthorpe]

This is preparation for the next patch.

Each iommu driver is associated with a 'struct iommu_device' handle. Pass
in the iommu_device to bus_iommu_probe() and all the way through to
probe_iommu_group().

omap is weird, it has a whole bunch of iommu devices that it creates a
struct omap_iommu for, but it only registers some of then with the
subsystem. In the case it doesn't register then it has to open code the
call to bus_iommu_probe() as it's omap_iommu_probe_device() function is
sensitive. Pass in the unregistered iommu_device struct and move this code
into an else block since there is no sense in calling bus_iommu_probe()
twice in a row.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommu.c      | 18 +++++++++++++-----
 drivers/iommu/omap-iommu.c | 11 ++++++++---
 include/linux/iommu.h      |  3 ++-
 3 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index ecf61bd3cfb076..19fdb1a220240f 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -273,7 +273,7 @@ int iommu_device_register(struct iommu_device *iommu,
 
 	for (int i = 0; i < ARRAY_SIZE(iommu_buses) && !err; i++) {
 		iommu_buses[i]->iommu_ops = ops;
-		err = bus_iommu_probe(iommu_buses[i]);
+		err = bus_iommu_probe(iommu_buses[i], iommu);
 	}
 	if (err)
 		iommu_device_unregister(iommu);
@@ -1792,13 +1792,18 @@ struct iommu_domain *iommu_group_default_domain(struct iommu_group *group)
 	return group->default_domain;
 }
 
+struct probe_iommu_args {
+	struct list_head *group_list;
+	struct iommu_device *iommu;
+};
+
 static int probe_iommu_group(struct device *dev, void *data)
 {
-	struct list_head *group_list = data;
+	struct probe_iommu_args *args = data;
 	int ret;
 
 	device_lock(dev);
-	ret = __iommu_probe_device(dev, group_list);
+	ret = __iommu_probe_device(dev, args->group_list);
 	device_unlock(dev);
 	if (ret == -ENODEV)
 		ret = 0;
@@ -1868,13 +1873,16 @@ static void iommu_group_do_probe_finalize(struct device *dev)
 		ops->probe_finalize(dev);
 }
 
-int bus_iommu_probe(const struct bus_type *bus)
+int bus_iommu_probe(const struct bus_type *bus, struct iommu_device *iommu)
 {
+	struct probe_iommu_args args = {};
 	struct iommu_group *group, *next;
 	LIST_HEAD(group_list);
 	int ret;
 
-	ret = bus_for_each_dev(bus, NULL, &group_list, probe_iommu_group);
+	args.group_list = &group_list;
+	args.iommu = iommu;
+	ret = bus_for_each_dev(bus, NULL, &args, probe_iommu_group);
 	if (ret)
 		return ret;
 
diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index 97c45f50bf4332..1e4a90ec64322b 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1234,6 +1234,14 @@ static int omap_iommu_probe(struct platform_device *pdev)
 		if (err)
 			goto out_sysfs;
 		obj->has_iommu_driver = true;
+	} else {
+		/*
+		 * omap_iommu_probe_device() requires all the iommus associated
+		 * with a device to have been probed to succeed. We just created
+		 * an iommu without registering it, so re-run probe again to try
+		 * to match any devices that are waiting for this iommu.
+		 */
+		bus_iommu_probe(&platform_bus_type, &obj->iommu);
 	}
 
 	pm_runtime_enable(obj->dev);
@@ -1242,9 +1250,6 @@ static int omap_iommu_probe(struct platform_device *pdev)
 
 	dev_info(&pdev->dev, "%s registered\n", obj->name);
 
-	/* Re-probe bus to probe device attached to this IOMMU */
-	bus_iommu_probe(&platform_bus_type);
-
 	return 0;
 
 out_sysfs:
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index cb4fc518797039..cc47e4086d69ec 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -465,7 +465,8 @@ static inline const struct iommu_ops *dev_iommu_ops(struct device *dev)
 	return dev->iommu->iommu_dev->ops;
 }
 
-extern int bus_iommu_probe(const struct bus_type *bus);
+extern int bus_iommu_probe(const struct bus_type *bus,
+			   struct iommu_device *iommu);
 extern bool iommu_present(const struct bus_type *bus);
 extern bool device_iommu_capable(struct device *dev, enum iommu_cap cap);
 extern bool iommu_group_has_isolated_msi(struct iommu_group *group);
-- 
2.41.0

RE: [PATCH 2/3] iommu: Pass in the iommu_device to probe for in bus_iommu_probe()

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Wednesday, August 9, 2023 1:27 AM
> 
> This is preparation for the next patch.
> 
> Each iommu driver is associated with a 'struct iommu_device' handle. Pass
> in the iommu_device to bus_iommu_probe() and all the way through to
> probe_iommu_group().
> 
> omap is weird, it has a whole bunch of iommu devices that it creates a
> struct omap_iommu for, but it only registers some of then with the

s/then/them/

> subsystem. In the case it doesn't register then it has to open code the
> call to bus_iommu_probe() as it's omap_iommu_probe_device() function is
> sensitive. Pass in the unregistered iommu_device struct and move this code
> into an else block since there is no sense in calling bus_iommu_probe()
> twice in a row.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

[PATCH 3/3] iommu: Do not attempt to re-lock the iommu device when probing

[Jason Gunthorpe]

When bus_iommu_probe() runs it can attempt to probe the iommu itself, for
instance if the iommu is located on a platform_bus. This will cause the
device_lock() to deadlock on itself as the device_driver probe() callback
for the device calling iommu_device_register() already holds the
device_lock():

 probe_iommu_group+0x18/0x38
 bus_for_each_dev+0xe4/0x168
 bus_iommu_probe+0x8c/0x240
 iommu_device_register+0x120/0x1b0
 mtk_iommu_probe+0x494/0x7a0
 platform_probe+0x94/0x100
 really_probe+0x1e4/0x3e8
 __driver_probe_device+0xc0/0x1a0
 driver_probe_device+0x110/0x1f0
 __device_attach_driver+0xf0/0x1b0
 bus_for_each_drv+0xf0/0x170
 __device_attach+0x120/0x240
 device_initial_probe+0x1c/0x30
 bus_probe_device+0xdc/0xe8
 deferred_probe_work_func+0xf0/0x140
 process_one_work+0x3b0/0x910
 worker_thread+0x33c/0x610
 kthread+0x1dc/0x1f0
 ret_from_fork+0x10/0x20

Keep track of the iommu itself and do not attempt to relock the device
while doing the probe_iommu_group scan.

To accommodate omap's use of unregistered struct iommu_device's add a new
'hwdev' member to keep track of the hwdev in all cases. Normally this
would be dev->parent, but since omap doesn't allocate that struct it won't
exist for it.

Fixes: a2dde836050f ("iommu: Complete the locking for dev->iommu_group")
Reported-by: Chen-Yu Tsai <wenst@chromium.org>
Closes: https://lore.kernel.org/linux-iommu/CAGXv+5E-f9AteAYkmXYzVDZFSA_royc7-bS5LcrzzuHDnXccwA@mail.gmail.com
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommu.c      | 12 ++++++++++--
 drivers/iommu/omap-iommu.c |  1 +
 include/linux/iommu.h      |  2 ++
 3 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 19fdb1a220240f..8842f4975ec4a8 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -264,6 +264,7 @@ int iommu_device_register(struct iommu_device *iommu,
 		return -EBUSY;
 
 	iommu->ops = ops;
+	iommu->hwdev = hwdev;
 	if (hwdev)
 		iommu->fwnode = dev_fwnode(hwdev);
 
@@ -1800,11 +1801,18 @@ struct probe_iommu_args {
 static int probe_iommu_group(struct device *dev, void *data)
 {
 	struct probe_iommu_args *args = data;
+	bool need_lock;
 	int ret;
 
-	device_lock(dev);
+	/* Probing the iommu itself is always done under the device_lock */
+	need_lock = !args->iommu || args->iommu->hwdev != dev;
+
+	if (need_lock)
+		device_lock(dev);
 	ret = __iommu_probe_device(dev, args->group_list);
-	device_unlock(dev);
+	if (need_lock)
+		device_unlock(dev);
+
 	if (ret == -ENODEV)
 		ret = 0;
 
diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index 1e4a90ec64322b..20fcc8ebab6ae3 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1241,6 +1241,7 @@ static int omap_iommu_probe(struct platform_device *pdev)
 		 * an iommu without registering it, so re-run probe again to try
 		 * to match any devices that are waiting for this iommu.
 		 */
+		obj->iommu.hwdev = &pdev->dev;
 		bus_iommu_probe(&platform_bus_type, &obj->iommu);
 	}
 
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index cc47e4086d69ec..96782bfb384462 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -361,6 +361,7 @@ struct iommu_domain_ops {
  * @list: Used by the iommu-core to keep a list of registered iommus
  * @ops: iommu-ops for talking to this iommu
  * @dev: struct device for sysfs handling
+ * @hwdev: The device HW that controls the iommu
  * @singleton_group: Used internally for drivers that have only one group
  * @max_pasids: number of supported PASIDs
  */
@@ -369,6 +370,7 @@ struct iommu_device {
 	const struct iommu_ops *ops;
 	struct fwnode_handle *fwnode;
 	struct device *dev;
+	struct device *hwdev;
 	struct iommu_group *singleton_group;
 	u32 max_pasids;
 };
-- 
2.41.0

RE: [PATCH 3/3] iommu: Do not attempt to re-lock the iommu device when probing

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Wednesday, August 9, 2023 1:27 AM
> 
> @@ -1800,11 +1801,18 @@ struct probe_iommu_args {
>  static int probe_iommu_group(struct device *dev, void *data)
>  {
>  	struct probe_iommu_args *args = data;
> +	bool need_lock;
>  	int ret;
> 
> -	device_lock(dev);
> +	/* Probing the iommu itself is always done under the device_lock */
> +	need_lock = !args->iommu || args->iommu->hwdev != dev;
> +

is !args->iommu a valid case?

btw probably a dumb question. Why do we continue to probe the
iommu device itself instead of skipping it? The group is a concept
for devices which require DMA protection from iommu instead of
for the iommu device itself...

Re: [PATCH 3/3] iommu: Do not attempt to re-lock the iommu device when probing

[Jason Gunthorpe]

On Wed, Aug 09, 2023 at 04:01:42AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Wednesday, August 9, 2023 1:27 AM
> > 
> > @@ -1800,11 +1801,18 @@ struct probe_iommu_args {
> >  static int probe_iommu_group(struct device *dev, void *data)
> >  {
> >  	struct probe_iommu_args *args = data;
> > +	bool need_lock;
> >  	int ret;
> > 
> > -	device_lock(dev);
> > +	/* Probing the iommu itself is always done under the device_lock */
> > +	need_lock = !args->iommu || args->iommu->hwdev != dev;
> > +
> 
> is !args->iommu a valid case?

Hmm, not any more, it used to happen in an earlier version
 
> btw probably a dumb question. Why do we continue to probe the
> iommu device itself instead of skipping it? The group is a concept
> for devices which require DMA protection from iommu instead of
> for the iommu device itself...

Yeah, that is how I originally did it, but since the locking appeared
here I thought it would be safer to just continue to invoke probe as
we have always done. I don't know for sure that there isn't some
driver that relies on this for some reason.

eg it might change the group layouts or something.

Thanks,
Jason

Re: [PATCH 3/3] iommu: Do not attempt to re-lock the iommu device when probing

[Marek Szyprowski]

On 09.08.2023 14:15, Jason Gunthorpe wrote:
> On Wed, Aug 09, 2023 at 04:01:42AM +0000, Tian, Kevin wrote:
>>> From: Jason Gunthorpe <jgg@nvidia.com>
>>> Sent: Wednesday, August 9, 2023 1:27 AM
>>>
>>> @@ -1800,11 +1801,18 @@ struct probe_iommu_args {
>>>   static int probe_iommu_group(struct device *dev, void *data)
>>>   {
>>>   	struct probe_iommu_args *args = data;
>>> +	bool need_lock;
>>>   	int ret;
>>>
>>> -	device_lock(dev);
>>> +	/* Probing the iommu itself is always done under the device_lock */
>>> +	need_lock = !args->iommu || args->iommu->hwdev != dev;
>>> +
>> is !args->iommu a valid case?
> Hmm, not any more, it used to happen in an earlier version
>   
>> btw probably a dumb question. Why do we continue to probe the
>> iommu device itself instead of skipping it? The group is a concept
>> for devices which require DMA protection from iommu instead of
>> for the iommu device itself...
> Yeah, that is how I originally did it, but since the locking appeared
> here I thought it would be safer to just continue to invoke probe as
> we have always done. I don't know for sure that there isn't some
> driver that relies on this for some reason.
>
> eg it might change the group layouts or something.

Well, I don't think that there is any driver doing such things, but the 
only way to check is simply change the behavior an wait for complaints.


Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 3/3] iommu: Do not attempt to re-lock the iommu device when probing

[Jason Gunthorpe]

On Wed, Aug 09, 2023 at 03:38:30PM +0200, Marek Szyprowski wrote:
> On 09.08.2023 14:15, Jason Gunthorpe wrote:
> > On Wed, Aug 09, 2023 at 04:01:42AM +0000, Tian, Kevin wrote:
> >>> From: Jason Gunthorpe <jgg@nvidia.com>
> >>> Sent: Wednesday, August 9, 2023 1:27 AM
> >>>
> >>> @@ -1800,11 +1801,18 @@ struct probe_iommu_args {
> >>>   static int probe_iommu_group(struct device *dev, void *data)
> >>>   {
> >>>   	struct probe_iommu_args *args = data;
> >>> +	bool need_lock;
> >>>   	int ret;
> >>>
> >>> -	device_lock(dev);
> >>> +	/* Probing the iommu itself is always done under the device_lock */
> >>> +	need_lock = !args->iommu || args->iommu->hwdev != dev;
> >>> +
> >> is !args->iommu a valid case?
> > Hmm, not any more, it used to happen in an earlier version
> >   
> >> btw probably a dumb question. Why do we continue to probe the
> >> iommu device itself instead of skipping it? The group is a concept
> >> for devices which require DMA protection from iommu instead of
> >> for the iommu device itself...
> > Yeah, that is how I originally did it, but since the locking appeared
> > here I thought it would be safer to just continue to invoke probe as
> > we have always done. I don't know for sure that there isn't some
> > driver that relies on this for some reason.
> >
> > eg it might change the group layouts or something.
> 
> Well, I don't think that there is any driver doing such things, but the 
> only way to check is simply change the behavior an wait for complaints.

My other concern is that we don't fully block iommu devices from being
self probed. They can still be probed during rescan and during 2nd
calls to bus_iommu_probe().

But OK, it makes more logical sense to just block it so lets try that.

Jason

Re: [PATCH 0/3] Fix device_lock deadlock on two probe() paths

[Mark Brown]

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

On Tue, Aug 08, 2023 at 02:27:04PM -0300, Jason Gunthorpe wrote:
> I missed two paths where __iommu_probe_device() can be called while
> already holding the device_lock() for the device that is to be probed.
> 
> This causes a deadlock because __iommu_probe_device() will attempt to
> re-acquire the lock.
> 
> Organize things so that these two paths can re-use the existing already
> held device_lock by marking the call chains based on if the lock is held
> or not.
> 
> This is an incremental on top of Joerg's next, but it could also be handled by
> respinning the last patch in that series. Please let me know.

The issues this series fixes have been causing quite a bit of breakage
in a range of CI systems (Arm's internal stuff, KernelCI and my personal
CI).  Both the KernelCI bot and my colleague Aishwarya (CCed) bisected
which pointed to this series so I've tested them - I didn't cover every
board but this does fix at least some boots so:

Tested-by: Mark Brown <broonie@kernel.org>

It'd be great to get these fixes into -next, thanks for getting the
patches out so quickly.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/3] Fix device_lock deadlock on two probe() paths

[Jason Gunthorpe]

On Tue, Aug 08, 2023 at 09:18:51PM +0100, Mark Brown wrote:
> On Tue, Aug 08, 2023 at 02:27:04PM -0300, Jason Gunthorpe wrote:
> > I missed two paths where __iommu_probe_device() can be called while
> > already holding the device_lock() for the device that is to be probed.
> > 
> > This causes a deadlock because __iommu_probe_device() will attempt to
> > re-acquire the lock.
> > 
> > Organize things so that these two paths can re-use the existing already
> > held device_lock by marking the call chains based on if the lock is held
> > or not.
> > 
> > This is an incremental on top of Joerg's next, but it could also be handled by
> > respinning the last patch in that series. Please let me know.
> 
> The issues this series fixes have been causing quite a bit of breakage
> in a range of CI systems (Arm's internal stuff, KernelCI and my personal
> CI).  Both the KernelCI bot and my colleague Aishwarya (CCed) bisected
> which pointed to this series so I've tested them - I didn't cover every
> board but this does fix at least some boots so:
> 
> Tested-by: Mark Brown <broonie@kernel.org>
> 
> It'd be great to get these fixes into -next, thanks for getting the
> patches out so quickly.

Yes, sorry about that, this series didn't get picked up by any arm testing
before getting merged..

Thanks,
Jason

Re: [PATCH 0/3] Fix device_lock deadlock on two probe() paths

[Chen-Yu Tsai]

On Wed, Aug 9, 2023 at 1:27 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> I missed two paths where __iommu_probe_device() can be called while
> already holding the device_lock() for the device that is to be probed.
>
> This causes a deadlock because __iommu_probe_device() will attempt to
> re-acquire the lock.
>
> Organize things so that these two paths can re-use the existing already
> held device_lock by marking the call chains based on if the lock is held
> or not.
>
> This is an incremental on top of Joerg's next, but it could also be handled by
> respinning the last patch in that series. Please let me know.
>
> Jason Gunthorpe (3):
>   iommu: Provide iommu_probe_device_locked()
>   iommu: Pass in the iommu_device to probe for in bus_iommu_probe()
>   iommu: Do not attempt to re-lock the iommu device when probing

Tested-by: Chen-Yu Tsai <wenst@chromium.org>

Thank you for coming up with fixes so quickly.