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 <[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: Wen Yang <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
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
> Reviewed-by: Markus Elfring <[email protected]>
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: [email protected]
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
> 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
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
>
>> + 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