linux-kernel - Re: [PATCH v2] coccinelle: semantic code search for missing of_node

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <904b9362-cd01-ffc9-600b-0c48848617a0@web.de>
Date:   Fri, 28 Jun 2019 11:38:18 +0200
From:   Markus Elfring <Markus.Elfring@....de>
To:     Wen Yang <wen.yang99@....com.cn>, linux-kernel@...r.kernel.org
Cc:     Yi Wang <wang.yi59@....com.cn>,
        Julia Lawall <Julia.Lawall@...6.fr>,
        Gilles Muller <Gilles.Muller@...6.fr>,
        Nicolas Palix <nicolas.palix@...g.fr>,
        Michal Marek <michal.lkml@...kovi.net>,
        Masahiro Yamada <yamada.masahiro@...ionext.com>,
        cocci@...teme.lip6.fr
Subject: Re: [PATCH v2] coccinelle: semantic code search for missing
 of_node_put

> The counter must be decremented after the last usage of a device node.

Thanks for your next try to improve the software situation
also in this area.

> We find these functions by using the following SmPL:

Would it be nicer to use the word “script” also here?

> <SmPL>
> @initialize:ocaml@
> @@

How do you think about to describe the chosen algorithm
in a way for contributors who might not be so familiar with
this programming language?

Will any information from previous discussions become relevant
for a better commit description?

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

Will such a literal need further development and software documentation considerations?

> 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 " "

I find such a concatenation suspicious after the space character
is used also for a string splitting before.
Can this delimiter be omitted for the combination?

>           (
>             (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

I would prefer to use the string constant in the called function directly
instead of passing it as another parameter.

>              then
>                Printf.printf "Found relevant function: %s\n" f;
>                relevant_functions := f :: !relevant_functions;
>       end

I find your choice for an output format unclear at the moment.
I imagine that the corresponding data processing of these function names
will need fine-tuning.
I am missing the software component for the conversion of this
identifier list into a disjunction for the SmPL rule “r1”.

> And this patch also looks for places where an of_node_put()

Does a patch or a script perform an action?

> call is on some paths but not on others.

Let us look at mentioned implementation details.

> +@...tialize:python@
> +@@
> +
> +seen = set()
> +
> +def add_if_not_present (p1, p2):

It seems that you would like to use iteration functionality.
https://github.com/coccinelle/coccinelle/blob/99e081e9b89d49301b7bd2c5e5aac88c66eaaa6a/docs/manual/cocci_syntax.tex#L1826

How will it matter here?

> +def display_report(p1, p2):
> +    if add_if_not_present(p1[0].line, p2[0].line):
> +       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.")
> +
> +def display_org(p1, p2):
> +    cocci.print_main("acquired a node pointer with refcount incremented", p1)
> +    cocci.print_secs("needed of_node_put", p2)

Can it be helpful to specify SmPL dependencies for these functions
according to the applied operation mode?

> +x = @p1\(of_find_all_nodes\|

I would find this SmPL disjunction easier to read without the usage
of extra backslashes.

+x =
+(of_…
+|of_…
+)@p1(...);

Which sort criteria were applied for the generation of the shown
function name list?

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

I wonder if this code exclusion specification is really required
after a null pointer was checked before.

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

Can a nested SmPL disjunction be helpful at such places?

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

> +    when != v4l2_async_notifier_add_fwnode_subdev(<...x...>)

Would the specification variant “<+... x ...+>” be relevant
for the parameter selection?

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

You propose once more to use a SmPL conjunction in the rule “r2”.
How does it fit to the previous exclusion specification “when != of_node_put(x)”?

Regards,
Markus