Re: [PATCH] dma: qcom: hidma_mgmt: Add of_node_put() before goto

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 24/07/2019 09:05, Nishka Dasgupta wrote:
On 23/07/19 5:32 PM, Robin Murphy wrote:
On 23/07/2019 11:35, Nishka Dasgupta wrote:
Each iteration of for_each_available_child_of_node puts the previous
node, but in the case of a goto from the middle of the loop, there is
no put, thus causing a memory leak. Add an of_node_put before the
goto in 4 places.

Why not just add it once at the "out" label itself? (Consider the conditions for the loop terminating naturally)

If the loop terminates naturally then, as far as I understand, child will be put by the loop itself; then an extra of_node_put() under the out label would put the child node even though it has already been put. If I'm understanding this correctly (and I might not be) is it okay to decrement refcount more times that it is incremented?

Ah, but is it really the same thing being put both times? The loop *iterator* will indeed drop its reference on the last valid child node, but what's the actual termination condition, and thus the state afterwards? ;)

Robin.

And if you're cleaning up the refcounting here anyway then I'd also note that the reference held by the loop iterator makes the extra get/put inside that loop entirely redundant. It's always worth taking a look at the wider context rather than just blindly focusing on what a given script picks up - it's fairly rare that a piece of code has one obvious issue but is otherwise perfect.

Thank  you for pointing this out; I've added it in v2.

Thanking you,
Nishka
Robin.

Issue found with Coccinelle.

Signed-off-by: Nishka Dasgupta <nishkadg.linux@xxxxxxxxx>
---
  drivers/dma/qcom/hidma_mgmt.c | 13 ++++++++++---
  1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/qcom/hidma_mgmt.c b/drivers/dma/qcom/hidma_mgmt.c
index 3022d66e7a33..209adc6ceabe 100644
--- a/drivers/dma/qcom/hidma_mgmt.c
+++ b/drivers/dma/qcom/hidma_mgmt.c
@@ -362,16 +362,22 @@ static int __init hidma_mgmt_of_populate_channels(struct device_node *np)
          struct platform_device *new_pdev;
          ret = of_address_to_resource(child, 0, &res[0]);
-        if (!ret)
+        if (!ret) {
+            of_node_put(child);
              goto out;
+        }
          ret = of_address_to_resource(child, 1, &res[1]);
-        if (!ret)
+        if (!ret) {
+            of_node_put(child);
              goto out;
+        }
          ret = of_irq_to_resource(child, 0, &res[2]);
-        if (ret <= 0)
+        if (ret <= 0) {
+            of_node_put(child);
              goto out;
+        }
          memset(&pdevinfo, 0, sizeof(pdevinfo));
          pdevinfo.fwnode = &child->fwnode;
@@ -386,6 +392,7 @@ static int __init hidma_mgmt_of_populate_channels(struct device_node *np)
          new_pdev = platform_device_register_full(&pdevinfo);
          if (IS_ERR(new_pdev)) {
              ret = PTR_ERR(new_pdev);
+            of_node_put(child);
              goto out;
          }
          of_node_get(child);





[Index of Archives]     [Linux Kernel]     [Linux ARM (vger)]     [Linux ARM MSM]     [Linux Omap]     [Linux Arm]     [Linux Tegra]     [Fedora ARM]     [Linux for Samsung SOC]     [eCos]     [Linux PCI]     [Linux Fastboot]     [Gcc Help]     [Git]     [DCCP]     [IETF Announce]     [Security]     [Linux MIPS]     [Yosemite Campsites]

  Powered by Linux