Re: [Cocci] [PATCH v5] coccinelle: semantic code search for missing put_device()

Re: [Cocci] [PATCH v5] coccinelle: semantic code search for missing put_device()

[Markus Elfring]

> 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
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH v5] Coccinelle: semantic code search for missing put_device()

[Markus Elfring]

> +@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
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH v5] Coccinelle: semantic code search for missing put_device()

[Julia Lawall]

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



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
>

[-- Attachment #2: Type: text/plain, Size: 136 bytes --]

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [v5] Coccinelle: semantic code search for missing put_device()

[Markus Elfring]

>>> +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
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [v5] Coccinelle: semantic code search for missing put_device()

[Markus Elfring]

>>> +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
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [v5] Coccinelle: semantic code search for missing put_device()

[Julia Lawall]

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



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

[-- Attachment #2: Type: text/plain, Size: 136 bytes --]

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [v5] Coccinelle: semantic code search for missing put_device()

[Markus Elfring]

>> 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
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH v5] coccinelle: semantic code search for missing put_device()

[Masahiro Yamada]

On Sat, Feb 16, 2019 at 12:35 AM Wen Yang <wen.yang99@zte.com.cn> 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 <wen.yang99@zte.com.cn>
> Reviewed-by: Julia Lawall <Julia.Lawall@lip6.fr>
> Reviewed-by: Markus Elfring <Markus.Elfring@web.de>
> 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: Markus Elfring <Markus.Elfring@web.de>
> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> Cc: Wen Yang <yellowriver2010@hotmail.com>
> Cc: cheng.shengyu@zte.com.cn
> Cc: cocci@systeme.lip6.fr
> Cc: linux-kernel@vger.kernel.org


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
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [v5] coccinelle: semantic code search for missing put_device()

[Markus Elfring]

> 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/201902191014156680299@zte.com.cn/

Regards,
Markus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [v5] coccinelle: semantic code search for missing put_device()

[Markus Elfring]

> 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/201902191014156680299@zte.com.cn/


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/1553321671-27749-1-git-send-email-wen.yang99@zte.com.cn/
https://systeme.lip6.fr/pipermail/cocci/2019-March/005679.html

Regards,
Markus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [v5] coccinelle: semantic code search for missing put_device()

[Masahiro Yamada]

On Sun, Mar 24, 2019 at 1:19 AM Markus Elfring <Markus.Elfring@web.de> 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/201902191014156680299@zte.com.cn/
>
>
> 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/1553321671-27749-1-git-send-email-wen.yang99@zte.com.cn/
> https://systeme.lip6.fr/pipermail/cocci/2019-March/005679.html
>
> Regards,
> Markus
--
Best Regards
Masahiro Yamada
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [v5] coccinelle: semantic code search for missing put_device()

[Julia Lawall]

On Tue, 26 Mar 2019, Masahiro Yamada wrote:

> On Sun, Mar 24, 2019 at 1:19 AM Markus Elfring <Markus.Elfring@web.de> 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/201902191014156680299@zte.com.cn/
> >
> >
> > 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/1553321671-27749-1-git-send-email-wen.yang99@zte.com.cn/
> > https://systeme.lip6.fr/pipermail/cocci/2019-March/005679.html
> >
> > Regards,
> > Markus
> --
> Best Regards
> Masahiro Yamada
>
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [v5] coccinelle: semantic code search for missing put_device()

[Markus Elfring]

> 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/201902191014156680299@zte.com.cn/

https://lore.kernel.org/lkml/1553321671-27749-1-git-send-email-wen.yang99@zte.com.cn/

Regards,
Markus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [v5] coccinelle: semantic code search for missing put_device()

[Markus Elfring]

>> 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
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [v5] coccinelle: semantic code search for missing put_device()

[Masahiro Yamada]

On Tue, Mar 26, 2019 at 6:06 PM Markus Elfring <Markus.Elfring@web.de> 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/201902191014156680299@zte.com.cn/


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 cocci@systeme.lip6.fr
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/1553321671-27749-1-git-send-email-wen.yang99@zte.com.cn/


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
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [v5] coccinelle: semantic code search for missing put_device()

[Julia Lawall]

On Tue, 26 Mar 2019, Masahiro Yamada wrote:

> On Tue, Mar 26, 2019 at 6:06 PM Markus Elfring <Markus.Elfring@web.de> 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/201902191014156680299@zte.com.cn/
>
>
> 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 cocci@systeme.lip6.fr
> 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/1553321671-27749-1-git-send-email-wen.yang99@zte.com.cn/
>
>
> 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
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci