[cocci] [PATCH v3 0/2] coccinelle: Introduce cocci script to detect missing mutext and spin lock initialization

[cocci] [PATCH v3 0/2] coccinelle: Introduce cocci script to detect missing mutext and spin lock initialization

[Yuan Can]

This series introduce two scripts to help detect the use of mutext or spin
locks without initialization.
---
changes in v3:
- remove the redundant matching expression
- remove the redundant asterisk

changes in v2:
- adjust commit msg
- add Confidence: tag
- print out more information in the report

Yuan Can (2):
  coccinelle: locks: add missing_mutex_init.cocci script
  coccinelle: locks: add missing_spin_lock_init.cocci script

 .../coccinelle/locks/missing_mutex_init.cocci | 76 ++++++++++++++++++
 .../locks/missing_spin_lock_init.cocci        | 79 +++++++++++++++++++
 2 files changed, 155 insertions(+)
 create mode 100644 scripts/coccinelle/locks/missing_mutex_init.cocci
 create mode 100644 scripts/coccinelle/locks/missing_spin_lock_init.cocci

-- 
2.17.1

[cocci] [PATCH v3 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>
---
 .../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..1a77119b1177
--- /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))
+
+@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

Re: [cocci] [PATCH v3 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?



…

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

…

> +// Comments:

Why do you insist on the addition of an empty comment field?


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


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


Can it be sufficient to detect only one kind of uninitialised system resource
for the discussed source code analysis approach?
https://cwe.mitre.org/data/definitions/909.html


> +@r4@
> +identifier r3.fld;
> +identifier r3.s;

…


I suggest to use the following SmPL code variant.

identifier r3.fld, r3.s;


> +struct s *mm;
> +position p;
> +@@
> +
> +mutex_lock@p(&mm->fld)


Would you like to support extra parentheses also at this place?

+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\)(...)

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


Regards,
Markus

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

[Julia Lawall]

> > +@r3@
> > +identifier s, fld;
> > +position p != {r2.p};
> > +@@
> > +
> > +struct s {
> > +  ...
> > +  struct mutex fld@p;
> > +  ...
> > +};
>
>
> How many mutexes (or spin locks) should be initialised before further data
> processing can be safely performed with corresponding structures?

I tried the semantic patch on this file:

struct s {
  struct mutex fld;
};

int main () {
  struct s *mm;
  mm = kmalloc();
  mutex_lock(&mm->fld);
}

and it reported the expected error message.  So what exactly is the
concern, Markus?

julia

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

[Markus Elfring]

>>> +@r3@
>>> +identifier s, fld;
>>> +position p != {r2.p};
>>> +@@
>>> +
>>> +struct s {
>>> +  ...
>>> +  struct mutex fld@p;
>>> +  ...
>>> +};
>>
>> How many mutexes (or spin locks) should be initialised before further data
>> processing can be safely performed with corresponding structures?
> I tried the semantic patch on this file:
>
> struct s {
>   struct mutex fld;
> };
>
> int main () {
>   struct s *mm;
>   mm = kmalloc();
>   mutex_lock(&mm->fld);
> }
>
> and it reported the expected error message.


This test result might be nice.


> So what exactly is the concern, Markus?


I just suggest to check once more if it would be really required to determine
the position for a data structure member twice (as proposed by the SmPL rules
“r2” and “r3”).

Would you like to compare the data processing for a SmPL code variant
like the following?

@find_member@
identifier s1, s2, f;
position p;
@@
 struct s1
 {
 ...
 struct s2 f@p;
 ...
 };

@show_member@
identifier s1, s2, f;
position p != find_member.p;
@@
 struct s1
 {
 ...
*struct s2 f@p;
 ...
 };


# How do you think about the handling of multiple members within data structures?

# How much does it matter here that curly brackets are used for a proposed SmPL constraint?



I got another development concern for the presented algorithm.
Why is a data initialisation function call searched in the first SmPL rule at all?

I imagine that potentially missing function calls can be determined also
by other approaches.

Regards,
Markus

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

[Julia Lawall]

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



On Fri, 23 Sep 2022, Markus Elfring wrote:

> >>> +@r3@
> >>> +identifier s, fld;
> >>> +position p != {r2.p};
> >>> +@@
> >>> +
> >>> +struct s {
> >>> +  ...
> >>> +  struct mutex fld@p;
> >>> +  ...
> >>> +};
> >>
> >> How many mutexes (or spin locks) should be initialised before further data
> >> processing can be safely performed with corresponding structures?
> > I tried the semantic patch on this file:
> >
> > struct s {
> >   struct mutex fld;
> > };
> >
> > int main () {
> >   struct s *mm;
> >   mm = kmalloc();
> >   mutex_lock(&mm->fld);
> > }
> >
> > and it reported the expected error message.
>
>
> This test result might be nice.
>
>
> > So what exactly is the concern, Markus?
>
>
> I just suggest to check once more if it would be really required to determine
> the position for a data structure member twice (as proposed by the SmPL rules
> “r2” and “r3”).
>
> Would you like to compare the data processing for a SmPL code variant
> like the following?
>
> @find_member@
> identifier s1, s2, f;
> position p;
> @@
>  struct s1
>  {
>  ...
>  struct s2 f@p;
>  ...
>  };
>
> @show_member@
> identifier s1, s2, f;
> position p != find_member.p;
> @@
>  struct s1
>  {
>  ...
> *struct s2 f@p;
>  ...
>  };
>
>
> # How do you think about the handling of multiple members within data structures?

There should be no problem with this.

> # How much does it matter here that curly brackets are used for a proposed SmPL constraint?

I have no idea what "How much does it matter" mean.  {} are used because
that's how struct types are declared.

>
>
> I got another development concern for the presented algorithm.
> Why is a data initialisation function call searched in the first SmPL rule at all?

Because he wants to find the fields for which mutex_init is already called
and to not report messages for them.  That is the whole point of the
semantic patch.

julia

>
> I imagine that potentially missing function calls can be determined also
> by other approaches.
>
> Regards,
> Markus
>
>

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

[Markus Elfring]

>> # How do you think about the handling of multiple members within data structures?
> There should be no problem with this.


Would it be relevant to use the SmPL construct “<+... … ...+>”?



>> # How much does it matter here that curly brackets are used for a proposed SmPL constraint?
> I have no idea what "How much does it matter" mean.  {} are used because
> that's how struct types are declared.


Please take another look at mentioned implementation details for
the clarification of such communication difficulties.

A)
position p != {r2.p};

B)
position p != find_member.p;



>> I got another development concern for the presented algorithm.
>> Why is a data initialisation function call searched in the first SmPL rule at all?
> Because he wants to find the fields for which mutex_init is already called
> and to not report messages for them.  That is the whole point of the
> semantic patch.


How do you think about an opposite source code search order?

Would you like to search for function calls which require initialised
data structures before any additional source code analysis?

Regards,
Markus

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

[Julia Lawall]

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



On Fri, 23 Sep 2022, Markus Elfring wrote:

> >> # How do you think about the handling of multiple members within data structures?
> > There should be no problem with this.
>
>
> Would it be relevant to use the SmPL construct “<+... … ...+>”?

Not in a structure definition.

>
>
>
> >> # How much does it matter here that curly brackets are used for a proposed SmPL constraint?
> > I have no idea what "How much does it matter" mean.  {} are used because
> > that's how struct types are declared.
>
>
> Please take another look at mentioned implementation details for
> the clarification of such communication difficulties.
>
> A)
> position p != {r2.p};
>
> B)
> position p != find_member.p;

OK, both are fine.  If there are multiple positions that p should be
different from then the {} would be required.

>
>
> >> I got another development concern for the presented algorithm.
> >> Why is a data initialisation function call searched in the first SmPL rule at all?
> > Because he wants to find the fields for which mutex_init is already called
> > and to not report messages for them.  That is the whole point of the
> > semantic patch.
>
>
> How do you think about an opposite source code search order?
>
> Would you like to search for function calls which require initialised
> data structures before any additional source code analysis?

It doesn't make much sense to have a mutex without initializing it.

julia

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

[Markus Elfring]

>> Would it be relevant to use the SmPL construct “<+... … ...+>”?
> Not in a structure definition.


Will further explanations become helpful for this functionality?



>> Please take another look at mentioned implementation details for
>> the clarification of such communication difficulties.
>>
>> A)
>> position p != {r2.p};
>>
>> B)
>> position p != find_member.p;
> OK, both are fine.  If there are multiple positions that p should be
> different from then the {} would be required.


I got the impression that different test results are involved.

Do you expect that identical results should be produced
(also according to the selection of such a SmPL constraint variant)?

Can this SmPL constraint be omitted because of an other data processing approach?


>> How do you think about an opposite source code search order?
>>
>> Would you like to search for function calls which require initialised
>> data structures before any additional source code analysis?
> It doesn't make much sense to have a mutex without initializing it.

It is nice that some programming mistakes can be found also by the means of
the semantic patch language.
Limitations of the Coccinelle software will trigger further development
considerations accordingly, won't they?

Can you find a source code search order more desirable than other directions?

Regards,
Markus

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

[Markus Elfring]

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


Would you like to support another case distinction?

How do you think about to handle variable initialisations besides assignments
with a corresponding SmPL code variant?

Regards,
Markus

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

[Markus Elfring]

…
> +++ b/scripts/coccinelle/locks/missing_mutex_init.cocci
…
> +/// Report mutex used without initialize. False positives can occur
> +/// when the mutex allocation and initialization happens in two
> +/// different files.


How do you think about to use the following wording variant?


+/// Report mutexes which are used without initialization.
+/// False positives can occur when
+/// * the mutex allocation and initialization will be performed in
+///   two different source files.
+/// * inappropriate data types will be found in the source code
+///   because of unsafe scope resolution.


> +///
> +// Confidence: Low


Will this categorisation trigger further development considerations?



…

> +@r1@
> +identifier s, fld;
> +struct s *mm;
> +@@
> +mutex_init(&(mm->fld))
> +
> +@r2@
> +identifier r1.s, r1.fld;
> +position p;
> +@@
> +
> +struct s {
> +  ...
> +  struct mutex fld@p;
> +  ...
> +};


How “safe” do you find the scope resolution for the searched data types?


Would you like to explain data processing consequences a bit more for
inheriting metavariables after these SmPL rules would occasionally not match?


…

> +@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)

I suggest to use the following statement variant.

coccilib.report.print_report(p[0],
                             "Mutex “%s” within the structure “%s” (which was allocated at line %s) is possibly used at line %s without initialization."
                             % (fld, s, p[0].line, p1[0].line))


Regards,
Markus

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

[Markus Elfring]

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

…

> +@r1@
…
> +@r2@
…
> +@r3@
…
> +@r4@
…
> +@r5 depends on r4@
…
> +@script:python depends on org@
…
> +cocci.print_main(msg, p)
> +
> +@script:python depends on report@
…
> +coccilib.report.print_report(p[0], msg)


I would like to point the need out for specifying dependencies also for more
SmPL rules according to supported operation modes before final messages
will be displayed (by the means of the selected scripting languages).

Would you like to achieve any improvements also for a feature request
like the following?

2019-04-07
Support for SmPL rule groups
https://github.com/coccinelle/coccinelle/issues/164

Regards,
Markus

[cocci] [PATCH v3 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>
---
 .../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..9724d348e28e
--- /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))
+
+@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 v3 0/2] coccinelle: Introduce cocci script to detect missing mutext and spin lock initialization

[Markus Elfring]

> This series introduce two scripts to help detect the use of mutext or spin
> locks without initialization.


I suggest to avoid a few typos in this wording.



> Yuan Can (2):
>   coccinelle: locks: add missing_mutex_init.cocci script
>   coccinelle: locks: add missing_spin_lock_init.cocci script

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

Would anybody get into the development mood to generalise and extend the source code
search pattern besides proposed adjustments for the handling of two APIs?
https://cwe.mitre.org/data/definitions/909.html

How many data structures would need further initialisations after memory areas
were allocated?

Regards,
Markus