diff mbox series

[V2] power: supply: Fix additional refcount leak in rk817_charger_probe

Message ID 20230920145644.57964-1-macroalpha82@gmail.com (mailing list archive)
State Handled Elsewhere, archived
Headers show
Series [V2] power: supply: Fix additional refcount leak in rk817_charger_probe | expand

Commit Message

Chris Morgan Sept. 20, 2023, 2:56 p.m. UTC 
From: Chris Morgan <macromorgan@hotmail.com>

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 <dan.carpenter@oracle.com>
Closes: https://lore.kernel.org/linux-pm/dc0bb0f8-212d-4be7-be69-becd2a3f9a80@kili.mountain/
Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
---
 drivers/power/supply/rk817_charger.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

Comments

Sebastian Reichel Sept. 20, 2023, 7:37 p.m. UTC | #1 
On Wed, 20 Sep 2023 09:56:44 -0500, Chris Morgan wrote:
> 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.
> 
> [...]

Applied, thanks!

[1/1] power: supply: Fix additional refcount leak in rk817_charger_probe
      commit: 488ef44c068e79752dba8eda0b75f524f111a695

Best regards,
Sebastian Reichel Sept. 20, 2023, 8:31 p.m. UTC | #2 
Hi,

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

On Wed, Sep 20, 2023 at 09:56:44AM -0500, Chris Morgan wrote:
> From: Chris Morgan <macromorgan@hotmail.com>
> 
> 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 ---

> 
> Fixes: 54c03bfd094f ("power: supply: Fix refcount leak in rk817_charger_probe")
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Closes: https://lore.kernel.org/linux-pm/dc0bb0f8-212d-4be7-be69-becd2a3f9a80@kili.mountain/
> Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
> ---
>  drivers/power/supply/rk817_charger.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/power/supply/rk817_charger.c b/drivers/power/supply/rk817_charger.c
> index 8328bcea1a29..708689d34959 100644
> --- a/drivers/power/supply/rk817_charger.c
> +++ b/drivers/power/supply/rk817_charger.c
> @@ -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);
> @@ -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

> -		return -ENOMEM;
> +		return ret;
>  	}
>  
> +	charger = devm_kzalloc(&pdev->dev, sizeof(*charger), GFP_KERNEL);
> +	if (!charger)
> +		return -ENOMEM;
> +
>  	charger->rk808 = rk808;
>  
>  	charger->dev = &pdev->dev;
> -- 
> 2.34.1
>
Chris Morgan Sept. 21, 2023, 1:18 p.m. UTC | #3 
On Wed, Sep 20, 2023 at 10:31:06PM +0200, Sebastian Reichel wrote:
> Hi,
> 
> As you saw I applied this, but I did some modifications:

Thank you. Still learning so I appreciate all the help.

Chris

> 
> On Wed, Sep 20, 2023 at 09:56:44AM -0500, Chris Morgan wrote:
> > From: Chris Morgan <macromorgan@hotmail.com>
> > 
> > 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 ---
> 
> > 
> > Fixes: 54c03bfd094f ("power: supply: Fix refcount leak in rk817_charger_probe")
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Closes: https://lore.kernel.org/linux-pm/dc0bb0f8-212d-4be7-be69-becd2a3f9a80@kili.mountain/
> > Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
> > ---
> >  drivers/power/supply/rk817_charger.c | 17 ++++++++++++++---
> >  1 file changed, 14 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/power/supply/rk817_charger.c b/drivers/power/supply/rk817_charger.c
> > index 8328bcea1a29..708689d34959 100644
> > --- a/drivers/power/supply/rk817_charger.c
> > +++ b/drivers/power/supply/rk817_charger.c
> > @@ -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);
> > @@ -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
> 
> > -		return -ENOMEM;
> > +		return ret;
> >  	}
> >  
> > +	charger = devm_kzalloc(&pdev->dev, sizeof(*charger), GFP_KERNEL);
> > +	if (!charger)
> > +		return -ENOMEM;
> > +
> >  	charger->rk808 = rk808;
> >  
> >  	charger->dev = &pdev->dev;
> > -- 
> > 2.34.1
> >
diff mbox series

Patch

diff --git a/drivers/power/supply/rk817_charger.c b/drivers/power/supply/rk817_charger.c
index 8328bcea1a29..708689d34959 100644
--- a/drivers/power/supply/rk817_charger.c
+++ b/drivers/power/supply/rk817_charger.c
@@ -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);
@@ -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);
-		return -ENOMEM;
+		return ret;
 	}
 
+	charger = devm_kzalloc(&pdev->dev, sizeof(*charger), GFP_KERNEL);
+	if (!charger)
+		return -ENOMEM;
+
 	charger->rk808 = rk808;
 
 	charger->dev = &pdev->dev;