[PATCH RFC] iommu: fix wrong DMA ops for iommu device

Charan Teja Kalla
2024-12-13T15:04:57Z

Race between smmu device probe and iommu device probe is resulting into
wrong DMA ops used for the device.

bus_iommu_probe(P1) of_iommu_configure(P2)
(from iommu_device_register) (from insmod of a driver)
-------------------------- --------------------------
1) probe_iommu_group()
(Fills just dev->iommu_group
under iommu_probe_device_lock)
2) iommu_probe_device():
(acquires the iommu_probe_device_lock,
but since dev->iommu_group is already
set, it just returns without
allocating the domain.)

3) of_dma_configure_id()->
arch_setup_dma_ops() gets called for
this device, but returns with the
error message:
"Failed to set up IOMMU for device;
retaining platform DMA ops"

4) device probe is succeeded where it
can now setup iommu mappings using
wrong ->dma_ops

5) domain will be allocated later
and fills iommu_group->domain.

Step 3) and 4) denies iommu device from using 'iommu_dma_ops'.

This issue does exists on 6.6 because of commit f188056352bc ("iommu:
Avoid locking/unlocking for iommu_probe_device()") without which 2)
returns only after filling the domain under group->mutex, thus no issue.

With commit b5c58b2fdc42("dma-mapping: direct calls for dma-iommu"),
->iommu_dma_ops is removed altogether and iommu api are directly being
called under "use_dma_iommu(): return dev->dma_iommu". Again,
->dma_iommu flag is set only after domain allocation. So, If
there is a sufficient delay between steps 4) and 5), issue is still
exists but that seems very narrow to me.

I am thinking, setup of the domain before returning from step2) is the
way to fix this issue. Can you please help if you have any suggestions?

Signed-off-by: Charan Teja Kalla <quic_charante@quicinc.com>
---
drivers/iommu/iommu.c | 26 +++++++++++++++++++++++++-
1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 3f1029c0825e..61fc8145f378 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -561,19 +561,43 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
int iommu_probe_device(struct device *dev)
{
const struct iommu_ops *ops;
+ struct iommu_group *group;
int ret;

mutex_lock(&iommu_probe_device_lock);
ret = __iommu_probe_device(dev, NULL);
mutex_unlock(&iommu_probe_device_lock);
if (ret)
- return ret;
+ goto err_out;
+
+ group = iommu_group_get(dev);
+ if (!group) {
+ ret = -ENODEV;
+ goto err_release;
+ }
+
+ mutex_lock(&group->mutex);
+ if (!group->default_domain) {
+ ret = iommu_setup_default_domain(group, 0);
+ if (ret)
+ goto err_unlock;
+ }
+ mutex_unlock(&group->mutex);
+ iommu_group_put(group);

ops = dev_iommu_ops(dev);
if (ops->probe_finalize)
ops->probe_finalize(dev);

return 0;
+
+err_unlock:
+ mutex_unlock(&group->mutex);
+ iommu_group_put(group);
+err_release:
+ iommu_release_device(dev);
+err_out:
+ return ret;
}

static void __iommu_group_free_device(struct iommu_group *group,
--
2.34.1