Re: [PATCH] iommu: Remove the device_lock_assert() from __iommu_probe_device()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2023-08-18 22:32, Jason Gunthorpe wrote:
It turns out several drivers are calling of_dma_configure() outside the
expected bus_type.dma_configure op. This ends up being mis-locked and
triggers a lockdep assertion, or instance:

   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

It is subtle and was never documented or enforced, but there has always
been an assumption that of_dma_configure_id() is not concurrent. It makes
several calls into the iommu layer that require this, including
dev_iommu_get(). The majority of cases have been preventing concurrency
using the device_lock().

Thus the new lock debugging added exposes an existing problem in
drivers. On inspection this looks like a theoretical locking problem as
generally the cases are already assuming they are the exclusive (single
threaded) user of the target device.

Sorry to be blunt, but the only problem is that you've introduced an idealistic new locking scheme which failed to take into account how things currently actually work, and is broken and achieving nothing but causing problems.

The solution is to drop those locking patches entirely and rethink the whole thing. When their sole purpose was to improve the locking and make it easier to reason about, and now the latest "fix" is now to remove one of the assertions which forms the fundamental basis for that reasoning, then the point has clearly been lost. All we've done is churned a dodgy and incomplete locking scheme into a *different* dodgy and incomplete locking scheme. I do not think that continuing to dig in deeper is the way out of the hole...

It's now rc7, and I have little confidence that aren't still more latent problems which just haven't been hit yet (e.g. acpi_dma_configure() is also called in different contexts relative to the device lock, which is absolutely by design and not broken).

And on the subject of idealism, the fact is that doing IOMMU configuration based on driver probe via bus->dma_configure is *fundamentally wrong* and breaking a bunch of other IOMMU API assumptions, so it is not a robust foundation to build anything upon in the first place. The problem it causes with broken groups has been known about for several years now, however it's needed a lot of work to get to the point of being able to fix it properly (FWIW that is now #2 on my priority list after getting the bus ops stuff done, which should also make it easier).

Thanks,
Robin.

Sadly, there are deeper technical problems with all of the places doing
this. There are several problemetic patterns:

1) Probe a driver on device A and then steal device B and use it as part
    of the driver operation.

    Since no driver was probed to device B it means we never called
    bus_type.dma_configure and thus the drivers hackily try to open code
    this.

    Unfortunately nothing prevents another driver from binding to device B
    and creating total chaos. eg vfio bind triggered by userspace

2) Probe a driver on device A and then create a new platform driver B for a
    fwnode that doesn't have one, then do #1

    This has the same essential problem as #1, the new device is never
    probed so the hack call to of_dma_configure() is needed to setup DMA,
    and we are at risk of something else trying to use the device.

3) Probe a driver on device A but the of_node was incorrect for DMA so fix
    it by figuring out the right node and calling of_dma_configure()

    This will blow up in the iommu code if the driver is unprobed because
    the bus_type now assumes that dma_configure and dma_cleanup are
    strictly paired. Since dma_configure will have done the wrong thing due
    to the missing of_node, dma_cleanup will be unpaired and
    iommu_device_unuse_default_domain() will blow up.

    Further the driver operating on device A will not be protected against
    changes to the iommu domain since it never called
    iommu_device_use_default_domain()

    At least this case will not throw a lockdep warning as probe
    is done under the device_lock already.

While it is tempting to just add a device_lock() around the few missing
places, this doesn't solve the above underlying problems with those
callers.

For this reason remove the locking assertion in the iommu code so that
theses driver problems continue to remain hidden.

Add a comment to all the problematic call sites to encourage fixing and
discourage future cut & paste expanding the problem.

Add documentation to of_dma_configure_id() explaining the locking.

Fixes: b19ca5922dff ("iommu: Do not attempt to re-lock the iommu device when probing")
Reported-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
Closes: https://lore.kernel.org/all/d090f196-a5c2-b188-31bf-e42553d8d251@xxxxxxxxxxx/
Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
---
  drivers/bcma/main.c                              |  4 ++++
  drivers/dma/qcom/hidma_mgmt.c                    |  4 ++++
  drivers/gpu/drm/etnaviv/etnaviv_drv.c            |  4 ++++
  drivers/gpu/drm/msm/adreno/a6xx_gmu.c            |  8 ++++++++
  drivers/gpu/drm/sun4i/sun4i_backend.c            |  4 ++++
  drivers/gpu/drm/sun4i/sun8i_mixer.c              |  4 ++++
  drivers/gpu/drm/vc4/vc4_drv.c                    |  4 ++++
  drivers/gpu/host1x/bus.c                         |  4 ++++
  drivers/iommu/iommu.c                            | 12 ++++++++----
  drivers/media/platform/qcom/venus/firmware.c     |  4 ++++
  drivers/media/platform/samsung/s5p-mfc/s5p_mfc.c |  4 ++++
  drivers/net/wireless/ath/ath10k/snoc.c           |  4 ++++
  drivers/net/wireless/ath/ath11k/ahb.c            |  4 ++++
  drivers/of/device.c                              |  3 +++
  sound/soc/bcm/bcm63xx-pcm-whistler.c             |  4 ++++
  15 files changed, 67 insertions(+), 4 deletions(-)

This is my suggestion to the issue, I don't like removing the locking
assertion but if we can't have debug kernels reporting existing bugs then this
would be the next best option.

I've CC'd all the driver owners, if people have better explanations for what
their drivers are doing please share :)

diff --git a/drivers/bcma/main.c b/drivers/bcma/main.c
index 7061d3ee836a15..4df97000d7e0a7 100644
--- a/drivers/bcma/main.c
+++ b/drivers/bcma/main.c
@@ -208,6 +208,10 @@ static void bcma_of_fill_device(struct device *parent,
core->irq = bcma_of_get_irq(parent, core, 0); + /*
+	 * FIXME: of_dma_configure() should not be called outside
+	 * bus_type.dma_configure
+	 */
  	of_dma_configure(&core->dev, node, false);
  }
diff --git a/drivers/dma/qcom/hidma_mgmt.c b/drivers/dma/qcom/hidma_mgmt.c
index 05e96b31d87169..2efd97747d37b3 100644
--- a/drivers/dma/qcom/hidma_mgmt.c
+++ b/drivers/dma/qcom/hidma_mgmt.c
@@ -390,6 +390,10 @@ static int __init hidma_mgmt_of_populate_channels(struct device_node *np)
  			goto out;
  		}
  		new_pdev->dev.of_node = child;
+		/*
+		 * FIXME: of_dma_configure() should not be called outside
+		 * bus_type.dma_configure
+		 */
  		of_dma_configure(&new_pdev->dev, child, true);
  		/*
  		 * It is assumed that calling of_msi_configure is safe on
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
index 31a7f59ccb49ee..22729c66e39a65 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
@@ -635,6 +635,10 @@ static int etnaviv_pdev_probe(struct platform_device *pdev)
  	 * device as the GPU we found. This assumes that all Vivante
  	 * GPUs in the system share the same DMA constraints.
  	 */
+	/*
+	 * FIXME: of_dma_configure() should not be called outside
+	 * bus_type.dma_configure
+	 */
  	if (first_node)
  		of_dma_configure(&pdev->dev, first_node, true);
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index 5deb79924897af..509bd8b7ec12ae 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -1492,6 +1492,10 @@ int a6xx_gmu_wrapper_init(struct a6xx_gpu *a6xx_gpu, struct device_node *node)
gmu->dev = &pdev->dev; + /*
+	 * FIXME: of_dma_configure() should not be called outside
+	 * bus_type.dma_configure
+	 */
  	of_dma_configure(gmu->dev, node, true);
pm_runtime_enable(gmu->dev);
@@ -1556,6 +1560,10 @@ int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct device_node *node)
gmu->dev = &pdev->dev; + /*
+	 * FIXME: of_dma_configure() should not be called outside
+	 * bus_type.dma_configure
+	 */
  	of_dma_configure(gmu->dev, node, true);
/* Fow now, don't do anything fancy until we get our feet under us */
diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
index b11dbd50d73e98..30cbefb6799e51 100644
--- a/drivers/gpu/drm/sun4i/sun4i_backend.c
+++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
@@ -800,6 +800,10 @@ static int sun4i_backend_bind(struct device *dev, struct device *master,
  		 * for us, and DRM doesn't do per-device allocation either, so
  		 * we would need to fix DRM first...
  		 */
+		/*
+		 * FIXME: of_dma_configure() should not be called outside
+		 * bus_type.dma_configure
+		 */
  		ret = of_dma_configure(drm->dev, dev->of_node, true);
  		if (ret)
  			return ret;
diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c
index 11d5244a5aa5fc..e26bea207599d3 100644
--- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
@@ -399,6 +399,10 @@ static int sun8i_mixer_bind(struct device *dev, struct device *master,
  		 * DRM doesn't do per-device allocation either, so we
  		 * would need to fix DRM first...
  		 */
+		/*
+		 * FIXME: of_dma_configure() should not be called outside
+		 * bus_type.dma_configure
+		 */
  		ret = of_dma_configure(drm->dev, dev->of_node, true);
  		if (ret)
  			return ret;
diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
index 823395c23cc308..82496d8fb50f84 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.c
+++ b/drivers/gpu/drm/vc4/vc4_drv.c
@@ -305,6 +305,10 @@ static int vc4_drm_bind(struct device *dev)
  	node = of_find_matching_node_and_match(NULL, vc4_dma_range_matches,
  					       NULL);
  	if (node) {
+		/*
+		 * FIXME: of_dma_configure() should not be called outside
+		 * bus_type.dma_configure
+		 */
  		ret = of_dma_configure(dev, node, true);
  		of_node_put(node);
diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c
index 4d16a3396c4abf..5c7ef738a7d7d7 100644
--- a/drivers/gpu/host1x/bus.c
+++ b/drivers/gpu/host1x/bus.c
@@ -475,6 +475,10 @@ static int host1x_device_add(struct host1x *host1x,
  	device->dev.bus = &host1x_bus_type;
  	device->dev.parent = host1x->dev;
+ /*
+	 * FIXME: of_dma_configure() should not be called outside
+	 * bus_type.dma_configure
+	 */
  	of_dma_configure(&device->dev, host1x->dev->of_node, true);
device->dev.dma_parms = &device->dma_parms;
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 18162049bd2294..eb623a611f571d 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -458,8 +458,15 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
  	 * 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.
+	 *
+	 * We cannot have the assertion yet because of a number of drivers
+	 * improperly calling of_dma_configure() and expecting to attach
+	 * the iommu outside the bus_type.dma_configure callback. This messes
+	 * up our ownership tracking, does not exclude other drivers from
+	 * binding an operating the device, and is technically mislocked.
+	 *
+	 * device_lock_assert(dev);
  	 */
-	device_lock_assert(dev);
if (!ops)
  		return -ENODEV;
@@ -527,8 +534,6 @@ 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;
@@ -1202,7 +1207,6 @@ void iommu_group_remove_device(struct device *dev)
  		__iommu_group_remove_device(dev);
  	}
  	device_unlock(dev);
-
  }
  EXPORT_SYMBOL_GPL(iommu_group_remove_device);
diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
index cfb11c551167ce..f70360c5534d64 100644
--- a/drivers/media/platform/qcom/venus/firmware.c
+++ b/drivers/media/platform/qcom/venus/firmware.c
@@ -296,6 +296,10 @@ int venus_firmware_init(struct venus_core *core)
pdev->dev.of_node = np; + /*
+	 * FIXME: of_dma_configure() should not be called outside
+	 * bus_type.dma_configure
+	 */
  	ret = of_dma_configure(&pdev->dev, np, true);
  	if (ret) {
  		dev_err(core->dev, "dma configure fail\n");
diff --git a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc.c b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc.c
index e30e54935d79a9..40fa7b62545d7a 100644
--- a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc.c
+++ b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc.c
@@ -1114,6 +1114,10 @@ static struct device *s5p_mfc_alloc_memdev(struct device *dev,
  	 * 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.
  	 */
+	/*
+	 * FIXME: of_dma_configure() should not be called outside
+	 * bus_type.dma_configure
+	 */
  	of_dma_configure(child, dev->of_node, true);
if (device_add(child) == 0) {
diff --git a/drivers/net/wireless/ath/ath10k/snoc.c b/drivers/net/wireless/ath/ath10k/snoc.c
index 26214c00cd0d7b..977e5c6186b131 100644
--- a/drivers/net/wireless/ath/ath10k/snoc.c
+++ b/drivers/net/wireless/ath/ath10k/snoc.c
@@ -1614,6 +1614,10 @@ static int ath10k_fw_init(struct ath10k *ar)
pdev->dev.of_node = node; + /*
+	 * FIXME: of_dma_configure() should not be called outside
+	 * bus_type.dma_configure
+	 */
  	ret = of_dma_configure(&pdev->dev, node, true);
  	if (ret) {
  		ath10k_err(ar, "dma configure fail: %d\n", ret);
diff --git a/drivers/net/wireless/ath/ath11k/ahb.c b/drivers/net/wireless/ath/ath11k/ahb.c
index 139da578831a44..a4d2a89ab96de0 100644
--- a/drivers/net/wireless/ath/ath11k/ahb.c
+++ b/drivers/net/wireless/ath/ath11k/ahb.c
@@ -1012,6 +1012,10 @@ static int ath11k_ahb_fw_resources_init(struct ath11k_base *ab)
  		return PTR_ERR(pdev);
  	}
+ /*
+	 * FIXME: of_dma_configure() should not be called outside
+	 * bus_type.dma_configure
+	 */
  	ret = of_dma_configure(&pdev->dev, node, true);
  	if (ret) {
  		ath11k_err(ab, "dma configure fail: %d\n", ret);
diff --git a/drivers/of/device.c b/drivers/of/device.c
index 0f00f1b8070829..bc6f62ebcbd040 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -108,6 +108,9 @@ of_dma_set_restricted_buffer(struct device *dev, struct device_node *np)
   * If platform code needs to use its own special DMA configuration, it
   * can use a platform bus notifier and handle BUS_NOTIFY_ADD_DEVICE events
   * to fix up DMA configuration.
+ *
+ * This function should only be called from the bus_type.dma_configure callback
+ * and the driver_lock(dev) must be held.
   */
  int of_dma_configure_id(struct device *dev, struct device_node *np,
  			bool force_dma, const u32 *id)
diff --git a/sound/soc/bcm/bcm63xx-pcm-whistler.c b/sound/soc/bcm/bcm63xx-pcm-whistler.c
index 2c600b017524f3..d8e0520ca61155 100644
--- a/sound/soc/bcm/bcm63xx-pcm-whistler.c
+++ b/sound/soc/bcm/bcm63xx-pcm-whistler.c
@@ -354,6 +354,10 @@ static int bcm63xx_soc_pcm_new(struct snd_soc_component *component,
i2s_priv = dev_get_drvdata(asoc_rtd_to_cpu(rtd, 0)->dev); + /*
+	 * FIXME: of_dma_configure() should not be called outside
+	 * bus_type.dma_configure
+	 */
  	of_dma_configure(pcm->card->dev, pcm->card->dev->of_node, 1);
ret = dma_coerce_mask_and_coherent(pcm->card->dev, DMA_BIT_MASK(32));

base-commit: 1bba4f1c0b4f2429e16da231a246ab4b99cc633c



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux