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

[PATCH v2 0/4] 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.

Also the order of iommu_init_device() was not correct for
generic_single_device_group()

v2:
 - New patch to correct the order of setting iommu_dev during
   iommu_init_device()
 - Spelling fixes
 - Simply block probing the iommu device itself instead of trying to do it
v1: https://lore.kernel.org/r/0-v1-8612b9ef48da+333-iommu_group_locking2_jgg@nvidia.com

Jason Gunthorpe (4):
  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
  iommu: dev->iommu->iommu_dev must be set before ops->device_group()

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


base-commit: 8d3740021d5d461e1ec4c17fc5625b9b4237c2f8
-- 
2.41.0

[PATCH v2 1/4] iommu: Provide iommu_probe_device_locked()

[Jason Gunthorpe]

The two callers coming from bus_type -> 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: a16b5d1681ab ("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>
Tested-by: Chen-Yu Tsai <wenst@chromium.org>
Reviewed-by: Kevin Tian <kevin.tian@intel.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

[PATCH v2 2/4] 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 them 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.

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Tested-by: Chen-Yu Tsai <wenst@chromium.org>
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

[PATCH v2 3/4] 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: a16b5d1681ab ("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>
Tested-by: Chen-Yu Tsai <wenst@chromium.org>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommu.c      | 10 ++++++++++
 drivers/iommu/omap-iommu.c |  1 +
 include/linux/iommu.h      |  2 ++
 3 files changed, 13 insertions(+)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 19fdb1a220240f..3ff365c9117850 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);
 
@@ -1802,9 +1803,18 @@ static int probe_iommu_group(struct device *dev, void *data)
 	struct probe_iommu_args *args = data;
 	int ret;
 
+	/*
+	 * The iommu device itself is not probed, in part because no sane iommu
+	 * should self-attach to its own HW, but specifically because we already
+	 * hold the device_lock for the hwdev when calling here.
+	 */
+	if (args->iommu->hwdev == dev)
+		return 0;
+
 	device_lock(dev);
 	ret = __iommu_probe_device(dev, args->group_list);
 	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

[PATCH v2 4/4] iommu: dev->iommu->iommu_dev must be set before ops->device_group()

[Jason Gunthorpe]

As generic_single_device_group() requires it. Otherwise it crashes:

  generic_single_device_group+0x24/0x88
  __iommu_probe_device+0xe8/0x444
  iommu_probe_device+0x1c/0x54
  of_iommu_configure+0x10c/0x200
  of_dma_configure_id+0x1e0/0x3b4
  platform_dma_configure+0x30/0x78
  really_probe+0x70/0x2b4
  __driver_probe_device+0x78/0x12c
  driver_probe_device+0x3c/0x160
  __driver_attach+0x9c/0x1ac
  bus_for_each_dev+0x74/0xd4
  driver_attach+0x24/0x30
  bus_add_driver+0xe4/0x1e8
  driver_register+0x60/0x128
  __platform_driver_register+0x28/0x34
  hantro_driver_init+0x20/0x1000 [hantro_vpu]
  do_one_initcall+0x74/0x2f0
  do_init_module+0x58/0x1ec
  load_module+0x1a20/0x1c64
  init_module_from_file+0x84/0xc0
  idempotent_init_module+0x180/0x250
  __arm64_sys_finit_module+0x64/0xa0
  invoke_syscall+0x48/0x114
  el0_svc_common.constprop.0+0xec/0x10c
  do_el0_svc+0x38/0xa4
  el0_svc+0x40/0xac
  el0t_64_sync_handler+0xc0/0xc4
  el0t_64_sync+0x190/0x194

Fixes: 5dd59857af60 ("iommu: Add generic_single_device_group()")
Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
Closes: https://lore.kernel.org/all/e5e75222-13a9-ee01-0c25-8311b622fe0d@samsung.com/
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 3ff365c9117850..18162049bd2294 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -366,6 +366,7 @@ static int iommu_init_device(struct device *dev, const struct iommu_ops *ops)
 		ret = PTR_ERR(iommu_dev);
 		goto err_module_put;
 	}
+	dev->iommu->iommu_dev = iommu_dev;
 
 	ret = iommu_device_link(iommu_dev, dev);
 	if (ret)
@@ -383,7 +384,6 @@ static int iommu_init_device(struct device *dev, const struct iommu_ops *ops)
 	dev->iommu_group = group;
 	mutex_unlock(&dev_iommu_group_lock);
 
-	dev->iommu->iommu_dev = iommu_dev;
 	dev->iommu->max_pasids = dev_iommu_get_max_pasids(dev);
 	if (ops->is_attach_deferred)
 		dev->iommu->attach_deferred = ops->is_attach_deferred(dev);
@@ -397,6 +397,7 @@ static int iommu_init_device(struct device *dev, const struct iommu_ops *ops)
 err_module_put:
 	module_put(ops->owner);
 err_free:
+	dev->iommu->iommu_dev = NULL;
 	dev_iommu_free(dev);
 	return ret;
 }
-- 
2.41.0

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

[Joerg Roedel]

On Wed, Aug 09, 2023 at 11:43:46AM -0300, Jason Gunthorpe wrote:
> Jason Gunthorpe (4):
>   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
>   iommu: dev->iommu->iommu_dev must be set before ops->device_group()

Applied, thanks for fixing this quickly.

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

[Jason Gunthorpe]

On Wed, Aug 09, 2023 at 05:49:44PM +0200, Joerg Roedel wrote:
> On Wed, Aug 09, 2023 at 11:43:46AM -0300, Jason Gunthorpe wrote:
> > Jason Gunthorpe (4):
> >   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
> >   iommu: dev->iommu->iommu_dev must be set before ops->device_group()
> 
> Applied, thanks for fixing this quickly.

Yes, of course, thanks!

Jason

RE: [PATCH v2 3/4] 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 10:44 PM
> 
> 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: a16b5d1681ab ("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>
> Tested-by: Chen-Yu Tsai <wenst@chromium.org>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

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

RE: [PATCH v2 4/4] iommu: dev->iommu->iommu_dev must be set before ops->device_group()

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Wednesday, August 9, 2023 10:44 PM
> 
> As generic_single_device_group() requires it. Otherwise it crashes:
> 
>   generic_single_device_group+0x24/0x88
>   __iommu_probe_device+0xe8/0x444
>   iommu_probe_device+0x1c/0x54
>   of_iommu_configure+0x10c/0x200
>   of_dma_configure_id+0x1e0/0x3b4
>   platform_dma_configure+0x30/0x78
>   really_probe+0x70/0x2b4
>   __driver_probe_device+0x78/0x12c
>   driver_probe_device+0x3c/0x160
>   __driver_attach+0x9c/0x1ac
>   bus_for_each_dev+0x74/0xd4
>   driver_attach+0x24/0x30
>   bus_add_driver+0xe4/0x1e8
>   driver_register+0x60/0x128
>   __platform_driver_register+0x28/0x34
>   hantro_driver_init+0x20/0x1000 [hantro_vpu]
>   do_one_initcall+0x74/0x2f0
>   do_init_module+0x58/0x1ec
>   load_module+0x1a20/0x1c64
>   init_module_from_file+0x84/0xc0
>   idempotent_init_module+0x180/0x250
>   __arm64_sys_finit_module+0x64/0xa0
>   invoke_syscall+0x48/0x114
>   el0_svc_common.constprop.0+0xec/0x10c
>   do_el0_svc+0x38/0xa4
>   el0_svc+0x40/0xac
>   el0t_64_sync_handler+0xc0/0xc4
>   el0t_64_sync+0x190/0x194
> 
> Fixes: 5dd59857af60 ("iommu: Add generic_single_device_group()")
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Closes: https://lore.kernel.org/all/e5e75222-13a9-ee01-0c25-
> 8311b622fe0d@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 v2 0/4] Fix device_lock deadlock on two probe() paths

[Jeffrey Hugo]

On 8/9/2023 8:43 AM, 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.
> 
> Also the order of iommu_init_device() was not correct for
> generic_single_device_group()
> 
> v2:
>   - New patch to correct the order of setting iommu_dev during
>     iommu_init_device()
>   - Spelling fixes
>   - Simply block probing the iommu device itself instead of trying to do it
> v1: https://lore.kernel.org/r/0-v1-8612b9ef48da+333-iommu_group_locking2_jgg@nvidia.com
> 
> Jason Gunthorpe (4):
>    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
>    iommu: dev->iommu->iommu_dev must be set before ops->device_group()
> 
>   drivers/acpi/scan.c        |  5 +--
>   drivers/iommu/iommu.c      | 65 +++++++++++++++++++++++++++-----------
>   drivers/iommu/of_iommu.c   |  2 +-
>   drivers/iommu/omap-iommu.c | 12 +++++--
>   include/linux/iommu.h      |  6 +++-
>   5 files changed, 65 insertions(+), 25 deletions(-)
> 
> 
> base-commit: 8d3740021d5d461e1ec4c17fc5625b9b4237c2f8

I found that -next broke boot on the Lenovo Miix 630 laptop (Qualcomm 
MSM8998).  Bisected to "iommu: Complete the locking for dev->iommu_group".

I applied this series and the boot hang failure shortly after iommu 
probe goes away.

Tested-by: Jeffrey Hugo <quic_jhugo@quicinc.com>

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

[Marek Szyprowski]

Hi Jason,

On 09.08.2023 16:43, 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.
>
> Also the order of iommu_init_device() was not correct for
> generic_single_device_group()

I've just noticed that there is still one more issue left to fix after 
applying this patchset. On Qualcomm's RB5 board I get the following 
warning (captured on vanilla next-20230817):

------------[ cut here ]------------
WARNING: CPU: 6 PID: 274 at include/linux/device.h:1012 
iommu_probe_device_locked+0xd4/0xe4
Modules linked in: apr pdr_interface venus_dec venus_enc 
videobuf2_dma_contig rpmsg_ctrl fastrpc qrtr_smd rpmsg_char 
lontium_lt9611uxc msm mcp251xfd qcom_camss can_dev videobuf2_dma_sg 
ocmem imx412 gpu_sched venus_core snd_soc_sm8250 phy_qcom_qmp_combo 
videobuf2_memops v4l2_fwnode leds_qcom_lpg v4l2_mem2mem drm_dp_aux_bus 
v4l2_async snd_soc_qcom_sdw videobuf2_v4l2 ax88179_178a onboard_usb_hub 
videodev rtc_pm8xxx qcom_spmi_adc5 qcom_pon qcom_spmi_adc_tm5 
led_class_multicolor qcom_spmi_temp_alarm qcom_vadc_common crct10dif_ce 
i2c_qcom_geni snd_soc_qcom_common qrtr drm_display_helper 
videobuf2_common camcc_sm8250 mc typec i2c_qcom_cci spi_geni_qcom 
qcom_stats llcc_qcom phy_qcom_qmp_usb qcom_rng icc_bwmon qcom_q6v5_pas 
qcrypto phy_qcom_snps_femto_v2 qcom_pil_info sha256_generic 
soundwire_qcom coresight_stm coresight_tmc stm_core coresight_funnel 
coresight_replicator libsha256 qcom_q6v5 pinctrl_sm8250_lpass_lpi 
soundwire_bus snd_soc_lpass_va_macro snd_soc_lpass_macro_common 
pinctrl_lpass_lpi coresight authenc
  lpass_gfm_sm8250 qcom_sysmon slimbus snd_soc_lpass_wsa_macro libdes 
qcom_common qcom_glink_smem socinfo mdt_loader qmi_helpers 
phy_qcom_qmp_pcie icc_osm_l3 qcom_wdt display_connector ip_tables 
x_tables ipv6
CPU: 6 PID: 274 Comm: kworker/u16:8 Not tainted 6.5.0-rc6-next-20230817 
#13867
Hardware name: Qualcomm Technologies, Inc. Robotics RB5 (DT)
Workqueue: events_unbound deferred_probe_work_func
pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : iommu_probe_device_locked+0xd4/0xe4
lr : iommu_probe_device_locked+0x78/0xe4
...
Call trace:
  iommu_probe_device_locked+0xd4/0xe4
  of_iommu_configure+0x10c/0x200
  of_dma_configure_id+0x104/0x3b8
  a6xx_gmu_init+0x4c/0xccc [msm]
  a6xx_gpu_init+0x3ac/0x770 [msm]
  adreno_bind+0x174/0x2ac [msm]
  component_bind_all+0x118/0x24c
  msm_drm_bind+0x1e8/0x6c4 [msm]
  try_to_bring_up_aggregate_device+0x168/0x1d4
  __component_add+0xa8/0x170
  component_add+0x14/0x20
  dsi_dev_attach+0x20/0x2c [msm]
  dsi_host_attach+0x9c/0x144 [msm]
  devm_mipi_dsi_attach+0x34/0xb4
  lt9611uxc_attach_dsi.isra.0+0x84/0xfc [lontium_lt9611uxc]
  lt9611uxc_probe+0x5c8/0x68c [lontium_lt9611uxc]
  i2c_device_probe+0x14c/0x290
  really_probe+0x148/0x2b4
  __driver_probe_device+0x78/0x12c
  driver_probe_device+0x3c/0x160
  __device_attach_driver+0xb8/0x138
  bus_for_each_drv+0x84/0xe0
  __device_attach+0xa8/0x1b0
  device_initial_probe+0x14/0x20
  bus_probe_device+0xb0/0xb4
  deferred_probe_work_func+0x8c/0xc8
  process_one_work+0x1ec/0x53c
  worker_thread+0x298/0x408
  kthread+0x124/0x128
  ret_from_fork+0x10/0x20
irq event stamp: 207712
hardirqs last  enabled at (207711): [<ffffcfe16b46b160>] 
_raw_spin_unlock_irqrestore+0x74/0x78
hardirqs last disabled at (207712): [<ffffcfe16b458630>] el1_dbg+0x24/0x8c
softirqs last  enabled at (206480): [<ffffcfe16a290a10>] 
__do_softirq+0x438/0x4ec
softirqs last disabled at (206473): [<ffffcfe16a296980>] 
____do_softirq+0x10/0x1c
---[ end trace 0000000000000000 ]---
------------[ cut here ]------------

Let me know if you need more information or some tests on the hardware.


>
> v2:
>   - New patch to correct the order of setting iommu_dev during
>     iommu_init_device()
>   - Spelling fixes
>   - Simply block probing the iommu device itself instead of trying to do it
> v1: https://lore.kernel.org/r/0-v1-8612b9ef48da+333-iommu_group_locking2_jgg@nvidia.com
>
> Jason Gunthorpe (4):
>    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
>    iommu: dev->iommu->iommu_dev must be set before ops->device_group()
>
>   drivers/acpi/scan.c        |  5 +--
>   drivers/iommu/iommu.c      | 65 +++++++++++++++++++++++++++-----------
>   drivers/iommu/of_iommu.c   |  2 +-
>   drivers/iommu/omap-iommu.c | 12 +++++--
>   include/linux/iommu.h      |  6 +++-
>   5 files changed, 65 insertions(+), 25 deletions(-)
>
>
> base-commit: 8d3740021d5d461e1ec4c17fc5625b9b4237c2f8

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

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

[Jason Gunthorpe]

On Thu, Aug 17, 2023 at 10:31:31AM +0200, Marek Szyprowski wrote:
> Hi Jason,
> 
> On 09.08.2023 16:43, 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.
> >
> > Also the order of iommu_init_device() was not correct for
> > generic_single_device_group()
> 
> I've just noticed that there is still one more issue left to fix after 
> applying this patchset. On Qualcomm's RB5 board I get the following 
> warning (captured on vanilla next-20230817):

> Call trace:
>   iommu_probe_device_locked+0xd4/0xe4
>   of_iommu_configure+0x10c/0x200
>   of_dma_configure_id+0x104/0x3b8

So it open codes a call to of_dma_configure_id for some reason

This is a bizzaro thing to do, it is taking the OF handle from a dt:

	node = of_parse_phandle(pdev->dev.of_node, "qcom,gmu", 0);

And the sort of co-opts the platform device it is associated with:

	struct platform_device *pdev = of_find_device_by_node(node);
	gmu->dev = &pdev->dev;
	of_dma_configure(gmu->dev, node, true);

And hackily sets up the iommu? It needs to do this because the iommu
doesn't get setup until something probes on the device. But this code
is just stealing the platform device, it doesn't actually probe a
driver.

So this is all kinds of wrong. Attach a driver if you intend to claim
the device and use it :(

Anyhow, you can "fix" it with this:

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index 5deb79924897af..8dbd75c200b91c 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -1556,7 +1556,9 @@ int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct device_node *node)
 
        gmu->dev = &pdev->dev;
 
+       device_lock(gmu->dev);
        of_dma_configure(gmu->dev, node, true);
+       device_unlock(gmu->dev);
 
        /* Fow now, don't do anything fancy until we get our feet under us */
        gmu->idle_level = GMU_IDLE_STATE_ACTIVE;

But there is much more broken here than just that. Maybe leaving the
warning is a good thing so people can fix this properly. (eg remove
the call to of_dma_configure and use the driver core correctly)

Though looking around I see other places open coding of_dma_configure,
so I suppose this will turn into a persisting issue.

Some of these are not going to throw a warning because they are
adjusting the same device the driver is probing for. However those are
busted up since they try to attach the iommu driver to a device after
the driver core has passed iommu_device_use_default_domain(). It will
explode if the driver is removed.

Bascially.. Yikes!

drivers/bcma/main.c:    of_dma_configure(&core->dev, node, false);

  No idea..

drivers/dma/qcom/hidma_mgmt.c:          of_dma_configure(&new_pdev->dev, child, true);

  Registers a platformdevice then does of_dma_configure(), seems
  wrong. of_dma_configure() should be done when a driver is probed.

drivers/gpu/drm/etnaviv/etnaviv_drv.c:          of_dma_configure(&pdev->dev, first_node, true);

  platform_dma_configure() chooses the wrong of_node so this overrides
  the logic?

drivers/gpu/drm/msm/adreno/a6xx_gmu.c:  of_dma_configure(gmu->dev, node, true);
drivers/gpu/drm/msm/adreno/a6xx_gmu.c:  of_dma_configure(gmu->dev, node, true);

  co-opts a platform device without attaching a driver to it

drivers/gpu/drm/sun4i/sun4i_backend.c:          ret = of_dma_configure(drm->dev, dev->of_node, true);
drivers/gpu/drm/sun4i/sun8i_mixer.c:            ret = of_dma_configure(drm->dev, dev->of_node, true);

  No idea.. Same issue with using the wrong of_node?

drivers/gpu/drm/vc4/vc4_drv.c:          ret = of_dma_configure(dev, node, true);
  
  Similar to adreno

drivers/gpu/host1x/bus.c:       of_dma_configure(&device->dev, host1x->dev->of_node, true);
drivers/gpu/host1x/context.c:           err = of_dma_configure_id(&ctx->dev, node, true, &i);
  
  Creating a device and then immediately doing of_dma_configure
  (before registering even).
  
drivers/media/platform/qcom/venus/firmware.c:   ret = of_dma_configure(&pdev->dev, np, true);

  Looks like wrong of_node on the platform_device

drivers/media/platform/samsung/s5p-mfc/s5p_mfc.c:       of_dma_configure(child, dev->of_node, true);

  Says it all:

	/*
	 * The memdevs are not proper OF platform devices, so in order for them
	 * to be treated as valid DMA masters we need a bit of a hack to force
	 * them to inherit the MFC node's DMA configuration.
	 */
	of_dma_configure(child, dev->of_node, true);

drivers/net/wireless/ath/ath10k/snoc.c: ret = of_dma_configure(&pdev->dev, node, true);

  Looks like wrong of_node on the platform_device
					
drivers/net/wireless/ath/ath11k/ahb.c:  ret = of_dma_configure(&pdev->dev, node, true);

  Registering a platform device

sound/soc/bcm/bcm63xx-pcm-whistler.c:   of_dma_configure(pcm->card->dev, pcm->card->dev->of_node, 1);

  No idea

Jason

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

[Joerg Roedel]

On Thu, Aug 17, 2023 at 03:33:16PM -0300, Jason Gunthorpe wrote:
> Bascially.. Yikes!

Hmm, that is a difficult situation. Even if the problem is a misuse of
the APIs we can not just blindly break other drivers by our core
changes.

We need to resolve this situation pretty soon, otherwise I need to
remove the locking rework patches from the IOMMU tree until the
callers are fixed.

Is there a way to keep the broken drivers working for now?

Regards,

-- 
Jörg Rödel
jroedel@suse.de

SUSE Software Solutions Germany GmbH
Frankenstraße 146
90461 Nürnberg
Germany

(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman

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

[Jason Gunthorpe]

On Fri, Aug 18, 2023 at 05:56:20PM +0200, Joerg Roedel wrote:
> On Thu, Aug 17, 2023 at 03:33:16PM -0300, Jason Gunthorpe wrote:
> > Bascially.. Yikes!
> 
> Hmm, that is a difficult situation. Even if the problem is a misuse of
> the APIs we can not just blindly break other drivers by our core
> changes.

They are not broken, they just throw a lockdep warning and keep going
as before. This is what triggers:

static inline void device_lock_assert(struct device *dev)
{
	lockdep_assert_held(&dev->mutex);
}

So non-debug builds won't even see anything.

> We need to resolve this situation pretty soon, otherwise I need to
> remove the locking rework patches from the IOMMU tree until the
> callers are fixed.
> 
> Is there a way to keep the broken drivers working for now?

Historically we've tolerated lockdep warnings as a way to motivate
people who care to fix their stuff properly. eg the Intel VT-D had a
lockdep warning at kernel boot for many releases before it was fixed.

The series doesn't make things any functionally worse for these places
misusing the API, but it now does throw a warning in some cases.

IMHO I'd rather keep the warning rather than supress it by adding
device_locks(). Do you agree?

Jason

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

[Eric Farman]

On Fri, 2023-08-18 at 13:06 -0300, Jason Gunthorpe wrote:
> On Fri, Aug 18, 2023 at 05:56:20PM +0200, Joerg Roedel wrote:
> > On Thu, Aug 17, 2023 at 03:33:16PM -0300, Jason Gunthorpe wrote:
> > > Bascially.. Yikes!
> > 
> > Hmm, that is a difficult situation. Even if the problem is a misuse
> > of
> > the APIs we can not just blindly break other drivers by our core
> > changes.
> 
> They are not broken, they just throw a lockdep warning and keep going
> as before. This is what triggers:
> 
> static inline void device_lock_assert(struct device *dev)
> {
>         lockdep_assert_held(&dev->mutex);
> }
> 
> So non-debug builds won't even see anything.
> 
> > We need to resolve this situation pretty soon, otherwise I need to
> > remove the locking rework patches from the IOMMU tree until the
> > callers are fixed.
> > 
> > Is there a way to keep the broken drivers working for now?
> 
> Historically we've tolerated lockdep warnings as a way to motivate
> people who care to fix their stuff properly. eg the Intel VT-D had a
> lockdep warning at kernel boot for many releases before it was fixed.
> 
> The series doesn't make things any functionally worse for these
> places
> misusing the API, but it now does throw a warning in some cases.

Hi Jason,

Well, I'm trying to chase down an apparent deadlock in the mdev cases
that is introduced with the commit [1] blamed by these fixes. Seems
that when iommu_group_{add|remove}_device gets called out of vfio (for
example, vfio-ap or -ccw), the device lock is already held so
attempting to get it again isn't going to go well.

As near as I see, mdev_device_create calls device_driver_attach, which
acquires the lock and eventually gets us into the vfio code, and then
vfio_device_set_group get us into the iommu side. Not sure how to best
untangle that.

I'm puzzled why lockdep isn't shouting over this for me, so I added a
lockdep_assert_not_held() in those paths to get a proper callchain:

[   49.319508] ------------[ cut here ]------------
[   49.319546] WARNING: CPU: 0 PID: 4472 at drivers/iommu/iommu.c:1216
iommu_group_add_device+0xfc/0x120
[   49.319562] Modules linked in: vhost_vsock
vmw_vsock_virtio_transport_common vsock vhost vhost_iotlb algif_hash
af_alg lcs ctcm fsm kvm xt_MASQUERADE xt_conntrack ipt_REJECT
nf_reject_ipv4 xt_tcpudp iptable_mangle iptable_nat nf_nat nf_conntrack
nf_defrag_ipv6 nf_defrag_ipv4 iptable_filter ip_tables x_tables bridge
stp llc vfio_ccw zcrypt_cex4 eadm_sch mdev vfio_iommu_type1 vfio loop
configfs zram zsmalloc ghash_s390 prng aes_s390 des_s390 libdes qeth_l2
sha512_s390 sha256_s390 sha1_s390 sha_common pkey zcrypt rng_core
dm_multipath dm_mod autofs4
[   49.319721] CPU: 0 PID: 4472 Comm: python Kdump: loaded Not tainted
6.5.0-rc6-next-20230818-dirty #2
[   49.319728] Hardware name: IBM 2964 NE1 749 (LPAR)
[   49.319734] Krnl PSW : 0704f00180000000 0000000190aab690
(iommu_group_add_device+0x100/0x120)
[   49.319748]            R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:3 CC:3
PM:0 RI:0 EA:3
[   49.319758] Krnl GPRS: 0000000000000000 0000000100000000
0000000000000001 00000000ae8924f0
[   49.319765]            00000001bc05a920 000000008e7f8000
0000000000000000 0000000000000001
[   49.319772]            00000000ae892400 00000000ae892400
000000008d23b200 0000000000000000
[   49.319779]            000003ff901d8d18 000003ff90616a08
0000000190aab686 0000038005c8ba50
[   49.319792] Krnl Code: 0000000190aab680:
c0e5001e5624	brasl	%r14,0000000190e762c8
                          0000000190aab686:
ec26ff9f017e	cij	%r2,1,6,0000000190aab5c4
                         #0000000190aab68c:
af000000		mc	0,0
                         >0000000190aab690:
a7f4ff9a		brc	15,0000000190aab5c4
                          0000000190aab694:
b9040028		lgr	%r2,%r8
                          0000000190aab698:
c0e5001eb210	brasl	%r14,0000000190e81ab8
                          0000000190aab69e:
182b		lr	%r2,%r11
                          0000000190aab6a0:
eb6ff0a00004	lmg	%r6,%r15,160(%r15)
[   49.319872] Call Trace:
[   49.319878]  [<0000000190aab690>] iommu_group_add_device+0x100/0x120
[   49.319886] ([<0000000190aab686>] iommu_group_add_device+0xf6/0x120)
[   49.319894]  [<000003ff8002f404>] vfio_device_set_group+0x9c/0x128
[vfio] 
[   49.319909]  [<000003ff8002cf12>]
vfio_register_emulated_iommu_dev+0x6a/0xd0 [vfio] 
[   49.319918]  [<000003ff80083906>] vfio_ccw_mdev_probe+0xce/0x108
[vfio_ccw] 
[   49.319931]  [<0000000190abdbc8>] really_probe+0xd0/0x338 
[   49.319939]  [<0000000190abe3cc>] device_driver_attach+0x5c/0xc8 
[   49.319947]  [<000003ff8003ca30>] mdev_device_create+0x200/0x278
[mdev] 
[   49.319956]  [<000003ff8003ce1c>] create_store+0x8c/0xb8 [mdev] 
[   49.319964]  [<000000019059fba2>] kernfs_fop_write_iter+0x142/0x1d8 
[   49.319975]  [<00000001904c7c76>] vfs_write+0x1de/0x440 
[   49.319986]  [<00000001904c814e>] ksys_write+0x6e/0x100 
[   49.320024]  [<0000000190e75bfe>] __do_syscall+0x1de/0x208 
[   49.320033]  [<0000000190e8c470>] system_call+0x70/0x98 
[   49.320042] 5 locks held by python/4472:
[   49.320048]  #0: 0000000086d86440 (sb_writers#3){.+.+}-{0:0}, at:
ksys_write+0x6e/0x100
[   49.320074]  #1: 000000008d239c90 (&of->mutex){+.+.}-{3:3}, at:
kernfs_fop_write_iter+0x102/0x1d8
[   49.320095]  #2: 000000008ad95428 (kn->active#266){.+.+}-{0:0}, at:
kernfs_fop_write_iter+0x10e/0x1d8
[   49.320119]  #3: 00000000ae888b78 (&parent->unreg_sem){.+.+}-{3:3},
at: mdev_device_create+0x1b0/0x278 [mdev]
[   49.320141]  #4: 00000000ae8924f0 (&dev->mutex){....}-{3:3}, at:
device_driver_attach+0x4e/0xc8
[   49.320162] Last Breaking-Event-Address:
[   49.320168]  [<0000000190f7734c>] __s390_indirect_jump_r14+0x0/0xc
[   49.320179] irq event stamp: 147555
[   49.320185] hardirqs last  enabled at (147563): [<00000001901edcb4>]
console_unlock+0x10c/0x118
[   49.320194] hardirqs last disabled at (147570): [<00000001901edc96>]
console_unlock+0xee/0x118
[   49.320201] softirqs last  enabled at (147304): [<0000000190e8e674>]
__do_softirq+0x47c/0x510
[   49.320209] softirqs last disabled at (147295): [<000000019015732a>]
__irq_exit_rcu+0x11a/0x130
[   49.320218] ---[ end trace 0000000000000000 ]---

Thanks,
Eric

[1] a16b5d1681ab ("iommu: Complete the locking for dev->iommu_group")

> 
> IMHO I'd rather keep the warning rather than supress it by adding
> device_locks(). Do you agree?
> 
> Jason

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

[Jason Gunthorpe]

On Fri, Aug 18, 2023 at 02:00:21PM -0400, Eric Farman wrote:

> Well, I'm trying to chase down an apparent deadlock in the mdev cases
> that is introduced with the commit [1] blamed by these fixes. Seems
> that when iommu_group_{add|remove}_device gets called out of vfio (for
> example, vfio-ap or -ccw), the device lock is already held so
> attempting to get it again isn't going to go well.

Oh, yes. Thankfully due to all the recent cleanup there is now only
one caller of iommu_group_add_device() - VFIO and only on the
vfio_register_emulated_iommu_dev() path.

All those callers are under mdev probe callbacks so they all must have
the device lock held. iommu_group_remove_device is the same. Simple
fix to just assert the device lock is held.

> I'm puzzled why lockdep isn't shouting over this for me, so I added a
> lockdep_assert_not_held() in those paths to get a proper callchain:

The driver core mostly disables lockdep on the device_lock() :(

Does this work for you?

Thanks,
Jason

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 18162049bd2294..1f58bfacb47951 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1166,12 +1166,11 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
 {
 	struct group_device *gdev;
 
-	device_lock(dev);
+	device_lock_assert(dev);
+
 	gdev = iommu_group_alloc_device(group, dev);
-	if (IS_ERR(gdev)) {
-		device_unlock(dev);
+	if (IS_ERR(gdev))
 		return PTR_ERR(gdev);
-	}
 
 	iommu_group_ref_get(group);
 	dev->iommu_group = group;
@@ -1179,7 +1178,6 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
 	mutex_lock(&group->mutex);
 	list_add_tail(&gdev->list, &group->devices);
 	mutex_unlock(&group->mutex);
-	device_unlock(dev);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(iommu_group_add_device);
@@ -1195,14 +1193,13 @@ void iommu_group_remove_device(struct device *dev)
 {
 	struct iommu_group *group;
 
-	device_lock(dev);
+	device_lock_assert(dev);
+
 	group = dev->iommu_group;
 	if (group) {
 		dev_info(dev, "Removing from iommu group %d\n", group->id);
 		__iommu_group_remove_device(dev);
 	}
-	device_unlock(dev);
-
 }
 EXPORT_SYMBOL_GPL(iommu_group_remove_device);
 

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

[Joerg Roedel]

On Fri, Aug 18, 2023 at 01:06:43PM -0300, Jason Gunthorpe wrote:
> On Fri, Aug 18, 2023 at 05:56:20PM +0200, Joerg Roedel wrote:
> > On Thu, Aug 17, 2023 at 03:33:16PM -0300, Jason Gunthorpe wrote:
> > > Bascially.. Yikes!
> > 
> > Hmm, that is a difficult situation. Even if the problem is a misuse of
> > the APIs we can not just blindly break other drivers by our core
> > changes.
> 
> They are not broken, they just throw a lockdep warning and keep going
> as before. This is what triggers:
> 
> static inline void device_lock_assert(struct device *dev)
> {
> 	lockdep_assert_held(&dev->mutex);
> }
> 
> So non-debug builds won't even see anything.

But this still means that a function is called without holding the
proper lock.

> Historically we've tolerated lockdep warnings as a way to motivate
> people who care to fix their stuff properly. eg the Intel VT-D had a
> lockdep warning at kernel boot for many releases before it was fixed.

There is a difference between knowingly introducing new lockdep warnings
into upstream and letting warnings discovered upstream rot for a while.

I can't send anything with known problems upstream.

Regards,

-- 
Jörg Rödel
jroedel@suse.de

SUSE Software Solutions Germany GmbH
Frankenstraße 146
90461 Nürnberg
Germany

(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman

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

[Eric Farman]

On Fri, 2023-08-18 at 15:15 -0300, Jason Gunthorpe wrote:
> On Fri, Aug 18, 2023 at 02:00:21PM -0400, Eric Farman wrote:
> 
> > Well, I'm trying to chase down an apparent deadlock in the mdev
> > cases
> > that is introduced with the commit [1] blamed by these fixes. Seems
> > that when iommu_group_{add|remove}_device gets called out of vfio
> > (for
> > example, vfio-ap or -ccw), the device lock is already held so
> > attempting to get it again isn't going to go well.
> 
> Oh, yes. Thankfully due to all the recent cleanup there is now only
> one caller of iommu_group_add_device() - VFIO and only on the
> vfio_register_emulated_iommu_dev() path.
> 
> All those callers are under mdev probe callbacks so they all must
> have
> the device lock held. iommu_group_remove_device is the same. Simple
> fix to just assert the device lock is held.

Agreed.

> 
> > I'm puzzled why lockdep isn't shouting over this for me, so I added
> > a
> > lockdep_assert_not_held() in those paths to get a proper callchain:
> 
> The driver core mostly disables lockdep on the device_lock() :(

Ah, TIL. Thanks!

> 
> Does this work for you?

Yup, for both -ap and -ccw devices that I have handy. Thanks for the
quick turnaround!

Thanks,
Eric

> 
> Thanks,
> Jason
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 18162049bd2294..1f58bfacb47951 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1166,12 +1166,11 @@ int iommu_group_add_device(struct iommu_group
> *group, struct device *dev)
>  {
>         struct group_device *gdev;
>  
> -       device_lock(dev);
> +       device_lock_assert(dev);
> +
>         gdev = iommu_group_alloc_device(group, dev);
> -       if (IS_ERR(gdev)) {
> -               device_unlock(dev);
> +       if (IS_ERR(gdev))
>                 return PTR_ERR(gdev);
> -       }
>  
>         iommu_group_ref_get(group);
>         dev->iommu_group = group;
> @@ -1179,7 +1178,6 @@ int iommu_group_add_device(struct iommu_group
> *group, struct device *dev)
>         mutex_lock(&group->mutex);
>         list_add_tail(&gdev->list, &group->devices);
>         mutex_unlock(&group->mutex);
> -       device_unlock(dev);
>         return 0;
>  }
>  EXPORT_SYMBOL_GPL(iommu_group_add_device);
> @@ -1195,14 +1193,13 @@ void iommu_group_remove_device(struct device
> *dev)
>  {
>         struct iommu_group *group;
>  
> -       device_lock(dev);
> +       device_lock_assert(dev);
> +
>         group = dev->iommu_group;
>         if (group) {
>                 dev_info(dev, "Removing from iommu group %d\n",
> group->id);
>                 __iommu_group_remove_device(dev);
>         }
> -       device_unlock(dev);
> -
>  }
>  EXPORT_SYMBOL_GPL(iommu_group_remove_device);
>  

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

[Robin Murphy]

On 2023-08-18 19:24, Joerg Roedel wrote:
> On Fri, Aug 18, 2023 at 01:06:43PM -0300, Jason Gunthorpe wrote:
>> On Fri, Aug 18, 2023 at 05:56:20PM +0200, Joerg Roedel wrote:
>>> On Thu, Aug 17, 2023 at 03:33:16PM -0300, Jason Gunthorpe wrote:
>>>> Bascially.. Yikes!
>>>
>>> Hmm, that is a difficult situation. Even if the problem is a misuse of
>>> the APIs we can not just blindly break other drivers by our core
>>> changes.
>>
>> They are not broken, they just throw a lockdep warning and keep going
>> as before. This is what triggers:
>>
>> static inline void device_lock_assert(struct device *dev)
>> {
>> 	lockdep_assert_held(&dev->mutex);
>> }
>>
>> So non-debug builds won't even see anything.
> 
> But this still means that a function is called without holding the
> proper lock.
> 
>> Historically we've tolerated lockdep warnings as a way to motivate
>> people who care to fix their stuff properly. eg the Intel VT-D had a
>> lockdep warning at kernel boot for many releases before it was fixed.
> 
> There is a difference between knowingly introducing new lockdep warnings
> into upstream and letting warnings discovered upstream rot for a while.

Furthermore I'd say it's one thing to have deadlocks or warnings slip in 
as part of some functional change, but quite another when the change is 
solely reworking the locking in an attempt to make it better. This is 
clearly not better.

Thanks,
Robin.

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

[Jason Gunthorpe]

On Fri, Aug 18, 2023 at 08:24:01PM +0200, Joerg Roedel wrote:
> On Fri, Aug 18, 2023 at 01:06:43PM -0300, Jason Gunthorpe wrote:
> > On Fri, Aug 18, 2023 at 05:56:20PM +0200, Joerg Roedel wrote:
> > > On Thu, Aug 17, 2023 at 03:33:16PM -0300, Jason Gunthorpe wrote:
> > > > Bascially.. Yikes!
> > > 
> > > Hmm, that is a difficult situation. Even if the problem is a misuse of
> > > the APIs we can not just blindly break other drivers by our core
> > > changes.
> > 
> > They are not broken, they just throw a lockdep warning and keep going
> > as before. This is what triggers:
> > 
> > static inline void device_lock_assert(struct device *dev)
> > {
> > 	lockdep_assert_held(&dev->mutex);
> > }
> > 
> > So non-debug builds won't even see anything.
> 
> But this still means that a function is called without holding the
> proper lock.

It has alway been like that. of_dma_configure_id() always required the
device_lock to be held!

eg as one example:

 of_iommu_configure
  of_iommu_configure_device
   of_iommu_configure_dev
    of_iommu_xlate
     iommu_fwspec_init
       dev_iommu_get

It is subtle, but the device_lock is what protects the store to
dev->iommu inside dev_iommu_get(). In v6.5-rc1 many callers held the
device lock here, and after this series only these broken drivers
don't.

The driver assumes it has exclusive use of the platform device it
steals. Not just for this call, but in general it does other stuff
that rests on this assumption. Since it is exclusive it doesn't
actually need any locking - this is why it works reliably as is today.

Again, there is no practical bug here, the driver works fine. On
non-debug kernels there is no warning or functional issue. Debug
kernels rightly highlight that the API is being used wrong.

It is wrong use of the APIs because someone could go and use sysfs to
attach, say, VFIO to the stolen platform_device and cause all kinds of
kernel problems.

> I can't send anything with known problems upstream.

In my view this is not creating a new problem. It is exposing existing
problems with a debugging message only on debug kernels.

However, if you view the debug message as a problem then I suggest we
simply comment out with a note the device_lock_assert() from the iommu
code. This would be sad because the vast majority of systems don't use
these badly designed drivers.

This puts it back to the v6.5-rc1 behavior where of_dma_configure_id()
won't make any prints if it is called with wrong locking.

Please let me know.

Jason

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

[Jason Gunthorpe]

On Fri, Aug 18, 2023 at 07:50:18PM +0100, Robin Murphy wrote:

> > There is a difference between knowingly introducing new lockdep warnings
> > into upstream and letting warnings discovered upstream rot for a while.
> 
> Furthermore I'd say it's one thing to have deadlocks or warnings slip in as
> part of some functional change, but quite another when the change is solely
> reworking the locking in an attempt to make it better. This is clearly not
> better.

Exactly what isn't better? We now print a warning when pre-existing
locking rules are violated. That is the only issue here.

Jason

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

[Robin Murphy]

On 2023-08-18 20:19, Jason Gunthorpe wrote:
> On Fri, Aug 18, 2023 at 07:50:18PM +0100, Robin Murphy wrote:
> 
>>> There is a difference between knowingly introducing new lockdep warnings
>>> into upstream and letting warnings discovered upstream rot for a while.
>>
>> Furthermore I'd say it's one thing to have deadlocks or warnings slip in as
>> part of some functional change, but quite another when the change is solely
>> reworking the locking in an attempt to make it better. This is clearly not
>> better.
> 
> Exactly what isn't better? We now print a warning when pre-existing
> locking rules are violated. That is the only issue here.

What locking rules? 9 years ago when of_iommu_configure() was introduced 
(97890ba9289c), of_dma_configure() ran at device creation time, and that 
remains the case in some places, so cannot necessarily be called wrong. 
Subsequently making it run off driver probe in the general case was a 
bit of a hack, but at the time probe deferral was the only tool we had 
available to easily enforce the ordering of IOMMU drivers relative to 
client drivers. In hindsight there are subtleties which weren't really 
apparent at the time, but what *was* clear was that the benefit of 
letting IOMMU drivers be normal drivers without any special 
OF_IOMMU_DECLARE£() weirdness was significant, and thus today's mess was 
unwittingly born.

The existing locking rules are that there aren't any locking rules 
(other than the probe_device_lock bodge), which obviously could be 
better, but does rather disprove the assertion that they could be 
violated. A robust locking scheme would be great, but designing one 
based on something which already violates *other* IOMMU API assumptions, 
such that a) it can't ever really work, and b) it makes those underlying 
issues even harder to fix, is not productive.

Thanks,
Robin.