LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] soc: fsl: guts: Fix a resource leak in the error handling path of 'fsl_guts_probe()'
@ 2021-08-18 21:21 Christophe JAILLET
  2021-10-22  0:26 ` Li Yang
  0 siblings, 1 reply; 3+ messages in thread
From: Christophe JAILLET @ 2021-08-18 21:21 UTC (permalink / raw)
  To: leoyang.li
  Cc: linuxppc-dev, linux-arm-kernel, linux-kernel, kernel-janitors,
	Christophe JAILLET

If an error occurs after 'of_find_node_by_path()', the reference taken for
'root' will never be released and some memory will leak.

Instead of adding an error handling path and modifying all the
'return -SOMETHING' into 'goto errorpath', use 'devm_add_action_or_reset()'
to release the reference when needed.

Simplify the remove function accordingly.

As an extra benefit, the 'root' global variable can now be removed as well.

Fixes: 3c0d64e867ed ("soc: fsl: guts: reuse machine name from device tree")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
Compile tested only
---
 drivers/soc/fsl/guts.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/soc/fsl/guts.c b/drivers/soc/fsl/guts.c
index d5e9a5f2c087..4d9476c7b87c 100644
--- a/drivers/soc/fsl/guts.c
+++ b/drivers/soc/fsl/guts.c
@@ -28,7 +28,6 @@ struct fsl_soc_die_attr {
 static struct guts *guts;
 static struct soc_device_attribute soc_dev_attr;
 static struct soc_device *soc_dev;
-static struct device_node *root;
 
 
 /* SoC die attribute definition for QorIQ platform */
@@ -136,14 +135,23 @@ static u32 fsl_guts_get_svr(void)
 	return svr;
 }
 
+static void fsl_guts_put_root(void *data)
+{
+	struct device_node *root = data;
+
+	of_node_put(root);
+}
+
 static int fsl_guts_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
 	struct device *dev = &pdev->dev;
+	struct device_node *root;
 	struct resource *res;
 	const struct fsl_soc_die_attr *soc_die;
 	const char *machine;
 	u32 svr;
+	int ret;
 
 	/* Initialize guts */
 	guts = devm_kzalloc(dev, sizeof(*guts), GFP_KERNEL);
@@ -159,6 +167,10 @@ static int fsl_guts_probe(struct platform_device *pdev)
 
 	/* Register soc device */
 	root = of_find_node_by_path("/");
+	ret = devm_add_action_or_reset(dev, fsl_guts_put_root, root);
+	if (ret)
+		return ret;
+
 	if (of_property_read_string(root, "model", &machine))
 		of_property_read_string_index(root, "compatible", 0, &machine);
 	if (machine)
@@ -197,7 +209,7 @@ static int fsl_guts_probe(struct platform_device *pdev)
 static int fsl_guts_remove(struct platform_device *dev)
 {
 	soc_device_unregister(soc_dev);
-	of_node_put(root);
+
 	return 0;
 }
 
-- 
2.30.2


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] soc: fsl: guts: Fix a resource leak in the error handling path of 'fsl_guts_probe()'
  2021-08-18 21:21 [PATCH] soc: fsl: guts: Fix a resource leak in the error handling path of 'fsl_guts_probe()' Christophe JAILLET
@ 2021-10-22  0:26 ` Li Yang
  2021-10-22 20:16   ` Tyrel Datwyler
  0 siblings, 1 reply; 3+ messages in thread
From: Li Yang @ 2021-10-22  0:26 UTC (permalink / raw)
  To: Christophe JAILLET
  Cc: linuxppc-dev,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, lkml,
	kernel-janitors

On Wed, Aug 18, 2021 at 4:23 PM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:
>
> If an error occurs after 'of_find_node_by_path()', the reference taken for
> 'root' will never be released and some memory will leak.

Thanks for finding this.  This truly is a problem.

>
> Instead of adding an error handling path and modifying all the
> 'return -SOMETHING' into 'goto errorpath', use 'devm_add_action_or_reset()'
> to release the reference when needed.
>
> Simplify the remove function accordingly.
>
> As an extra benefit, the 'root' global variable can now be removed as well.
>
> Fixes: 3c0d64e867ed ("soc: fsl: guts: reuse machine name from device tree")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> Compile tested only
> ---
>  drivers/soc/fsl/guts.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/soc/fsl/guts.c b/drivers/soc/fsl/guts.c
> index d5e9a5f2c087..4d9476c7b87c 100644
> --- a/drivers/soc/fsl/guts.c
> +++ b/drivers/soc/fsl/guts.c
> @@ -28,7 +28,6 @@ struct fsl_soc_die_attr {
>  static struct guts *guts;
>  static struct soc_device_attribute soc_dev_attr;
>  static struct soc_device *soc_dev;
> -static struct device_node *root;
>
>
>  /* SoC die attribute definition for QorIQ platform */
> @@ -136,14 +135,23 @@ static u32 fsl_guts_get_svr(void)
>         return svr;
>  }
>
> +static void fsl_guts_put_root(void *data)
> +{
> +       struct device_node *root = data;
> +
> +       of_node_put(root);
> +}
> +
>  static int fsl_guts_probe(struct platform_device *pdev)
>  {
>         struct device_node *np = pdev->dev.of_node;
>         struct device *dev = &pdev->dev;
> +       struct device_node *root;
>         struct resource *res;
>         const struct fsl_soc_die_attr *soc_die;
>         const char *machine;
>         u32 svr;
> +       int ret;
>
>         /* Initialize guts */
>         guts = devm_kzalloc(dev, sizeof(*guts), GFP_KERNEL);
> @@ -159,6 +167,10 @@ static int fsl_guts_probe(struct platform_device *pdev)
>
>         /* Register soc device */
>         root = of_find_node_by_path("/");
> +       ret = devm_add_action_or_reset(dev, fsl_guts_put_root, root);
> +       if (ret)
> +               return ret;

We probably only need to hold the reference when we do get "machine"
from the device tree, otherwise we can put it directly.

Or maybe we just maintain a local copy of string machine which means
we can release the reference right away?

> +
>         if (of_property_read_string(root, "model", &machine))
>                 of_property_read_string_index(root, "compatible", 0, &machine);
>         if (machine)
> @@ -197,7 +209,7 @@ static int fsl_guts_probe(struct platform_device *pdev)
>  static int fsl_guts_remove(struct platform_device *dev)
>  {
>         soc_device_unregister(soc_dev);
> -       of_node_put(root);
> +
>         return 0;
>  }
>
> --
> 2.30.2
>

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] soc: fsl: guts: Fix a resource leak in the error handling path of 'fsl_guts_probe()'
  2021-10-22  0:26 ` Li Yang
@ 2021-10-22 20:16   ` Tyrel Datwyler
  0 siblings, 0 replies; 3+ messages in thread
From: Tyrel Datwyler @ 2021-10-22 20:16 UTC (permalink / raw)
  To: Li Yang, Christophe JAILLET
  Cc: kernel-janitors, linuxppc-dev, lkml,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE

On 10/21/21 5:26 PM, Li Yang wrote:
> On Wed, Aug 18, 2021 at 4:23 PM Christophe JAILLET
> <christophe.jaillet@wanadoo.fr> wrote:
>>
>> If an error occurs after 'of_find_node_by_path()', the reference taken for
>> 'root' will never be released and some memory will leak.
> 
> Thanks for finding this.  This truly is a problem.
> 
>>
>> Instead of adding an error handling path and modifying all the
>> 'return -SOMETHING' into 'goto errorpath', use 'devm_add_action_or_reset()'
>> to release the reference when needed.
>>
>> Simplify the remove function accordingly.
>>
>> As an extra benefit, the 'root' global variable can now be removed as well.
>>
>> Fixes: 3c0d64e867ed ("soc: fsl: guts: reuse machine name from device tree")
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>> Compile tested only
>> ---
>>  drivers/soc/fsl/guts.c | 16 ++++++++++++++--
>>  1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/soc/fsl/guts.c b/drivers/soc/fsl/guts.c
>> index d5e9a5f2c087..4d9476c7b87c 100644
>> --- a/drivers/soc/fsl/guts.c
>> +++ b/drivers/soc/fsl/guts.c
>> @@ -28,7 +28,6 @@ struct fsl_soc_die_attr {
>>  static struct guts *guts;
>>  static struct soc_device_attribute soc_dev_attr;
>>  static struct soc_device *soc_dev;
>> -static struct device_node *root;
>>
>>
>>  /* SoC die attribute definition for QorIQ platform */
>> @@ -136,14 +135,23 @@ static u32 fsl_guts_get_svr(void)
>>         return svr;
>>  }
>>
>> +static void fsl_guts_put_root(void *data)
>> +{
>> +       struct device_node *root = data;
>> +
>> +       of_node_put(root);
>> +}
>> +
>>  static int fsl_guts_probe(struct platform_device *pdev)
>>  {
>>         struct device_node *np = pdev->dev.of_node;
>>         struct device *dev = &pdev->dev;
>> +       struct device_node *root;
>>         struct resource *res;
>>         const struct fsl_soc_die_attr *soc_die;
>>         const char *machine;
>>         u32 svr;
>> +       int ret;
>>
>>         /* Initialize guts */
>>         guts = devm_kzalloc(dev, sizeof(*guts), GFP_KERNEL);
>> @@ -159,6 +167,10 @@ static int fsl_guts_probe(struct platform_device *pdev)
>>
>>         /* Register soc device */
>>         root = of_find_node_by_path("/");
>> +       ret = devm_add_action_or_reset(dev, fsl_guts_put_root, root);
>> +       if (ret)
>> +               return ret;
> 
> We probably only need to hold the reference when we do get "machine"
> from the device tree, otherwise we can put it directly.

To be pedantic since you are using the a properties string value of the root
node directly you need to hold the reference of that node for the lifetime of
the device. Realistically, its not like the root node and its model/compatible
properties are going to need to get modified at runtime so you could also argue
that holding the reference is unnecessary.

> 
> Or maybe we just maintain a local copy of string machine which means
> we can release the reference right away?

Looks like that is the original behavior that commit 3c0d64e867ed changed.
Although it looks like that behavior neglected to handle a memory allocation
failure during the string copy. How much memory is really being saved by not
keeping a local copy? Maybe revert 3c0d64e867ed and add the memory allocation check?

-Tyrel

> 
>> +
>>         if (of_property_read_string(root, "model", &machine))
>>                 of_property_read_string_index(root, "compatible", 0, &machine);
>>         if (machine)
>> @@ -197,7 +209,7 @@ static int fsl_guts_probe(struct platform_device *pdev)
>>  static int fsl_guts_remove(struct platform_device *dev)
>>  {
>>         soc_device_unregister(soc_dev);
>> -       of_node_put(root);
>> +
>>         return 0;
>>  }
>>
>> --
>> 2.30.2
>>


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-10-22 20:16 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-18 21:21 [PATCH] soc: fsl: guts: Fix a resource leak in the error handling path of 'fsl_guts_probe()' Christophe JAILLET
2021-10-22  0:26 ` Li Yang
2021-10-22 20:16   ` Tyrel Datwyler

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).