power: supply: Fix additional refcount leak in rk817_charger_probe

Dan Carpenter reports that the Smatch static checker warning has found that there is another refcount leak in the probe function. While of_node_put() was added in one of the return paths, it should in fact be added for ALL return paths that return an error.

Changes Since V1:
- Use devm_add_action_or_reset() to call of_node_put() instead of calling it in every single error path from the probe() function.

[...]

From: Chris Morgan <[email protected]>

Dan Carpenter reports that the Smatch static checker warning has found that there is another refcount leak in the probe function. While of_node_put() was added in one of the return paths, it should in fact be added for ALL return paths that return an error.

Changes Since V1:
- Use devm_add_action_or_reset() to call of_node_put() instead of calling it in every single error path from the probe() function.

Fixes 54c03bfd094f ("power: supply: Fix refcount leak in rk817_charger_probe")

Reported-by Dan Carpenter <[email protected]>

Closes https://lore.kernel.org/linux-pm/[email protected]/ open here

Signed-off-by Chris Morgan <[email protected]>

---
 drivers/power/supply/rk817_charger.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

drivers/power/supply/rk817_charger.c +7 -0

index 8328bcea1a29..708689d34959 100644
@@ -1045,6 +1045,13 @@ static void rk817_charging_monitor(struct work_struct *work)
 	queue_delayed_work(system_wq, &charger->work, msecs_to_jiffies(8000));
 }
 
+static void rk817_cleanup_node(void *data)
+{
+	struct device_node *node = data;
+
+	of_node_put(node);
+}
+
 static int rk817_charger_probe(struct platform_device *pdev)
 {
 	struct rk808 *rk808 = dev_get_drvdata(pdev->dev.parent);

drivers/power/supply/rk817_charger.c +5 -1

-		return -ENOMEM;
+		return ret;
 	}
 
+	charger = devm_kzalloc(&pdev->dev, sizeof(*charger), GFP_KERNEL);
+	if (!charger)
+		return -ENOMEM;
+
 	charger->rk808 = rk808;
 
 	charger->dev = &pdev->dev;

Signature · 1 line

2.34.1

Hi,

As you saw I applied this, but I did some modifications:

Quoted 10 lines

From: Chris Morgan <[email protected]>

Dan Carpenter reports that the Smatch static checker warning has found that there is another refcount leak in the probe function. While of_node_put() was added in one of the return paths, it should in fact be added for ALL return paths that return an error.

Changes Since V1:
- Use devm_add_action_or_reset() to call of_node_put() instead of calling it in every single error path from the probe() function.

^ this should be below the ---

Quoted 28 lines

Fixes 54c03bfd094f ("power: supply: Fix refcount leak in rk817_charger_probe")

Reported-by Dan Carpenter <[email protected]>

Closes https://lore.kernel.org/linux-pm/[email protected]/ open here

Signed-off-by Chris Morgan <[email protected]>

---
 drivers/power/supply/rk817_charger.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

drivers/power/supply/rk817_charger.c +7 -0

index 8328bcea1a29..708689d34959 100644
@@ -1045,6 +1045,13 @@ static void rk817_charging_monitor(struct work_struct *work)
 	queue_delayed_work(system_wq, &charger->work, msecs_to_jiffies(8000));
 }
 
+static void rk817_cleanup_node(void *data)
+{
+	struct device_node *node = data;
+
+	of_node_put(node);
+}
+
 static int rk817_charger_probe(struct platform_device *pdev)
 {
 	struct rk808 *rk808 = dev_get_drvdata(pdev->dev.parent);

drivers/power/supply/rk817_charger.c static int rk817_charger_probe(struct platform_device *pdev) · line 1073

Quoted 8 lines

drivers/power/supply/rk817_charger.c +2 -2

@@ -1061,12 +1068,16 @@ static int rk817_charger_probe(struct platform_device *pdev)
 	if (!node)
 		return -ENODEV;
 
-	charger = devm_kzalloc(&pdev->dev, sizeof(*charger), GFP_KERNEL);
-	if (!charger) {
+	ret = devm_add_action_or_reset(&pdev->dev, rk817_cleanup_node, node);
+	if (ret) {
 		of_node_put(node);

devm_add_action_or_reset() calls the action on failure (that's the '_or_reset' part). So this of_node_put() is wrong.

Thanks,

-- Sebastian

Quoted 14 lines

drivers/power/supply/rk817_charger.c +5 -1

-		return -ENOMEM;
+		return ret;
 	}
 
+	charger = devm_kzalloc(&pdev->dev, sizeof(*charger), GFP_KERNEL);
+	if (!charger)
+		return -ENOMEM;
+
 	charger->rk808 = rk808;
 
 	charger->dev = &pdev->dev;

Signature · 1 line

2.34.1