The of_find_device_by_node() takes a reference to the underlying device
structure, we should release that reference.
The implementation of this semantic code search is:
In a function, for a local variable returned by calling
of_find_device_by_node(),
a, if it is released by a function such as
put_device()/of_dev_put()/platform_device_put() after the last use,
it is considered that there is no reference leak;
b, if it is passed back to the caller via
dev_get_drvdata()/platform_get_drvdata()/get_device(), etc., the
reference will be released in other functions, and the current function
also considers that there is no reference leak;
c, for the rest of the situation, the current function should release the
reference by calling put_device, this code search will report the
corresponding error message.
By using this semantic code search, we have found some object reference leaks,
such as:
commit 11907e9d3533 ("ASoC: fsl-asoc-card: fix object reference leaks in
fsl_asoc_card_probe")
commit a12085d13997 ("mtd: rawnand: atmel: fix possible object reference leak")
commit 11493f26856a ("mtd: rawnand: jz4780: fix possible object reference leak")
There are still dozens of reference leaks in the current kernel code.
Further, for the case of b, the object returned to other functions may also
have a reference leak, we will continue to develop other cocci scripts to
further check the reference leak.
Signed-off-by: Wen Yang <[email protected]>
Reviewed-by: Julia Lawall <[email protected]>
Reviewed-by: Markus Elfring <[email protected]>
Cc: Julia Lawall <[email protected]>
Cc: Gilles Muller <[email protected]>
Cc: Nicolas Palix <[email protected]>
Cc: Michal Marek <[email protected]>
Cc: Markus Elfring <[email protected]>
Cc: Masahiro Yamada <[email protected]>
Cc: Wen Yang <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
v5->v4:
- exchange the word “patch” by “code search”
- add a SPDX identifierfix
- a split string literal can be unwanted.
- Change the content of the reported information.
v4->v3:
- add Masahiro Yamada
- omit a blank line
- split the long message parameter
- reduce the number of metavariables
- Describe the implementation of the semantic patch,
explain the scenarios it can detect,
and further software development considerations.
v3->v2:
- reduction of a bit of redundant C code within SmPL search specifications.
- consider the message construction without using the extra Python variable “msg”
v2->v1:
- put exists after search, and then drop the when exists below.
- should not use the same e as in the when's below.
- Make a new type metavariable and use it to put a cast on the result of
platform_get_drvdata.
scripts/coccinelle/free/put_device.cocci | 56 ++++++++++++++++++++++++++++++++
1 file changed, 56 insertions(+)
create mode 100644 scripts/coccinelle/free/put_device.cocci
diff --git a/scripts/coccinelle/free/put_device.cocci b/scripts/coccinelle/free/put_device.cocci
new file mode 100644
index 0000000..7395697
--- /dev/null
+++ b/scripts/coccinelle/free/put_device.cocci
@@ -0,0 +1,56 @@
+// SPDX-License-Identifier: GPL-2.0
+/// Find missing put_device for every of_find_device_by_node.
+///
+// Confidence: Moderate
+// Copyright: (C) 2018-2019 Wen Yang, ZTE.
+// Comments:
+// Options: --no-includes --include-headers
+
+virtual report
+virtual org
+
+@search exists@
+local idexpression id;
+expression x,e,e1;
+position p1,p2;
+type T,T1,T2,T3;
+@@
+
+id = of_find_device_by_node@p1(x)
+... when != e = id
+if (id == NULL || ...) { ... return ...; }
+... when != put_device(&id->dev)
+ when != platform_device_put(id)
+ when != of_dev_put(id)
+ when != if (id) { ... put_device(&id->dev) ... }
+ when != e1 = (T)id
+ when != e1 = &id->dev
+ when != e1 = get_device(&id->dev)
+ when != e1 = (T1)platform_get_drvdata(id)
+(
+ return
+( id
+| (T2)dev_get_drvdata(&id->dev)
+| (T3)platform_get_drvdata(id)
+);
+| return@p2 ...;
+)
+
+@script:python depends on report@
+p1 << search.p1;
+p2 << search.p2;
+@@
+
+coccilib.report.print_report(p2[0], "ERROR: missing put_device; "
+ + "call of_find_device_by_node on line "
+ + p1[0].line
+ + ", but without a corresponding object release "
+ + "within this function.")
+
+@script:python depends on org@
+p1 << search.p1;
+p2 << search.p2;
+@@
+
+cocci.print_main("of_find_device_by_node", p1)
+cocci.print_secs("needed put_device", p2)
--
2.9.5
> In a function, for a local variable returned by calling
> of_find_device_by_node(),
I suggest to reconsider this information once more.
1. Will an other wording be more appropriate for the storage of
a function return value?
2. Can the restriction “local” be omitted?
3. Will any macros be involved eventually?
> c, for the rest of the situation, the current function should release the
> reference by calling put_device,
Can it happen that on other function will perform the desired reference release?
> this code search will report the
> corresponding error message.
Rewording?
A code search can report an error with a specific confidence.
> v5->v4:
Such version information would be sufficient also without arrows, wouldn't it?
> - add a SPDX identifierfix
Would you like to fix a typo at the end?
> +@script:python depends on report@
> +p1 << search.p1;
> +p2 << search.p2;
> +@@
> +
> +coccilib.report.print_report(p2[0], "ERROR: missing put_device; "
> + + "call of_find_device_by_node on line "
> + + p1[0].line
> + + ", but without a corresponding object release "
> + + "within this function.")
I find your interpretation of my reminder for the preferred avoidance
of split string literals interesting somehow.
Can the following source code variant be more appropriate?
+coccilib.report.print_report(p2[0],
+ "WARNING: missing put_device - of_find_device_by_node() call on line "
+ + p1[0].line
+ + ", but without a corresponding object release within this function.")
Will any more advanced error diagnostics be eventually developed?
Regards,
Markus
> +@search exists@
> +local idexpression id;
> +expression x,e,e1;
> +position p1,p2;
> +type T,T1,T2,T3;
> +@@
> +
> +id = of_find_device_by_node@p1(x)
> +... when != e = id
> +if (id == NULL || ...) { ... return ...; }
> +... when != put_device(&id->dev)
…
> + when != if (id) { ... put_device(&id->dev) ... }
…
I would interpret this SmPL code in the way that the if statement
for the pointer check is “optional” in this line.
Is it an extra and redundant SmPL specification when the reference
release function could eventually be found just anywhere within
an implementation?
Will a need evolve to develop a similar source code search approach
for safer resource management with other function combinations?
Regards,
Markus
On Fri, 15 Feb 2019, Markus Elfring wrote:
> > +@search exists@
> > +local idexpression id;
> > +expression x,e,e1;
> > +position p1,p2;
> > +type T,T1,T2,T3;
> > +@@
> > +
> > +id = of_find_device_by_node@p1(x)
> > +... when != e = id
> > +if (id == NULL || ...) { ... return ...; }
> > +... when != put_device(&id->dev)
> …
> > + when != if (id) { ... put_device(&id->dev) ... }
> …
>
> I would interpret this SmPL code in the way that the if statement
> for the pointer check is “optional” in this line.
> Is it an extra and redundant SmPL specification when the reference
> release function could eventually be found just anywhere within
> an implementation?
The proposed when code is correct. It is not redundant, because it checks
for a particular control-flow pattern.
julia
>
>
> Will a need evolve to develop a similar source code search approach
> for safer resource management with other function combinations?
>
> Regards,
> Markus
>
On Fri, 15 Feb 2019, Markus Elfring wrote:
> >>> +id = of_find_device_by_node@p1(x)
> …
> >>> +if (id == NULL || ...) { ... return ...; }
> >>> +... when != put_device(&id->dev)
> >> …
> >>> + when != if (id) { ... put_device(&id->dev) ... }
> >> …
> >>
> >> I would interpret this SmPL code in the way that the if statement
> >> for the pointer check is “optional” in this line.
> >> Is it an extra and redundant SmPL specification when the reference
> >> release function could eventually be found just anywhere within
> >> an implementation?
> >
> > The proposed when code is correct.
>
> I agree that this SmPL code can work in the way it was designed.
>
>
> > It is not redundant, because it checks for a particular control-flow pattern.
>
> It took another moment until I dared to express a different software
> development opinion also on this implementation detail.
>
> Does the first SmPL when specification include the case that a call
> of the function “put_device” can occur within a branch of an if statement?
It does include that, but there is another execution path where the
put device is not present. But given the test in the if in the when code,
on that execution path id is NULL, an so there is no need to put it.
julia
>>> +id = of_find_device_by_node@p1(x)
…
>>> +if (id == NULL || ...) { ... return ...; }
>>> +... when != put_device(&id->dev)
>> …
>>> + when != if (id) { ... put_device(&id->dev) ... }
>> …
>>
>> I would interpret this SmPL code in the way that the if statement
>> for the pointer check is “optional” in this line.
>> Is it an extra and redundant SmPL specification when the reference
>> release function could eventually be found just anywhere within
>> an implementation?
>
> The proposed when code is correct.
I agree that this SmPL code can work in the way it was designed.
> It is not redundant, because it checks for a particular control-flow pattern.
It took another moment until I dared to express a different software
development opinion also on this implementation detail.
Does the first SmPL when specification include the case that a call
of the function “put_device” can occur within a branch of an if statement?
Regards,
Markus
>>> +id = of_find_device_by_node@p1(x)
…
>>> +if (id == NULL || ...) { ... return ...; }
>>> +... when != put_device(&id->dev)
>> …
>>> + when != if (id) { ... put_device(&id->dev) ... }
>> …
>>
>> I would interpret this SmPL code in the way that the if statement
>> for the pointer check is “optional” in this line.
>> Is it an extra and redundant SmPL specification when the reference
>> release function could eventually be found just anywhere within
>> an implementation?
>
> The proposed when code is correct.
I agree that this SmPL code can work in the way it was designed.
> It is not redundant, because it checks for a particular control-flow pattern.
It took another moment until I dared to express a different software
development opinion also on this implementation detail.
Does the first SmPL when specification include the case that a call
of the function “put_device” can occur within a branch of an if statement?
Regards,
Markus
>> Does the first SmPL when specification include the case that a call
>> of the function “put_device” can occur within a branch of an if statement?
>
> It does include that,
Thanks for this acknowledgement.
So it seems that you find my interpretation of this bit of SmPL code appropriate.
> but there is another execution path where the put device is not present.
It is tried to find such cases.
> But given the test in the if in the when code,
> on that execution path id is NULL, an so there is no need to put it.
I would like to point out that the function “put_device” belongs also to
the category of functions which tolerate the passing of null pointers.
https://elixir.bootlin.com/linux/v5.0-rc6/source/drivers/base/core.c#L2053
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/base/core.c?id=cb5b020a8d38f77209d0472a0fea755299a8ec78#n2053
Have we got still different software development opinions about the need
for an extra pointer check in the “second” SmPL when specification?
Regards,
Markus
On Sat, Feb 16, 2019 at 12:35 AM Wen Yang <[email protected]> wrote:
>
> The of_find_device_by_node() takes a reference to the underlying device
> structure, we should release that reference.
> The implementation of this semantic code search is:
> In a function, for a local variable returned by calling
> of_find_device_by_node(),
> a, if it is released by a function such as
> put_device()/of_dev_put()/platform_device_put() after the last use,
> it is considered that there is no reference leak;
> b, if it is passed back to the caller via
> dev_get_drvdata()/platform_get_drvdata()/get_device(), etc., the
> reference will be released in other functions, and the current function
> also considers that there is no reference leak;
> c, for the rest of the situation, the current function should release the
> reference by calling put_device, this code search will report the
> corresponding error message.
>
> By using this semantic code search, we have found some object reference leaks,
> such as:
> commit 11907e9d3533 ("ASoC: fsl-asoc-card: fix object reference leaks in
> fsl_asoc_card_probe")
> commit a12085d13997 ("mtd: rawnand: atmel: fix possible object reference leak")
> commit 11493f26856a ("mtd: rawnand: jz4780: fix possible object reference leak")
>
> There are still dozens of reference leaks in the current kernel code.
>
> Further, for the case of b, the object returned to other functions may also
> have a reference leak, we will continue to develop other cocci scripts to
> further check the reference leak.
>
> Signed-off-by: Wen Yang <[email protected]>
> Reviewed-by: Julia Lawall <[email protected]>
> Reviewed-by: Markus Elfring <[email protected]>
> Cc: Julia Lawall <[email protected]>
> Cc: Gilles Muller <[email protected]>
> Cc: Nicolas Palix <[email protected]>
> Cc: Michal Marek <[email protected]>
> Cc: Markus Elfring <[email protected]>
> Cc: Masahiro Yamada <[email protected]>
> Cc: Wen Yang <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
Applied to linux-kbuild. Thanks.
> ---
> v5->v4:
> - exchange the word “patch” by “code search”
> - add a SPDX identifierfix
> - a split string literal can be unwanted.
> - Change the content of the reported information.
> v4->v3:
> - add Masahiro Yamada
> - omit a blank line
> - split the long message parameter
> - reduce the number of metavariables
> - Describe the implementation of the semantic patch,
> explain the scenarios it can detect,
> and further software development considerations.
> v3->v2:
> - reduction of a bit of redundant C code within SmPL search specifications.
> - consider the message construction without using the extra Python variable “msg”
> v2->v1:
> - put exists after search, and then drop the when exists below.
> - should not use the same e as in the when's below.
> - Make a new type metavariable and use it to put a cast on the result of
> platform_get_drvdata.
>
> scripts/coccinelle/free/put_device.cocci | 56 ++++++++++++++++++++++++++++++++
> 1 file changed, 56 insertions(+)
> create mode 100644 scripts/coccinelle/free/put_device.cocci
>
> diff --git a/scripts/coccinelle/free/put_device.cocci b/scripts/coccinelle/free/put_device.cocci
> new file mode 100644
> index 0000000..7395697
> --- /dev/null
> +++ b/scripts/coccinelle/free/put_device.cocci
> @@ -0,0 +1,56 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/// Find missing put_device for every of_find_device_by_node.
> +///
> +// Confidence: Moderate
> +// Copyright: (C) 2018-2019 Wen Yang, ZTE.
> +// Comments:
> +// Options: --no-includes --include-headers
> +
> +virtual report
> +virtual org
> +
> +@search exists@
> +local idexpression id;
> +expression x,e,e1;
> +position p1,p2;
> +type T,T1,T2,T3;
> +@@
> +
> +id = of_find_device_by_node@p1(x)
> +... when != e = id
> +if (id == NULL || ...) { ... return ...; }
> +... when != put_device(&id->dev)
> + when != platform_device_put(id)
> + when != of_dev_put(id)
> + when != if (id) { ... put_device(&id->dev) ... }
> + when != e1 = (T)id
> + when != e1 = &id->dev
> + when != e1 = get_device(&id->dev)
> + when != e1 = (T1)platform_get_drvdata(id)
> +(
> + return
> +( id
> +| (T2)dev_get_drvdata(&id->dev)
> +| (T3)platform_get_drvdata(id)
> +);
> +| return@p2 ...;
> +)
> +
> +@script:python depends on report@
> +p1 << search.p1;
> +p2 << search.p2;
> +@@
> +
> +coccilib.report.print_report(p2[0], "ERROR: missing put_device; "
> + + "call of_find_device_by_node on line "
> + + p1[0].line
> + + ", but without a corresponding object release "
> + + "within this function.")
> +
> +@script:python depends on org@
> +p1 << search.p1;
> +p2 << search.p2;
> +@@
> +
> +cocci.print_main("of_find_device_by_node", p1)
> +cocci.print_secs("needed put_device", p2)
> --
> 2.9.5
>
--
Best Regards
Masahiro Yamada
> Applied to linux-kbuild.
How does this information fit to the published sixth version
of the discussed script for the semantic patch language?
https://lore.kernel.org/cocci/HK0PR02MB36344E2B29CEB195892F6420B2610@HK0PR02MB3634.apcprd02.prod.outlook.com/
https://systeme.lip6.fr/pipermail/cocci/2019-February/005598.html
Are possible changes still in the waiting queue for a seventh variant?
https://lore.kernel.org/cocci/[email protected]/
Regards,
Markus
> Applied to linux-kbuild.
A questionable development version was integrated for this SmPL script.
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/scripts/coccinelle/free/put_device.cocci?id=da9cfb87a44da61f2403c4312916befcb6b6d7e8
https://lore.kernel.org/cocci/[email protected]/
Now I am curious on how such a source code analysis approach will be
improved further.
Which changes will get the desired acceptance?
https://lore.kernel.org/patchwork/patch/1053979/
https://lore.kernel.org/lkml/[email protected]/
https://systeme.lip6.fr/pipermail/cocci/2019-March/005679.html
Regards,
Markus
On Sun, Mar 24, 2019 at 1:19 AM Markus Elfring <[email protected]> wrote:
>
> > Applied to linux-kbuild.
>
> A questionable development version was integrated for this SmPL script.
Sorry for my bad job.
I usually use LKML patchwork to find patches.
When I searched "Wen Yang", v6 did not show up for some reasons.
https://lore.kernel.org/patchwork/project/lkml/list/?series=&submitter=22638&state=*&q=&archive=&delegate=
So, I just thought v5 was the latest one
and I was completely missing the context.
Apology.
Masahiro
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/scripts/coccinelle/free/put_device.cocci?id=da9cfb87a44da61f2403c4312916befcb6b6d7e8
>
> https://lore.kernel.org/cocci/[email protected]/
>
>
> Now I am curious on how such a source code analysis approach will be
> improved further.
> Which changes will get the desired acceptance?
>
> https://lore.kernel.org/patchwork/patch/1053979/
> https://lore.kernel.org/lkml/[email protected]/
> https://systeme.lip6.fr/pipermail/cocci/2019-March/005679.html
>
> Regards,
> Markus
--
Best Regards
Masahiro Yamada
On Tue, 26 Mar 2019, Masahiro Yamada wrote:
> On Sun, Mar 24, 2019 at 1:19 AM Markus Elfring <[email protected]> wrote:
> >
> > > Applied to linux-kbuild.
> >
> > A questionable development version was integrated for this SmPL script.
>
>
> Sorry for my bad job.
>
> I usually use LKML patchwork to find patches.
>
> When I searched "Wen Yang", v6 did not show up for some reasons.
> https://lore.kernel.org/patchwork/project/lkml/list/?series=&submitter=22638&state=*&q=&archive=&delegate=
>
> So, I just thought v5 was the latest one
> and I was completely missing the context.
I think it is a minor detail that will have no impact in practice.
julia
>
>
> Apology.
>
> Masahiro
>
>
>
> > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/scripts/coccinelle/free/put_device.cocci?id=da9cfb87a44da61f2403c4312916befcb6b6d7e8
> >
> > https://lore.kernel.org/cocci/[email protected]/
> >
> >
> > Now I am curious on how such a source code analysis approach will be
> > improved further.
> > Which changes will get the desired acceptance?
> >
> > https://lore.kernel.org/patchwork/patch/1053979/
> > https://lore.kernel.org/lkml/[email protected]/
> > https://systeme.lip6.fr/pipermail/cocci/2019-March/005679.html
> >
> > Regards,
> > Markus
> --
> Best Regards
> Masahiro Yamada
>
> When I searched "Wen Yang", v6 did not show up for some reasons.
> https://lore.kernel.org/patchwork/project/lkml/list/?series=&submitter=22638&state=*&q=&archive=&delegate=
I find such a situation also interesting somehow.
I assume that there was another temporary technical difficulty involved
with the Linux mailing list (and related information systems).
> So, I just thought v5 was the latest one
> and I was completely missing the context.
Did any messages for the sixth (and seventh) script version reach you?
https://lore.kernel.org/cocci/[email protected]/
https://lore.kernel.org/lkml/[email protected]/
Regards,
Markus
>> So, I just thought v5 was the latest one
>> and I was completely missing the context.
>
> I think it is a minor detail
Additional implementation details were discussed also for this script
of the semantic patch language.
> that will have no impact in practice.
It will take another while to understand the relevance for the circumstances
of corresponding software adjustments better, won't it?
Will mentioned change possibilities be repeated with other update steps?
Regards,
Markus
On Tue, Mar 26, 2019 at 6:06 PM Markus Elfring <[email protected]> wrote:
>
> > When I searched "Wen Yang", v6 did not show up for some reasons.
> > https://lore.kernel.org/patchwork/project/lkml/list/?series=&submitter=22638&state=*&q=&archive=&delegate=
>
> I find such a situation also interesting somehow.
> I assume that there was another temporary technical difficulty involved
> with the Linux mailing list (and related information systems).
I think v6 fell into a crack somehow.
Check the LKML archive:
https://lkml.org/lkml/2019/2/16
I see your reply:
Re: [v6] coccinelle: semantic code search for missing put_device()Markus Elfring
But, I cannot find the v6 patch itself.
>
> > So, I just thought v5 was the latest one
> > and I was completely missing the context.
>
> Did any messages for the sixth (and seventh) script version reach you?
>
> https://lore.kernel.org/cocci/[email protected]/
I use both my corporate email address and Gmail,
and play it by ear.
I see it in my corporate mail box now.
It reached me probably because I subscribe to [email protected]
with my socionext email address.
However, this time I only checked my Gmail box,
which subscribes to linux-kernel ML, but not cocci ML.
I just skimmed over the thread, then
I missed the fact you were giving some comments about v6.
> https://lore.kernel.org/lkml/[email protected]/
Yes. I see this one.
I will apply it to fixes branch
if nobody sees a problem in it.
>
> Regards,
> Markus
--
Best Regards
Masahiro Yamada
On Tue, 26 Mar 2019, Masahiro Yamada wrote:
> On Tue, Mar 26, 2019 at 6:06 PM Markus Elfring <[email protected]> wrote:
> >
> > > When I searched "Wen Yang", v6 did not show up for some reasons.
> > > https://lore.kernel.org/patchwork/project/lkml/list/?series=&submitter=22638&state=*&q=&archive=&delegate=
> >
> > I find such a situation also interesting somehow.
> > I assume that there was another temporary technical difficulty involved
> > with the Linux mailing list (and related information systems).
>
>
> I think v6 fell into a crack somehow.
>
>
> Check the LKML archive:
> https://lkml.org/lkml/2019/2/16
>
>
> I see your reply:
>
> Re: [v6] coccinelle: semantic code search for missing put_device()Markus Elfring
>
>
> But, I cannot find the v6 patch itself.
>
>
>
> >
> > > So, I just thought v5 was the latest one
> > > and I was completely missing the context.
> >
> > Did any messages for the sixth (and seventh) script version reach you?
> >
> > https://lore.kernel.org/cocci/[email protected]/
>
>
> I use both my corporate email address and Gmail,
> and play it by ear.
>
>
> I see it in my corporate mail box now.
> It reached me probably because I subscribe to [email protected]
> with my socionext email address.
>
> However, this time I only checked my Gmail box,
> which subscribes to linux-kernel ML, but not cocci ML.
>
> I just skimmed over the thread, then
> I missed the fact you were giving some comments about v6.
>
>
>
> > https://lore.kernel.org/lkml/[email protected]/
>
>
> Yes. I see this one.
>
> I will apply it to fixes branch
> if nobody sees a problem in it.
This one is fine independent of anything else.
thanks,
julia