[Cocci] [PATCH] coccinelle: semantic patch for missing of_node_put

[Cocci] [PATCH] coccinelle: semantic patch for missing of_node_put

[Wen Yang]

Looking for places where there is an of_node_put on some paths
but not on others. This SmPL checks that there is a put of the
same data elsewhere in the function, so perhaps that will
alleviate the concern about puts where they are not needed,
while still making it possible to find the ones that are needed.

Suggested-by: Julia Lawall <Julia.Lawall@lip6.fr>
Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
Reviewed-by: Julia Lawall <Julia.Lawall@lip6.fr>
---
 scripts/coccinelle/free/of_node_put.cocci | 57 +++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)
 create mode 100644 scripts/coccinelle/free/of_node_put.cocci

diff --git a/scripts/coccinelle/free/of_node_put.cocci b/scripts/coccinelle/free/of_node_put.cocci
new file mode 100644
index 0000000..6a29830
--- /dev/null
+++ b/scripts/coccinelle/free/of_node_put.cocci
@@ -0,0 +1,57 @@
+// SPDX-License-Identifier: GPL-2.0
+/// Find missing of_node_put
+///
+// Confidence: Moderate
+// Comments:
+// Options: --no-includes --include-headers
+
+virtual report
+virtual org
+
+@r exists@
+local idexpression struct device_node *x;
+identifier f;
+statement S,S1,S2;
+expression e,e1;
+position p1,p2;
+type T,T1;
+@@
+
+(
+x = f@p1(...);
+... when != e = (T)x
+if (x == NULL || ...) S1
+else S2
+... when != of_node_put(x)
+ when != if (x) { ... of_node_put(x) ... }
+ when != e1 = (T1)x
+(
+return x;
+|
+return@p2 ...;
+)
+&
+x = f(...)
+...
+if (<+...x...+>) S
+...
+of_node_put(x);
+)
+
+@script:python depends on report@
+p1 << r.p1;
+p2 << r.p2;
+@@
+
+coccilib.report.print_report(p2[0],
+			     "ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line "
+			     + p1[0].line
+			     + ", but without a corresponding object release within this function.")
+
+@script:python depends on org@
+p1 << r.p1;
+p2 << r.p2;
+@@
+
+cocci.print_main("acquired a node pointer with refcount incremented", p1)
+cocci.print_secs("needed of_node_put", p2)
-- 
2.9.5

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH] coccinelle: semantic patch for missing of_node_put

[Julia Lawall]

On Fri, 15 Mar 2019, Wen Yang wrote:

> Looking for places where there is an of_node_put on some paths
> but not on others. This SmPL checks that there is a put of the
> same data elsewhere in the function, so perhaps that will
> alleviate the concern about puts where they are not needed,
> while still making it possible to find the ones that are needed.
>
> Suggested-by: Julia Lawall <Julia.Lawall@lip6.fr>
> Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
> Reviewed-by: Julia Lawall <Julia.Lawall@lip6.fr>
> ---
>  scripts/coccinelle/free/of_node_put.cocci | 57 +++++++++++++++++++++++++++++++
>  1 file changed, 57 insertions(+)
>  create mode 100644 scripts/coccinelle/free/of_node_put.cocci
>
> diff --git a/scripts/coccinelle/free/of_node_put.cocci b/scripts/coccinelle/free/of_node_put.cocci
> new file mode 100644
> index 0000000..6a29830
> --- /dev/null
> +++ b/scripts/coccinelle/free/of_node_put.cocci
> @@ -0,0 +1,57 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/// Find missing of_node_put
> +///
> +// Confidence: Moderate
> +// Comments:
> +// Options: --no-includes --include-headers
> +
> +virtual report
> +virtual org
> +
> +@r exists@
> +local idexpression struct device_node *x;
> +identifier f;
> +statement S,S1,S2;
> +expression e,e1;
> +position p1,p2;
> +type T,T1;
> +@@
> +
> +(
> +x = f@p1(...);
> +... when != e = (T)x

I suggest the following:

(
if (x) { ... of_node_put(x) ... }
|
> +if (x == NULL || ...) S1
> +else S2
> +... when != of_node_put(x)
> + when != if (x) { ... of_node_put(x) ... }
> + when != e1 = (T1)x
> +(
> +return x;
> +|
> +return@p2 ...;
> +)
)

If the first test is for success of the allocation and may lead to an
of_node_put, then you can stop.  Perhaps


if (x) { ... when forall
         of_node_put(x) ... }

there would be better, to check if there is always a put.  This could also
be done on the other if (x)

> +&
> +x = f(...)
> +...
> +if (<+...x...+>) S
> +...
> +of_node_put(x);

There is actually an opportunity here for more reports.  Perhaps we can
assume that if the function calls of_node_put on anything, then it is
needed on everything.  So this could be of_node_put(...).  But the
downside of that is that the original x = f(...) may now let through
things that are not reference counted.  So you would want two rules, first
this one where the function is f and there is a of_node_put on the result
of the function, and another one where you consider a known set of
functions, and then allow a subsequent of_node_put on anything.

It would take some care to ensure that there is only one report per call
site.

julia

> +)
> +
> +@script:python depends on report@
> +p1 << r.p1;
> +p2 << r.p2;
> +@@
> +
> +coccilib.report.print_report(p2[0],
> +			     "ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line "
> +			     + p1[0].line
> +			     + ", but without a corresponding object release within this function.")
> +
> +@script:python depends on org@
> +p1 << r.p1;
> +p2 << r.p2;
> +@@
> +
> +cocci.print_main("acquired a node pointer with refcount incremented", p1)
> +cocci.print_secs("needed of_node_put", p2)
> --
> 2.9.5
>
>
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH] coccinelle: semantic patch for missing of_node_put

[Markus Elfring]

> +/// Find missing of_node_put
> +///
> +// Confidence: Moderate

How much would you like to improve the situation around recurring software
development concerns for such source code analysis approaches?


> +virtual report
> +virtual org

I would interpret the provided SmPL code in the way that it will not generate
adjusted (patched) C code so far.
Source code search results will be presented by these operation modes.

How do you think about to exchange the word “patch” by “code search”
at affected places (and in the subject) then?


> +@r exists@
> +local idexpression struct device_node *x;
> +identifier f;
> +statement S,S1,S2;
> +expression e,e1;
> +position p1,p2;
> +type T,T1;
> +@@
> +
> +(
> +x = f@p1(...);

How do you think about to express any more constraints for this function call?


> +... when != e = (T)x

Will it be safer to add another exclusion for the initial assignment target?


> + when != if (x) { ... of_node_put(x) ... }

I find the specification of this extra condition check questionable.



Should Masahiro Yamada be explicitly notified also for this attempt
to integrate another SmPL script?

Regards,
Markus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

[Cocci] [PATCH] coccinelle: semantic patch for missing of_node_put

[Wen Yang]

The call to of_parse_phandle()/of_find_node_by_name() ... returns a node
pointer with refcount incremented thus it must be explicitly decremented
after the last usage.

This SmPL is also looking for places where there is an of_node_put on
some path but not on others.

Suggested-by: Julia Lawall <Julia.Lawall@lip6.fr>
Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
Cc: Julia Lawall <Julia.Lawall@lip6.fr>
Cc: Gilles Muller <Gilles.Muller@lip6.fr>
Cc: Nicolas Palix <nicolas.palix@imag.fr>
Cc: Michal Marek <michal.lkml@markovi.net>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: cocci@systeme.lip6.fr
---
 scripts/coccinelle/free/of_node_put.cocci | 133 ++++++++++++++++++++++++++++++
 1 file changed, 133 insertions(+)
 create mode 100644 scripts/coccinelle/free/of_node_put.cocci

diff --git a/scripts/coccinelle/free/of_node_put.cocci b/scripts/coccinelle/free/of_node_put.cocci
new file mode 100644
index 0000000..304293c
--- /dev/null
+++ b/scripts/coccinelle/free/of_node_put.cocci
@@ -0,0 +1,133 @@
+// SPDX-License-Identifier: GPL-2.0
+/// Find missing of_node_put
+///
+// Confidence: Moderate
+// Copyright: (C) 2018-2019 Wen Yang, ZTE.
+// Comments:
+// Options: --no-includes --include-headers
+
+virtual report
+virtual org
+
+@initialize:python@
+@@
+
+msg_prefix = "ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line "
+msg_suffix = ", but without a corresponding object release within this function."
+
+seen = set()
+
+def add_if_not_present (p1, p2):
+    if (p1, p2) not in seen:
+        seen.add((p1, p2))
+        return True
+    return False
+
+@r1 exists@
+local idexpression struct device_node *x;
+expression e, e1;
+position p1, p2;
+identifier f;
+statement S;
+type T;
+@@
+
+(
+x = f@p1(...);
+... when != e = (T)x
+    when any
+    when != true x == NULL
+    when != of_node_put(x)
+    when != of_get_next_parent(x)
+    when != of_find_matching_node(x, ...)
+    when != if (x) { ... of_node_put(x) ... }
+    when != if (x) { ... return x; }
+    when != v4l2_async_notifier_add_fwnode_subdev(<...x...>)
+    when != e1 = of_fwnode_handle(x)
+(
+if (x) { ... when forall
+         of_node_put(x) ... }
+|
+return x;
+|
+return of_fwnode_handle(x);
+|
+return@p2 ...;
+)
+&
+x = f(...)
+...
+if (<+...x...+>) S
+...
+of_node_put(x);
+)
+
+@script:python depends on report && r1@
+p1 << r1.p1;
+p2 << r1.p2;
+@@
+
+if(add_if_not_present(p1[0].line, p2[0].line)):
+  coccilib.report.print_report(p2[0], msg_prefix + p1[0].line + msg_suffix)
+
+@script:python depends on org && r1@
+p1 << r1.p1;
+p2 << r1.p2;
+@@
+
+cocci.print_main("acquired a node pointer with refcount incremented", p1)
+cocci.print_secs("needed of_node_put", p2)
+
+@r2 exists@
+local idexpression struct device_node *x;
+expression e, e1;
+position p1, p2;
+statement S;
+type T;
+@@
+
+x = @p1\(of_find_compatible_node\|of_find_node_by_name\|of_parse_phandle\|
+    of_find_node_by_type\|of_find_node_by_name\|of_find_all_nodes\|
+    of_get_cpu_node\|of_get_parent\|of_get_next_parent\|
+    of_get_next_child\|of_get_next_available_child\|of_get_next_cpu_node\|
+    of_get_compatible_child\|of_get_child_by_name\|of_find_node_opts_by_path\|
+    of_find_node_with_property\|of_find_matching_node_and_match\|of_find_node_by_phandle\|
+    of_parse_phandle\)(...);
+...
+if (x == NULL || ...) S
+... when != e = (T)x
+    when any
+    when != true x == NULL
+    when != of_node_put(x)
+    when != of_get_next_parent(x)
+    when != of_find_matching_node(x, ...)
+    when != if (x) { ... of_node_put(x) ... }
+    when != if (x) { ... return x; }
+    when != v4l2_async_notifier_add_fwnode_subdev(<...x...>)
+    when != e1 = of_fwnode_handle(x)
+(
+if (x) { ... when forall
+         of_node_put(x) ... }
+|
+return x;
+|
+return of_fwnode_handle(x);
+|
+return@p2 ...;
+)
+
+@script:python depends on report && r2@
+p1 << r2.p1;
+p2 << r2.p2;
+@@
+
+if(add_if_not_present(p1[0].line, p2[0].line)):
+  coccilib.report.print_report(p2[0], msg_prefix + p1[0].line + msg_suffix)
+
+@script:python depends on org && r2@
+p1 << r2.p1;
+p2 << r2.p2;
+@@
+
+cocci.print_main("acquired a node pointer with refcount incremented", p1)
+cocci.print_secs("needed of_node_put", p2)
-- 
2.9.5

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH] coccinelle: semantic patch for missing of_node_put

[Markus Elfring]

> The call to of_parse_phandle()/of_find_node_by_name() ... returns a node
> pointer with refcount incremented thus it must be explicitly decremented
> after the last usage.
>
> This SmPL is also looking for places where there is an of_node_put on
> some path but not on others.

I suggest to improve this commit description.

* Possible wording:
  There are functions which increment a reference counter for a device node.
  These functions belong to a programming interface for the management
  of information from device trees.
  The counter must be decremented after the last usage of a device node.

  This SmPL script looks also for places where a of_node_put() call is on
  some paths but not on others.

* Will the word “patch” be replaced by “code search” in the commit subject
  because the operation modes “report” and “org” are supported here?


> +@initialize:python@
> +@@

Such a SmPL rule would apply to every possible operation mode.
I have noticed then that the two Python variables from here will be needed
only in two SmPL rules which depend on the mode “report”.

* Thus I would prefer to adjust the dependency specification accordingly.

* Please replace these variables by a separate function like
  the following.
  def display1(p1 ,p2):
     if add_if_not_present(p1[0].line, p2[0].line):
        coccilib.report.print_report(p2[0],
                                     "prefix"
                                     + p1[0].line
                                     + "suffix")


* Please move another bit of duplicate code to a separate function like
  the following.
  def display2(p1 ,p2):
     cocci.print_main("Choose info 1", p1)
     cocci.print_secs("Choose info 2", p2)


> +x = @p1\(of_find_compatible_node\|of_find_node_by_name\|of_parse_phandle\|

If you would like to insist to use such a SmPL disjunction, I would prefer
an other code formatting here.
How do you think about to put each function name on a separate line?

Can such a name list be ever automatically determined from an other
information source?
(Are there circumstances to consider under which the application of
a detailed regular expression would become interesting for a SmPL constraint?)

Will it be influenced by any sort criteria?


> +    when != of_node_put(x)
…
> +    when != if (x) { ... of_node_put(x) ... }

I find the second when constraint specification unnecessary because
the previous one should be sufficient to exclude such a function call.


Can the specification “when != \( of_node_put \| of_get_next_parent \) (x)”
be useful?


> +return x;
> +|
> +return of_fwnode_handle(x);

Can it be nicer to merge this bit of code into another SmPL disjunction?

+return \( x \| of_fwnode_handle(x) \);


Regards,
Markus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH] coccinelle: semantic patch for missing of_node_put

[wen.yang99]

[-- Attachment #1.1: Type: text/plain, Size: 4235 bytes --]

Hi Markus,
Thanks for the review.  

> > The call to of_parse_phandle()/of_find_node_by_name() ... returns a node
> > pointer with refcount incremented thus it must be explicitly decremented
> > after the last usage.
> >
> > This SmPL is also looking for places where there is an of_node_put on
> > some path but not on others.
> 
> I suggest to improve this commit description.
> 
> * Possible wording:
> There are functions which increment a reference counter for a device node.
> These functions belong to a programming interface for the management
> of information from device trees.
> The counter must be decremented after the last usage of a device node.
> 
> This SmPL script looks also for places where a of_node_put() call is on
> some paths but not on others.
> 
> * Will the word “patch” be replaced by “code search” in the commit subject
> because the operation modes “report” and “org” are supported here?
> 
> 
> > +@initialize:python@
> > +@@
> 
> Such a SmPL rule would apply to every possible operation mode.
> I have noticed then that the two Python variables from here will be needed
> only in two SmPL rules which depend on the mode “report”.
> 
> * Thus I would prefer to adjust the dependency specification accordingly.
> 
> * Please replace these variables by a separate function like
> the following.
> def display1(p1 ,p2):
> if add_if_not_present(p1[0].line, p2[0].line):
> coccilib.report.print_report(p2[0],
> "prefix"
> + p1[0].line
> + "suffix")
> 
> 
> * Please move another bit of duplicate code to a separate function like
> the following.
> def display2(p1 ,p2):
> cocci.print_main("Choose info 1", p1)
> cocci.print_secs("Choose info 2", p2)
> 
Thanks.
I will update the patch according to your suggestions above.

> > +x = @p1\(of_find_compatible_node\|of_find_node_by_name\|of_parse_phandle\|
> 
> If you would like to insist to use such a SmPL disjunction, I would prefer
> an other code formatting here.
> How do you think about to put each function name on a separate line?
> 
> Can such a name list be ever automatically determined from an other
> information source?
> (Are there circumstances to consider under which the application of
> a detailed regular expression would become interesting for a SmPL constraint?)
> 
> Will it be influenced by any sort criteria?
> 
Thanks. 
It's interesting to get the function list automatically.
I'll try to parse the drivers/of/base.c file based on comments like this
"* Returns a node pointer with refcount incremented, use
* of_node_put() on it when done."
to automatically get the name of the function that needs to be checked.

> > +    when != of_node_put(x)
> …
> > +    when != if (x) { ... of_node_put(x) ... }
> 
> I find the second when constraint specification unnecessary because
> the previous one should be sufficient to exclude such a function call.
> 
Thanks.
I added the "when != if (x) { ... of_node_put(x) ... }" statement to avoid
 false positives similar to the following:
./arch/powerpc/platforms/powermac/setup.c:513:1-7: ERROR: missing of_node_put; 
acquired a node pointer with refcount incremented on line 503, 
but without a corresponding object release within this function.

484 static int __init pmac_declare_of_platform_devices(void)
485 {
486         struct device_node *np;
...
503         np = of_find_node_by_type(NULL, "fcu");
504         if (np == NULL) {
505                 /* Some machines have strangely broken device-tree */
506                 np = of_find_node_by_path("/u3@0,f8000000/i2c@f8001000/fan@15e");
507         }
508         if (np) {
509                 of_platform_device_create(np, "temperature", NULL);
510                 of_node_put(np);
511         }
512 
513         return 0;
514 }

We will continue to analyze the code of coccinelle to confirm whether
this false positive is a bug in coccinelle.
But this statement is currently needed here.

--
Regards,
Wen

> 
> Can the specification “when != \( of_node_put \| of_get_next_parent \) (x)”
> be useful?
> 
> 
> > +return x;
> > +|
> > +return of_fwnode_handle(x);
> 
> Can it be nicer to merge this bit of code into another SmPL disjunction?
> 
> +return \( x \| of_fwnode_handle(x) \);
> 
> 
> Regards,
> Markus

[-- Attachment #2: Type: text/plain, Size: 136 bytes --]

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci