linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3] coccinelle: semantic patch for missing put_device()
@ 2019-02-13  6:23 Wen Yang
  2019-02-13 15:40 ` Markus Elfring
  2019-02-13 18:29 ` [PATCH v3] Coccinelle: " Markus Elfring
  0 siblings, 2 replies; 5+ messages in thread
From: Wen Yang @ 2019-02-13  6:23 UTC (permalink / raw)
  To: Julia.Lawall
  Cc: Gilles.Muller, nicolas.palix, michal.lkml, Markus.Elfring,
	xue.zhihong, wang.yi59, Wen Yang, Wen Yang, cocci, linux-kernel

The of_find_device_by_node() takes a reference to the underlying device
structure, we should release that reference.
By using this semantic patch, 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.

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: Wen Yang <yellowriver2010@hotmail.com>
Cc: cocci@systeme.lip6.fr
Cc: linux-kernel@vger.kernel.org
---
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 | 52 ++++++++++++++++++++++++++++++++
 1 file changed, 52 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..9a94b8d
--- /dev/null
+++ b/scripts/coccinelle/free/put_device.cocci
@@ -0,0 +1,52 @@
+/// Find missing put_device for every of_find_device_by_node.
+///
+// Confidence: Moderate
+// Copyright: (C) 2018-2019 Wen Yang, ZTE.  GPLv2.
+// Comments:
+// Options: --no-includes --include-headers
+
+virtual report
+virtual org
+
+@search exists@
+local idexpression id;
+expression x,e,e1,e2,e3,e4;
+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 != e2 = &id->dev
+    when != e3 = get_device(&id->dev)
+    when != e4 = (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; of_find_device_by_node on line " + p1[0].line + " and return without releasing.")
+
+@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


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v3] coccinelle: semantic patch for missing put_device()
  2019-02-13  6:23 [PATCH v3] coccinelle: semantic patch for missing put_device() Wen Yang
@ 2019-02-13 15:40 ` Markus Elfring
  2019-02-13 18:29 ` [PATCH v3] Coccinelle: " Markus Elfring
  1 sibling, 0 replies; 5+ messages in thread
From: Markus Elfring @ 2019-02-13 15:40 UTC (permalink / raw)
  To: Wen Yang
  Cc: Coccinelle, kernel-janitors, LKML, Gilles Muller, Julia Lawall,
	Masahiro Yamada, Michal Marek, Nicolas Palix, Wen Yang,
	Xue Zhihong, Yi Wang

> Reviewed-by: Markus Elfring <Markus.Elfring@web.de>

I have got doubts that my code review comments qualify already
for this tag.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=1f947a7a011fcceb14cb912f5481a53b18f1879a#n565


> Cc: cocci@systeme.lip6.fr

I assume that Masahiro Yamada will also need to get explicitly informed
for a possible patch integration.


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

I find it nice that you would like to take this information into account.


> +    when != e4 = (T1)platform_get_drvdata(id)
> +(
> +

Should a blank line be omitted at such a source code place?


> +  return
> +(    id
> +|    (T2)dev_get_drvdata(&id->dev)
> +|    (T3)platform_get_drvdata(id)
> +);

It seems that you would like to express a different coding style.
Would anybody like to reconsider the position once more for semicolons
in nested disjunctions for such a SmPL search specification?


> +coccilib.report.print_report(p2[0], "ERROR: missing put_device; of_find_device_by_node on line " + p1[0].line + " and return without releasing.")

Your willingness for such a rearrangement is interesting.
How do you think about to move the long message parameter to a subsequent line?

Regards,
Markus

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3] Coccinelle: semantic patch for missing put_device()
  2019-02-13  6:23 [PATCH v3] coccinelle: semantic patch for missing put_device() Wen Yang
  2019-02-13 15:40 ` Markus Elfring
@ 2019-02-13 18:29 ` Markus Elfring
  2019-02-13 19:52   ` Julia Lawall
  1 sibling, 1 reply; 5+ messages in thread
From: Markus Elfring @ 2019-02-13 18:29 UTC (permalink / raw)
  To: Wen Yang
  Cc: Gilles Muller, Julia Lawall, Masahiro Yamada, Michal Marek,
	Nicolas Palix, Wen Yang, Xue Zhihong, Yi Wang, linux-kernel,
	kernel-janitors, Coccinelle

> The of_find_device_by_node() takes a reference to the underlying device
> structure, we should release that reference.

I have got another concern for further software development considerations.

How do you think about to describe here if it can be determined
by source code analysis that the desired release should be performed
only in the same function implementation (or not)?

How much does this aspect influence the source code search confidence?


> +    when != e1 = (T)id
> +    when != e2 = &id->dev
> +    when != e3 = get_device(&id->dev)
> +    when != e4 = (T1)platform_get_drvdata(id)

I have got another idea for a bit of software fine-tuning at such a place.
I am unsure if it can become relevant to reduce the number of metavariables
here by introducing a SmPL disjunction.

+    when != ex = \( (T)id \| &id->dev \| get_device(&id->dev) \| (T1)platform_get_drvdata(id) \)


Regards,
Markus

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3] Coccinelle: semantic patch for missing put_device()
  2019-02-13 18:29 ` [PATCH v3] Coccinelle: " Markus Elfring
@ 2019-02-13 19:52   ` Julia Lawall
  2019-02-14  8:13     ` [v3] " Markus Elfring
  0 siblings, 1 reply; 5+ messages in thread
From: Julia Lawall @ 2019-02-13 19:52 UTC (permalink / raw)
  To: Markus Elfring
  Cc: Wen Yang, Gilles Muller, Masahiro Yamada, Michal Marek,
	Nicolas Palix, Wen Yang, Xue Zhihong, Yi Wang, linux-kernel,
	kernel-janitors, Coccinelle



On Wed, 13 Feb 2019, Markus Elfring wrote:

> > The of_find_device_by_node() takes a reference to the underlying device
> > structure, we should release that reference.
>
> I have got another concern for further software development considerations.
>
> How do you think about to describe here if it can be determined
> by source code analysis that the desired release should be performed
> only in the same function implementation (or not)?
>
> How much does this aspect influence the source code search confidence?
>
>
> > +    when != e1 = (T)id
> > +    when != e2 = &id->dev
> > +    when != e3 = get_device(&id->dev)
> > +    when != e4 = (T1)platform_get_drvdata(id)
>
> I have got another idea for a bit of software fine-tuning at such a place.
> I am unsure if it can become relevant to reduce the number of metavariables
> here by introducing a SmPL disjunction.
>
> +    when != ex = \( (T)id \| &id->dev \| get_device(&id->dev) \| (T1)platform_get_drvdata(id) \)

There is no need for the disjunction.  There is also no need for the
different variables.  Different variables are only needed when the when
conditions are on different ...s

julia

>
>
> Regards,
> Markus
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [v3] Coccinelle: semantic patch for missing put_device()
  2019-02-13 19:52   ` Julia Lawall
@ 2019-02-14  8:13     ` Markus Elfring
  0 siblings, 0 replies; 5+ messages in thread
From: Markus Elfring @ 2019-02-14  8:13 UTC (permalink / raw)
  To: Julia Lawall, Wen Yang
  Cc: Gilles Muller, Masahiro Yamada, Michal Marek, Nicolas Palix,
	Wen Yang, Xue Zhihong, Yi Wang, linux-kernel, kernel-janitors,
	Coccinelle

>> +    when != ex = \( (T)id \| &id->dev \| get_device(&id->dev) \| (T1)platform_get_drvdata(id) \)
>
> There is no need for the disjunction.  There is also no need for the
> different variables.

Really?

Would you like to distinguish the shown assignment expressions anyhow?


> Different variables are only needed when the when conditions are on
> different ...s

I find that this information will need further clarification.
Will any additional adjustments become relevant for such SmPL when specifications?

Regards,
Markus

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2019-02-14  8:14 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-13  6:23 [PATCH v3] coccinelle: semantic patch for missing put_device() Wen Yang
2019-02-13 15:40 ` Markus Elfring
2019-02-13 18:29 ` [PATCH v3] Coccinelle: " Markus Elfring
2019-02-13 19:52   ` Julia Lawall
2019-02-14  8:13     ` [v3] " Markus Elfring

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).