Re: [Cocci] [PATCH v3] coccinelle: semantic code search for missingof_node_put
Hi Markus, > > 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-3cbc3343a...@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-0c4884861...@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. > Let me try to guess what you mean: 1), Provides a SmPL A that parses the annotations of a particular kernel file and extracts a list of function names to be followed; 2), Then SmPL A generates another SmPL B based on the function name list; 3), Finally SmPL A executes SmPL B on the entire kernel code, looking for the missing of_node_put. You expect the entire process above to be automated. This idea may be interesting, but it can't be done now, and it will introduce uncontrollable factors. We agree with julia's comments: I would prefer not to put semantic patches that involve iteration into the kernel, for simplicity. > > > And this patch also looks for places … > > Does a SmPL script perform an action? > Thanks. We'll continue to improve the description here. > > 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. We found that adding the missing of_node_put may cause use-after-free if not handled properly, so we have added a new r_use_after_put to detect it. Our file is called of_node_put.cocci, which contains three rules: r_miss_put, r_miss_put_ext and r_use_after_put. If you separate them, it seems inappropriate. > > 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 We are very grateful to her. This is a real, valuable comment that can be applied in code practice. > > +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 > Thanks. Here are some improvements. > > +@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? Adding an asterisk here is more convenient to use, it can mark the location of the code of interest, such as: $ /usr/local/bin/spatch -D report --cocci-file scripts/coccinelle/free/of_node_put.cocci arch/arm/mach-axxia/platsmp.c init_defs_builtins: /usr/local/bin/../lib/coccinelle/standard.h HANDLING: arch/arm/mach-axxia/platsmp.c arch/arm/mach-axxia/platsmp.c:43:2-8: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 37, but without a corresponding object release within this function. arch/arm/mach-axxia/platsmp.c:50:1-7: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 37, but without a corresponding object release within this function. diff = --- arch/arm/mach-axxia/platsmp.c +++ /tmp/cocci-output-13026-88f3a1-platsmp.c @@ -34,20 +34,17 @@ static int axxia_boot_secondary(unsigned void __iomem *syscon; u32 tmp; - syscon_np = of_find_compatible_node(NULL, NULL, "lsi,axxia-syscon"); if (!syscon_np) return -ENOENT; syscon = of_iomap(syscon_np, 0); if (!syscon)
Re: [Cocci] [PATCH v2] coccinelle: semantic code search formissingof_node_put
> > > > > +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(...);
> > >
> > > Did you actually test this? I doubt that a position metavariable can be
> > > put on a ) of a disjunction.
> > >
> > > > > +|
> > > > > +return x;
> > > > > +|
> > > > > +return of_fwnode_handle(x);
> > > >
> > > > Can a nested SmPL disjunction be helpful at such places?
> > > >
> > > > +|return
> > > > +(x
> > > > +|of_fwnode_handle(x)
> > > > +);
> > >
> > > The original code is much more readable. The internal representation will
> > > be the same.
> > >
> > > > > +when != v4l2_async_notifier_add_fwnode_subdev(<...x...>)
> > > >
> > > > Would the specification variant “<+... x ...+>” be relevant
> > > > for the parameter selection?
> > >
> > > I'm indeed quite surprised that <...x...> would be accepted by the
> > > parser..
> >
> > Hi julia,
> >
> > Thank you for your comments.
> > We tested and found that both <...x...> and <+... x ...+> variants work
> > fine.
> > We use <... x ...> instead of <+... x ...+> here to eliminate the following
> > false positives:
> >
> > ./drivers/media/platform/qcom/camss/camss.c:504:1-7: ERROR: missing
> > of_node_put; acquired a node pointer with refcount incremented on line 479,
> > but without a corresponding object release within this function.
> >
> > 465 static int camss_of_parse_ports(struct camss *camss)
> > 466 {
> > ...
> > 479 remote = of_graph_get_remote_port_parent(node);
> > ...
> > 486 asd = v4l2_async_notifier_add_fwnode_subdev(
> > 487 &camss->notifier, of_fwnode_handle(remote), --->
> > v4l2_async_notifier_add_fwnode_subdev will pass remote to camss->notifier.
> > 488 sizeof(*csd));
> > ...
> > 504 return num_subdevs;
>
> I suspect that what is happening is that there is a runtime error, but
> that error is caught somewhere and you don't see it.
Thanks.
You are right, there is indeed a runtime error.
Since make coccicheck adds the "-very-quiet" parameter by default, we didn't
find it.
$ spatch --sp-file of_node_put.cocci -D report
drivers/media/platform/am437x/am437x-vpfe.c
init_defs_builtins: /usr/local/bin/../lib/coccinelle/standard.h
HANDLING: drivers/media/platform/am437x/am437x-vpfe.c
exn while in timeout_function
only handling multi and no when code in a nest expr
> Could you send me again the entire semantic patch so I can check on this?
>
Thanks.
The entire SmPL is as follows:
$ cat of_node_put.cocci
// 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@
@@
seen = set()
def add_if_not_present (p1, p2):
if (p1, p2) not in seen:
seen.add((p1, p2))
return True
return False
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)
@r1 exists@
local idexpression struct device_node *x;
expression e, e1;
position p1, p2;
statement S;
type T;
@@
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 != 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 x;
|
return of_fwnode_handle(x);
|
return@p2 ...;
)
@script:python depends on report && r1@
p1 << r1.p1;
p2 << r1.p2;
@@
display_report(p1, p2)
@script:python depends on org && r1@
p1 << r1.p1;
p2 << r1.p2;
@@
display_org(p1, p2)
@r2 exists@
local idexpression struct device_node *x;
expression e, e1;
position p1, p2;
identifier f;
statement S;
type T;
@@
(
x = f@p1(...);
... when != e
Re: [Cocci] [PATCH v2] coccinelle: semantic code search for missingof_node_put
> > > +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(...);
>
> Did you actually test this? I doubt that a position metavariable can be
> put on a ) of a disjunction.
>
> > > +|
> > > +return x;
> > > +|
> > > +return of_fwnode_handle(x);
> >
> > Can a nested SmPL disjunction be helpful at such places?
> >
> > +|return
> > +(x
> > +|of_fwnode_handle(x)
> > +);
>
> The original code is much more readable. The internal representation will
> be the same.
>
> > > +when != v4l2_async_notifier_add_fwnode_subdev(<...x...>)
> >
> > Would the specification variant “<+... x ...+>” be relevant
> > for the parameter selection?
>
> I'm indeed quite surprised that <...x...> would be accepted by the parser..
Hi julia,
Thank you for your comments.
We tested and found that both <...x...> and <+... x ...+> variants work fine.
We use <... x ...> instead of <+... x ...+> here to eliminate the following
false positives:
./drivers/media/platform/qcom/camss/camss.c:504:1-7: ERROR: missing
of_node_put; acquired a node pointer with refcount incremented on line 479, but
without a corresponding object release within this function.
465 static int camss_of_parse_ports(struct camss *camss)
466 {
...
479 remote = of_graph_get_remote_port_parent(node);
...
486 asd = v4l2_async_notifier_add_fwnode_subdev(
487 &camss->notifier, of_fwnode_handle(remote), --->
v4l2_async_notifier_add_fwnode_subdev will pass remote to camss->notifier.
488 sizeof(*csd));
...
504 return num_subdevs;
--
Thanks and regards,
Wen___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
[Cocci] 答复: Re: [PATCH v2] coccinelle: semantic code search for missingof_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?
>
OK, we will replace "SmPL" with "scrilt" later.
>
> >
> > @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?
>
We will also provide an example written in Python later.
>
> > 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?
>
We first divide the comment by word, then use spaces to concatenate
them into a string, and finally find the substring (use of_node_put() on it
when done)
in this string.
>
> > (
> > (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.
Thanks. Here the relevant_str is a string constant.
> > 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”.
Thanks.
We first use this script to find out all the function names to be processed,
and then copy these function names into r1.
> > And this patch also looks for places where an of_node_put()
>
> Does a patch or a script perform an action?
>
OK, Thanks.
We'll fix it soon.
> > call is on some paths but not on others.
>
> Let us look at mentioned implementation details.
>
>
> > +@initialize: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?
As julia pointed out, your current writing is not compiled.
> > +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.
Thanks.
Our previous version used the "when any" clause, so we need
"when != true x == NULL". We can delete this code exclusion
specification for this version.
We will fix it later.
> > +|
> > +return x;
> > +|
> > +return of_fwnode_handle(x);
>
> Can a nested SmPL disjunction be helpful at such places?
>
> +|return
> +(x
> +|of_fwnode_handle(x)
> +);
We are more in agreement with julia's comments:
The original code is much more readable. The inter
Re: [Cocci] Coccinelle: semantic patch for missing of_node_put
> > We currently use the following Ocaml script to automatically > > collect functions that need to be considered. > > > > @initialize:ocaml@ > > @@ > > > > let relevant_str = "use of_node_put() on it when done" > > I suggest to reconsider this search pattern. > > The mentioned words are distributed over text lines in the discussed > software documentation. > Thus I imagine that an other documentation format would be safer > and more helpful for the determination of a corresponding API > system property. Our script will remove '* ','\ n','\t' and so on from the comments in the function header and then merge them into one line, so we can exactly match the target string 'use of_node_put() on it when done ' -- Regards, Wen___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] Coccinelle: semantic patch for missing of_node_put
> > 2, A general method.
> > We also try to get the list of functions to consider by writing a SmPL,
> > but this method is not feasible at present, because it is not easy to parse
> > the comment
> > header information of these functions.
>
> The situation was improved once more also for the Coccinelle software.
> How do you think about to develop any more variants based on information
> from a script (like the following) for the semantic patch language?
>
> @initialize:python@
> @@
> import re, sys
> filter = re.compile(" when done")
>
> @find@
> comments c;
> identifier x;
> type t;
> @@
> t@c x(...)
> { ... }
>
> @script:python selection@
> input << find.c;
> @@
> if filter.search(input[0].before, 2):
> sys.stderr.write(input[0].before + "\n=\n")
> else:
> cocci.include_match(False)
>
> @display@
> identifier find.x;
> type find.t;
> @@
> *t x(...)
> { ... }
>
>
> Does such a source code analysis approach indicate any details
> which should be improved for the affected software documentation?
Thank you for your example.
We currently use the following Ocaml script to automatically
collect functions that need to be considered.
@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
Printf.printf "comments: %s\n" s;
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
--
Regards,
Wen___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [4/5] Coccinelle: put_device: Extend when constraints for twoSmPL ellipses
> Subject: Re: [4/5] Coccinelle: put_device: Extend when constraints for > twoSmPL ellipses > >> Can you agree to any information which I presented in the commit message? > > Do you find this description inappropriate? > > > >>> You don't need so many type metavariables. > >> > >> It seems that the Coccinelle software can cope also with my SmPL code > >> addition. > >> You might feel uncomfortable with the suggested changes for a while. > > > > It's ugly. Much more ugly than msg = > > The clarification of this change reluctance might become more interesting. > I got convinced that there is a need for further software updates. > > > >> * Can it become required to identify involved source code placeholders > >> by extra metavariables? > > > > I don't understand the question. > > Wen Yang was planning a corresponding modification since 2019-02-19. > https://lore.kernel.org/cocci/201902191014156680...@zte.com.cn/ > https://systeme.lip6.fr/pipermail/cocci/2019-February/005620.html > Hi Markus, I did another experiment at that time and found that this modification will reduce the false positive rate, but it may also reduce the recall rate. Could we use it to find out as many bugs as possible in the current kernel and then modify it? https://systeme.lip6.fr/pipermail/cocci/2019-February/005626.html Thanks. -- Regards, Wen > > I got into the development mood to contribute another concrete update > suggestion > for an open issue in affected scripts for the semantic patch language. > Do you recognise the need for the extension of exclusion specifications here? >___ 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
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,f800/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___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [v6] coccinelle: semantic code search for missing put_device()
> > Do you have any other questions?
>
> Obviously, yes.
> I am curious if this development discussion and code review will trigger
> further software adjustments.
> I guess that you will need additional time to reconsider specific items
> from recent feedback.
>
> Will corrections become relevant for specifications in (assignment) exclusions
> of the second SmPL ellipsis in the discussed script?
Let's do some experiments with the code in the current kernel.
Let us take this code as an example:
Https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/sound/soc/fsl/imx-sgtl5000.c
1, Original code
static int imx_sgtl5000_probe(struct platform_device *pdev)
{
struct device_node *np = pdev->dev.of_node;
struct device_node *ssi_np, *codec_np;
struct platform_device *ssi_pdev;
...
ssi_pdev = of_find_device_by_node(ssi_np);
...
}
Our current patch will find the problem:
./sound/soc/fsl/imx-sgtl5000.c:169:1-7: ERROR: missing put_device; call
of_find_device_by_node on line 105, but without a corresponding object release
within this function.
./sound/soc/fsl/imx-sgtl5000.c:177:1-7: ERROR: missing put_device; call
of_find_device_by_node on line 105, but without a corresponding object release
within this function.
The problem can be detected by both of the following methods:
+id = of_find_device_by_node@p1(x)
+... when != e = id
...
Or:
...
+ ... when != id = e
2, Suppose we change it to:
static int imx_sgtl5000_probe(struct platform_device *pdev)
{
struct device_node *np = pdev->dev.of_node;
struct device_node *ssi_np, *codec_np;
struct platform_device *ssi_pdev;
...
ssi_pdev = of_find_device_by_node(ssi_np);
...
ssi_pdev = to_platform_device(XYZ);
}
But this time, only the first method can detect the problem.
The second method, although the false positive rate is lower,
but the recall rate is reduced, we may miss some real issues.
Thanks,
Regards,
Wen___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [v6] coccinelle: semantic code search for missing put_device()
> > I would have a hard time saying which one is more reasonable to test, > I suggest to reconsider the interpretation of this software situation once > more. > > since both are extremely unlikely. > I disagree to this view because two ellipses were intentionally specified > in published SmPL scripts. > So some software developers found these “special use cases” important enough. > >> In addition, we feel that we should probably accept this patch first, > I disagree to this imagination because I would prefer to integrate a source > code variant > without a bug (which was copied from a version on 2013-05-08 by Petr Strnad). > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/coccinelle/free/pci_free_consistent.cocci?id=f7b167113753e95ae61383e234f8d10142782ace#n12 > I hope that nicer run time behaviour can become also relevant here. Both cases are extremely unlikely. Although we have tested these two methods in the existing kernel code, considering the evolution of the kernel code, these special cases may occur, so we are willing to take them into account. We plan to modify the code like this: id = of_find_device_by_node@p1(x) -... when != e = id +... when != e = (T)id +when != id = (T)e Do you have any other questions? Thanks. Regards, Wen___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [v6] coccinelle: semantic code search for missing put_device()
> > when != e = id achieves this behavior. > > I can not agree to this view completely because of the meaning that is > connected > with these variable identifiers. > > Both metavariables share the kind “expression”. So I can imagine > that there is an intersection for the source code match possibility. > But one was intentionally restricted to the kind “local idexpression” so far. > > Which data element should not get reassigned here (before a corresponding > null pointer check)? > Thank you for your comments. We did some experiments: +id = of_find_device_by_node@p1(x) +... when != e = id ... Or: ... + ... when != id = e The number of issuses found by these two methods is the same. When != e = id achieves this behavior. In addition, we feel that we should probably accept this patch first, use it to find more memory leaks, and solve the actual problems in the kernel code. As for the patch itself, we can continue to pursue perfect in the process of using it to solve practical problems. Regards, Wen___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
