[PATCH 00/14] iommu: fix device leaks

[PATCH 00/14] iommu: fix device leaks

[Johan Hovold]

This series fixes device leaks in the iommu drivers, which pretty
consistently failed to drop the reference taken by
of_find_device_by_node() when looking up iommu platform devices.

Included are also a couple of related cleanups.

Johan


Johan Hovold (14):
  iommu/apple-dart: fix device leak on of_xlate()
  iommu/qcom: fix device leak on of_xlate()
  iommu/exynos: fix device leak on of_xlate()
  iommu/ipmmu-vmsa: fix device leak on of_xlate()
  iommu/mediatek: fix device leak on of_xlate()
  iommu/mediatek: fix device leaks on probe()
  iommu/mediatek: simplify dt parsing error handling
  iommu/mediatek-v1: fix device leak on probe_device()
  iommu/mediatek-v1: fix device leaks on probe()
  iommu/mediatek-v1: add missing larb count sanity check
  iommu/omap: fix device leaks on probe_device()
  iommu/omap: simplify probe_device() error handling
  iommu/sun50i: fix device leak on of_xlate()
  iommu/tegra: fix device leak on probe_device()

 drivers/iommu/apple-dart.c              |  2 ++
 drivers/iommu/arm/arm-smmu/qcom_iommu.c | 10 +++-----
 drivers/iommu/exynos-iommu.c            |  9 +++----
 drivers/iommu/ipmmu-vmsa.c              |  2 ++
 drivers/iommu/mtk_iommu.c               | 33 +++++++++++++++++--------
 drivers/iommu/mtk_iommu_v1.c            | 28 +++++++++++++++++----
 drivers/iommu/omap-iommu.c              | 32 +++++++++++++++---------
 drivers/iommu/sun50i-iommu.c            |  2 ++
 drivers/iommu/tegra-smmu.c              |  5 ++--
 9 files changed, 81 insertions(+), 42 deletions(-)

-- 
2.49.1

[PATCH 01/14] iommu/apple-dart: fix device leak on of_xlate()

[Johan Hovold]

Make sure to drop the reference taken to the iommu platform device when
looking up its driver data during of_xlate().

Fixes: 46d1fb072e76 ("iommu/dart: Add DART iommu driver")
Cc: stable@vger.kernel.org	# 5.15
Cc: Sven Peter <sven@kernel.org>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/iommu/apple-dart.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/iommu/apple-dart.c b/drivers/iommu/apple-dart.c
index 190f28d76615..1aa7c10262a8 100644
--- a/drivers/iommu/apple-dart.c
+++ b/drivers/iommu/apple-dart.c
@@ -790,6 +790,8 @@ static int apple_dart_of_xlate(struct device *dev,
 	struct apple_dart *cfg_dart;
 	int i, sid;
 
+	put_device(&iommu_pdev->dev);
+
 	if (args->args_count != 1)
 		return -EINVAL;
 	sid = args->args[0];
-- 
2.49.1

[PATCH 02/14] iommu/qcom: fix device leak on of_xlate()

[Johan Hovold]

Make sure to drop the reference taken to the iommu platform device when
looking up its driver data during of_xlate().

Note that commit e2eae09939a8 ("iommu/qcom: add missing put_device()
call in qcom_iommu_of_xlate()") fixed the leak in a couple of error
paths, but the reference is still leaking on success and late failures.

Fixes: 0ae349a0f33f ("iommu/qcom: Add qcom_iommu")
Cc: stable@vger.kernel.org	# 4.14: e2eae09939a8
Cc: Rob Clark <robin.clark@oss.qualcomm.com>
Cc: Yu Kuai <yukuai3@huawei.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/iommu/arm/arm-smmu/qcom_iommu.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
index c5be95e56031..9c1166a3af6c 100644
--- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
+++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
@@ -565,14 +565,14 @@ static int qcom_iommu_of_xlate(struct device *dev,
 
 	qcom_iommu = platform_get_drvdata(iommu_pdev);
 
+	put_device(&iommu_pdev->dev);
+
 	/* make sure the asid specified in dt is valid, so we don't have
 	 * to sanity check this elsewhere:
 	 */
 	if (WARN_ON(asid > qcom_iommu->max_asid) ||
-	    WARN_ON(qcom_iommu->ctxs[asid] == NULL)) {
-		put_device(&iommu_pdev->dev);
+	    WARN_ON(qcom_iommu->ctxs[asid] == NULL))
 		return -EINVAL;
-	}
 
 	if (!dev_iommu_priv_get(dev)) {
 		dev_iommu_priv_set(dev, qcom_iommu);
@@ -581,10 +581,8 @@ static int qcom_iommu_of_xlate(struct device *dev,
 		 * multiple different iommu devices.  Multiple context
 		 * banks are ok, but multiple devices are not:
 		 */
-		if (WARN_ON(qcom_iommu != dev_iommu_priv_get(dev))) {
-			put_device(&iommu_pdev->dev);
+		if (WARN_ON(qcom_iommu != dev_iommu_priv_get(dev)))
 			return -EINVAL;
-		}
 	}
 
 	return iommu_fwspec_add_ids(dev, &asid, 1);
-- 
2.49.1

[PATCH 03/14] iommu/exynos: fix device leak on of_xlate()

[Johan Hovold]

Make sure to drop the reference taken to the iommu platform device when
looking up its driver data during of_xlate().

Fixes: aa759fd376fb ("iommu/exynos: Add callback for initializing devices from device tree")
Cc: stable@vger.kernel.org	# 4.2
Cc: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/iommu/exynos-iommu.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index b6edd178fe25..ce9e935cb84c 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -1446,17 +1446,14 @@ static int exynos_iommu_of_xlate(struct device *dev,
 		return -ENODEV;
 
 	data = platform_get_drvdata(sysmmu);
-	if (!data) {
-		put_device(&sysmmu->dev);
+	put_device(&sysmmu->dev);
+	if (!data)
 		return -ENODEV;
-	}
 
 	if (!owner) {
 		owner = kzalloc(sizeof(*owner), GFP_KERNEL);
-		if (!owner) {
-			put_device(&sysmmu->dev);
+		if (!owner)
 			return -ENOMEM;
-		}
 
 		INIT_LIST_HEAD(&owner->controllers);
 		mutex_init(&owner->rpm_lock);
-- 
2.49.1

[PATCH 04/14] iommu/ipmmu-vmsa: fix device leak on of_xlate()

[Johan Hovold]

Make sure to drop the reference taken to the iommu platform device when
looking up its driver data during of_xlate().

Fixes: 7b2d59611fef ("iommu/ipmmu-vmsa: Replace local utlb code with fwspec ids")
Cc: stable@vger.kernel.org	# 4.14
Cc: Magnus Damm <damm+renesas@opensource.se>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/iommu/ipmmu-vmsa.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index ffa892f65714..02a2a55ffa0a 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -720,6 +720,8 @@ static int ipmmu_init_platform_device(struct device *dev,
 
 	dev_iommu_priv_set(dev, platform_get_drvdata(ipmmu_pdev));
 
+	put_device(&ipmmu_pdev->dev);
+
 	return 0;
 }
 
-- 
2.49.1

[PATCH 05/14] iommu/mediatek: fix device leak on of_xlate()

[Johan Hovold]

Make sure to drop the reference taken to the iommu platform device when
looking up its driver data during of_xlate().

Fixes: 0df4fabe208d ("iommu/mediatek: Add mt8173 IOMMU driver")
Cc: stable@vger.kernel.org	# 4.6
Cc: Yong Wu <yong.wu@mediatek.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/iommu/mtk_iommu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 0e0285348d2b..8d8e85186188 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -974,6 +974,8 @@ static int mtk_iommu_of_xlate(struct device *dev,
 			return -EINVAL;
 
 		dev_iommu_priv_set(dev, platform_get_drvdata(m4updev));
+
+		put_device(&m4updev->dev);
 	}
 
 	return iommu_fwspec_add_ids(dev, args->args, 1);
-- 
2.49.1

[PATCH 06/14] iommu/mediatek: fix device leaks on probe()

[Johan Hovold]

Make sure to drop the references taken to the larb devices during
probe on probe failure (e.g. probe deferral) and on driver unbind.

Note that commit 26593928564c ("iommu/mediatek: Add error path for loop
of mm_dts_parse") fixed the leaks in a couple of error paths, but the
references are still leaking on success and late failures.

Fixes: 0df4fabe208d ("iommu/mediatek: Add mt8173 IOMMU driver")
Cc: stable@vger.kernel.org	# 4.6
Cc: Yong Wu <yong.wu@mediatek.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/iommu/mtk_iommu.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 8d8e85186188..20a5ba80f983 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -1216,13 +1216,17 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m
 		platform_device_put(plarbdev);
 	}
 
-	if (!frst_avail_smicomm_node)
-		return -EINVAL;
+	if (!frst_avail_smicomm_node) {
+		ret = -EINVAL;
+		goto err_larbdev_put;
+	}
 
 	pcommdev = of_find_device_by_node(frst_avail_smicomm_node);
 	of_node_put(frst_avail_smicomm_node);
-	if (!pcommdev)
-		return -ENODEV;
+	if (!pcommdev) {
+		ret = -ENODEV;
+		goto err_larbdev_put;
+	}
 	data->smicomm_dev = &pcommdev->dev;
 
 	link = device_link_add(data->smicomm_dev, dev,
@@ -1230,7 +1234,8 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m
 	platform_device_put(pcommdev);
 	if (!link) {
 		dev_err(dev, "Unable to link %s.\n", dev_name(data->smicomm_dev));
-		return -EINVAL;
+		ret = -EINVAL;
+		goto err_larbdev_put;
 	}
 	return 0;
 
@@ -1402,8 +1407,12 @@ static int mtk_iommu_probe(struct platform_device *pdev)
 	iommu_device_sysfs_remove(&data->iommu);
 out_list_del:
 	list_del(&data->list);
-	if (MTK_IOMMU_IS_TYPE(data->plat_data, MTK_IOMMU_TYPE_MM))
+	if (MTK_IOMMU_IS_TYPE(data->plat_data, MTK_IOMMU_TYPE_MM)) {
 		device_link_remove(data->smicomm_dev, dev);
+
+		for (i = 0; i < MTK_LARB_NR_MAX; i++)
+			put_device(data->larb_imu[i].dev);
+	}
 out_runtime_disable:
 	pm_runtime_disable(dev);
 	return ret;
@@ -1423,6 +1432,9 @@ static void mtk_iommu_remove(struct platform_device *pdev)
 	if (MTK_IOMMU_IS_TYPE(data->plat_data, MTK_IOMMU_TYPE_MM)) {
 		device_link_remove(data->smicomm_dev, &pdev->dev);
 		component_master_del(&pdev->dev, &mtk_iommu_com_ops);
+
+		for (i = 0; i < MTK_LARB_NR_MAX; i++)
+			put_device(data->larb_imu[i].dev);
 	}
 	pm_runtime_disable(&pdev->dev);
 	for (i = 0; i < data->plat_data->banks_num; i++) {
-- 
2.49.1

[PATCH 07/14] iommu/mediatek: simplify dt parsing error handling

[Johan Hovold]

As previously documented by commit 26593928564c ("iommu/mediatek: Add
error path for loop of mm_dts_parse"), the id mapping may not be linear
so the whole larb array needs to be iterated on devicetree parsing
errors.

Simplify the loop by iterating from index zero while dropping the
redundant NULL check for consistency with later cleanups.

Also add back the comment which was removed by commit 462e768b55a2
("iommu/mediatek: Fix forever loop in error handling") to prevent anyone
from trying to optimise the loop by iterating backwards from 'i'.

Cc: Yong Wu <yong.wu@mediatek.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/iommu/mtk_iommu.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 20a5ba80f983..24bb8b646edc 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -1240,11 +1240,10 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m
 	return 0;
 
 err_larbdev_put:
-	for (i = MTK_LARB_NR_MAX - 1; i >= 0; i--) {
-		if (!data->larb_imu[i].dev)
-			continue;
+	/* id mapping may not be linear, loop the whole array */
+	for (i = 0; i < MTK_LARB_NR_MAX; i++)
 		put_device(data->larb_imu[i].dev);
-	}
+
 	return ret;
 }
 
-- 
2.49.1

[PATCH 08/14] iommu/mediatek-v1: fix device leak on probe_device()

[Johan Hovold]

Make sure to drop the reference taken to the iommu platform device when
looking up its driver data during probe_device().

Fixes: b17336c55d89 ("iommu/mediatek: add support for mtk iommu generation one HW")
Cc: stable@vger.kernel.org	# 4.8
Cc: Honghui Zhang <honghui.zhang@mediatek.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/iommu/mtk_iommu_v1.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
index 10cc0b1197e8..de9153c0a82f 100644
--- a/drivers/iommu/mtk_iommu_v1.c
+++ b/drivers/iommu/mtk_iommu_v1.c
@@ -435,6 +435,8 @@ static int mtk_iommu_v1_create_mapping(struct device *dev,
 			return -EINVAL;
 
 		dev_iommu_priv_set(dev, platform_get_drvdata(m4updev));
+
+		put_device(&m4updev->dev);
 	}
 
 	ret = iommu_fwspec_add_ids(dev, args->args, 1);
-- 
2.49.1

[PATCH 09/14] iommu/mediatek-v1: fix device leaks on probe()

[Johan Hovold]

Make sure to drop the references taken to the larb devices during
probe on probe failure (e.g. probe deferral) and on driver unbind.

Fixes: b17336c55d89 ("iommu/mediatek: add support for mtk iommu generation one HW")
Cc: stable@vger.kernel.org	# 4.8
Cc: Honghui Zhang <honghui.zhang@mediatek.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/iommu/mtk_iommu_v1.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
index de9153c0a82f..44b965a2db92 100644
--- a/drivers/iommu/mtk_iommu_v1.c
+++ b/drivers/iommu/mtk_iommu_v1.c
@@ -648,8 +648,10 @@ static int mtk_iommu_v1_probe(struct platform_device *pdev)
 		struct platform_device *plarbdev;
 
 		larbnode = of_parse_phandle(dev->of_node, "mediatek,larbs", i);
-		if (!larbnode)
-			return -EINVAL;
+		if (!larbnode) {
+			ret = -EINVAL;
+			goto out_put_larbs;
+		}
 
 		if (!of_device_is_available(larbnode)) {
 			of_node_put(larbnode);
@@ -659,11 +661,14 @@ static int mtk_iommu_v1_probe(struct platform_device *pdev)
 		plarbdev = of_find_device_by_node(larbnode);
 		if (!plarbdev) {
 			of_node_put(larbnode);
-			return -ENODEV;
+			ret = -ENODEV;
+			goto out_put_larbs;
 		}
 		if (!plarbdev->dev.driver) {
 			of_node_put(larbnode);
-			return -EPROBE_DEFER;
+			put_device(&plarbdev->dev);
+			ret = -EPROBE_DEFER;
+			goto out_put_larbs;
 		}
 		data->larb_imu[i].dev = &plarbdev->dev;
 
@@ -675,7 +680,7 @@ static int mtk_iommu_v1_probe(struct platform_device *pdev)
 
 	ret = mtk_iommu_v1_hw_init(data);
 	if (ret)
-		return ret;
+		goto out_put_larbs;
 
 	ret = iommu_device_sysfs_add(&data->iommu, &pdev->dev, NULL,
 				     dev_name(&pdev->dev));
@@ -697,12 +702,17 @@ static int mtk_iommu_v1_probe(struct platform_device *pdev)
 	iommu_device_sysfs_remove(&data->iommu);
 out_clk_unprepare:
 	clk_disable_unprepare(data->bclk);
+out_put_larbs:
+	for (i = 0; i < MTK_LARB_NR_MAX; i++)
+		put_device(data->larb_imu[i].dev);
+
 	return ret;
 }
 
 static void mtk_iommu_v1_remove(struct platform_device *pdev)
 {
 	struct mtk_iommu_v1_data *data = platform_get_drvdata(pdev);
+	int i;
 
 	iommu_device_sysfs_remove(&data->iommu);
 	iommu_device_unregister(&data->iommu);
@@ -710,6 +720,9 @@ static void mtk_iommu_v1_remove(struct platform_device *pdev)
 	clk_disable_unprepare(data->bclk);
 	devm_free_irq(&pdev->dev, data->irq, data);
 	component_master_del(&pdev->dev, &mtk_iommu_v1_com_ops);
+
+	for (i = 0; i < MTK_LARB_NR_MAX; i++)
+		put_device(data->larb_imu[i].dev);
 }
 
 static int __maybe_unused mtk_iommu_v1_suspend(struct device *dev)
-- 
2.49.1

[PATCH 10/14] iommu/mediatek-v1: add missing larb count sanity check

[Johan Hovold]

Add the missing larb count sanity check to avoid writing beyond a fixed
sized array in case of a malformed devicetree.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/iommu/mtk_iommu_v1.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
index 44b965a2db92..55d6615a41a9 100644
--- a/drivers/iommu/mtk_iommu_v1.c
+++ b/drivers/iommu/mtk_iommu_v1.c
@@ -643,6 +643,9 @@ static int mtk_iommu_v1_probe(struct platform_device *pdev)
 	if (larb_nr < 0)
 		return larb_nr;
 
+	if (larb_nr > MTK_LARB_NR_MAX)
+		return -EINVAL;
+
 	for (i = 0; i < larb_nr; i++) {
 		struct device_node *larbnode;
 		struct platform_device *plarbdev;
-- 
2.49.1

[PATCH 11/14] iommu/omap: fix device leaks on probe_device()

[Johan Hovold]

Make sure to drop the reference taken to the iommu platform devices
during probe_device() on errors and when the device is later released.

Fixes: 9d5018deec86 ("iommu/omap: Add support to program multiple iommus")
Fixes: 7d6827748d54 ("iommu/omap: Fix iommu archdata name for DT-based devices")
Cc: stable@vger.kernel.org	# 3.18
Cc: Suman Anna <s-anna@ti.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/iommu/omap-iommu.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index 6fb93927bdb9..77023d49bd24 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1636,7 +1636,7 @@ static struct iommu_device *omap_iommu_probe_device(struct device *dev)
 	struct platform_device *pdev;
 	struct omap_iommu *oiommu;
 	struct device_node *np;
-	int num_iommus, i;
+	int num_iommus, i, ret;
 
 	/*
 	 * Allocate the per-device iommu structure for DT-based devices.
@@ -1663,22 +1663,22 @@ static struct iommu_device *omap_iommu_probe_device(struct device *dev)
 	for (i = 0, tmp = arch_data; i < num_iommus; i++, tmp++) {
 		np = of_parse_phandle(dev->of_node, "iommus", i);
 		if (!np) {
-			kfree(arch_data);
-			return ERR_PTR(-EINVAL);
+			ret = -EINVAL;
+			goto err_put_iommus;
 		}
 
 		pdev = of_find_device_by_node(np);
 		if (!pdev) {
 			of_node_put(np);
-			kfree(arch_data);
-			return ERR_PTR(-ENODEV);
+			ret = -ENODEV;
+			goto err_put_iommus;
 		}
 
 		oiommu = platform_get_drvdata(pdev);
 		if (!oiommu) {
 			of_node_put(np);
-			kfree(arch_data);
-			return ERR_PTR(-EINVAL);
+			ret = -EINVAL;
+			goto err_put_iommus;
 		}
 
 		tmp->iommu_dev = oiommu;
@@ -1697,17 +1697,28 @@ static struct iommu_device *omap_iommu_probe_device(struct device *dev)
 	oiommu = arch_data->iommu_dev;
 
 	return &oiommu->iommu;
+
+err_put_iommus:
+	for (tmp = arch_data; tmp->dev; tmp++)
+		put_device(tmp->dev);
+
+	kfree(arch_data);
+
+	return ERR_PTR(ret);
 }
 
 static void omap_iommu_release_device(struct device *dev)
 {
 	struct omap_iommu_arch_data *arch_data = dev_iommu_priv_get(dev);
+	struct omap_iommu_arch_data *tmp;
 
 	if (!dev->of_node || !arch_data)
 		return;
 
-	kfree(arch_data);
+	for (tmp = arch_data; tmp->dev; tmp++)
+		put_device(tmp->dev);
 
+	kfree(arch_data);
 }
 
 static int omap_iommu_of_xlate(struct device *dev, const struct of_phandle_args *args)
-- 
2.49.1

Re: [PATCH 11/14] iommu/omap: fix device leaks on probe_device()

[Robin Murphy]

On 2025-09-25 1:27 pm, Johan Hovold wrote:
> Make sure to drop the reference taken to the iommu platform devices
> during probe_device() on errors and when the device is later released.
> 
> Fixes: 9d5018deec86 ("iommu/omap: Add support to program multiple iommus")
> Fixes: 7d6827748d54 ("iommu/omap: Fix iommu archdata name for DT-based devices")
> Cc: stable@vger.kernel.org	# 3.18
> Cc: Suman Anna <s-anna@ti.com>
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>   drivers/iommu/omap-iommu.c | 27 +++++++++++++++++++--------
>   1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
> index 6fb93927bdb9..77023d49bd24 100644
> --- a/drivers/iommu/omap-iommu.c
> +++ b/drivers/iommu/omap-iommu.c
> @@ -1636,7 +1636,7 @@ static struct iommu_device *omap_iommu_probe_device(struct device *dev)
>   	struct platform_device *pdev;
>   	struct omap_iommu *oiommu;
>   	struct device_node *np;
> -	int num_iommus, i;
> +	int num_iommus, i, ret;
>   
>   	/*
>   	 * Allocate the per-device iommu structure for DT-based devices.
> @@ -1663,22 +1663,22 @@ static struct iommu_device *omap_iommu_probe_device(struct device *dev)
>   	for (i = 0, tmp = arch_data; i < num_iommus; i++, tmp++) {
>   		np = of_parse_phandle(dev->of_node, "iommus", i);
>   		if (!np) {
> -			kfree(arch_data);
> -			return ERR_PTR(-EINVAL);
> +			ret = -EINVAL;
> +			goto err_put_iommus;
>   		}
>   
>   		pdev = of_find_device_by_node(np);
>   		if (!pdev) {
>   			of_node_put(np);
> -			kfree(arch_data);
> -			return ERR_PTR(-ENODEV);
> +			ret = -ENODEV;
> +			goto err_put_iommus;
>   		}
>   
>   		oiommu = platform_get_drvdata(pdev);
>   		if (!oiommu) {
>   			of_node_put(np);
> -			kfree(arch_data);
> -			return ERR_PTR(-EINVAL);
> +			ret = -EINVAL;
> +			goto err_put_iommus;
>   		}
>   
>   		tmp->iommu_dev = oiommu;
> @@ -1697,17 +1697,28 @@ static struct iommu_device *omap_iommu_probe_device(struct device *dev)
>   	oiommu = arch_data->iommu_dev;
>   
>   	return &oiommu->iommu;
> +
> +err_put_iommus:
> +	for (tmp = arch_data; tmp->dev; tmp++)
> +		put_device(tmp->dev);

This should just pair with the of_node_put() calls (other than the first 
one, of course), i.e. do it in the success path as well and drop the 
release_device change below. It doesn't serve any purpose for client 
devices to hold additional references on the IOMMU device when those are 
strictly within the lifetime of the IOMMU driver being bound to it anyway.

Thanks,
Robin.

> +
> +	kfree(arch_data);
> +
> +	return ERR_PTR(ret);
>   }
>   
>   static void omap_iommu_release_device(struct device *dev)
>   {
>   	struct omap_iommu_arch_data *arch_data = dev_iommu_priv_get(dev);
> +	struct omap_iommu_arch_data *tmp;
>   
>   	if (!dev->of_node || !arch_data)
>   		return;
>   
> -	kfree(arch_data);
> +	for (tmp = arch_data; tmp->dev; tmp++)
> +		put_device(tmp->dev);
>   
> +	kfree(arch_data);
>   }
>   
>   static int omap_iommu_of_xlate(struct device *dev, const struct of_phandle_args *args)

Re: [PATCH 11/14] iommu/omap: fix device leaks on probe_device()

[Johan Hovold]

On Thu, Oct 02, 2025 at 01:05:08PM +0100, Robin Murphy wrote:
> On 2025-09-25 1:27 pm, Johan Hovold wrote:

> > @@ -1663,22 +1663,22 @@ static struct iommu_device *omap_iommu_probe_device(struct device *dev)
> >   	for (i = 0, tmp = arch_data; i < num_iommus; i++, tmp++) {
> >   		np = of_parse_phandle(dev->of_node, "iommus", i);
> >   		if (!np) {
> > -			kfree(arch_data);
> > -			return ERR_PTR(-EINVAL);
> > +			ret = -EINVAL;
> > +			goto err_put_iommus;
> >   		}
> >   
> >   		pdev = of_find_device_by_node(np);
> >   		if (!pdev) {
> >   			of_node_put(np);
> > -			kfree(arch_data);
> > -			return ERR_PTR(-ENODEV);
> > +			ret = -ENODEV;
> > +			goto err_put_iommus;
> >   		}
> >   
> >   		oiommu = platform_get_drvdata(pdev);
> >   		if (!oiommu) {
> >   			of_node_put(np);
> > -			kfree(arch_data);
> > -			return ERR_PTR(-EINVAL);
> > +			ret = -EINVAL;
> > +			goto err_put_iommus;
> >   		}
> >   
> >   		tmp->iommu_dev = oiommu;
> > @@ -1697,17 +1697,28 @@ static struct iommu_device *omap_iommu_probe_device(struct device *dev)
> >   	oiommu = arch_data->iommu_dev;
> >   
> >   	return &oiommu->iommu;
> > +
> > +err_put_iommus:
> > +	for (tmp = arch_data; tmp->dev; tmp++)
> > +		put_device(tmp->dev);
> 
> This should just pair with the of_node_put() calls (other than the first 
> one, of course), i.e. do it in the success path as well and drop the 
> release_device change below. It doesn't serve any purpose for client 
> devices to hold additional references on the IOMMU device when those are 
> strictly within the lifetime of the IOMMU driver being bound to it anyway.

I kept the reference until release() (even if not strictly needed) as I
mistakenly thought the driver was using the arch data device pointer
directly.

Turns out that one has never been used so I'll drop it as well as part
of v2.

Johan

[PATCH 12/14] iommu/omap: simplify probe_device() error handling

[Johan Hovold]

Simplify the probe_device() error handling by dropping the iommu OF node
reference sooner.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/iommu/omap-iommu.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index 77023d49bd24..844def804777 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1668,23 +1668,20 @@ static struct iommu_device *omap_iommu_probe_device(struct device *dev)
 		}
 
 		pdev = of_find_device_by_node(np);
+		of_node_put(np);
 		if (!pdev) {
-			of_node_put(np);
 			ret = -ENODEV;
 			goto err_put_iommus;
 		}
 
 		oiommu = platform_get_drvdata(pdev);
 		if (!oiommu) {
-			of_node_put(np);
 			ret = -EINVAL;
 			goto err_put_iommus;
 		}
 
 		tmp->iommu_dev = oiommu;
 		tmp->dev = &pdev->dev;
-
-		of_node_put(np);
 	}
 
 	dev_iommu_priv_set(dev, arch_data);
-- 
2.49.1

[PATCH 13/14] iommu/sun50i: fix device leak on of_xlate()

[Johan Hovold]

Make sure to drop the reference taken to the iommu platform device when
looking up its driver data during of_xlate().

Fixes: 4100b8c229b3 ("iommu: Add Allwinner H6 IOMMU driver")
Cc: stable@vger.kernel.org	# 5.8
Cc: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/iommu/sun50i-iommu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/iommu/sun50i-iommu.c b/drivers/iommu/sun50i-iommu.c
index de10b569d9a9..6306570d57db 100644
--- a/drivers/iommu/sun50i-iommu.c
+++ b/drivers/iommu/sun50i-iommu.c
@@ -839,6 +839,8 @@ static int sun50i_iommu_of_xlate(struct device *dev,
 
 	dev_iommu_priv_set(dev, platform_get_drvdata(iommu_pdev));
 
+	put_device(&iommu_pdev->dev);
+
 	return iommu_fwspec_add_ids(dev, &id, 1);
 }
 
-- 
2.49.1

[PATCH 14/14] iommu/tegra: fix device leak on probe_device()

[Johan Hovold]

Make sure to drop the reference taken to the iommu platform device when
looking up its driver data during probe_device().

Fixes: 891846516317 ("memory: Add NVIDIA Tegra memory controller support")
Cc: stable@vger.kernel.org	# 3.19
Cc: Thierry Reding <treding@nvidia.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/iommu/tegra-smmu.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 36cdd5fbab07..f6f26a072820 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -830,10 +830,9 @@ static struct tegra_smmu *tegra_smmu_find(struct device_node *np)
 		return NULL;
 
 	mc = platform_get_drvdata(pdev);
-	if (!mc) {
-		put_device(&pdev->dev);
+	put_device(&pdev->dev);
+	if (!mc)
 		return NULL;
-	}
 
 	return mc->smmu;
 }
-- 
2.49.1

Re: [PATCH 00/14] iommu: fix device leaks

[Jason Gunthorpe]

On Thu, Sep 25, 2025 at 02:27:42PM +0200, Johan Hovold wrote:
> This series fixes device leaks in the iommu drivers, which pretty
> consistently failed to drop the reference taken by
> of_find_device_by_node() when looking up iommu platform devices.

Yes, they are mis-designed in many ways :\

IDK if it is worth fixing like this, or if more effort should be put
to make the drivers use of_xlate properly - the arm smmu drivers show
the only way to use it..

But if staying like this then maybe add a little helper?

void *iommu_xlate_to_iommu_drvdata(const struct of_phandle_args *args);

Put the whole racy of_find_device_by_node / put_device /
platform_get_drvdata sequence is in one tidy function.. With
documentation it is not safe don't use it in new code?

Jason

Re: [PATCH 00/14] iommu: fix device leaks

[Robin Murphy]

On 2025-09-30 7:21 pm, Jason Gunthorpe wrote:
> On Thu, Sep 25, 2025 at 02:27:42PM +0200, Johan Hovold wrote:
>> This series fixes device leaks in the iommu drivers, which pretty
>> consistently failed to drop the reference taken by
>> of_find_device_by_node() when looking up iommu platform devices.
> 
> Yes, they are mis-designed in many ways :\

Historically they weren't really leaks either, just spare references on 
a device which at that point could definitely never go away. I suppose 
now that we finally have the .of_xlate calls in IOMMU registration, it 
would be possible for some error during probe to cause the IOMMU driver 
to fail to bind, at which point the references could rightly be 
considered leaked, however these are all embedded platforms with 
essentially zero chance of platform device hotplug, especially not of 
IOMMU devices, so realistically those references still don't matter to 
anything other than code checkers.

In summary; meh.
> IDK if it is worth fixing like this, or if more effort should be put
> to make the drivers use of_xlate properly - the arm smmu drivers show
> the only way to use it..

The SMMU drivers are really doing the same thing, they just defer that 
lookup operation to .probe_device time (largely for historical reasons, 
I think), and they use bus_find_device_by_node() rather than the 
specific of_ version since they support ACPI too. And they do happen to 
include the put_device(), since apparently I was paying full attention 
that day.

> But if staying like this then maybe add a little helper?
> 
> void *iommu_xlate_to_iommu_drvdata(const struct of_phandle_args *args);
> 
> Put the whole racy of_find_device_by_node / put_device /
> platform_get_drvdata sequence is in one tidy function.. With
> documentation it is not safe don't use it in new code?

It's not racy; if we're calling the .of_xlate op (or really any op for 
that matter), it's because an IOMMU driver has registered (or is 
registering) for the given fwnode, which means it is bound to the 
corresponding struct device. Thus as above, in that context said struct 
device, nor said IOMMU driver's drvdata, ain't goin' nowhere.

Thanks,
Robin.

Re: [PATCH 00/14] iommu: fix device leaks

[Jason Gunthorpe]

On Tue, Sep 30, 2025 at 08:35:01PM +0100, Robin Murphy wrote:
> On 2025-09-30 7:21 pm, Jason Gunthorpe wrote:
> > On Thu, Sep 25, 2025 at 02:27:42PM +0200, Johan Hovold wrote:
> > > This series fixes device leaks in the iommu drivers, which pretty
> > > consistently failed to drop the reference taken by
> > > of_find_device_by_node() when looking up iommu platform devices.
> > 
> > Yes, they are mis-designed in many ways :\
> 
> Historically they weren't really leaks either, just spare references on a
> device which at that point could definitely never go away. I suppose now
> that we finally have the .of_xlate calls in IOMMU registration, it would be
> possible for some error during probe to cause the IOMMU driver to fail to
> bind, at which point the references could rightly be considered leaked,
> however these are all embedded platforms with essentially zero chance of
> platform device hotplug, especially not of IOMMU devices, so realistically
> those references still don't matter to anything other than code checkers.
> 
> In summary; meh.

Yeah, it isn't a real practical bug, but it still ugly and no doubt
upsets static tools.

> > IDK if it is worth fixing like this, or if more effort should be put
> > to make the drivers use of_xlate properly - the arm smmu drivers show
> > the only way to use it..
> 
> The SMMU drivers are really doing the same thing, they just defer that
> lookup operation to .probe_device time (largely for historical reasons, I
> think), and they use bus_find_device_by_node() 

However the SMMU drivers are doing this under the
iommu_probe_device_lock and not stashing the pointer into a drvdata
where there is no locking protecting it. 

> It's not racy; if we're calling the .of_xlate op (or really any op for that
> matter), it's because an IOMMU driver has registered (or is registering) for
> the given fwnode, which means it is bound to the corresponding struct
> device.

It is not racy if you make the very practical assumption there is no
iommu driver unplug.

Anyhow, I drafted a nice fix for all of this. After all the rework it
is trivial for the core code to pass in the struct iommu_device * to
probe and then most of the drivers just drop this ugly code
completely.

https://github.com/jgunthorpe/linux/commits/iommu-fwspec/

Jason

commit cca42a9b5325b96bfd3d74e24628511f537afbe9
Author: Jason Gunthorpe <jgg@ziepe.ca>
Date:   Wed Oct 1 09:18:13 2025 -0300

    iommu: Add a probe_device_fwspec() op
    
    For fwspec using drivers the core code has already determined which struct
    iommu_device instance the fwspec is linked to. It can trivially pass that
    instance pointer into the driver.
    
    This frees fwspec drivers from having to repeat the work of trying to find
    the struct iommu_device for the fwspec.
    
    Non-fwspec drivers (x86/etc) continue to use the old probe function which
    is called on the first non-fwspec iommu driver registered.
    
    This is only usable by drivers that support a single iommu instance per
    device. There are some drivers (msm, exynos, dart) that do have an iommus
    property that list multiple iommu_devices with multiple IDs. They cannot
    use this simplified mechanism.
    
    Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 060ebe330ee163..718a1da8f54710 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -142,6 +142,8 @@ static void __iommu_group_free_device(struct iommu_group *group,
 				      struct group_device *grp_dev);
 static void iommu_domain_init(struct iommu_domain *domain, unsigned int type,
 			      const struct iommu_ops *ops);
+static struct iommu_device *
+iommu_from_fwnode(const struct fwnode_handle *fwnode);
 
 #define IOMMU_GROUP_ATTR(_name, _mode, _show, _store)		\
 struct iommu_group_attribute iommu_group_attr_##_name =		\
@@ -406,13 +408,75 @@ void dev_iommu_priv_set(struct device *dev, void *priv)
 }
 EXPORT_SYMBOL_GPL(dev_iommu_priv_set);
 
+static int iommu_do_probe_device(struct device *dev)
+{
+	struct iommu_device *iommu;
+	int ret;
+
+	lockdep_assert_held(&iommu_probe_device_lock);
+
+	if (dev->iommu->fwspec) {
+		struct iommu_device *fwspec_iommu;
+
+		fwspec_iommu =
+			iommu_from_fwnode(dev->iommu->fwspec->iommu_fwnode);
+		if (!fwspec_iommu)
+			return -ENODEV;
+
+		if (fwspec_iommu->ops->probe_device_fwspec) {
+			ret = fwspec_iommu->ops->probe_device_fwspec(
+				fwspec_iommu, dev);
+			if (ret)
+				return ret;
+			iommu = fwspec_iommu;
+		} else {
+			iommu = fwspec_iommu->ops->probe_device(dev);
+			if (IS_ERR(iommu))
+				return PTR_ERR(iommu);
+			if (WARN_ON(iommu != fwspec_iommu)) {
+				ret = -EINVAL;
+				goto err_release;
+			}
+		}
+	} else {
+		/*
+		 * At this point, relevant devices either now have a fwspec
+		 * which will match ops registered with a non-NULL fwnode, or we
+		 * can reasonably assume that only one of Intel, AMD, s390, PAMU
+		 * or legacy SMMUv2 can be present, and that any of their
+		 * registered instances has suitable ops for probing, and thus
+		 * cheekily co-opt the same mechanism.
+		 */
+		iommu = iommu_from_fwnode(NULL);
+		if (!iommu)
+			return -ENODEV;
+		/* Non fwspec drivers must identify their instance internally */
+		if (WARN_ON(!iommu->ops->probe_device))
+			return -EINVAL;
+		iommu = iommu->ops->probe_device(dev);
+		if (IS_ERR(iommu))
+			return PTR_ERR(iommu);
+	}
+
+	if (!try_module_get(iommu->ops->owner)) {
+		ret = -EINVAL;
+		goto err_release;
+	}
+
+	dev->iommu->iommu_dev = iommu;
+
+err_release:
+	if (iommu->ops->release_device)
+		iommu->ops->release_device(dev);
+	return ret;
+}
+
 /*
  * Init the dev->iommu and dev->iommu_group in the struct device and get the
  * driver probed
  */
 static int iommu_init_device(struct device *dev)
 {
-	const struct iommu_ops *ops;
 	struct iommu_device *iommu_dev;
 	struct iommu_group *group;
 	int ret;
@@ -434,36 +498,17 @@ static int iommu_init_device(struct device *dev)
 		if (!dev->iommu || dev->iommu_group)
 			return -ENODEV;
 	}
-	/*
-	 * At this point, relevant devices either now have a fwspec which will
-	 * match ops registered with a non-NULL fwnode, or we can reasonably
-	 * assume that only one of Intel, AMD, s390, PAMU or legacy SMMUv2 can
-	 * be present, and that any of their registered instances has suitable
-	 * ops for probing, and thus cheekily co-opt the same mechanism.
-	 */
-	ops = iommu_fwspec_ops(dev->iommu->fwspec);
-	if (!ops) {
-		ret = -ENODEV;
-		goto err_free;
-	}
 
-	if (!try_module_get(ops->owner)) {
-		ret = -EINVAL;
+	ret = iommu_do_probe_device(dev);
+	if (ret)
 		goto err_free;
-	}
-
-	iommu_dev = ops->probe_device(dev);
-	if (IS_ERR(iommu_dev)) {
-		ret = PTR_ERR(iommu_dev);
-		goto err_module_put;
-	}
-	dev->iommu->iommu_dev = iommu_dev;
+	iommu_dev = dev->iommu->iommu_dev;
 
 	ret = iommu_device_link(iommu_dev, dev);
 	if (ret)
 		goto err_release;
 
-	group = ops->device_group(dev);
+	group = iommu_dev->ops->device_group(dev);
 	if (WARN_ON_ONCE(group == NULL))
 		group = ERR_PTR(-EINVAL);
 	if (IS_ERR(group)) {
@@ -473,17 +518,17 @@ static int iommu_init_device(struct device *dev)
 	dev->iommu_group = group;
 
 	dev->iommu->max_pasids = dev_iommu_get_max_pasids(dev);
-	if (ops->is_attach_deferred)
-		dev->iommu->attach_deferred = ops->is_attach_deferred(dev);
+	if (iommu_dev->ops->is_attach_deferred)
+		dev->iommu->attach_deferred =
+			iommu_dev->ops->is_attach_deferred(dev);
 	return 0;
 
 err_unlink:
 	iommu_device_unlink(iommu_dev, dev);
 err_release:
-	if (ops->release_device)
-		ops->release_device(dev);
-err_module_put:
-	module_put(ops->owner);
+	if (iommu_dev->ops->release_device)
+		iommu_dev->ops->release_device(dev);
+	module_put(iommu_dev->ops->owner);
 err_free:
 	dev->iommu->iommu_dev = NULL;
 	dev_iommu_free(dev);
@@ -2855,9 +2900,10 @@ bool iommu_default_passthrough(void)
 }
 EXPORT_SYMBOL_GPL(iommu_default_passthrough);
 
-static const struct iommu_device *iommu_from_fwnode(const struct fwnode_handle *fwnode)
+static struct iommu_device *iommu_from_fwnode(
+	const struct fwnode_handle *fwnode)
 {
-	const struct iommu_device *iommu, *ret = NULL;
+	struct iommu_device *iommu, *ret = NULL;
 
 	spin_lock(&iommu_device_lock);
 	list_for_each_entry(iommu, &iommu_device_list, list)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index c30d12e16473df..52e74ccdb4dc79 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -48,6 +48,7 @@ struct iommufd_ctx;
 struct iommufd_viommu;
 struct msi_desc;
 struct msi_msg;
+struct iommu_device;
 
 #define IOMMU_FAULT_PERM_READ	(1 << 0) /* read */
 #define IOMMU_FAULT_PERM_WRITE	(1 << 1) /* write */
@@ -680,6 +681,8 @@ struct iommu_ops {
 		struct device *dev, struct iommu_domain *parent, u32 flags,
 		const struct iommu_user_data *user_data);
 
+	int (*probe_device_fwspec)(struct iommu_device *iommu,
+				   struct device *dev);
 	struct iommu_device *(*probe_device)(struct device *dev);
 	void (*release_device)(struct device *dev);
 	void (*probe_finalize)(struct device *dev);

Re: [PATCH 00/14] iommu: fix device leaks

[Robin Murphy]

On 2025-10-01 4:58 pm, Jason Gunthorpe wrote:
> On Tue, Sep 30, 2025 at 08:35:01PM +0100, Robin Murphy wrote:
>> On 2025-09-30 7:21 pm, Jason Gunthorpe wrote:
>>> On Thu, Sep 25, 2025 at 02:27:42PM +0200, Johan Hovold wrote:
>>>> This series fixes device leaks in the iommu drivers, which pretty
>>>> consistently failed to drop the reference taken by
>>>> of_find_device_by_node() when looking up iommu platform devices.
>>>
>>> Yes, they are mis-designed in many ways :\
>>
>> Historically they weren't really leaks either, just spare references on a
>> device which at that point could definitely never go away. I suppose now
>> that we finally have the .of_xlate calls in IOMMU registration, it would be
>> possible for some error during probe to cause the IOMMU driver to fail to
>> bind, at which point the references could rightly be considered leaked,
>> however these are all embedded platforms with essentially zero chance of
>> platform device hotplug, especially not of IOMMU devices, so realistically
>> those references still don't matter to anything other than code checkers.
>>
>> In summary; meh.
> 
> Yeah, it isn't a real practical bug, but it still ugly and no doubt
> upsets static tools.
> 
>>> IDK if it is worth fixing like this, or if more effort should be put
>>> to make the drivers use of_xlate properly - the arm smmu drivers show
>>> the only way to use it..
>>
>> The SMMU drivers are really doing the same thing, they just defer that
>> lookup operation to .probe_device time (largely for historical reasons, I
>> think), and they use bus_find_device_by_node()
> 
> However the SMMU drivers are doing this under the
> iommu_probe_device_lock and not stashing the pointer into a drvdata
> where there is no locking protecting it.

Huh? Every .of_xlate call is under probe_device_lock just as much as 
.probe_device calls are; they *have* to be, since they too are in the 
position of working with dev->iommu before dev->iommu_group is set to 
guarantee its stability.

>> It's not racy; if we're calling the .of_xlate op (or really any op for that
>> matter), it's because an IOMMU driver has registered (or is registering) for
>> the given fwnode, which means it is bound to the corresponding struct
>> device.
> 
> It is not racy if you make the very practical assumption there is no
> iommu driver unplug.

If the IOMMU code allowed a driver to be removed while it's in the 
middle of calling into that driver, that would hardly be the driver's 
fault. And indeed the driver cannot be removed, because we hold a module 
reference around the call (and if a buggy driver failed to suppress 
manual sysfs unbinding, this case is frankly one of the smallest drops 
in the ocean of catastrophic consequences there.)

> Anyhow, I drafted a nice fix for all of this. After all the rework it
> is trivial for the core code to pass in the struct iommu_device * to
> probe and then most of the drivers just drop this ugly code
> completely.
 >
 > https://github.com/jgunthorpe/linux/commits/iommu-fwspec/

Eww, that is neither nice nor a "fix". Once again it's just piling on a 
load of extra complexity to have multiple 
confusingly-overlapping-but-different ways of doing the same thing, one 
of which is still the exact same one you've decided to object to because 
you've failed to understand it (as demonstrated by the commit message 
below, the obvious bug in the hideous mess below that, and at a glance 
the other patches actively *breaking* at least one driver.)

Thanks,
Robin.

> 
> Jason
> 
> commit cca42a9b5325b96bfd3d74e24628511f537afbe9
> Author: Jason Gunthorpe <jgg@ziepe.ca>
> Date:   Wed Oct 1 09:18:13 2025 -0300
> 
>      iommu: Add a probe_device_fwspec() op
>      
>      For fwspec using drivers the core code has already determined which struct
>      iommu_device instance the fwspec is linked to. It can trivially pass that
>      instance pointer into the driver.
>      
>      This frees fwspec drivers from having to repeat the work of trying to find
>      the struct iommu_device for the fwspec.
>      
>      Non-fwspec drivers (x86/etc) continue to use the old probe function which
>      is called on the first non-fwspec iommu driver registered.
>      
>      This is only usable by drivers that support a single iommu instance per
>      device. There are some drivers (msm, exynos, dart) that do have an iommus
>      property that list multiple iommu_devices with multiple IDs. They cannot
>      use this simplified mechanism.
>      
>      Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 060ebe330ee163..718a1da8f54710 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -142,6 +142,8 @@ static void __iommu_group_free_device(struct iommu_group *group,
>   				      struct group_device *grp_dev);
>   static void iommu_domain_init(struct iommu_domain *domain, unsigned int type,
>   			      const struct iommu_ops *ops);
> +static struct iommu_device *
> +iommu_from_fwnode(const struct fwnode_handle *fwnode);
>   
>   #define IOMMU_GROUP_ATTR(_name, _mode, _show, _store)		\
>   struct iommu_group_attribute iommu_group_attr_##_name =		\
> @@ -406,13 +408,75 @@ void dev_iommu_priv_set(struct device *dev, void *priv)
>   }
>   EXPORT_SYMBOL_GPL(dev_iommu_priv_set);
>   
> +static int iommu_do_probe_device(struct device *dev)
> +{
> +	struct iommu_device *iommu;
> +	int ret;
> +
> +	lockdep_assert_held(&iommu_probe_device_lock);
> +
> +	if (dev->iommu->fwspec) {
> +		struct iommu_device *fwspec_iommu;
> +
> +		fwspec_iommu =
> +			iommu_from_fwnode(dev->iommu->fwspec->iommu_fwnode);
> +		if (!fwspec_iommu)
> +			return -ENODEV;
> +
> +		if (fwspec_iommu->ops->probe_device_fwspec) {
> +			ret = fwspec_iommu->ops->probe_device_fwspec(
> +				fwspec_iommu, dev);
> +			if (ret)
> +				return ret;
> +			iommu = fwspec_iommu;
> +		} else {
> +			iommu = fwspec_iommu->ops->probe_device(dev);
> +			if (IS_ERR(iommu))
> +				return PTR_ERR(iommu);
> +			if (WARN_ON(iommu != fwspec_iommu)) {
> +				ret = -EINVAL;
> +				goto err_release;
> +			}
> +		}
> +	} else {
> +		/*
> +		 * At this point, relevant devices either now have a fwspec
> +		 * which will match ops registered with a non-NULL fwnode, or we
> +		 * can reasonably assume that only one of Intel, AMD, s390, PAMU
> +		 * or legacy SMMUv2 can be present, and that any of their
> +		 * registered instances has suitable ops for probing, and thus
> +		 * cheekily co-opt the same mechanism.
> +		 */
> +		iommu = iommu_from_fwnode(NULL);
> +		if (!iommu)
> +			return -ENODEV;
> +		/* Non fwspec drivers must identify their instance internally */
> +		if (WARN_ON(!iommu->ops->probe_device))
> +			return -EINVAL;
> +		iommu = iommu->ops->probe_device(dev);
> +		if (IS_ERR(iommu))
> +			return PTR_ERR(iommu);
> +	}
> +
> +	if (!try_module_get(iommu->ops->owner)) {
> +		ret = -EINVAL;
> +		goto err_release;
> +	}
> +
> +	dev->iommu->iommu_dev = iommu;
> +
> +err_release:
> +	if (iommu->ops->release_device)
> +		iommu->ops->release_device(dev);
> +	return ret;
> +}
> +
>   /*
>    * Init the dev->iommu and dev->iommu_group in the struct device and get the
>    * driver probed
>    */
>   static int iommu_init_device(struct device *dev)
>   {
> -	const struct iommu_ops *ops;
>   	struct iommu_device *iommu_dev;
>   	struct iommu_group *group;
>   	int ret;
> @@ -434,36 +498,17 @@ static int iommu_init_device(struct device *dev)
>   		if (!dev->iommu || dev->iommu_group)
>   			return -ENODEV;
>   	}
> -	/*
> -	 * At this point, relevant devices either now have a fwspec which will
> -	 * match ops registered with a non-NULL fwnode, or we can reasonably
> -	 * assume that only one of Intel, AMD, s390, PAMU or legacy SMMUv2 can
> -	 * be present, and that any of their registered instances has suitable
> -	 * ops for probing, and thus cheekily co-opt the same mechanism.
> -	 */
> -	ops = iommu_fwspec_ops(dev->iommu->fwspec);
> -	if (!ops) {
> -		ret = -ENODEV;
> -		goto err_free;
> -	}
>   
> -	if (!try_module_get(ops->owner)) {
> -		ret = -EINVAL;
> +	ret = iommu_do_probe_device(dev);
> +	if (ret)
>   		goto err_free;
> -	}
> -
> -	iommu_dev = ops->probe_device(dev);
> -	if (IS_ERR(iommu_dev)) {
> -		ret = PTR_ERR(iommu_dev);
> -		goto err_module_put;
> -	}
> -	dev->iommu->iommu_dev = iommu_dev;
> +	iommu_dev = dev->iommu->iommu_dev;
>   
>   	ret = iommu_device_link(iommu_dev, dev);
>   	if (ret)
>   		goto err_release;
>   
> -	group = ops->device_group(dev);
> +	group = iommu_dev->ops->device_group(dev);
>   	if (WARN_ON_ONCE(group == NULL))
>   		group = ERR_PTR(-EINVAL);
>   	if (IS_ERR(group)) {
> @@ -473,17 +518,17 @@ static int iommu_init_device(struct device *dev)
>   	dev->iommu_group = group;
>   
>   	dev->iommu->max_pasids = dev_iommu_get_max_pasids(dev);
> -	if (ops->is_attach_deferred)
> -		dev->iommu->attach_deferred = ops->is_attach_deferred(dev);
> +	if (iommu_dev->ops->is_attach_deferred)
> +		dev->iommu->attach_deferred =
> +			iommu_dev->ops->is_attach_deferred(dev);
>   	return 0;
>   
>   err_unlink:
>   	iommu_device_unlink(iommu_dev, dev);
>   err_release:
> -	if (ops->release_device)
> -		ops->release_device(dev);
> -err_module_put:
> -	module_put(ops->owner);
> +	if (iommu_dev->ops->release_device)
> +		iommu_dev->ops->release_device(dev);
> +	module_put(iommu_dev->ops->owner);
>   err_free:
>   	dev->iommu->iommu_dev = NULL;
>   	dev_iommu_free(dev);
> @@ -2855,9 +2900,10 @@ bool iommu_default_passthrough(void)
>   }
>   EXPORT_SYMBOL_GPL(iommu_default_passthrough);
>   
> -static const struct iommu_device *iommu_from_fwnode(const struct fwnode_handle *fwnode)
> +static struct iommu_device *iommu_from_fwnode(
> +	const struct fwnode_handle *fwnode)
>   {
> -	const struct iommu_device *iommu, *ret = NULL;
> +	struct iommu_device *iommu, *ret = NULL;
>   
>   	spin_lock(&iommu_device_lock);
>   	list_for_each_entry(iommu, &iommu_device_list, list)
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index c30d12e16473df..52e74ccdb4dc79 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -48,6 +48,7 @@ struct iommufd_ctx;
>   struct iommufd_viommu;
>   struct msi_desc;
>   struct msi_msg;
> +struct iommu_device;
>   
>   #define IOMMU_FAULT_PERM_READ	(1 << 0) /* read */
>   #define IOMMU_FAULT_PERM_WRITE	(1 << 1) /* write */
> @@ -680,6 +681,8 @@ struct iommu_ops {
>   		struct device *dev, struct iommu_domain *parent, u32 flags,
>   		const struct iommu_user_data *user_data);
>   
> +	int (*probe_device_fwspec)(struct iommu_device *iommu,
> +				   struct device *dev);
>   	struct iommu_device *(*probe_device)(struct device *dev);
>   	void (*release_device)(struct device *dev);
>   	void (*probe_finalize)(struct device *dev);

Re: [PATCH 00/14] iommu: fix device leaks

[Jason Gunthorpe]

On Thu, Oct 02, 2025 at 12:48:35PM +0100, Robin Murphy wrote:
> > However the SMMU drivers are doing this under the
> > iommu_probe_device_lock and not stashing the pointer into a drvdata
> > where there is no locking protecting it.
> 
> Huh? Every .of_xlate call is under probe_device_lock just as much as
> .probe_device calls are; they *have* to be, since they too are in the
> position of working with dev->iommu before dev->iommu_group is set to
> guarantee its stability.

Yes, of_xlate is under the lock, but IIRC there are still cases where
probe gets deferred, the probe_device_lock is unlocked, and the
drvdata continues to exist.

> indeed the driver cannot be removed, because we hold a module reference
> around the call

That's not how module reference counts work. Drivers can be unbound
through sysfs at any time.

> > Anyhow, I drafted a nice fix for all of this. After all the rework it
> > is trivial for the core code to pass in the struct iommu_device * to
> > probe and then most of the drivers just drop this ugly code
> > completely.
> >
> > https://github.com/jgunthorpe/linux/commits/iommu-fwspec/
> 
> Eww, that is neither nice nor a "fix". Once again it's just piling on a load
> of extra complexity to have multiple confusingly-overlapping-but-different
> ways of doing the same thing, one of which is still the exact same one
> you've decided to object to because you've failed to understand it (as
> demonstrated by the commit message below, the obvious bug in the hideous
> mess below that, and at a glance the other patches actively *breaking* at
> least one driver.)

It is hard to take you seriously with such vauge objections
Robin. Please try to be constructive. If you are specific I'll go fix
the things and maybe other people besides you can understand this
stuff.

I'm shocked you disagree with the core code helping the drivers find
their iommu_driver. This seems like very basic obvious good stuff. We
don't need all sorts of different open coded ugly ways for drivers to
do this. It removes 200 lines of junk for drivers :\

> > +			iommu = fwspec_iommu->ops->probe_device(dev);
> > +			if (IS_ERR(iommu))
> > +				return PTR_ERR(iommu);
> > +			if (WARN_ON(iommu != fwspec_iommu)) {

This is at least one typo.

Jason

Re: [PATCH 00/14] iommu: fix device leaks

[Johan Hovold]

On Tue, Sep 30, 2025 at 03:21:58PM -0300, Jason Gunthorpe wrote:
> On Thu, Sep 25, 2025 at 02:27:42PM +0200, Johan Hovold wrote:
> > This series fixes device leaks in the iommu drivers, which pretty
> > consistently failed to drop the reference taken by
> > of_find_device_by_node() when looking up iommu platform devices.
> 
> Yes, they are mis-designed in many ways :\

This seems to be more a case of developers not reading documentation and
copying implementations from existing drivers.

I amended the documentation for of_find_device_by_node() in 2016 and at
least of some these drivers were merged later. Fixing up the existing
uses will hopefully reduce the likelihood of further leaks being
introduced (e.g. in places were it matters more).

> IDK if it is worth fixing like this, or if more effort should be put
> to make the drivers use of_xlate properly - the arm smmu drivers show
> the only way to use it..

As Robin pointed out, those drivers just drop the reference as they
should (even if I'd drop the reference after looking up the driver
data).

> But if staying like this then maybe add a little helper?
> 
> void *iommu_xlate_to_iommu_drvdata(const struct of_phandle_args *args);
> 
> Put the whole racy of_find_device_by_node / put_device /
> platform_get_drvdata sequence is in one tidy function.. With
> documentation it is not safe don't use it in new code?

It's not racy in the context of iommu drivers as Robin also explained.

Johan

Re: [PATCH 00/14] iommu: fix device leaks

[Jason Gunthorpe]

On Wed, Oct 01, 2025 at 11:16:09AM +0200, Johan Hovold wrote:

> This seems to be more a case of developers not reading documentation and
> copying implementations from existing drivers.

IMHO the drivers are mis-using of_xlate, it shouldn't be looking up
drvdata at all.
 
> > IDK if it is worth fixing like this, or if more effort should be put
> > to make the drivers use of_xlate properly - the arm smmu drivers show
> > the only way to use it..
> 
> As Robin pointed out, those drivers just drop the reference as they
> should (even if I'd drop the reference after looking up the driver
> data).

So I wrote the series to clean this up and drop the calls to
of_find_device_by_node() and so on.

I left dart, exynos, msm and omap as-is, so I suggest you trim this
series to only those drivers and rely on my version to fix the rest.

Jason

Re: [PATCH 00/14] iommu: fix device leaks

[Johan Hovold]

On Wed, Oct 01, 2025 at 01:01:32PM -0300, Jason Gunthorpe wrote:
> On Wed, Oct 01, 2025 at 11:16:09AM +0200, Johan Hovold wrote:
> 
> > This seems to be more a case of developers not reading documentation and
> > copying implementations from existing drivers.
> 
> IMHO the drivers are mis-using of_xlate, it shouldn't be looking up
> drvdata at all.

Ok, but that would in any case be a separate issue.

> > > IDK if it is worth fixing like this, or if more effort should be put
> > > to make the drivers use of_xlate properly - the arm smmu drivers show
> > > the only way to use it..
> > 
> > As Robin pointed out, those drivers just drop the reference as they
> > should (even if I'd drop the reference after looking up the driver
> > data).
> 
> So I wrote the series to clean this up and drop the calls to
> of_find_device_by_node() and so on.
> 
> I left dart, exynos, msm and omap as-is, so I suggest you trim this
> series to only those drivers and rely on my version to fix the rest.

Looks like there's some room for unification and code reuse, but that
can be done on top of these fixes.

Johan

Re: [PATCH 00/14] iommu: fix device leaks

[Robin Murphy]

On 2025-09-25 1:27 pm, Johan Hovold wrote:
> This series fixes device leaks in the iommu drivers, which pretty
> consistently failed to drop the reference taken by
> of_find_device_by_node() when looking up iommu platform devices.
> 
> Included are also a couple of related cleanups.
Modulo the nitpick for OMAP,

Acked-by: Robin Murphy <robin.murphy@arm.com>

We could in fact also clean up nearly all the NULL checks in these areas 
that are now entirely redundant since per-instance ops lookup, but that 
might just le;ad to more patches from the static checker brigade trying 
to put them back... :/

Thanks,
Robin.

> Johan
> 
> 
> Johan Hovold (14):
>    iommu/apple-dart: fix device leak on of_xlate()
>    iommu/qcom: fix device leak on of_xlate()
>    iommu/exynos: fix device leak on of_xlate()
>    iommu/ipmmu-vmsa: fix device leak on of_xlate()
>    iommu/mediatek: fix device leak on of_xlate()
>    iommu/mediatek: fix device leaks on probe()
>    iommu/mediatek: simplify dt parsing error handling
>    iommu/mediatek-v1: fix device leak on probe_device()
>    iommu/mediatek-v1: fix device leaks on probe()
>    iommu/mediatek-v1: add missing larb count sanity check
>    iommu/omap: fix device leaks on probe_device()
>    iommu/omap: simplify probe_device() error handling
>    iommu/sun50i: fix device leak on of_xlate()
>    iommu/tegra: fix device leak on probe_device()
> 
>   drivers/iommu/apple-dart.c              |  2 ++
>   drivers/iommu/arm/arm-smmu/qcom_iommu.c | 10 +++-----
>   drivers/iommu/exynos-iommu.c            |  9 +++----
>   drivers/iommu/ipmmu-vmsa.c              |  2 ++
>   drivers/iommu/mtk_iommu.c               | 33 +++++++++++++++++--------
>   drivers/iommu/mtk_iommu_v1.c            | 28 +++++++++++++++++----
>   drivers/iommu/omap-iommu.c              | 32 +++++++++++++++---------
>   drivers/iommu/sun50i-iommu.c            |  2 ++
>   drivers/iommu/tegra-smmu.c              |  5 ++--
>   9 files changed, 81 insertions(+), 42 deletions(-)
>