| Message ID | 20241216102609.760571-1-joe@pf.is.s.u-tokyo.ac.jp (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | dmaengine: ti: edma: fix OF node reference leaks in edma_driver | expand |
On Mon, Dec 16, 2024 at 07:26:09PM +0900, Joe Hattori wrote: > The .probe() of edma_driver calls of_parse_phandle_with_fixed_args() but > does not release the obtained OF nodes. Thus implement > edma_cleanup_tc_list(), which releases those OF nodes, and call it in > the error path of .probe() and in .remove(). > > This bug was found by an experimental static analysis tool that I am > developing. > > Fixes: 1be5336bc7ba ("dmaengine: edma: New device tree binding") > Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp> > --- > drivers/dma/ti/edma.c | 23 +++++++++++++++++++---- > 1 file changed, 19 insertions(+), 4 deletions(-) > > diff --git a/drivers/dma/ti/edma.c b/drivers/dma/ti/edma.c > index 343e986e66e7..e6eee6cfa94a 100644 > --- a/drivers/dma/ti/edma.c > +++ b/drivers/dma/ti/edma.c > @@ -2279,6 +2279,18 @@ static struct dma_chan *of_edma_xlate(struct of_phandle_args *dma_spec, > > static bool edma_filter_fn(struct dma_chan *chan, void *param); > > +static void edma_cleanup_tc_list(struct edma_cc *ecc) > +{ > + int i; > + > + if (!ecc->tc_list) > + return; > + for (i = 0; i < ecc->num_tc; i++) { > + if (ecc->tc_list[i].node) > + of_node_put(ecc->tc_list[i].node); No need for this NULL check. In a way, it would be cleanest to just get rid of the .node struct member. We never use it. We could just save the .id and call of_node_put() right away in probe. That's really how it's normally done. regards, dan carpenter
On 12/17/24 23:21, Dan Carpenter wrote: > On Mon, Dec 16, 2024 at 07:26:09PM +0900, Joe Hattori wrote: >> The .probe() of edma_driver calls of_parse_phandle_with_fixed_args() but >> does not release the obtained OF nodes. Thus implement >> edma_cleanup_tc_list(), which releases those OF nodes, and call it in >> the error path of .probe() and in .remove(). >> >> This bug was found by an experimental static analysis tool that I am >> developing. >> >> Fixes: 1be5336bc7ba ("dmaengine: edma: New device tree binding") >> Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp> >> --- >> drivers/dma/ti/edma.c | 23 +++++++++++++++++++---- >> 1 file changed, 19 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/dma/ti/edma.c b/drivers/dma/ti/edma.c >> index 343e986e66e7..e6eee6cfa94a 100644 >> --- a/drivers/dma/ti/edma.c >> +++ b/drivers/dma/ti/edma.c >> @@ -2279,6 +2279,18 @@ static struct dma_chan *of_edma_xlate(struct of_phandle_args *dma_spec, >> >> static bool edma_filter_fn(struct dma_chan *chan, void *param); >> >> +static void edma_cleanup_tc_list(struct edma_cc *ecc) >> +{ >> + int i; >> + >> + if (!ecc->tc_list) >> + return; >> + for (i = 0; i < ecc->num_tc; i++) { >> + if (ecc->tc_list[i].node) >> + of_node_put(ecc->tc_list[i].node); > > No need for this NULL check. > > In a way, it would be cleanest to just get rid of the .node struct > member. We never use it. We could just save the .id and call > of_node_put() right away in probe. That's really how it's normally > done. Thank you for your review. Yes, makes sense. Fixed in the v2 patch, so please take a look at it. > > regards, > dan carpenter > Best, Joe
diff --git a/drivers/dma/ti/edma.c b/drivers/dma/ti/edma.c index 343e986e66e7..e6eee6cfa94a 100644 --- a/drivers/dma/ti/edma.c +++ b/drivers/dma/ti/edma.c @@ -2279,6 +2279,18 @@ static struct dma_chan *of_edma_xlate(struct of_phandle_args *dma_spec, static bool edma_filter_fn(struct dma_chan *chan, void *param); +static void edma_cleanup_tc_list(struct edma_cc *ecc) +{ + int i; + + if (!ecc->tc_list) + return; + for (i = 0; i < ecc->num_tc; i++) { + if (ecc->tc_list[i].node) + of_node_put(ecc->tc_list[i].node); + } +} + static int edma_probe(struct platform_device *pdev) { struct edma_soc_info *info = pdev->dev.platform_data; @@ -2460,10 +2472,10 @@ static int edma_probe(struct platform_device *pdev) goto err_reg1; } - for (i = 0;; i++) { + for (i = 0; i < ecc->num_tc; i++) { ret = of_parse_phandle_with_fixed_args(node, "ti,tptcs", 1, i, &tc_args); - if (ret || i == ecc->num_tc) + if (ret) break; ecc->tc_list[i].node = tc_args.np; @@ -2521,7 +2533,7 @@ static int edma_probe(struct platform_device *pdev) ret = dma_async_device_register(&ecc->dma_slave); if (ret) { dev_err(dev, "slave ddev registration failed (%d)\n", ret); - goto err_reg1; + goto err_put; } if (ecc->dma_memcpy) { @@ -2530,7 +2542,7 @@ static int edma_probe(struct platform_device *pdev) dev_err(dev, "memcpy ddev registration failed (%d)\n", ret); dma_async_device_unregister(&ecc->dma_slave); - goto err_reg1; + goto err_put; } } @@ -2541,6 +2553,8 @@ static int edma_probe(struct platform_device *pdev) return 0; +err_put: + edma_cleanup_tc_list(ecc); err_reg1: edma_free_slot(ecc, ecc->dummy_slot); err_disable_pm: @@ -2575,6 +2589,7 @@ static void edma_remove(struct platform_device *pdev) dma_async_device_unregister(&ecc->dma_slave); if (ecc->dma_memcpy) dma_async_device_unregister(ecc->dma_memcpy); + edma_cleanup_tc_list(ecc); edma_free_slot(ecc, ecc->dummy_slot); pm_runtime_put_sync(dev); pm_runtime_disable(dev);
The .probe() of edma_driver calls of_parse_phandle_with_fixed_args() but does not release the obtained OF nodes. Thus implement edma_cleanup_tc_list(), which releases those OF nodes, and call it in the error path of .probe() and in .remove(). This bug was found by an experimental static analysis tool that I am developing. Fixes: 1be5336bc7ba ("dmaengine: edma: New device tree binding") Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp> --- drivers/dma/ti/edma.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-)