iommu: fix device leaks
Subsystems (12)
v2 cover iommu: fix device leaks
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
Changes in v2
- drop omap reference sooner; remove unused pointer
- amend exynos commit message with reference to partial fix
- amend tegra commit message with reference to partial fix
- add Robin's ack
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 | 7 ++---- drivers/iommu/omap-iommu.h | 2 -- drivers/iommu/sun50i-iommu.c | 2 ++ drivers/iommu/tegra-smmu.c | 5 ++-- 10 files changed, 63 insertions(+), 37 deletions(-)
Signature · 1 line
2.49.1
PATCH v2 1/14 iommu/apple-dart: fix device leak on of_xlate()
Make sure to drop the reference taken to the iommu platform device when looking up its driver data during of_xlate().
--- drivers/iommu/apple-dart.c | 2 ++ 1 file changed, 2 insertions(+)
drivers/iommu/apple-dart.c +2 -0
@@ -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 ];
Signature · 1 line
2.49.1
PATCH v2 2/14 iommu/qcom: fix device leak on of_xlate()
In reply to Johan HovoldMake 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.
--- drivers/iommu/arm/arm-smmu/qcom_iommu.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
drivers/iommu/arm/arm-smmu/qcom_iommu.c +4 -6
@@ -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 );
Signature · 1 line
2.49.1
PATCH v2 3/14 iommu/exynos: fix device leak on of_xlate()
In reply to Johan HovoldMake sure to drop the reference taken to the iommu platform device when looking up its driver data during of_xlate().
Note that commit 1a26044954a6 ("iommu/exynos: add missing put_device() call in exynos_iommu_of_xlate()") fixed the leak in a couple of error paths, but the reference is still leaking on success.
--- drivers/iommu/exynos-iommu.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
drivers/iommu/exynos-iommu.c +3 -6
@@ -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);
Signature · 1 line
2.49.1
Re: [PATCH v2 03/14] iommu/exynos: fix device leak on of_xlate()
Quoted 7 lines · 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
1a26044954a6("iommu/exynos: add missingput_device()call inexynos_iommu_of_xlate()") fixed the leak in a couple of error paths, but the reference is still leaking on success.
Quoted 6 lines
Fixes aa759fd376fb ("iommu/exynos: Add callback for initializing devices from device tree")Cc [email protected] # 4.2: 1a26044954a6Cc Marek Szyprowski <[email protected]>Cc Yu Kuai <[email protected]>Acked-by Robin Murphy <[email protected]>Signed-off-by Johan Hovold <[email protected]>
Quoted 19 lines
--- drivers/iommu/exynos-iommu.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)drivers/iommu/exynos-iommu.c +2
-3@@ -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 ; - }
drivers/iommu/exynos-iommu.c static int exynos_iommu_of_xlate(struct device *dev, · line 1459
Quoted 10 lines
drivers/iommu/exynos-iommu.c +1
-3if (!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);
Best regards
Signature · 2 lines
Marek Szyprowski, PhD Samsung R&D Institute Poland
PATCH v2 4/14 iommu/ipmmu-vmsa: fix device leak on of_xlate()
In reply to Johan HovoldMake sure to drop the reference taken to the iommu platform device when looking up its driver data during of_xlate().
--- drivers/iommu/ipmmu-vmsa.c | 2 ++ 1 file changed, 2 insertions(+)
drivers/iommu/ipmmu-vmsa.c +2 -0
@@ -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 ; }
Signature · 1 line
2.49.1
PATCH v2 5/14 iommu/mediatek: fix device leak on of_xlate()
In reply to Johan HovoldMake sure to drop the reference taken to the iommu platform device when looking up its driver data during of_xlate().
--- drivers/iommu/mtk_iommu.c | 2 ++ 1 file changed, 2 insertions(+)
drivers/iommu/mtk_iommu.c +2 -0
@@ -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 );
Signature · 1 line
2.49.1
Re: [PATCH v2 05/14] iommu/mediatek: fix device leak on of_xlate()
Quoted 8 lines · Johan Hovold
External email : Please do not click links or open attachments until you have verified the sender or the content.
Make sure to drop the reference taken to the iommu platform device when
looking up its driver data duringof_xlate().
Quoted 5 lines
Fixes 0df4fabe208d ("iommu/mediatek: Add mt8173 IOMMU driver")Cc [email protected] # 4.6Cc Yong Wu <[email protected]>Acked-by Robin Murphy <[email protected]>Signed-off-by Johan Hovold <[email protected]>
PATCH v2 6/14 iommu/mediatek: fix device leaks on probe()
In reply to Johan HovoldMake 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.
--- drivers/iommu/mtk_iommu.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-)
drivers/iommu/mtk_iommu.c +18 -6
@@ -1216,13 +1216,17 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **mplatform_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 **mplatform_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++) {
Signature · 1 line
2.49.1
Yong Wu (吴勇)
maintainerRe: [PATCH v2 06/14] iommu/mediatek: fix device leaks on probe()
Quoted 30 lines · Johan Hovold
External email : Please do not click links or open attachments until you have verified the sender or the content.
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
ofmm_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 [email protected] # 4.6Cc Yong Wu <[email protected]>Acked-by Robin Murphy <[email protected]>Signed-off-by Johan Hovold <[email protected]>--- drivers/iommu/mtk_iommu.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-)drivers/iommu/mtk_iommu.c +0
-0@@ -1216,13 +1216,17 @@ static int mtk_iommu_mm_dts_parse(structdevice *dev, struct
component_match**mplatform_device_put(plarbdev); }
drivers/iommu/mtk_iommu.c static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m · line 1221
Quoted 5 lines
drivers/iommu/mtk_iommu.c +3
-2-if (!frst_avail_smicomm_node) -return -EINVAL ; +if (!frst_avail_smicomm_node) { + ret = -EINVAL ; + goto err_larbdev_put;
There already is a "platform_device_put(plarbdev);" at the end of "for" loop, then no need put_device for it outside the "for" loop or outside this function?
Thanks.
Quoted 54 lines
drivers/iommu/mtk_iommu.c +7
-3+ } 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 **mplatform_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;drivers/iommu/mtk_iommu.c +5
-1@@ -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;drivers/iommu/mtk_iommu.c +3
-0@@ -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
Re: [PATCH v2 06/14] iommu/mediatek: fix device leaks on probe()
Quoted 15 lines · Yong Wu (吴勇)
Quoted 13 lines · 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
ofmm_dts_parse") fixed the leaks in a couple of error paths, but the references are still leaking on success and late failures.(quoted diff) +0
-0@@ -1216,13 +1216,17 @@ static int mtk_iommu_mm_dts_parse(structdevice *dev, struct
component_match**mplatform_device_put(plarbdev); }
Quoted 9 lines
drivers/iommu/mtk_iommu.c static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m · line 1221
Quoted 5 lines
drivers/iommu/mtk_iommu.c +3
-2-if (!frst_avail_smicomm_node) -return -EINVAL ; +if (!frst_avail_smicomm_node) { + ret = -EINVAL ; + goto err_larbdev_put;There already is a "
platform_device_put(plarbdev);" at the end of "for" loop, then no needput_devicefor it outside the "for" loop or outside this function?
You're right, thanks for catching that.
But this means that we have an existing potential use-after-free as if, for example, the driver probe defers we would put the reference to any previously looked up larbs twice.
I've just sent a v3 which fixes this by dropping the platform_device_put() after successful lookup as it is expected that the driver keeps the references while it uses the larb devices:
https://lore.kernel.org/lkml/[email protected]/
Johan
PATCH v2 7/14 iommu/mediatek: simplify dt parsing error handling
In reply to Johan HovoldAs 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'.
--- drivers/iommu/mtk_iommu.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
drivers/iommu/mtk_iommu.c +3 -4
@@ -1240,11 +1240,10 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **mreturn 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; }
Signature · 1 line
2.49.1
PATCH v2 8/14 iommu/mediatek-v1: fix device leak on probe_device()
In reply to Johan HovoldMake sure to drop the reference taken to the iommu platform device when looking up its driver data during probe_device().
--- drivers/iommu/mtk_iommu_v1.c | 2 ++ 1 file changed, 2 insertions(+)
drivers/iommu/mtk_iommu_v1.c +2 -0
@@ -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 );
Signature · 1 line
2.49.1
Re: [PATCH v2 08/14] iommu/mediatek-v1: fix device leak on probe_device()
Quoted 8 lines · Johan Hovold
External email : Please do not click links or open attachments until you have verified the sender or the content.
Make sure to drop the reference taken to the iommu platform device when
looking up its driver data duringprobe_device().
Quoted 6 lines
Fixes b17336c55d89 ("iommu/mediatek: add support for mtk iommugeneration one HW")
Cc [email protected] # 4.8Cc Honghui Zhang <[email protected]>Acked-by Robin Murphy <[email protected]>Signed-off-by Johan Hovold <[email protected]>
PATCH v2 9/14 iommu/mediatek-v1: fix device leaks on probe()
In reply to Johan HovoldMake sure to drop the references taken to the larb devices during probe on probe failure (e.g. probe deferral) and on driver unbind.
--- drivers/iommu/mtk_iommu_v1.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-)
drivers/iommu/mtk_iommu_v1.c +18 -5
@@ -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)
Signature · 1 line
2.49.1
PATCH v2 10/14 iommu/mediatek-v1: add missing larb count sanity check
In reply to Johan HovoldAdd the missing larb count sanity check to avoid writing beyond a fixed sized array in case of a malformed devicetree.
--- drivers/iommu/mtk_iommu_v1.c | 3 +++ 1 file changed, 3 insertions(+)
drivers/iommu/mtk_iommu_v1.c +3 -0
@@ -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;
Signature · 1 line
2.49.1
Re: [PATCH v2 10/14] iommu/mediatek-v1: add missing larb count sanity check
Quoted 10 lines · Johan Hovold
External email : Please do not click links or open attachments until you have verified the sender or the content.
Add the missing larb count sanity check to avoid writing beyond a fixed
sized array in case of a malformed devicetree.Acked-by Robin Murphy <[email protected]>Signed-off-by Johan Hovold <[email protected]>
PATCH v2 11/14 iommu/omap: fix device leaks on probe_device()
In reply to Johan HovoldMake sure to drop the references taken to the iommu platform devices when looking up their driver data during probe_device().
Note that the arch data device pointer added by commit 604629bcb505 ("iommu/omap: add support for late attachment of iommu devices") has never been used. Remove it to underline that the references are not needed.
--- drivers/iommu/omap-iommu.c | 2 +- drivers/iommu/omap-iommu.h | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-)
drivers/iommu/omap-iommu.c +1 -1
@@ -1675,6 +1675,7 @@ static struct iommu_device *omap_iommu_probe_device(struct device *dev) } oiommu =platform_get_drvdata (pdev); +put_device (&pdev->dev);if (!oiommu) {of_node_put (np);kfree (arch_data);
@@ -1682,7 +1683,6 @@ static struct iommu_device *omap_iommu_probe_device(struct device *dev) } tmp->iommu_dev = oiommu; - tmp->dev = &pdev->dev;of_node_put (np); }
drivers/iommu/omap-iommu.h +0 -2
@@ -88,7 +88,6 @@ struct omap_iommu {/** * struct omap_iommu_arch_data - omap iommu private data * @iommu_dev: handle of the OMAP iommu device -* @dev: handle of the iommu device * * This is an omap iommu private data object, which binds an iommu user * to its iommu device. This object should be placed at the iommu user's
@@ -97,7 +96,6 @@ struct omap_iommu {*/ struct omap_iommu_arch_data {struct omap_iommu *iommu_dev; -struct device *dev; };struct cr_regs {
Signature · 1 line
2.49.1
PATCH v2 12/14 iommu/omap: simplify probe_device() error handling
In reply to Johan HovoldSimplify the probe_device() error handling by dropping the iommu OF node reference sooner.
--- drivers/iommu/omap-iommu.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
drivers/iommu/omap-iommu.c +1 -4
@@ -1668,8 +1668,8 @@ 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);kfree (arch_data);return ERR_PTR (-ENODEV ); }
@@ -1677,14 +1677,11 @@ static struct iommu_device *omap_iommu_probe_device(struct device *dev) oiommu =platform_get_drvdata (pdev);put_device (&pdev->dev);if (!oiommu) { -of_node_put (np);kfree (arch_data);return ERR_PTR (-EINVAL ); } tmp->iommu_dev = oiommu; - -of_node_put (np); }dev_iommu_priv_set (dev, arch_data);
Signature · 1 line
2.49.1
PATCH v2 13/14 iommu/sun50i: fix device leak on of_xlate()
In reply to Johan HovoldMake sure to drop the reference taken to the iommu platform device when looking up its driver data during of_xlate().
--- drivers/iommu/sun50i-iommu.c | 2 ++ 1 file changed, 2 insertions(+)
drivers/iommu/sun50i-iommu.c +2 -0
@@ -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 ); }
Signature · 1 line
2.49.1
PATCH v2 14/14 iommu/tegra: fix device leak on probe_device()
In reply to Johan HovoldMake sure to drop the reference taken to the iommu platform device when looking up its driver data during probe_device().
Note that commit 9826e393e4a8 ("iommu/tegra-smmu: Fix missing put_device() call in tegra_smmu_find") fixed the leak in an error path, but the reference is still leaking on success.
--- drivers/iommu/tegra-smmu.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
drivers/iommu/tegra-smmu.c +2 -3
@@ -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; }
Signature · 1 line
2.49.1
Re: [PATCH v2 14/14] iommu/tegra: fix device leak on probe_device()
Quoted 23 lines · Johan Hovold
Make sure to drop the reference taken to the iommu platform device when looking up its driver data during
probe_device().Note that commit
9826e393e4a8("iommu/tegra-smmu: Fix missingput_device()call integra_smmu_find") fixed the leak in an error path, but the reference is still leaking on success.Fixes 891846516317 ("memory: Add NVIDIA Tegra memory controller support")Cc [email protected] # 3.19: 9826e393e4a8Cc Thierry Reding <[email protected]>Cc Miaoqian Lin <[email protected]>Acked-by Robin Murphy <[email protected]>Signed-off-by Johan Hovold <[email protected]>--- drivers/iommu/tegra-smmu.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)drivers/iommu/tegra-smmu.c +0
-0@@ -830,10 +830,9 @@ static struct tegra_smmu *tegra_smmu_find(struct device_node *np)return NULL ;
drivers/iommu/tegra-smmu.c static struct tegra_smmu *tegra_smmu_find(struct device_node *np) · line 837
Quoted 10 lines
drivers/iommu/tegra-smmu.c +2
-3mc =platform_get_drvdata (pdev); -if (!mc) { -put_device (&pdev->dev); +put_device (&pdev->dev); +if (!mc)return NULL ; - }return mc->smmu;
pdev->dev is what's backing mc, so if we use put_device() here, then the MC could go away at any time, right?
So the goal here was to make sure that the MC stays around during the entire lifetime of the IOMMU attachment. We don't currently release that reference, ever, so there is a leak, but wouldn't it be more appropriate to release it in a .release_device implementation?
Thierry
Re: [PATCH v2 14/14] iommu/tegra: fix device leak on probe_device()
Quoted 13 lines · Thierry Reding
Quoted 11 lines · Johan Hovold
drivers/iommu/tegra-smmu.c +2
-3@@ -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 ; - }
Quoted 4 lines
drivers/iommu/tegra-smmu.c static struct tegra_smmu *tegra_smmu_find(struct device_node *np) · line 837
Quoted 1 line
drivers/iommu/tegra-smmu.c +0
-0return mc->smmu;
pdev->devis what's backing mc, so if we useput_device()here, then the MC could go away at any time, right?
Holding a reference to a device does not prevent its driver data from going away so there is no point in keeping the reference.
But from what I can tell, you don't need to worry about that anyway since it's the memory controller driver that registers the iommu (and the driver can't be unbound).
Johan
Thierry Reding
Acked-byRe: [PATCH v2 14/14] iommu/tegra: fix device leak on probe_device()
Quoted 19 lines · Johan Hovold
Quoted 13 lines · Thierry Reding
Quoted 11 lines · Johan Hovold
drivers/iommu/tegra-smmu.c +2
-3@@ -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 ; - }Quoted 4 lines
drivers/iommu/tegra-smmu.c static struct tegra_smmu *tegra_smmu_find(struct device_node *np) · line 837
Quoted 1 line
drivers/iommu/tegra-smmu.c +0
-0return mc->smmu;
pdev->devis what's backing mc, so if we useput_device()here, then the MC could go away at any time, right?
Quoted 6 lines
Holding a reference to a device does not prevent its driver data from going away so there is no point in keeping the reference.
But from what I can tell, you don't need to worry about that anyway since it's the memory controller driver that registers the iommu (and the driver can't be unbound).
That's true. It'd be nice to at least conceptually do the right thing here, but not sure it's worth it. As you said, driver data going away would need special handling and it's not even clear what that would mean in terms of the clients...