[PATCH v3] coccinelle: semantic code search for missing of_node_put

[PATCH v3] coccinelle: semantic code search for missing of_node_put

[Wen Yang]

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.
We find these functions by using the following script:

<SmPL>
@initialize:ocaml@
@@

let relevant_str = "use of_node_put() on it when done"

let contains s1 s2 =
    let re = Str.regexp_string s2
    in
        try ignore (Str.search_forward re s1 0); true
        with Not_found -> false

let relevant_functions = ref []

let add_function f c =
    if not (List.mem f !relevant_functions)
    then
      begin
        let s = String.concat " "
          (
            (List.map String.lowercase_ascii
              (List.filter
                (function x ->
                  Str.string_match
                  (Str.regexp "[a-zA-Z_\\(\\)][-a-zA-Z0-9_\\(\\)]*$")
                x 0) (Str.split (Str.regexp "[ .;\t\n]+") c)))) in
             if contains s relevant_str
             then
               Printf.printf "Found relevant function: %s\n" f;
               relevant_functions := f :: !relevant_functions;
      end

@r@
identifier fn;
comments c;
type T = struct device_node *;
@@

T@c fn(...) {
...
}

@script:ocaml@
f << r.fn;
c << r.c;
@@

let (cb,cm,ca) = List.hd c in
let c = String.concat " " cb in
add_function f c
</SmPL>

Then copy the function names found by the above script to the r_miss_put
rule. This rule checks for missing of_node_put.

And this patch also looks for places where an of_node_put() call is on some
paths but not on others (implemented by the r_miss_put_ext rule).

Finally, this patch finds use-after-free issues for a node.
(implemented by the r_use_after_put rule)

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: Wen Yang <yellowriver2010@hotmail.com>
Cc: Markus Elfring <Markus.Elfring@web.de>
Cc: cocci@systeme.lip6.fr
---
v3: delete the global set, add a rule that checks for use-after-free.
v2: improve the commit description and delete duplicate code.

 scripts/coccinelle/free/of_node_put.cocci | 192 ++++++++++++++++++++++++++++++
 1 file changed, 192 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..cda43fa
--- /dev/null
+++ b/scripts/coccinelle/free/of_node_put.cocci
@@ -0,0 +1,192 @@
+// 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@
+@@
+
+report_miss_prefix = "ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line "
+report_miss_suffix = ", but without a corresponding object release within this function."
+org_miss_main = "acquired a node pointer with refcount incremented"
+org_miss_sec = "needed of_node_put"
+report_use_after_put = "ERROR: use-after-free; reference preceded by of_node_put on line "
+org_use_after_put_main = "of_node_put"
+org_use_after_put_sec = "reference"
+
+@r_miss_put exists@
+local idexpression struct device_node *x;
+expression e, e1;
+position p1, p2;
+statement S;
+type T, T1;
+@@
+
+* x = @p1\(of_find_all_nodes\|
+         of_get_cpu_node\|
+         of_get_parent\|
+         of_get_next_parent\|
+         of_get_next_child\|
+         of_get_next_cpu_node\|
+         of_get_compatible_child\|
+         of_get_child_by_name\|
+         of_find_node_opts_by_path\|
+         of_find_node_by_name\|
+         of_find_node_by_type\|
+         of_find_compatible_node\|
+         of_find_node_with_property\|
+         of_find_matching_node_and_match\|
+         of_find_node_by_phandle\|
+         of_parse_phandle\|
+         of_find_next_cache_node\|
+         of_get_next_available_child\)(...);
+...
+if (x == NULL || ...) S
+... when != e = (T)x
+    when != of_node_put(x)
+    when != of_get_next_parent(x)
+    when != of_find_matching_node(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 (T1)x;
+|
+ return of_fwnode_handle(x);
+|
+* return@p2 ...;
+)
+
+@script:python depends on report && r_miss_put@
+p1 << r_miss_put.p1;
+p2 << r_miss_put.p2;
+@@
+
+coccilib.report.print_report(p2[0], report_miss_prefix + p1[0].line + report_miss_suffix)
+
+@script:python depends on org && r_miss_put@
+p1 << r_miss_put.p1;
+p2 << r_miss_put.p2;
+@@
+
+cocci.print_main(org_miss_main, p1)
+cocci.print_secs(org_miss_sec, p2)
+
+@r_miss_put_ext exists@
+local idexpression struct device_node *x;
+expression e, e1;
+position p1 != r_miss_put.p1, p2 != r_miss_put.p2;
+identifier f;
+statement S;
+type T, T1;
+@@
+
+(
+* x = f@p1(...);
+... when != e = (T)x
+    when != true x == NULL
+    when != of_node_put(x)
+    when != of_get_next_parent(x)
+    when != of_find_matching_node(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 (T1)x;
+|
+ return of_fwnode_handle(x);
+|
+* return@p2 ...;
+)
+&
+x = f(...)
+...
+if (<+...x...+>) S
+...
+of_node_put(x);
+)
+
+@script:python depends on report && r_miss_put_ext@
+p1 << r_miss_put_ext.p1;
+p2 << r_miss_put_ext.p2;
+@@
+
+coccilib.report.print_report(p2[0], report_miss_prefix + p1[0].line + report_miss_suffix)
+
+@script:python depends on org && r_miss_put_ext@
+p1 << r_miss_put_ext.p1;
+p2 << r_miss_put_ext.p2;
+@@
+cocci.print_main(org_miss_main, p1)
+cocci.print_secs(org_miss_sec, p2)
+
+@r_put@
+expression E;
+position p1;
+@@
+
+* of_node_put@p1(E)
+
+@r_use_after_put exists@
+expression r_put.E, subE<=r_put.E;
+constant char [] c;
+expression E1;
+iterator iter;
+identifier f;
+statement S;
+type T;
+position r_put.p1, p2;
+@@
+
+* of_node_put@p1(E,...)
+...
+(
+  iter(...,subE,...) S
+|
+ subE = (T)E1
+|
+ &(T)subE
+|
+ f(...,c,...,(T)E,...)
+|
+ E == (T)E1
+|
+ E != (T)E1
+|
+ E1 == (T)E
+|
+ E1 != (T)E
+|
+ !E
+|
+ (T)E || ...
+|
+* (T)E@p2
+)
+
+@script:python depends on r_use_after_put && report@
+p1 << r_put.p1;
+p2 << r_use_after_put.p2;
+@@
+
+coccilib.report.print_report(p2[0], report_use_after_put + p1[0].line)
+
+@script:python depends on r_use_after_put && org@
+p1 << r_put.p1;
+p2 << r_use_after_put.p2;
+@@
+
+cocci.print_main(org_use_after_put_main, p1)
+cocci.print_secs(org_use_after_put_sec, p2)
-- 
2.9.5

Re: [PATCH v3] coccinelle: semantic code search for missing of_node_put

[Markus Elfring]

> We find these functions by using the following script:

Why would you like to keep this SmPL code in the commit description?

I would prefer software evolution in an other direction.
https://lore.kernel.org/lkml/44be5924-26ca-5106-aa25-3cbc3343aa2c@web.de/
https://lkml.org/lkml/2019/7/4/21


> @initialize:ocaml@
> @@
>
> let relevant_str = "use of_node_put() on it when done"

I see further possibilities to improve this data processing approach.
https://lore.kernel.org/lkml/904b9362-cd01-ffc9-600b-0c48848617a0@web.de/
https://lore.kernel.org/patchwork/patch/1095169/#1291378
https://lkml.org/lkml/2019/6/28/326


I am missing more constructive answers for mentioned development concerns.


> And this patch also looks for places …

Does a SmPL script perform an action?


> Finally, this patch finds use-after-free issues for a node.
> (implemented by the r_use_after_put rule)

This software extension is another interesting contribution.
But I imagine that a separate SmPL script can be more helpful for
this source code search pattern.


> v3: delete the global set, …

To which previous implementation detail do you refer here?


> +virtual report
> +virtual org
> +
> +@initialize:python@
> +@@
> +
> +report_miss_prefix = "ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line "
> +report_miss_suffix = ", but without a corresponding object release within this function."
> +org_miss_main = "acquired a node pointer with refcount incremented"
> +org_miss_sec = "needed of_node_put"
> +report_use_after_put = "ERROR: use-after-free; reference preceded by of_node_put on line "
> +org_use_after_put_main = "of_node_put"
> +org_use_after_put_sec = "reference"

If you would insist on the usage of these variables, they should be applied
only for the selected analysis operation mode.
I would expect corresponding SmPL dependency specifications.
https://github.com/coccinelle/coccinelle/blob/b4509f6e7fb06d5616bb19dd5a110b203fd0e566/docs/manual/cocci_syntax.tex#L559


> +@r_miss_put exists@
> +local idexpression struct device_node *x;
> +expression e, e1;
> +position p1, p2;
> +statement S;
> +type T, T1;
> +@@
> +
> +* x = @p1\(of_find_all_nodes\|

The usage of the SmPL asterisk functionality can fit to the operation mode “context”.
https://bottest.wiki.kernel.org/coccicheck#modes
Would you like to add any corresponding SmPL details?

Under which circumstances will remaining programming concerns be clarified
for such SmPL disjunctions?


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

Will assignment exclusions get any more software development attention?
https://lore.kernel.org/lkml/03cc4df5-ce7f-ba91-36b5-687fec8c7297@web.de/
https://lore.kernel.org/patchwork/patch/1095169/#1291892
https://lkml.org/lkml/2019/6/29/193


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

You propose once more to use a SmPL conjunction in the rule “r_miss_put_ext”.
I am also still waiting for a definitive explanation on the applicability
of this combination.


> +@r_put@
> +expression E;
> +position p1;
> +@@
> +
> +* of_node_put@p1(E)

I guess that this SmPL code will need further adjustments.


> +@r_use_after_put exists@
> +expression r_put.E, subE<=r_put.E;

I have got an understanding difficulty around the interpretation
of the shown SmPL constraint.
How will the clarification be continued?

Regards,
Markus

Re: [PATCH v3] coccinelle: semantic code search for missing of_node_put

[Julia Lawall]

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



On Tue, 16 Jul 2019, Markus Elfring wrote:

> > We find these functions by using the following script:
>
> Why would you like to keep this SmPL code in the commit description?

I don't know indetail what you are proposing, but I would prefer not to
put semantic patches that involve iteration into the kernel, for
simplicity.

julia


>
> I would prefer software evolution in an other direction.
> https://lore.kernel.org/lkml/44be5924-26ca-5106-aa25-3cbc3343aa2c@web.de/
> https://lkml.org/lkml/2019/7/4/21
>
>
> > @initialize:ocaml@
> > @@
> >
> > let relevant_str = "use of_node_put() on it when done"
>
> I see further possibilities to improve this data processing approach.
> https://lore.kernel.org/lkml/904b9362-cd01-ffc9-600b-0c48848617a0@web.de/
> https://lore.kernel.org/patchwork/patch/1095169/#1291378
> https://lkml.org/lkml/2019/6/28/326
>
>
> I am missing more constructive answers for mentioned development concerns.
>
>
> > And this patch also looks for places …
>
> Does a SmPL script perform an action?
>
>
> > Finally, this patch finds use-after-free issues for a node.
> > (implemented by the r_use_after_put rule)
>
> This software extension is another interesting contribution.
> But I imagine that a separate SmPL script can be more helpful for
> this source code search pattern.
>
>
> > v3: delete the global set, …
>
> To which previous implementation detail do you refer here?
>
>
> > +virtual report
> > +virtual org
> > +
> > +@initialize:python@
> > +@@
> > +
> > +report_miss_prefix = "ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line "
> > +report_miss_suffix = ", but without a corresponding object release within this function."
> > +org_miss_main = "acquired a node pointer with refcount incremented"
> > +org_miss_sec = "needed of_node_put"
> > +report_use_after_put = "ERROR: use-after-free; reference preceded by of_node_put on line "
> > +org_use_after_put_main = "of_node_put"
> > +org_use_after_put_sec = "reference"
>
> If you would insist on the usage of these variables, they should be applied
> only for the selected analysis operation mode.
> I would expect corresponding SmPL dependency specifications.
> https://github.com/coccinelle/coccinelle/blob/b4509f6e7fb06d5616bb19dd5a110b203fd0e566/docs/manual/cocci_syntax.tex#L559
>
>
> > +@r_miss_put exists@
> > +local idexpression struct device_node *x;
> > +expression e, e1;
> > +position p1, p2;
> > +statement S;
> > +type T, T1;
> > +@@
> > +
> > +* x = @p1\(of_find_all_nodes\|
>
> The usage of the SmPL asterisk functionality can fit to the operation mode “context”.
> https://bottest.wiki.kernel.org/coccicheck#modes
> Would you like to add any corresponding SmPL details?
>
> Under which circumstances will remaining programming concerns be clarified
> for such SmPL disjunctions?
>
>
> > +... when != e = (T)x
> > +    when != true x == NULL
>
> Will assignment exclusions get any more software development attention?
> https://lore.kernel.org/lkml/03cc4df5-ce7f-ba91-36b5-687fec8c7297@web.de/
> https://lore.kernel.org/patchwork/patch/1095169/#1291892
> https://lkml.org/lkml/2019/6/29/193
>
>
> > +    when != of_node_put(x)
> …
> > +)
> > +&
> > +x = f(...)
> > +...
> > +if (<+...x...+>) S
> > +...
> > +of_node_put(x);
> > +)
>
> You propose once more to use a SmPL conjunction in the rule “r_miss_put_ext”.
> I am also still waiting for a definitive explanation on the applicability
> of this combination.
>
>
> > +@r_put@
> > +expression E;
> > +position p1;
> > +@@
> > +
> > +* of_node_put@p1(E)
>
> I guess that this SmPL code will need further adjustments.
>
>
> > +@r_use_after_put exists@
> > +expression r_put.E, subE<=r_put.E;
>
> I have got an understanding difficulty around the interpretation
> of the shown SmPL constraint.
> How will the clarification be continued?
>
> Regards,
> Markus
>

Re: [v3] coccinelle: semantic code search for missing of_node_put

[Markus Elfring]

>> Why would you like to keep this SmPL code in the commit description?
>
> I don't know indetail what you are proposing,

I imagine that you can get more interesting software development ideas
from links to previous messages.
I hope that the desired clarification can become more constructive.

How are the chances to move such code into SmPL script files?


> but I would prefer not to put semantic patches that involve iteration
> into the kernel, for simplicity.

This view is also interesting.

But I hope that this functionality will become more helpful
if we can agree on value combinations which should be iterated
for powerful source code analysis.


>> I would prefer software evolution in an other direction.
>> https://lore.kernel.org/lkml/44be5924-26ca-5106-aa25-3cbc3343aa2c@web.de/
>> https://lkml.org/lkml/2019/7/4/21

Would you like to add any more advices for affected software components?

Regards,
Markus

Re: [v3] coccinelle: semantic code search for missing of_node_put

[Markus Elfring]

>> Why would you like to keep this SmPL code in the commit description?
>
> I don't know indetail what you are proposing,

I imagine that you can get more interesting software development ideas
from links to previous messages.
I hope that the desired clarification can become more constructive.

How are the chances to move such code into SmPL script files?


> but I would prefer not to put semantic patches that involve iteration
> into the kernel, for simplicity.

This view is also interesting.

But I hope that this functionality will become more helpful
if we can agree on value combinations which should be iterated
for powerful source code analysis.


>> I would prefer software evolution in an other direction.
>> https://lore.kernel.org/lkml/44be5924-26ca-5106-aa25-3cbc3343aa2c@web.de/
>> https://lkml.org/lkml/2019/7/4/21

Would you like to add any more advices for affected software components?

Regards,
Markus

Re: [v3] Coccinelle: semantic code search for “use after …”

[Markus Elfring]

> Finally, this patch finds use-after-free issues for a node.
> (implemented by the r_use_after_put rule)

I suggest to take another look also at information from a clarification attempt
on a topic like “Checking statement order for patch generation with SmPL support”.
https://systeme.lip6.fr/pipermail/cocci/2017-September/004483.html
https://lore.kernel.org/cocci/alpine.DEB.2.20.1709071519240.3168@hadrien/

Under which circumstances will it become safer to develop SmPL script variants
for such source code search patterns?

Regards,
Markus

Re: [v3] Coccinelle: semantic code search for “use after …”

[Markus Elfring]

> Finally, this patch finds use-after-free issues for a node.
> (implemented by the r_use_after_put rule)

I suggest to take another look also at information from a clarification attempt
on a topic like “Checking statement order for patch generation with SmPL support”.
https://systeme.lip6.fr/pipermail/cocci/2017-September/004483.html
https://lore.kernel.org/cocci/alpine.DEB.2.20.1709071519240.3168@hadrien/

Under which circumstances will it become safer to develop SmPL script variants
for such source code search patterns?

Regards,
Markus

Re: [v3] coccinelle: semantic code search for missing of_node_put

[Markus Elfring]

> 2), Then SmPL A generates another SmPL B based on the function name list;

This would be a general data processing possibility.
Another option would be to let SmPL scripts to import relevant data
from external files or to query facts from databases.


> You expect the entire process above to be automated.

I hope that this can be achieved finally.


> This idea may be interesting,

Thanks for your feedback.


> but it can't be done now,

I got an other view. - Why is your view so limited at the moment?


> and it will introduce uncontrollable factors.

I suggest to take additional design options into account so that you might get
more control on some factors.
Which software development challenges are still waiting for better solutions?


> We agree with julia's comments:
> I would prefer not to put semantic patches that involve iteration into the kernel, for simplicity.

I guess that this kind of change reluctance can be also adjusted.
Some source code analysis approaches can look simple enough
while advanced ones will show more of the inherent complexity.


> Our file is called of_node_put.cocci, which contains three rules: r_miss_put,
>  r_miss_put_ext and r_use_after_put.

This combination is interesting, isn't it?


> If you separate them, it seems inappropriate.

* Would you like to be able to let each source code analysis task to be executed
  on its own?

* I guess that it can become possible with additional development efforts
  to support also a mixture of analysis patterns.

* The patch subject “… missing …” does probably not fit to the detection “use after …”.


>>> v3: delete the global set, …
>>
>> To which previous implementation detail do you refer here?
>
> Here is an improvement based on julia's comments:
> https://lkml.org/lkml/2019/7/5/55

I would find an other description clearer then.
* Drop of functions around “add_if_not_present”
* Omission of iteration functionality


Are any more adjustments worth to be explicitly mentioned in this patch change log?


> Here are some improvements.

Are you going to contribute further patch versions?


> Adding an asterisk here is more convenient to use,

This might be. - I wonder how good additional data fit to supported output formats.


> it can mark the location of the code of interest, such as:

I know its functionality also. - I got the impression that the use of SmPL asterisks
will be safe for the operation mode “context”.


>>> +... when != e = (T)x
>>> +    when != true x == NULL
>>
>> Will assignment exclusions get any more software development attention?
>> https://lore.kernel.org/lkml/03cc4df5-ce7f-ba91-36b5-687fec8c7297@web.de/
>> https://lore.kernel.org/patchwork/patch/1095169/#1291892
>> https://lkml.org/lkml/2019/6/29/193

Will this aspect evolve further anyhow?


>> You propose once more to use a SmPL conjunction in the rule “r_miss_put_ext”.
>> I am also still waiting for a definitive explanation on the applicability
>> of this combination.

Would you like to clarify this software detail any more?


>>> +@r_use_after_put exists@
>>> +expression r_put.E, subE<=r_put.E;
>>
>> I have got an understanding difficulty around the interpretation
>> of the shown SmPL constraint.
>> How will the clarification be continued?

More helpful information?


> +|
> + f(...,c,...,(T)E,...)

I would interpret such passing of a pointer for a device node
as an undesirable “use after free (or put)”.
Will this SmPL disjunction need further adjustments?

Regards,
Markus