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

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

[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