[PATCH 1/7] drivers/macintosh/via-pmu-led.c: Add of_node_put to avoid memory leak

[PATCH 1/7] drivers/macintosh/via-pmu-led.c: Add of_node_put to avoid memory leak

[Julia Lawall]

Add a call to of_node_put in the error handling code following a call to
of_find_node_by_path.

The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@r exists@
local idexpression x;
expression E,E1;
statement S;
@@

*x = 
(of_find_node_by_path
|of_find_node_by_name
|of_find_node_by_phandle
|of_get_parent
|of_get_next_parent
|of_get_next_child
|of_find_compatible_node
|of_match_node
)(...);
...
if (x == NULL) S
<... when != x = E
*if (...) {
  ... when != of_node_put(x)
      when != if (...) { ... of_node_put(x); ... }
(
  return <+...x...+>;
|
*  return ...;
)
}
...>
of_node_put(x);
// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>

---
 drivers/macintosh/via-pmu-led.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/macintosh/via-pmu-led.c b/drivers/macintosh/via-pmu-led.c
index d242976..19c3718 100644
--- a/drivers/macintosh/via-pmu-led.c
+++ b/drivers/macintosh/via-pmu-led.c
@@ -92,8 +92,10 @@ static int __init via_pmu_led_init(void)
 	if (dt == NULL)
 		return -ENODEV;
 	model = of_get_property(dt, "model", NULL);
-	if (model == NULL)
+	if (model == NULL) {
+		of_node_put(dt);
 		return -ENODEV;
+	}
 	if (strncmp(model, "PowerBook", strlen("PowerBook")) != 0 &&
 	    strncmp(model, "iBook", strlen("iBook")) != 0 &&
 	    strcmp(model, "PowerMac7,2") != 0 &&

Re: [PATCH 1/7] drivers/macintosh/via-pmu-led.c: Add of_node_put to avoid memory leak

[walter harms]

Julia Lawall schrieb:
> Add a call to of_node_put in the error handling code following a call to
> of_find_node_by_path.
> 
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @r exists@
> local idexpression x;
> expression E,E1;
> statement S;
> @@
> 
> *x = 
> (of_find_node_by_path
> |of_find_node_by_name
> |of_find_node_by_phandle
> |of_get_parent
> |of_get_next_parent
> |of_get_next_child
> |of_find_compatible_node
> |of_match_node
> )(...);
> ...
> if (x == NULL) S
> <... when != x = E
> *if (...) {
>   ... when != of_node_put(x)
>       when != if (...) { ... of_node_put(x); ... }
> (
>   return <+...x...+>;
> |
> *  return ...;
> )
> }
> ...>
> of_node_put(x);
> // </smpl>
> 
> Signed-off-by: Julia Lawall <julia@diku.dk>
> 
> ---
>  drivers/macintosh/via-pmu-led.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/macintosh/via-pmu-led.c b/drivers/macintosh/via-pmu-led.c
> index d242976..19c3718 100644
> --- a/drivers/macintosh/via-pmu-led.c
> +++ b/drivers/macintosh/via-pmu-led.c
> @@ -92,8 +92,10 @@ static int __init via_pmu_led_init(void)
>  	if (dt == NULL)
>  		return -ENODEV;
>  	model = of_get_property(dt, "model", NULL);
> -	if (model == NULL)
> +	if (model == NULL) {
> +		of_node_put(dt);
>  		return -ENODEV;
> +	}
>  	if (strncmp(model, "PowerBook", strlen("PowerBook")) != 0 &&
>  	    strncmp(model, "iBook", strlen("iBook")) != 0 &&
>  	    strcmp(model, "PowerMac7,2") != 0 &&
> 

is there any rule that says when to use strncmp ? it seems perfecly valid to use strcpy here
(what is done in the last cmp).

re,
 wh

Re: [PATCH 1/7] drivers/macintosh/via-pmu-led.c: Add of_node_put to avoid memory leak

[Julia Lawall]

On Tue, 31 Aug 2010, walter harms wrote:

> 
> 
> Julia Lawall schrieb:
> > Add a call to of_node_put in the error handling code following a call to
> > of_find_node_by_path.
> > 
> > The semantic match that finds this problem is as follows:
> > (http://coccinelle.lip6.fr/)
> > 
> > // <smpl>
> > @r exists@
> > local idexpression x;
> > expression E,E1;
> > statement S;
> > @@
> > 
> > *x = 
> > (of_find_node_by_path
> > |of_find_node_by_name
> > |of_find_node_by_phandle
> > |of_get_parent
> > |of_get_next_parent
> > |of_get_next_child
> > |of_find_compatible_node
> > |of_match_node
> > )(...);
> > ...
> > if (x == NULL) S
> > <... when != x = E
> > *if (...) {
> >   ... when != of_node_put(x)
> >       when != if (...) { ... of_node_put(x); ... }
> > (
> >   return <+...x...+>;
> > |
> > *  return ...;
> > )
> > }
> > ...>
> > of_node_put(x);
> > // </smpl>
> > 
> > Signed-off-by: Julia Lawall <julia@diku.dk>
> > 
> > ---
> >  drivers/macintosh/via-pmu-led.c |    4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/macintosh/via-pmu-led.c b/drivers/macintosh/via-pmu-led.c
> > index d242976..19c3718 100644
> > --- a/drivers/macintosh/via-pmu-led.c
> > +++ b/drivers/macintosh/via-pmu-led.c
> > @@ -92,8 +92,10 @@ static int __init via_pmu_led_init(void)
> >  	if (dt == NULL)
> >  		return -ENODEV;
> >  	model = of_get_property(dt, "model", NULL);
> > -	if (model == NULL)
> > +	if (model == NULL) {
> > +		of_node_put(dt);
> >  		return -ENODEV;
> > +	}
> >  	if (strncmp(model, "PowerBook", strlen("PowerBook")) != 0 &&
> >  	    strncmp(model, "iBook", strlen("iBook")) != 0 &&
> >  	    strcmp(model, "PowerMac7,2") != 0 &&
> > 
> 
> is there any rule that says when to use strncmp ? it seems perfecly valid to use strcpy here
> (what is done in the last cmp).

Perhaps there are some characters after eg PowerBook that one doesn't want 
to compare with?

julia

Re: [PATCH 1/7] drivers/macintosh/via-pmu-led.c: Add of_node_put to avoid memory leak

[Grant Likely]

On Tue, Aug 31, 2010 at 06:08:19PM +0200, Julia Lawall wrote:
> On Tue, 31 Aug 2010, walter harms wrote:
> > > @@ -92,8 +92,10 @@ static int __init via_pmu_led_init(void)
> > >  	if (dt == NULL)
> > >  		return -ENODEV;
> > >  	model = of_get_property(dt, "model", NULL);
> > > -	if (model == NULL)
> > > +	if (model == NULL) {
> > > +		of_node_put(dt);
> > >  		return -ENODEV;
> > > +	}
> > >  	if (strncmp(model, "PowerBook", strlen("PowerBook")) != 0 &&
> > >  	    strncmp(model, "iBook", strlen("iBook")) != 0 &&
> > >  	    strcmp(model, "PowerMac7,2") != 0 &&
> > > 
> > 
> > is there any rule that says when to use strncmp ? it seems perfecly valid to use strcpy here
> > (what is done in the last cmp).
> 
> Perhaps there are some characters after eg PowerBook that one doesn't want 
> to compare with?

Yes.  The model property on powermacs always has the version number.   strncmp makes sure that *all* PowerBooks and iBooks are matched.

g.

Re: [PATCH 1/7] drivers/macintosh/via-pmu-led.c: Add of_node_put to avoid memory leak

[Vasiliy Kulikov]

On Tue, Aug 31, 2010 at 18:08 +0200, Julia Lawall wrote:
> On Tue, 31 Aug 2010, walter harms wrote:
> 
> > 
> > 
> > Julia Lawall schrieb:
> > > Add a call to of_node_put in the error handling code following a call to
> > > of_find_node_by_path.
[...]
> > > --- a/drivers/macintosh/via-pmu-led.c
> > > +++ b/drivers/macintosh/via-pmu-led.c
> > > @@ -92,8 +92,10 @@ static int __init via_pmu_led_init(void)
> > >  	if (dt == NULL)
> > >  		return -ENODEV;
> > >  	model = of_get_property(dt, "model", NULL);
> > > -	if (model == NULL)
> > > +	if (model == NULL) {
> > > +		of_node_put(dt);
> > >  		return -ENODEV;
> > > +	}
> > >  	if (strncmp(model, "PowerBook", strlen("PowerBook")) != 0 &&
> > >  	    strncmp(model, "iBook", strlen("iBook")) != 0 &&
> > >  	    strcmp(model, "PowerMac7,2") != 0 &&
> > > 
> > 
> > is there any rule that says when to use strncmp ? it seems perfecly valid to use strcpy here
> > (what is done in the last cmp).
> 
> Perhaps there are some characters after eg PowerBook that one doesn't want 
> to compare with?

It seems to me that model has no '\0' in the end. If model is got from
the hardware then we should double check it - maybe harware is buggy.
Otherwise we'll overflow model.

But why strcmp(model, "PowerMac7,2")? IMO it should be replaced
with strncmp().

-- 
Vasiliy

Re: [PATCH 1/7] drivers/macintosh/via-pmu-led.c: Add of_node_put to avoid memory leak

[Grant Likely]

On Tue, Aug 31, 2010 at 10:16 AM, Vasiliy Kulikov <segooon@gmail.com> wrote=
:
> On Tue, Aug 31, 2010 at 18:08 +0200, Julia Lawall wrote:
>> On Tue, 31 Aug 2010, walter harms wrote:
>> > > =A0 if (strncmp(model, "PowerBook", strlen("PowerBook")) !=3D 0 &&
>> > > =A0 =A0 =A0 strncmp(model, "iBook", strlen("iBook")) !=3D 0 &&
>> > > =A0 =A0 =A0 strcmp(model, "PowerMac7,2") !=3D 0 &&
>> > >
>> >
>> > is there any rule that says when to use strncmp ? it seems perfecly va=
lid to use strcpy here
>> > (what is done in the last cmp).
>>
>> Perhaps there are some characters after eg PowerBook that one doesn't wa=
nt
>> to compare with?
>
> It seems to me that model has no '\0' in the end. If model is got from
> the hardware then we should double check it - maybe harware is buggy.
> Otherwise we'll overflow model.

Model does have \0 at the end.  This code is using strncmp to
purposefully ignore the model suffix.

> But why strcmp(model, "PowerMac7,2")? IMO it should be replaced
> with strncmp().

We use strcmp when parsing the device tree because the the length of
the model property string is unknown and in most cases we *must* match
the exact entire string, such as with this PowerMac7,2 example.  Using
strncmp would also happen to match with something like
"PowerMac7,2345" which is not the desired behaviour.

g.

Re: [PATCH 1/7] drivers/macintosh/via-pmu-led.c: Add of_node_put to avoid memory leak

[walter harms]

Grant Likely schrieb:
> On Tue, Aug 31, 2010 at 10:16 AM, Vasiliy Kulikov <segooon@gmail.com> wrote:
>> On Tue, Aug 31, 2010 at 18:08 +0200, Julia Lawall wrote:
>>> On Tue, 31 Aug 2010, walter harms wrote:
>>>>>   if (strncmp(model, "PowerBook", strlen("PowerBook")) != 0 &&
>>>>>       strncmp(model, "iBook", strlen("iBook")) != 0 &&
>>>>>       strcmp(model, "PowerMac7,2") != 0 &&
>>>>>
>>>> is there any rule that says when to use strncmp ? it seems perfecly valid to use strcpy here
>>>> (what is done in the last cmp).
>>> Perhaps there are some characters after eg PowerBook that one doesn't want
>>> to compare with?
>> It seems to me that model has no '\0' in the end. If model is got from
>> the hardware then we should double check it - maybe harware is buggy.
>> Otherwise we'll overflow model.
> 
> Model does have \0 at the end.  This code is using strncmp to
> purposefully ignore the model suffix.
> 
>> But why strcmp(model, "PowerMac7,2")? IMO it should be replaced
>> with strncmp().
> 
> We use strcmp when parsing the device tree because the the length of
> the model property string is unknown and in most cases we *must* match
> the exact entire string, such as with this PowerMac7,2 example.  Using
> strncmp would also happen to match with something like
> "PowerMac7,2345" which is not the desired behaviour.
> 

hi Grant,
whould you mind to use you explanation as comment in the code ?
Tthat the strncpy/strcpy difference is important should be noted. that would be clearly a
bonos with further audits.

re,
 wh

[PATCH 4/7] arch/powerpc/platforms/powermac/pfunc_core.c: Add of_node_put to avoid memory leak

[Julia Lawall]

Add a call to of_node_put in the error handling code following a call to
of_find_node_by_phandle.

The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@r exists@
local idexpression x;
expression E,E1;
statement S;
@@

*x = 
(of_find_node_by_path
|of_find_node_by_name
|of_find_node_by_phandle
|of_get_parent
|of_get_next_parent
|of_get_next_child
|of_find_compatible_node
|of_match_node
)(...);
...
if (x == NULL) S
<... when != x = E
*if (...) {
  ... when != of_node_put(x)
      when != if (...) { ... of_node_put(x); ... }
(
  return <+...x...+>;
|
*  return ...;
)
}
...>
of_node_put(x);
// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>

---
 arch/powerpc/platforms/powermac/pfunc_core.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/platforms/powermac/pfunc_core.c b/arch/powerpc/platforms/powermac/pfunc_core.c
index cec6359..b0c3777 100644
--- a/arch/powerpc/platforms/powermac/pfunc_core.c
+++ b/arch/powerpc/platforms/powermac/pfunc_core.c
@@ -837,8 +837,10 @@ struct pmf_function *__pmf_find_function(struct device_node *target,
 		return NULL;
  find_it:
 	dev = pmf_find_device(actor);
-	if (dev == NULL)
-		return NULL;
+	if (dev == NULL) {
+		result = NULL;
+		goto out;
+	}
 
 	list_for_each_entry(func, &dev->functions, link) {
 		if (name && strcmp(name, func->name))
@@ -850,8 +852,9 @@ struct pmf_function *__pmf_find_function(struct device_node *target,
 		result = func;
 		break;
 	}
-	of_node_put(actor);
 	pmf_put_device(dev);
+out:
+	of_node_put(actor);
 	return result;
 }
 

Re: [PATCH 4/7] arch/powerpc/platforms/powermac/pfunc_core.c: Add of_node_put to avoid memory leak

[Grant Likely]

[cc'ing linuxppc-dev]

On Sun, Aug 29, 2010 at 11:52:43AM +0200, Julia Lawall wrote:
> Add a call to of_node_put in the error handling code following a call to
> of_find_node_by_phandle.
> 
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @r exists@
> local idexpression x;
> expression E,E1;
> statement S;
> @@
> 
> *x = 
> (of_find_node_by_path
> |of_find_node_by_name
> |of_find_node_by_phandle
> |of_get_parent
> |of_get_next_parent
> |of_get_next_child
> |of_find_compatible_node
> |of_match_node
> )(...);
> ...
> if (x == NULL) S
> <... when != x = E
> *if (...) {
>   ... when != of_node_put(x)
>       when != if (...) { ... of_node_put(x); ... }
> (
>   return <+...x...+>;
> |
> *  return ...;
> )
> }
> ...>
> of_node_put(x);
> // </smpl>
> 
> Signed-off-by: Julia Lawall <julia@diku.dk>

Acked-by: Grant Likely <grant.likely@secretlab.ca>

> 
> ---
>  arch/powerpc/platforms/powermac/pfunc_core.c |    9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/powermac/pfunc_core.c b/arch/powerpc/platforms/powermac/pfunc_core.c
> index cec6359..b0c3777 100644
> --- a/arch/powerpc/platforms/powermac/pfunc_core.c
> +++ b/arch/powerpc/platforms/powermac/pfunc_core.c
> @@ -837,8 +837,10 @@ struct pmf_function *__pmf_find_function(struct device_node *target,
>  		return NULL;
>   find_it:
>  	dev = pmf_find_device(actor);
> -	if (dev == NULL)
> -		return NULL;
> +	if (dev == NULL) {
> +		result = NULL;
> +		goto out;
> +	}
>  
>  	list_for_each_entry(func, &dev->functions, link) {
>  		if (name && strcmp(name, func->name))
> @@ -850,8 +852,9 @@ struct pmf_function *__pmf_find_function(struct device_node *target,
>  		result = func;
>  		break;
>  	}
> -	of_node_put(actor);
>  	pmf_put_device(dev);
> +out:
> +	of_node_put(actor);
>  	return result;
>  }
>  
> 

[PATCH 5/7] arch/powerpc/sysdev/qe_lib/qe.c: Add of_node_put to avoid memory leak

[Julia Lawall]

Add a call to of_node_put in the error handling code following a call to
of_find_compatible_node.

The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@r exists@
local idexpression x;
expression E,E1;
statement S;
@@

*x = 
(of_find_node_by_path
|of_find_node_by_name
|of_find_node_by_phandle
|of_get_parent
|of_get_next_parent
|of_get_next_child
|of_find_compatible_node
|of_match_node
)(...);
...
if (x == NULL) S
<... when != x = E
*if (...) {
  ... when != of_node_put(x)
      when != if (...) { ... of_node_put(x); ... }
(
  return <+...x...+>;
|
*  return ...;
)
}
...>
of_node_put(x);
// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>

---
 arch/powerpc/sysdev/qe_lib/qe.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/powerpc/sysdev/qe_lib/qe.c b/arch/powerpc/sysdev/qe_lib/qe.c
index 3da8014..90020de 100644
--- a/arch/powerpc/sysdev/qe_lib/qe.c
+++ b/arch/powerpc/sysdev/qe_lib/qe.c
@@ -640,6 +640,7 @@ unsigned int qe_get_num_of_snums(void)
 		if ((num_of_snums < 28) || (num_of_snums > QE_NUM_OF_SNUM)) {
 			/* No QE ever has fewer than 28 SNUMs */
 			pr_err("QE: number of snum is invalid\n");
+			of_node_put(qe);
 			return -EINVAL;
 		}
 	}

Re: [PATCH 5/7] arch/powerpc/sysdev/qe_lib/qe.c: Add of_node_put to avoid memory leak

[Timur Tabi]

Julia Lawall wrote:
> Add a call to of_node_put in the error handling code following a call to
> of_find_compatible_node.

Acked-by: Timur Tabi <timur@freescale.com>

Thanks, Julia.  Your work in finding these kinds of bugs is very much
appreciated.

Re: [PATCH 5/7] arch/powerpc/sysdev/qe_lib/qe.c: Add of_node_put to avoid memory leak

[Kumar Gala]

On Aug 29, 2010, at 4:52 AM, Julia Lawall wrote:

> Add a call to of_node_put in the error handling code following a call to
> of_find_compatible_node.
> 
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @r exists@
> local idexpression x;
> expression E,E1;
> statement S;
> @@
> 
> *x = 
> (of_find_node_by_path
> |of_find_node_by_name
> |of_find_node_by_phandle
> |of_get_parent
> |of_get_next_parent
> |of_get_next_child
> |of_find_compatible_node
> |of_match_node
> )(...);
> ...
> if (x == NULL) S
> <... when != x = E
> *if (...) {
>  ... when != of_node_put(x)
>      when != if (...) { ... of_node_put(x); ... }
> (
>  return <+...x...+>;
> |
> *  return ...;
> )
> }
> ...>
> of_node_put(x);
> // </smpl>
> 
> Signed-off-by: Julia Lawall <julia@diku.dk>
> 
> ---
> arch/powerpc/sysdev/qe_lib/qe.c |    1 +
> 1 file changed, 1 insertion(+)

applied to merge

- k

[PATCH 6/7] arch/powerpc/platforms/maple/setup.c: Add of_node_put to avoid memory leak

[Julia Lawall]

Add a call to of_node_put in the error handling code following a call to
of_find_node_by_path.

The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@r exists@
local idexpression x;
expression E,E1;
statement S;
@@

*x = 
(of_find_node_by_path
|of_find_node_by_name
|of_find_node_by_phandle
|of_get_parent
|of_get_next_parent
|of_get_next_child
|of_find_compatible_node
|of_match_node
)(...);
...
if (x == NULL) S
<... when != x = E
*if (...) {
  ... when != of_node_put(x)
      when != if (...) { ... of_node_put(x); ... }
(
  return <+...x...+>;
|
*  return ...;
)
}
...>
of_node_put(x);
// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>

---
 arch/powerpc/platforms/maple/setup.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/powerpc/platforms/maple/setup.c b/arch/powerpc/platforms/maple/setup.c
index 3fff8d9..fe34c3d 100644
--- a/arch/powerpc/platforms/maple/setup.c
+++ b/arch/powerpc/platforms/maple/setup.c
@@ -358,6 +358,7 @@ static int __init maple_cpc925_edac_setup(void)
 	model = (const unsigned char *)of_get_property(np, "model", NULL);
 	if (!model) {
 		printk(KERN_ERR "%s: Unabel to get model info\n", __func__);
+		of_node_put(np);
 		return -ENODEV;
 	}
 

Re: [PATCH 6/7] arch/powerpc/platforms/maple/setup.c: Add of_node_put to avoid memory leak

[Grant Likely]

On Sun, Aug 29, 2010 at 11:52:45AM +0200, Julia Lawall wrote:
> Add a call to of_node_put in the error handling code following a call to
> of_find_node_by_path.
> 
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @r exists@
> local idexpression x;
> expression E,E1;
> statement S;
> @@
> 
> *x = 
> (of_find_node_by_path
> |of_find_node_by_name
> |of_find_node_by_phandle
> |of_get_parent
> |of_get_next_parent
> |of_get_next_child
> |of_find_compatible_node
> |of_match_node
> )(...);
> ...
> if (x == NULL) S
> <... when != x = E
> *if (...) {
>   ... when != of_node_put(x)
>       when != if (...) { ... of_node_put(x); ... }
> (
>   return <+...x...+>;
> |
> *  return ...;
> )
> }
> ...>
> of_node_put(x);
> // </smpl>
> 
> Signed-off-by: Julia Lawall <julia@diku.dk>
> 
> ---
>  arch/powerpc/platforms/maple/setup.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/powerpc/platforms/maple/setup.c b/arch/powerpc/platforms/maple/setup.c
> index 3fff8d9..fe34c3d 100644
> --- a/arch/powerpc/platforms/maple/setup.c
> +++ b/arch/powerpc/platforms/maple/setup.c
> @@ -358,6 +358,7 @@ static int __init maple_cpc925_edac_setup(void)
>  	model = (const unsigned char *)of_get_property(np, "model", NULL);
>  	if (!model) {
>  		printk(KERN_ERR "%s: Unabel to get model info\n", __func__);
> +		of_node_put(np);

Acked-by: Grant Likely <grant.likely@secretlab.ca>

This patch should also fix the exact same problem after a call to
of_find_node_by_type() in the same function (line 370).

>  		return -ENODEV;
>  	}
>  
> 

[PATCH 7/7] arch/powerpc/platforms/cell: Add of_node_put to avoid memory leak

[Julia Lawall]

Add calls to of_node_put in the error handling code following calls to
of_find_node_by_path and of_find_node_by_phandle.

The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@r exists@
local idexpression x;
expression E,E1;
statement S;
@@

*x = 
(of_find_node_by_path
|of_find_node_by_name
|of_find_node_by_phandle
|of_get_parent
|of_get_next_parent
|of_get_next_child
|of_find_compatible_node
|of_match_node
)(...);
...
if (x == NULL) S
<... when != x = E
*if (...) {
  ... when != of_node_put(x)
      when != if (...) { ... of_node_put(x); ... }
(
  return <+...x...+>;
|
*  return ...;
)
}
...>
of_node_put(x);
// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>

---
 arch/powerpc/platforms/cell/ras.c        |    4 +++-
 arch/powerpc/platforms/cell/spider-pic.c |    4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/platforms/cell/ras.c b/arch/powerpc/platforms/cell/ras.c
index 1d3c4ef..5ec1e47 100644
--- a/arch/powerpc/platforms/cell/ras.c
+++ b/arch/powerpc/platforms/cell/ras.c
@@ -173,8 +173,10 @@ static int __init cbe_ptcal_enable(void)
 		return -ENODEV;
 
 	size = of_get_property(np, "ibm,cbe-ptcal-size", NULL);
-	if (!size)
+	if (!size) {
+		of_node_put(np);
 		return -ENODEV;
+	}
 
 	pr_debug("%s: enabling PTCAL, size = 0x%x\n", __func__, *size);
 	order = get_order(*size);
diff --git a/arch/powerpc/platforms/cell/spider-pic.c b/arch/powerpc/platforms/cell/spider-pic.c
index 5876e88..3f2e557 100644
--- a/arch/powerpc/platforms/cell/spider-pic.c
+++ b/arch/powerpc/platforms/cell/spider-pic.c
@@ -258,8 +258,10 @@ static unsigned int __init spider_find_cascade_and_node(struct spider_pic *pic)
 		return NO_IRQ;
 	imap += intsize + 1;
 	tmp = of_get_property(iic, "#interrupt-cells", NULL);
-	if (tmp == NULL)
+	if (tmp == NULL) {
+		of_node_put(iic);
 		return NO_IRQ;
+	}
 	intsize = *tmp;
 	/* Assume unit is last entry of interrupt specifier */
 	unit = imap[intsize - 1];

Re: [PATCH 7/7] arch/powerpc/platforms/cell: Add of_node_put to avoid memory leak

[Grant Likely]

On Sun, Aug 29, 2010 at 11:52:46AM +0200, Julia Lawall wrote:
> Add calls to of_node_put in the error handling code following calls to
> of_find_node_by_path and of_find_node_by_phandle.
> 
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @r exists@
> local idexpression x;
> expression E,E1;
> statement S;
> @@
> 
> *x = 
> (of_find_node_by_path
> |of_find_node_by_name
> |of_find_node_by_phandle
> |of_get_parent
> |of_get_next_parent
> |of_get_next_child
> |of_find_compatible_node
> |of_match_node
> )(...);
> ...
> if (x == NULL) S
> <... when != x = E
> *if (...) {
>   ... when != of_node_put(x)
>       when != if (...) { ... of_node_put(x); ... }
> (
>   return <+...x...+>;
> |
> *  return ...;
> )
> }
> ...>
> of_node_put(x);
> // </smpl>
> 
> Signed-off-by: Julia Lawall <julia@diku.dk>

Acked-by: Grant Likely <grant.likely@secretlab.ca>

> 
> ---
>  arch/powerpc/platforms/cell/ras.c        |    4 +++-
>  arch/powerpc/platforms/cell/spider-pic.c |    4 +++-
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/cell/ras.c b/arch/powerpc/platforms/cell/ras.c
> index 1d3c4ef..5ec1e47 100644
> --- a/arch/powerpc/platforms/cell/ras.c
> +++ b/arch/powerpc/platforms/cell/ras.c
> @@ -173,8 +173,10 @@ static int __init cbe_ptcal_enable(void)
>  		return -ENODEV;
>  
>  	size = of_get_property(np, "ibm,cbe-ptcal-size", NULL);
> -	if (!size)
> +	if (!size) {
> +		of_node_put(np);
>  		return -ENODEV;
> +	}
>  
>  	pr_debug("%s: enabling PTCAL, size = 0x%x\n", __func__, *size);
>  	order = get_order(*size);
> diff --git a/arch/powerpc/platforms/cell/spider-pic.c b/arch/powerpc/platforms/cell/spider-pic.c
> index 5876e88..3f2e557 100644
> --- a/arch/powerpc/platforms/cell/spider-pic.c
> +++ b/arch/powerpc/platforms/cell/spider-pic.c
> @@ -258,8 +258,10 @@ static unsigned int __init spider_find_cascade_and_node(struct spider_pic *pic)
>  		return NO_IRQ;
>  	imap += intsize + 1;
>  	tmp = of_get_property(iic, "#interrupt-cells", NULL);
> -	if (tmp == NULL)
> +	if (tmp == NULL) {
> +		of_node_put(iic);
>  		return NO_IRQ;
> +	}
>  	intsize = *tmp;
>  	/* Assume unit is last entry of interrupt specifier */
>  	unit = imap[intsize - 1];
> 

Re: [PATCH 2/7] drivers/serial/mpc52xx_uart.c: Add of_node_put to avoid memory leak

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1894 bytes --]

On Sun, Aug 29, 2010 at 11:52:41AM +0200, Julia Lawall wrote:
> Add a call to of_node_put in the error handling code following a call to
> of_find_compatible_node.
> 
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @r exists@
> local idexpression x;
> expression E,E1;
> statement S;
> @@
> 
> *x = 
> (of_find_node_by_path
> |of_find_node_by_name
> |of_find_node_by_phandle
> |of_get_parent
> |of_get_next_parent
> |of_get_next_child
> |of_find_compatible_node
> |of_match_node
> )(...);
> ...
> if (x == NULL) S
> <... when != x = E
> *if (...) {
>   ... when != of_node_put(x)
>       when != if (...) { ... of_node_put(x); ... }
> (
>   return <+...x...+>;
> |
> *  return ...;
> )
> }
> ...>
> of_node_put(x);
> // </smpl>
> 
> Signed-off-by: Julia Lawall <julia@diku.dk>

Acked-by: Wolfram Sang <w.sang@pengutronix.de>

adding ppc-list to CC.

> 
> ---
>  drivers/serial/mpc52xx_uart.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/serial/mpc52xx_uart.c b/drivers/serial/mpc52xx_uart.c
> index 8dedb26..c4399e2 100644
> --- a/drivers/serial/mpc52xx_uart.c
> +++ b/drivers/serial/mpc52xx_uart.c
> @@ -500,6 +500,7 @@ static int __init mpc512x_psc_fifoc_init(void)
>  	psc_fifoc = of_iomap(np, 0);
>  	if (!psc_fifoc) {
>  		pr_err("%s: Can't map FIFOC\n", __func__);
> +		of_node_put(np);
>  		return -ENODEV;
>  	}
>  
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 3/7] drivers/mtd/nand/mpc5121_nfc.c: Add of_node_put to avoid memory leak

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2434 bytes --]

On Sun, Aug 29, 2010 at 11:52:42AM +0200, Julia Lawall wrote:
> Add a call to of_node_put in the error handling code following a call to
> of_find_compatible_node.
> 
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @r exists@
> local idexpression x;
> expression E,E1;
> statement S;
> @@
> 
> *x = 
> (of_find_node_by_path
> |of_find_node_by_name
> |of_find_node_by_phandle
> |of_get_parent
> |of_get_next_parent
> |of_get_next_child
> |of_find_compatible_node
> |of_match_node
> )(...);
> ...
> if (x == NULL) S
> <... when != x = E
> *if (...) {
>   ... when != of_node_put(x)
>       when != if (...) { ... of_node_put(x); ... }
> (
>   return <+...x...+>;
> |
> *  return ...;
> )
> }
> ...>
> of_node_put(x);
> // </smpl>
> 
> Signed-off-by: Julia Lawall <julia@diku.dk>

Acked-by: Wolfram Sang <w.sang@pengutronix.de>

adding ppc-list to cc.

> 
> ---
>  drivers/mtd/nand/mpc5121_nfc.c |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/mpc5121_nfc.c b/drivers/mtd/nand/mpc5121_nfc.c
> index df0c1da..f4610bc 100644
> --- a/drivers/mtd/nand/mpc5121_nfc.c
> +++ b/drivers/mtd/nand/mpc5121_nfc.c
> @@ -568,6 +568,7 @@ static int mpc5121_nfc_read_hw_config(struct mtd_info *mtd)
>  	uint rcw_width;
>  	uint rcwh;
>  	uint romloc, ps;
> +	int ret = 0;
>  
>  	rmnode = of_find_compatible_node(NULL, NULL, "fsl,mpc5121-reset");
>  	if (!rmnode) {
> @@ -579,7 +580,8 @@ static int mpc5121_nfc_read_hw_config(struct mtd_info *mtd)
>  	rm = of_iomap(rmnode, 0);
>  	if (!rm) {
>  		dev_err(prv->dev, "Error mapping reset module node!\n");
> -		return -EBUSY;
> +		ret = -EBUSY;
> +		goto out;
>  	}
>  
>  	rcwh = in_be32(&rm->rcwhr);
> @@ -628,8 +630,9 @@ static int mpc5121_nfc_read_hw_config(struct mtd_info *mtd)
>  				rcw_width * 8, rcw_pagesize,
>  				rcw_sparesize);
>  	iounmap(rm);
> +out:
>  	of_node_put(rmnode);
> -	return 0;
> +	return ret;
>  }
>  
>  /* Free driver resources */
> 
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]