| Message ID | 1555468879-34684-1-git-send-email-wen.yang99@zte.com.cn (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Commit | 1eb8e4e2b35b4d83cc40cbf379493490741eb5b8 |
| Headers | show |
| Series | iommu/mediatek: fix leaked of_node references | expand |
On 17/04/2019 04:41, Wen Yang wrote: > The call to of_parse_phandle returns a node pointer with refcount > incremented thus it must be explicitly decremented after the last > usage. > > 581 static int mtk_iommu_probe(struct platform_device *pdev) > 582 { > ... > 626 for (i = 0; i < larb_nr; i++) { > 627 struct device_node *larbnode; > ... > 631 larbnode = of_parse_phandle(...); > 632 if (!larbnode) > 633 return -EINVAL; > 634 > 635 if (!of_device_is_available(larbnode)) > 636 continue; ---> leaked here > 637 > ... > 643 if (!plarbdev) > 644 return -EPROBE_DEFER; ---> leaked here > ... > 647 component_match_add_release(dev, &match, release_of, > 648 compare_of, larbnode); > ---> release_of will call of_node_put > 649 } > ... > 650 > > Detected by coccinelle with the following warnings: > ./drivers/iommu/mtk_iommu.c:644:3-9: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 631, but without a corresponding object release within this function. > > Signed-off-by: Wen Yang <wen.yang99@zte.com.cn> > Cc: Joerg Roedel <joro@8bytes.org> > Cc: Matthias Brugger <matthias.bgg@gmail.com> > Cc: iommu@lists.linux-foundation.org > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-mediatek@lists.infradead.org > Cc: linux-kernel@vger.kernel.org Reviewed-by: Matthias Brugger <mbrugger@suse.com> > --- > drivers/iommu/mtk_iommu.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c > index de3e022..b66d11b 100644 > --- a/drivers/iommu/mtk_iommu.c > +++ b/drivers/iommu/mtk_iommu.c > @@ -632,16 +632,20 @@ static int mtk_iommu_probe(struct platform_device *pdev) > if (!larbnode) > return -EINVAL; > > - if (!of_device_is_available(larbnode)) > + if (!of_device_is_available(larbnode)) { > + of_node_put(larbnode); > continue; > + } > > ret = of_property_read_u32(larbnode, "mediatek,larb-id", &id); > if (ret)/* The id is consecutive if there is no this property */ > id = i; > > plarbdev = of_find_device_by_node(larbnode); > - if (!plarbdev) > + if (!plarbdev) { > + of_node_put(larbnode); > return -EPROBE_DEFER; > + } > data->smi_imu.larb_imu[id].dev = &plarbdev->dev; > > component_match_add_release(dev, &match, release_of, >
> The call to of_parse_phandle returns a node pointer with refcount > incremented thus it must be explicitly decremented after the last > usage. Can a splitting of this information into two sentences help? > 581 static int mtk_iommu_probe(struct platform_device *pdev) > 582 { > ... I suggest to reconsider such a commit description once more. Would it be better to mention that another function call should be added in two if branches so that the exception handling would be completed (instead of copying source code where a software update should become clear also from the provided diff hunk)? Regards, Markus
> The call to of_parse_phandle returns a node pointer with refcount > incremented thus it must be explicitly decremented after the last > usage. Can a splitting of this information into two sentences help? > 581 static int mtk_iommu_probe(struct platform_device *pdev) > 582 { > ... I suggest to reconsider such a commit description once more. Would it be better to mention that another function call should be added in two if branches so that the exception handling would be completed (instead of copying source code where a software update should become clear also from the provided diff hunk)? Regards, Markus
On Wed, Apr 17, 2019 at 10:41:19AM +0800, Wen Yang wrote: > drivers/iommu/mtk_iommu.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) Applied, thanks.
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c index de3e022..b66d11b 100644 --- a/drivers/iommu/mtk_iommu.c +++ b/drivers/iommu/mtk_iommu.c @@ -632,16 +632,20 @@ static int mtk_iommu_probe(struct platform_device *pdev) if (!larbnode) return -EINVAL; - if (!of_device_is_available(larbnode)) + if (!of_device_is_available(larbnode)) { + of_node_put(larbnode); continue; + } ret = of_property_read_u32(larbnode, "mediatek,larb-id", &id); if (ret)/* The id is consecutive if there is no this property */ id = i; plarbdev = of_find_device_by_node(larbnode); - if (!plarbdev) + if (!plarbdev) { + of_node_put(larbnode); return -EPROBE_DEFER; + } data->smi_imu.larb_imu[id].dev = &plarbdev->dev; component_match_add_release(dev, &match, release_of,
The call to of_parse_phandle returns a node pointer with refcount incremented thus it must be explicitly decremented after the last usage. 581 static int mtk_iommu_probe(struct platform_device *pdev) 582 { ... 626 for (i = 0; i < larb_nr; i++) { 627 struct device_node *larbnode; ... 631 larbnode = of_parse_phandle(...); 632 if (!larbnode) 633 return -EINVAL; 634 635 if (!of_device_is_available(larbnode)) 636 continue; ---> leaked here 637 ... 643 if (!plarbdev) 644 return -EPROBE_DEFER; ---> leaked here ... 647 component_match_add_release(dev, &match, release_of, 648 compare_of, larbnode); ---> release_of will call of_node_put 649 } ... 650 Detected by coccinelle with the following warnings: ./drivers/iommu/mtk_iommu.c:644:3-9: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 631, but without a corresponding object release within this function. Signed-off-by: Wen Yang <wen.yang99@zte.com.cn> Cc: Joerg Roedel <joro@8bytes.org> Cc: Matthias Brugger <matthias.bgg@gmail.com> Cc: iommu@lists.linux-foundation.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-mediatek@lists.infradead.org Cc: linux-kernel@vger.kernel.org --- drivers/iommu/mtk_iommu.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)