Re: [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@xxxxxxx>
> Signed-off-by: Wen Yang <wen.yang99@xxxxxxxxxx>
> Reviewed-by: Julia Lawall <Julia.Lawall@xxxxxxx>
> ---
>  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
>
>