[cocci] [PATCH v2 1/2] coccinelle: locks: add missing_mutex_init.cocci script

[cocci] [PATCH v2 1/2] coccinelle: locks: add missing_mutex_init.cocci script

[Yuan Can]

Find mutex inside struct which is possibly used without init,
provide the name of the struct and of the mutex, the position
where the struct is malloced and where the mutex get locked.

Signed-off-by: Yuan Can <yuancan@huawei.com>
---
changes in v2:
- adjust commit msg
- add Confidence: tag
- print out more information in the report
---
 .../coccinelle/locks/missing_mutex_init.cocci | 76 +++++++++++++++++++
 1 file changed, 76 insertions(+)
 create mode 100644 scripts/coccinelle/locks/missing_mutex_init.cocci

diff --git a/scripts/coccinelle/locks/missing_mutex_init.cocci b/scripts/coccinelle/locks/missing_mutex_init.cocci
new file mode 100644
index 000000000000..bf522efc7f24
--- /dev/null
+++ b/scripts/coccinelle/locks/missing_mutex_init.cocci
@@ -0,0 +1,76 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/// report missing mutex_init()
+///
+/// Report mutex used without initialize. False positives can occur
+/// when the mutex allocation and initialization happens in two
+/// different files.
+///
+// Confidence: Low
+// Copyright: (C) 2022 Huawei Technologies Co, Ltd.
+// Comments:
+// Options: --include-headers
+
+virtual org
+virtual report
+
+@r1@
+identifier s, fld;
+struct s *mm;
+@@
+mutex_init(\(&mm->fld\|&(mm->fld)\))
+
+@r2@
+identifier r1.s, r1.fld;
+position p;
+@@
+
+struct s {
+  ...
+  struct mutex fld@p;
+  ...
+};
+
+@r3@
+identifier s, fld;
+position p != {r2.p};
+@@
+
+struct s {
+  ...
+  struct mutex fld@p;
+  ...
+};
+
+@r4@
+identifier r3.fld;
+identifier r3.s;
+struct s *mm;
+position p;
+@@
+
+mutex_lock@p(&mm->fld)
+
+@r5 depends on r4@
+identifier r3.s;
+struct s *mm;
+position p;
+@@
+* mm@p = \(kmalloc\|kzalloc\|devm_kmalloc\|devm_kzalloc\)(...)
+
+@script:python depends on org@
+p << r5.p;
+p1 << r4.p;
+s << r3.s;
+fld << r3.fld;
+@@
+msg = "Mutex %s inside the struct %s malloced at line %s is possibly used at line %s without init." % (fld, s, p[0].line, p1[0].line);
+cocci.print_main(msg, p)
+
+@script:python depends on report@
+p << r5.p;
+p1 << r4.p;
+s << r3.s;
+fld << r3.fld;
+@@
+msg = "Mutex %s inside the struct %s malloced at line %s is possibly used at line %s without init." % (fld, s, p[0].line, p1[0].line);
+coccilib.report.print_report(p[0], msg)
-- 
2.17.1

[cocci] [PATCH v2 2/2] coccinelle: locks: add missing_spin_lock_init.cocci script

[Yuan Can]

Find spin lock inside struct which is possibly used without init,
provide the name of the struct and of the spin lock, the position
where the struct is malloced and where the spin lock get locked.

Signed-off-by: Yuan Can <yuancan@huawei.com>
---
changes in v2:
- adjust commit msg
- add Confidence: tag
- print out more information in the report
---
 .../locks/missing_spin_lock_init.cocci        | 79 +++++++++++++++++++
 1 file changed, 79 insertions(+)
 create mode 100644 scripts/coccinelle/locks/missing_spin_lock_init.cocci

diff --git a/scripts/coccinelle/locks/missing_spin_lock_init.cocci b/scripts/coccinelle/locks/missing_spin_lock_init.cocci
new file mode 100644
index 000000000000..0bb642e5ed4a
--- /dev/null
+++ b/scripts/coccinelle/locks/missing_spin_lock_init.cocci
@@ -0,0 +1,79 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/// report missing spin_lock_init()
+///
+/// Report spin lock used without initialize. False positives
+/// can occur when the spin lock allocation and initialization
+/// happens in two different files.
+///
+// Confidence: Low
+// Copyright: (C) 2022 Huawei Technologies Co, Ltd.
+// Comments:
+// Options: --include-headers
+
+virtual org
+virtual report
+
+@r1@
+identifier s, fld;
+struct s *mm;
+@@
+  spin_lock_init(\(&mm->fld\|&(mm->fld)\))
+
+@r2@
+identifier r1.s, r1.fld;
+position p;
+@@
+
+struct s {
+  ...
+  spinlock_t fld@p;
+  ...
+};
+
+@r3@
+identifier s, fld;
+position p != {r2.p};
+@@
+
+struct s {
+  ...
+  spinlock_t fld@p;
+  ...
+};
+
+@r4@
+identifier r3.fld;
+identifier r3.s;
+struct s *mm;
+position p;
+@@
+(
+ \(spin_lock\|spin_lock_bh\|spin_trylock\|spin_lock_irq\)(&mm->fld@p)
+|
+ spin_lock_irqsave(&mm->fld@p,...)
+)
+
+@r5 depends on r4@
+identifier r3.s;
+struct s *mm;
+position p;
+@@
+* mm@p = \(kmalloc\|kzalloc\|devm_kmalloc\|devm_kzalloc\)(...)
+
+@script:python depends on org@
+p << r5.p;
+p1 << r4.p;
+s << r3.s;
+fld << r3.fld;
+@@
+msg = "Spin lock %s inside the struct %s malloced at line %s is possibly used at line %s without init." % (fld, s, p[0].line, p1[0].line);
+cocci.print_main(msg, p)
+
+@script:python depends on report@
+p << r5.p;
+p1 << r4.p;
+s << r3.s;
+fld << r3.fld;
+@@
+msg = "Spin lock %s inside the struct %s malloced at line %s is possibly used at line %s without init." % (fld, s, p[0].line, p1[0].line);
+coccilib.report.print_report(p[0], msg)
-- 
2.17.1

Re: [cocci] [PATCH v2 1/2] coccinelle: locks: add missing_mutex_init.cocci script

[Markus Elfring]

> Find mutex inside struct which is possibly used without init,
> provide the name of the struct and of the mutex, the position
> where the struct is malloced and where the mutex get locked.


I find this commit message variant also improvable.

Will terms like “data structure” and “initialisation” become relevant?


I would appreciate answers to my previous questions.
https://lore.kernel.org/cocci/fb101290-3ec7-9170-9fec-43e1b5f54c52@web.de/
https://sympa.inria.fr/sympa/arc/cocci/2022-09/msg00022.html


I would expect a cover letter for patch series.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.0-rc6#n321



…

> +++ b/scripts/coccinelle/locks/missing_mutex_init.cocci

…

> +// Comments:


Why do you suggest the addition of an empty comment field?



> +mutex_init(\(&mm->fld\|&(mm->fld)\))


An extra SmPL disjunction is probably unnecessary because of an isomorphism.
https://gitlab.inria.fr/coccinelle/coccinelle/-/blob/3f7496ff9c2c5d4fadae1e585aa458e1a0037972/standard.iso#L382
https://github.com/coccinelle/coccinelle/blob/19ee1697bf152d37a78a20cefe148775bf4b0e0d/standard.iso#L382


+mutex_init(&(mm->fld))



…

> +@r3@
> +identifier s, fld;
> +position p != {r2.p};


Why do you think that such a SmPL constraint would be required?


> +@@
> +
> +struct s {
> +  ...
> +  struct mutex fld@p;
> +  ...
> +};


Why would the source code search repetition matter here?



…

> +@r5 depends on r4@
> +identifier r3.s;
> +struct s *mm;
> +position p;
> +@@
> +* mm@p = \(kmalloc\|kzalloc\|devm_kmalloc\|devm_kzalloc\)(...)


I would expect that the usage of the asterisk in the first column should belong
to the operation mode “context”.

Will it become relevant to detect any more memory allocation function calls?


Regards,
Markus

Re: [cocci] [PATCH v2 1/2] coccinelle: locks: add missing_mutex_init.cocci script

[Julia Lawall]

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

> > +mutex_init(\(&mm->fld\|&(mm->fld)\))
>
>
> An extra SmPL disjunction is probably unnecessary because of an isomorphism.
> https://gitlab.inria.fr/coccinelle/coccinelle/-/blob/3f7496ff9c2c5d4fadae1e585aa458e1a0037972/standard.iso#L382
> https://github.com/coccinelle/coccinelle/blob/19ee1697bf152d37a78a20cefe148775bf4b0e0d/standard.iso#L382

This is correct.  It would be better to only have the version with the
parentheses, because Coccinelle will consider the case without parentheses
anyway.

>
>
> +mutex_init(&(mm->fld))
>
>
>
> …
>
> > +@r3@
> > +identifier s, fld;
> > +position p != {r2.p};
>
>
> Why do you think that such a SmPL constraint would be required?
>
>
> > +@@
> > +
> > +struct s {
> > +  ...
> > +  struct mutex fld@p;
> > +  ...
> > +};
>
>
> Why would the source code search repetition matter here?

He is searching for a structure that is different from the ones matched
previously.

>
>
> …
>
> > +@r5 depends on r4@
> > +identifier r3.s;
> > +struct s *mm;
> > +position p;
> > +@@
> > +* mm@p = \(kmalloc\|kzalloc\|devm_kmalloc\|devm_kzalloc\)(...)
>
>
> I would expect that the usage of the asterisk in the first column should belong
> to the operation mode “context”.

This is correct.  Either the context mode should be fully supported or the
asterisk should be removed.

julia

Re: [cocci] [v2 1/2] coccinelle: locks: add missing_mutex_init.cocci script

[Markus Elfring]

>>> +@r3@
>>> +identifier s, fld;
>>> +position p != {r2.p};
>>
>> Why do you think that such a SmPL constraint would be required?
>>
>>
>>> +@@
>>> +
>>> +struct s {
>>> +  ...
>>> +  struct mutex fld@p;
>>> +  ...
>>> +};
>>
>> Why would the source code search repetition matter here?
> He is searching for a structure that is different from the ones matched previously.

How many mutexes (or spin locks) should be initialised before further data
processing can be safely performed with corresponding structures?

Regards,
Markus

Re: [cocci] [v2 1/2] coccinelle: locks: add missing_mutex_init.cocci script

[Yuan Can]

在 2022/9/21 4:24, Markus Elfring 写道:
>>>> +@r3@
>>>> +identifier s, fld;
>>>> +position p != {r2.p};
>>> Why do you think that such a SmPL constraint would be required?
>>>
>>>
>>>> +@@
>>>> +
>>>> +struct s {
>>>> +  ...
>>>> +  struct mutex fld@p;
>>>> +  ...
>>>> +};
>>> Why would the source code search repetition matter here?
>> He is searching for a structure that is different from the ones matched previously.
> How many mutexes (or spin locks) should be initialised before further data
> processing can be safely performed with corresponding structures?

In my opinion, every mutexes and spin locks needs to be initialised 
before use.

Best regards,
Yuan Can

Re: [cocci] [v2 1/2] coccinelle: locks: add missing_mutex_init.cocci script

[Julia Lawall]

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



On Thu, 22 Sep 2022, Yuan Can wrote:

>
> 在 2022/9/21 4:24, Markus Elfring 写道:
> > > > > +@r3@
> > > > > +identifier s, fld;
> > > > > +position p != {r2.p};
> > > > Why do you think that such a SmPL constraint would be required?
> > > >
> > > >
> > > > > +@@
> > > > > +
> > > > > +struct s {
> > > > > +  ...
> > > > > +  struct mutex fld@p;
> > > > > +  ...
> > > > > +};
> > > > Why would the source code search repetition matter here?
> > > He is searching for a structure that is different from the ones matched
> > > previously.
> > How many mutexes (or spin locks) should be initialised before further data
> > processing can be safely performed with corresponding structures?
>
> In my opinion, every mutexes and spin locks needs to be initialised before
> use.

I think that the concern is that you require that some spinlock is
initialized in the file before you are able to find one that is not.  It
could be good to check this with an artificial test case.

julia

Re: [cocci] [PATCH v2 1/2] coccinelle: locks: add missing_mutex_init.cocci script

[Yuan Can]

在 2022/9/21 4:13, Julia Lawall 写道:
>>> +mutex_init(\(&mm->fld\|&(mm->fld)\))
>>
>> An extra SmPL disjunction is probably unnecessary because of an isomorphism.
>> https://gitlab.inria.fr/coccinelle/coccinelle/-/blob/3f7496ff9c2c5d4fadae1e585aa458e1a0037972/standard.iso#L382
>> https://github.com/coccinelle/coccinelle/blob/19ee1697bf152d37a78a20cefe148775bf4b0e0d/standard.iso#L382
> This is correct.  It would be better to only have the version with the
> parentheses, because Coccinelle will consider the case without parentheses
> anyway.
I see, I will former one in the next version.
>
>>
>> +mutex_init(&(mm->fld))
>>
>>
>>
>> …
>>
>>> +@r3@
>>> +identifier s, fld;
>>> +position p != {r2.p};
>>
>> Why do you think that such a SmPL constraint would be required?
>>
>>
>>> +@@
>>> +
>>> +struct s {
>>> +  ...
>>> +  struct mutex fld@p;
>>> +  ...
>>> +};
>>
>> Why would the source code search repetition matter here?
> He is searching for a structure that is different from the ones matched
> previously.
>
>>
>> …
>>
>>> +@r5 depends on r4@
>>> +identifier r3.s;
>>> +struct s *mm;
>>> +position p;
>>> +@@
>>> +* mm@p = \(kmalloc\|kzalloc\|devm_kmalloc\|devm_kzalloc\)(...)
>>
>> I would expect that the usage of the asterisk in the first column should belong
>> to the operation mode “context”.
> This is correct.  Either the context mode should be fully supported or the
> asterisk should be removed.

Ok, I do not mean to support context mode, the asterisk will be remove 
in the next version.

Thanks for your reply.

Best regards,

Yuan Can

Re: [cocci] [PATCH v2 1/2] coccinelle: locks: add missing_mutex_init.cocci script

[Yuan Can]

在 2022/9/21 3:21, Markus Elfring 写道:
>> Find mutex inside struct which is possibly used without init,
>> provide the name of the struct and of the mutex, the position
>> where the struct is malloced and where the mutex get locked.
>
> I find this commit message variant also improvable.
>
> Will terms like “data structure” and “initialisation” become relevant?
>
>
> I would appreciate answers to my previous questions.
> https://lore.kernel.org/cocci/fb101290-3ec7-9170-9fec-43e1b5f54c52@web.de/
> https://sympa.inria.fr/sympa/arc/cocci/2022-09/msg00022.html
>
>
> I would expect a cover letter for patch series.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.0-rc6#n321
Thanks for the advice, I will send with cover letter in the next version.
>
>
>
> …
>
>> +++ b/scripts/coccinelle/locks/missing_mutex_init.cocci
> …
>
>> +// Comments:
>
> Why do you suggest the addition of an empty comment field?
>
>
>
>> +mutex_init(\(&mm->fld\|&(mm->fld)\))
>
> An extra SmPL disjunction is probably unnecessary because of an isomorphism.
> https://gitlab.inria.fr/coccinelle/coccinelle/-/blob/3f7496ff9c2c5d4fadae1e585aa458e1a0037972/standard.iso#L382
> https://github.com/coccinelle/coccinelle/blob/19ee1697bf152d37a78a20cefe148775bf4b0e0d/standard.iso#L382
>
>
> +mutex_init(&(mm->fld))
Thanks for pointing out this, I will remove the former one in the next 
version.
>
>
>
> …
>
>> +@r3@
>> +identifier s, fld;
>> +position p != {r2.p};
>
> Why do you think that such a SmPL constraint would be required?
>
>
>> +@@
>> +
>> +struct s {
>> +  ...
>> +  struct mutex fld@p;
>> +  ...
>> +};
>
> Why would the source code search repetition matter here?
>
>
>
> …
>
>> +@r5 depends on r4@
>> +identifier r3.s;
>> +struct s *mm;
>> +position p;
>> +@@
>> +* mm@p = \(kmalloc\|kzalloc\|devm_kmalloc\|devm_kzalloc\)(...)
>
> I would expect that the usage of the asterisk in the first column should belong
> to the operation mode “context”.
>
> Will it become relevant to detect any more memory allocation function calls?

I do not mean to support context mode, I will remove it in the next version.

Thanks for you review!

Best regards,

Yuan Can