[PATCH] iommu/omap: Fix error handling in omap_iommu_probe_device

[PATCH] iommu/omap: Fix error handling in omap_iommu_probe_device

[Ma Ke]

omap_iommu_probe_device() calls of_find_device_by_node() which
increments the reference count of the platform device, but fails to
decrement the reference count in both success and error paths. This
could lead to resource leakage and prevent proper device cleanup when
the IOMMU is unbound or the device is removed.

Found by code review.

Cc: stable@vger.kernel.org
Fixes: 9d5018deec86 ("iommu/omap: Add support to program multiple iommus")
Signed-off-by: Ma Ke <make24@iscas.ac.cn>
---
 drivers/iommu/omap-iommu.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index 5c6f5943f44b..4df06cb09623 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1637,6 +1637,7 @@ static struct iommu_device *omap_iommu_probe_device(struct device *dev)
 	struct omap_iommu *oiommu;
 	struct device_node *np;
 	int num_iommus, i;
+	int ret = 0;
 
 	/*
 	 * Allocate the per-device iommu structure for DT-based devices.
@@ -1663,28 +1664,26 @@ 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_cleanup;
 		}
 
 		pdev = of_find_device_by_node(np);
+		of_node_put(np);
 		if (!pdev) {
-			of_node_put(np);
-			kfree(arch_data);
-			return ERR_PTR(-ENODEV);
+			ret = -ENODEV;
+			goto err_cleanup;
 		}
 
 		oiommu = platform_get_drvdata(pdev);
 		if (!oiommu) {
-			of_node_put(np);
-			kfree(arch_data);
-			return ERR_PTR(-EINVAL);
+			put_device(&pdev->dev);
+			ret = -EINVAL;
+			goto err_cleanup;
 		}
 
 		tmp->iommu_dev = oiommu;
 		tmp->dev = &pdev->dev;
-
-		of_node_put(np);
 	}
 
 	dev_iommu_priv_set(dev, arch_data);
@@ -1697,17 +1696,28 @@ static struct iommu_device *omap_iommu_probe_device(struct device *dev)
 	oiommu = arch_data->iommu_dev;
 
 	return &oiommu->iommu;
+
+err_cleanup:
+	for (tmp = arch_data; tmp < arch_data + i; tmp++) {
+		if (tmp->dev)
+			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.17.1

Re: [PATCH] iommu/omap: Fix error handling in omap_iommu_probe_device

[Markus Elfring]

> omap_iommu_probe_device() calls of_find_device_by_node() which
> increments the reference count of the platform device, but fails to
> decrement the reference count in both success and error paths. This
> could lead to resource leakage and prevent proper device cleanup when
> the IOMMU is unbound or the device is removed.

1. See also once more:
   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.18-rc5#n94

2. May the statement “ret = -EINVAL;” be moved behind an additional label?

3. Would it be helpful to append parentheses to the function name in the summary phrase?


Regards,
Markus

Re: [PATCH] iommu/omap: Fix error handling in omap_iommu_probe_device

[Robin Murphy]

On 2025-11-17 3:39 am, Ma Ke wrote:
> omap_iommu_probe_device() calls of_find_device_by_node() which
> increments the reference count of the platform device, but fails to
> decrement the reference count in both success and error paths. This
> could lead to resource leakage and prevent proper device cleanup when
> the IOMMU is unbound or the device is removed.

This is already fixed by Johan's comprehensive cleanup series:

https://lore.kernel.org/linux-iommu/20251020045318.30690-1-johan@kernel.org/

Thanks,
Robin.

> Found by code review.
> 
> Cc: stable@vger.kernel.org
> Fixes: 9d5018deec86 ("iommu/omap: Add support to program multiple iommus")
> Signed-off-by: Ma Ke <make24@iscas.ac.cn>
> ---
>   drivers/iommu/omap-iommu.c | 32 +++++++++++++++++++++-----------
>   1 file changed, 21 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
> index 5c6f5943f44b..4df06cb09623 100644
> --- a/drivers/iommu/omap-iommu.c
> +++ b/drivers/iommu/omap-iommu.c
> @@ -1637,6 +1637,7 @@ static struct iommu_device *omap_iommu_probe_device(struct device *dev)
>   	struct omap_iommu *oiommu;
>   	struct device_node *np;
>   	int num_iommus, i;
> +	int ret = 0;
>   
>   	/*
>   	 * Allocate the per-device iommu structure for DT-based devices.
> @@ -1663,28 +1664,26 @@ 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_cleanup;
>   		}
>   
>   		pdev = of_find_device_by_node(np);
> +		of_node_put(np);
>   		if (!pdev) {
> -			of_node_put(np);
> -			kfree(arch_data);
> -			return ERR_PTR(-ENODEV);
> +			ret = -ENODEV;
> +			goto err_cleanup;
>   		}
>   
>   		oiommu = platform_get_drvdata(pdev);
>   		if (!oiommu) {
> -			of_node_put(np);
> -			kfree(arch_data);
> -			return ERR_PTR(-EINVAL);
> +			put_device(&pdev->dev);
> +			ret = -EINVAL;
> +			goto err_cleanup;
>   		}
>   
>   		tmp->iommu_dev = oiommu;
>   		tmp->dev = &pdev->dev;
> -
> -		of_node_put(np);
>   	}
>   
>   	dev_iommu_priv_set(dev, arch_data);
> @@ -1697,17 +1696,28 @@ static struct iommu_device *omap_iommu_probe_device(struct device *dev)
>   	oiommu = arch_data->iommu_dev;
>   
>   	return &oiommu->iommu;
> +
> +err_cleanup:
> +	for (tmp = arch_data; tmp < arch_data + i; tmp++) {
> +		if (tmp->dev)
> +			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)