[PATCH v3] provide rule for finding refcounters

[PATCH v3] provide rule for finding refcounters

[Elena Reshetova]

changes in v3:
Removed unnessesary rule 4 conditions pointed by Julia.

changes in v2:
Following the suggestion from Julia the first rule is split into
2. The output does not differ that much between these two versions,
but rule became more precise.

Elena Reshetova (1):
  Coccinelle: add atomic_as_refcounter script

 scripts/coccinelle/api/atomic_as_refcounter.cocci | 133 ++++++++++++++++++++++
 1 file changed, 133 insertions(+)
 create mode 100644 scripts/coccinelle/api/atomic_as_refcounter.cocci

-- 
2.7.4

[Cocci] [PATCH v3] provide rule for finding refcounters

[Elena Reshetova]

changes in v3:
Removed unnessesary rule 4 conditions pointed by Julia.

changes in v2:
Following the suggestion from Julia the first rule is split into
2. The output does not differ that much between these two versions,
but rule became more precise.

Elena Reshetova (1):
  Coccinelle: add atomic_as_refcounter script

 scripts/coccinelle/api/atomic_as_refcounter.cocci | 133 ++++++++++++++++++++++
 1 file changed, 133 insertions(+)
 create mode 100644 scripts/coccinelle/api/atomic_as_refcounter.cocci

-- 
2.7.4

[PATCH] Coccinelle: add atomic_as_refcounter script

[Elena Reshetova]

atomic_as_refcounter.cocci script allows detecting
cases when refcount_t type and API should be used
instead of atomic_t.

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
---
 scripts/coccinelle/api/atomic_as_refcounter.cocci | 133 ++++++++++++++++++++++
 1 file changed, 133 insertions(+)
 create mode 100644 scripts/coccinelle/api/atomic_as_refcounter.cocci

diff --git a/scripts/coccinelle/api/atomic_as_refcounter.cocci b/scripts/coccinelle/api/atomic_as_refcounter.cocci
new file mode 100644
index 0000000..64c97d1
--- /dev/null
+++ b/scripts/coccinelle/api/atomic_as_refcounter.cocci
@@ -0,0 +1,133 @@
+// Check if refcount_t type and API should be used
+// instead of atomic_t type when dealing with refcounters
+//
+// Copyright (c) 2016-2017, Elena Reshetova, Intel Corporation
+//
+// Confidence: Moderate
+// URL: http://coccinelle.lip6.fr/
+// Options: --include-headers --very-quiet
+
+virtual report
+
+@r1 exists@
+identifier a, x, y;
+position p1, p2;
+identifier fname =~ ".*free.*";
+identifier fname2 =~ ".*destroy.*";
+identifier fname3 =~ ".*del.*";
+identifier fname4 =~ ".*queue_work.*";
+identifier fname5 =~ ".*schedule_work.*";
+identifier fname6 =~ ".*call_rcu.*";
+
+@@
+
+(
+ atomic_dec_and_test@p1(&(a)->x)
+|
+ atomic_dec_and_lock@p1(&(a)->x, ...)
+|
+ atomic_long_dec_and_lock@p1(&(a)->x, ...)
+|
+ atomic_long_dec_and_test@p1(&(a)->x)
+|
+ atomic64_dec_and_test@p1(&(a)->x)
+|
+ local_dec_and_test@p1(&(a)->x)
+)
+...
+(
+ fname@p2(a, ...);
+|
+ fname2@p2(...);
+|
+ fname3@p2(...);
+|
+ fname4@p2(...);
+|
+ fname5@p2(...);
+|
+ fname6@p2(...);
+)
+
+
+@script:python depends on report@
+p1 << r1.p1;
+p2 << r1.p2;
+@@
+msg = "atomic_dec_and_test variation before object free at line %s."
+coccilib.report.print_report(p1[0], msg % (p2[0].line))
+
+@r4 exists@
+identifier a, x, y;
+position p1, p2;
+identifier fname =~ ".*free.*";
+
+@@
+
+(
+ atomic_dec_and_test@p1(&(a)->x)
+|
+ atomic_dec_and_lock@p1(&(a)->x, ...)
+|
+ atomic_long_dec_and_lock@p1(&(a)->x, ...)
+|
+ atomic_long_dec_and_test@p1(&(a)->x)
+|
+ atomic64_dec_and_test@p1(&(a)->x)
+|
+ local_dec_and_test@p1(&(a)->x)
+)
+...
+y=a
+...
+(
+ fname@p2(y, ...);
+)
+
+
+@script:python depends on report@
+p1 << r4.p1;
+p2 << r4.p2;
+@@
+msg = "atomic_dec_and_test variation before object free at line %s."
+coccilib.report.print_report(p1[0], msg % (p2[0].line))
+
+@r2 exists@
+identifier a, x;
+position p1;
+@@
+
+(
+atomic_add_unless(&(a)->x,-1,1)@p1
+|
+atomic_long_add_unless(&(a)->x,-1,1)@p1
+|
+atomic64_add_unless(&(a)->x,-1,1)@p1
+)
+
+@script:python depends on report@
+p1 << r2.p1;
+@@
+msg = "atomic_add_unless"
+coccilib.report.print_report(p1[0], msg)
+
+@r3 exists@
+identifier x;
+position p1;
+@@
+
+(
+x = atomic_add_return@p1(-1, ...);
+|
+x = atomic_long_add_return@p1(-1, ...);
+|
+x = atomic64_add_return@p1(-1, ...);
+)
+
+@script:python depends on report@
+p1 << r3.p1;
+@@
+msg = "x = atomic_add_return(-1, ...)"
+coccilib.report.print_report(p1[0], msg)
+
+
-- 
2.7.4

[Cocci] [PATCH] Coccinelle: add atomic_as_refcounter script

[Elena Reshetova]

atomic_as_refcounter.cocci script allows detecting
cases when refcount_t type and API should be used
instead of atomic_t.

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
---
 scripts/coccinelle/api/atomic_as_refcounter.cocci | 133 ++++++++++++++++++++++
 1 file changed, 133 insertions(+)
 create mode 100644 scripts/coccinelle/api/atomic_as_refcounter.cocci

diff --git a/scripts/coccinelle/api/atomic_as_refcounter.cocci b/scripts/coccinelle/api/atomic_as_refcounter.cocci
new file mode 100644
index 0000000..64c97d1
--- /dev/null
+++ b/scripts/coccinelle/api/atomic_as_refcounter.cocci
@@ -0,0 +1,133 @@
+// Check if refcount_t type and API should be used
+// instead of atomic_t type when dealing with refcounters
+//
+// Copyright (c) 2016-2017, Elena Reshetova, Intel Corporation
+//
+// Confidence: Moderate
+// URL: http://coccinelle.lip6.fr/
+// Options: --include-headers --very-quiet
+
+virtual report
+
+ at r1 exists@
+identifier a, x, y;
+position p1, p2;
+identifier fname =~ ".*free.*";
+identifier fname2 =~ ".*destroy.*";
+identifier fname3 =~ ".*del.*";
+identifier fname4 =~ ".*queue_work.*";
+identifier fname5 =~ ".*schedule_work.*";
+identifier fname6 =~ ".*call_rcu.*";
+
+@@
+
+(
+ atomic_dec_and_test at p1(&(a)->x)
+|
+ atomic_dec_and_lock at p1(&(a)->x, ...)
+|
+ atomic_long_dec_and_lock at p1(&(a)->x, ...)
+|
+ atomic_long_dec_and_test at p1(&(a)->x)
+|
+ atomic64_dec_and_test at p1(&(a)->x)
+|
+ local_dec_and_test at p1(&(a)->x)
+)
+...
+(
+ fname at p2(a, ...);
+|
+ fname2 at p2(...);
+|
+ fname3 at p2(...);
+|
+ fname4 at p2(...);
+|
+ fname5 at p2(...);
+|
+ fname6 at p2(...);
+)
+
+
+ at script:python depends on report@
+p1 << r1.p1;
+p2 << r1.p2;
+@@
+msg = "atomic_dec_and_test variation before object free at line %s."
+coccilib.report.print_report(p1[0], msg % (p2[0].line))
+
+ at r4 exists@
+identifier a, x, y;
+position p1, p2;
+identifier fname =~ ".*free.*";
+
+@@
+
+(
+ atomic_dec_and_test@p1(&(a)->x)
+|
+ atomic_dec_and_lock at p1(&(a)->x, ...)
+|
+ atomic_long_dec_and_lock at p1(&(a)->x, ...)
+|
+ atomic_long_dec_and_test at p1(&(a)->x)
+|
+ atomic64_dec_and_test at p1(&(a)->x)
+|
+ local_dec_and_test at p1(&(a)->x)
+)
+...
+y=a
+...
+(
+ fname at p2(y, ...);
+)
+
+
+ at script:python depends on report@
+p1 << r4.p1;
+p2 << r4.p2;
+@@
+msg = "atomic_dec_and_test variation before object free at line %s."
+coccilib.report.print_report(p1[0], msg % (p2[0].line))
+
+@r2 exists@
+identifier a, x;
+position p1;
+@@
+
+(
+atomic_add_unless(&(a)->x,-1,1)@p1
+|
+atomic_long_add_unless(&(a)->x,-1,1)@p1
+|
+atomic64_add_unless(&(a)->x,-1,1)@p1
+)
+
+ at script:python depends on report@
+p1 << r2.p1;
+@@
+msg = "atomic_add_unless"
+coccilib.report.print_report(p1[0], msg)
+
+ at r3 exists@
+identifier x;
+position p1;
+@@
+
+(
+x = atomic_add_return at p1(-1, ...);
+|
+x = atomic_long_add_return at p1(-1, ...);
+|
+x = atomic64_add_return at p1(-1, ...);
+)
+
+@script:python depends on report@
+p1 << r3.p1;
+@@
+msg = "x = atomic_add_return(-1, ...)"
+coccilib.report.print_report(p1[0], msg)
+
+
-- 
2.7.4

[Cocci] Coccinelle: add atomic_as_refcounter script

[SF Markus Elfring]

Dear Elena,

Why do you insist to use the variables ?fname2? till ?fname6? in
this evolving SmPL script (instead of merging them into a single one
with a special constraint)?

How do you think about to omit the cover letter for the addition
of such a script (when the change log can be integrated into
the same message for your update suggestion)?

Regards,
Markus

[Cocci] Coccinelle: add atomic_as_refcounter script

[Reshetova, Elena]

> Dear Elena,

Hi Markus!

> 
> Why do you insist to use the variables ?fname2? till ?fname6? in
> this evolving SmPL script (instead of merging them into a single one
> with a special constraint)?

I am pretty new to Coccinelle and Julia has recommended against this approach, so I was merely following her advice. 
I really do not understand the implications of the change as well as she does. 
Your approach would certainly look prettier script-wise, but I don't want to cause any undesirable side-effects. 

> 
> How do you think about to omit the cover letter for the addition
> of such a script (when the change log can be integrated into
> the same message for your update suggestion)?

Sorry, not sure I understood this. Could you please explain more? 
> 
> Regards,
> Markus

[Cocci] Coccinelle: add atomic_as_refcounter script

[SF Markus Elfring]

>> Why do you insist to use the variables ?fname2? till ?fname6? in
>> this evolving SmPL script (instead of merging them into a single one
>> with a special constraint)?
> 
> I am pretty new to Coccinelle

This is fine. I am curious then how your interests will evolve further
in this software area.


> and Julia has recommended against this approach,

She showed a special response.

If you search in the mailing list archive, you will find some details
where she had different opinions than me for some technical aspects
during usual discussions.


> so I was merely following her advice.

Not completely. - You expressed a need to use regular expressions for
constraints in the SmPL rule ?r1? already.
I suggest to take another look at available design choices.
Under which circumstances will it be helpful to switch between
involved programming languages?


> I really do not understand the implications of the change as well 
> as she does.

My software understanding is also still evolving in this area.

* Some clarification approaches did not reach the point so that
  missing information could be resolved in a desired way.

* Some knowledge did not find their way from research papers and
  corresponding OCaml source code into other documentation formats
  for a better understanding of system dependencies so far.


> Your approach would certainly look prettier script-wise,

Thanks for such a feedback.


> but I don't want to cause any undesirable side-effects.

You would like to implement a special search (and transformation) pattern.

* Are you keen to find the ?effects? out which are really desirable for you?

* Would you try any precise system tests out for the determination of
  preferred run time behaviour?


>> How do you think about to omit the cover letter for the addition
>> of such a script (when the change log can be integrated into
>> the same message for your update suggestion)?
> 
> Sorry, not sure I understood this. Could you please explain more?

You replied with the message ?[PATCH] Coccinelle: add atomic_as_refcounter
script? (from ?Aug 16 13:52:22 CEST 2017?) to your own message
?[PATCH v3] provide rule for finding refcounters? (from ?Aug 16
13:52:21 CEST 2017?), didn't you?
https://systeme.lip6.fr/pipermail/cocci/2017-August/004333.html
https://systeme.lip6.fr/pipermail/cocci/2017-August/004334.html

Will it be sufficient to send only SmPL script variants for
further clarification to achieve the desired consensus?


How do you think about to reconsider another implementation detail?
Example:
Why do you see a need to enclose the identifier ?a? by parentheses?

Regards,
Markus

Re: [PATCH] Coccinelle: add atomic_as_refcounter script

[Julia Lawall]

> +identifier fname =~ ".*free.*";
> +identifier fname2 =~ ".*destroy.*";
> +identifier fname3 =~ ".*del.*";
> +identifier fname4 =~ ".*queue_work.*";
> +identifier fname5 =~ ".*schedule_work.*";
> +identifier fname6 =~ ".*call_rcu.*";

Personally, I find the above regular expressions much easier to understand
than the merged version that Markus proposed.  But the performance issue is
only on whether to use regular expressions or not.  If you use regular
expressions, Coccinelle will not do some optimizations.  But once you
decide to use regular expressions, the performance hit is already taken -
for a good cause here, to my understanding.  So just put whatever you find
convenient, in terms of readability, precision, etc.  It seems that del is
not very precise, because it is a substring of multiple words with
different meanings.  Maybe it should be improved, or maybe one can just
live with the false positives (eg delay), if they actually are false
positives.

julia

[Cocci] [PATCH] Coccinelle: add atomic_as_refcounter script

[Julia Lawall]

> +identifier fname =~ ".*free.*";
> +identifier fname2 =~ ".*destroy.*";
> +identifier fname3 =~ ".*del.*";
> +identifier fname4 =~ ".*queue_work.*";
> +identifier fname5 =~ ".*schedule_work.*";
> +identifier fname6 =~ ".*call_rcu.*";

Personally, I find the above regular expressions much easier to understand
than the merged version that Markus proposed.  But the performance issue is
only on whether to use regular expressions or not.  If you use regular
expressions, Coccinelle will not do some optimizations.  But once you
decide to use regular expressions, the performance hit is already taken -
for a good cause here, to my understanding.  So just put whatever you find
convenient, in terms of readability, precision, etc.  It seems that del is
not very precise, because it is a substring of multiple words with
different meanings.  Maybe it should be improved, or maybe one can just
live with the false positives (eg delay), if they actually are false
positives.

julia

RE: [PATCH] Coccinelle: add atomic_as_refcounter script

[Reshetova, Elena]

> > +identifier fname =~ ".*free.*";
> > +identifier fname2 =~ ".*destroy.*";
> > +identifier fname3 =~ ".*del.*";
> > +identifier fname4 =~ ".*queue_work.*";
> > +identifier fname5 =~ ".*schedule_work.*";
> > +identifier fname6 =~ ".*call_rcu.*";
> 
> Personally, I find the above regular expressions much easier to understand
> than the merged version that Markus proposed.  

I really don't have a strong opinion on the presentation side. 
One is more compact, the above one perhaps a bit clearer for unexperienced reader (like myself). 
Sometimes when I try to read patterns from cocci folder,
it takes me really a while to understand what is happening, which is not the case
with simple expression above. I have just considered myself to be too new into this
and therefore sticked to simple expressions to make sure I am minimizing functional mistakes. 

But the performance issue is
> only on whether to use regular expressions or not.  If you use regular
> expressions, Coccinelle will not do some optimizations.  But once you
> decide to use regular expressions, the performance hit is already taken -
> for a good cause here, to my understanding.  

Ok, so then performance is not even a factor. Thank you for the explanation! 

So just put whatever you find
> convenient, in terms of readability, precision, etc.  It seems that del is
> not very precise, because it is a substring of multiple words with
> different meanings.  Maybe it should be improved, or maybe one can just
> live with the false positives (eg delay), if they actually are false
> positives.

This is the problem that some of them might be and some not. 
I can call the queuing works explicitly:
identifier fname4 =~ ".*queue_work.*";
identifier fname5 =~ ".*queue_delayed_work.*";

Then there is no need to match "delay", but I still like to match both "delete" and "del". 

Best Regards,
Elena.
> 
> julia

[Cocci] [PATCH] Coccinelle: add atomic_as_refcounter script

[Reshetova, Elena]

> > +identifier fname =~ ".*free.*";
> > +identifier fname2 =~ ".*destroy.*";
> > +identifier fname3 =~ ".*del.*";
> > +identifier fname4 =~ ".*queue_work.*";
> > +identifier fname5 =~ ".*schedule_work.*";
> > +identifier fname6 =~ ".*call_rcu.*";
> 
> Personally, I find the above regular expressions much easier to understand
> than the merged version that Markus proposed.  

I really don't have a strong opinion on the presentation side. 
One is more compact, the above one perhaps a bit clearer for unexperienced reader (like myself). 
Sometimes when I try to read patterns from cocci folder,
it takes me really a while to understand what is happening, which is not the case
with simple expression above. I have just considered myself to be too new into this
and therefore sticked to simple expressions to make sure I am minimizing functional mistakes. 

But the performance issue is
> only on whether to use regular expressions or not.  If you use regular
> expressions, Coccinelle will not do some optimizations.  But once you
> decide to use regular expressions, the performance hit is already taken -
> for a good cause here, to my understanding.  

Ok, so then performance is not even a factor. Thank you for the explanation! 

So just put whatever you find
> convenient, in terms of readability, precision, etc.  It seems that del is
> not very precise, because it is a substring of multiple words with
> different meanings.  Maybe it should be improved, or maybe one can just
> live with the false positives (eg delay), if they actually are false
> positives.

This is the problem that some of them might be and some not. 
I can call the queuing works explicitly:
identifier fname4 =~ ".*queue_work.*";
identifier fname5 =~ ".*queue_delayed_work.*";

Then there is no need to match "delay", but I still like to match both "delete" and "del". 

Best Regards,
Elena.
> 
> julia

Re: [PATCH v3] provide rule for finding refcounters

[Julia Lawall]

A few more small issues:

When you deleted the disjunction, you kept the surrounding parentheses.
you can drop them (lines 83 and 85).

I guess that the "del" regular expression is supposed to be matching
delete.  But it also matches delayed, eg

net/batman-adv/bridge_loop_avoidance.c:1495:8-27:
atomic_dec_and_test variation before object free at line 1507.

In the following result, the lines are at least quite far apart.  I don't
know if there is some way to consider this to be a false positive:

fs/btrfs/disk-io.c:708:14-33: atomic_dec_and_test
variation before object free at line 775.

julia

On Wed, 16 Aug 2017, Elena Reshetova wrote:

> changes in v3:
> Removed unnessesary rule 4 conditions pointed by Julia.
>
> changes in v2:
> Following the suggestion from Julia the first rule is split into
> 2. The output does not differ that much between these two versions,
> but rule became more precise.
>
> Elena Reshetova (1):
>   Coccinelle: add atomic_as_refcounter script
>
>  scripts/coccinelle/api/atomic_as_refcounter.cocci | 133 ++++++++++++++++++++++
>  1 file changed, 133 insertions(+)
>  create mode 100644 scripts/coccinelle/api/atomic_as_refcounter.cocci
>
> --
> 2.7.4
>
>

[Cocci] [PATCH v3] provide rule for finding refcounters

[Julia Lawall]

A few more small issues:

When you deleted the disjunction, you kept the surrounding parentheses.
you can drop them (lines 83 and 85).

I guess that the "del" regular expression is supposed to be matching
delete.  But it also matches delayed, eg

net/batman-adv/bridge_loop_avoidance.c:1495:8-27:
atomic_dec_and_test variation before object free at line 1507.

In the following result, the lines are at least quite far apart.  I don't
know if there is some way to consider this to be a false positive:

fs/btrfs/disk-io.c:708:14-33: atomic_dec_and_test
variation before object free at line 775.

julia

On Wed, 16 Aug 2017, Elena Reshetova wrote:

> changes in v3:
> Removed unnessesary rule 4 conditions pointed by Julia.
>
> changes in v2:
> Following the suggestion from Julia the first rule is split into
> 2. The output does not differ that much between these two versions,
> but rule became more precise.
>
> Elena Reshetova (1):
>   Coccinelle: add atomic_as_refcounter script
>
>  scripts/coccinelle/api/atomic_as_refcounter.cocci | 133 ++++++++++++++++++++++
>  1 file changed, 133 insertions(+)
>  create mode 100644 scripts/coccinelle/api/atomic_as_refcounter.cocci
>
> --
> 2.7.4
>
>

RE: [PATCH v3] provide rule for finding refcounters

[Reshetova, Elena]

Hi, I am very sorry for the delayed reply. Finally unrigging my inbox :(

> A few more small issues:
> 
> When you deleted the disjunction, you kept the surrounding parentheses.
> you can drop them (lines 83 and 85).
> 
> I guess that the "del" regular expression is supposed to be matching
> delete.  But it also matches delayed, eg
> 
> net/batman-adv/bridge_loop_avoidance.c:1495:8-27:
> atomic_dec_and_test variation before object free at line 1507.

Actually the idea is to match them both :) "delete" because it is obvious, 
"delay", exactly because "queue_delayed_work" (in addition to "queue_work") is a common way some 
structure destruction might be scheduled. It might give false positives, since
the queued work might not be related to freeing the object, but at least
we don't miss such cases. The issue also that you do want to have "del" pattern
since I think some functions are of kind xyz_del() also and I want to catch them as well. 
Of course del then might catch some other non-queue related "delay", but I haven't seen that many
to consider it a problem. 

> 
> In the following result, the lines are at least quite far apart.  I don't
> know if there is some way to consider this to be a false positive:
> 
> fs/btrfs/disk-io.c:708:14-33: atomic_dec_and_test
> variation before object free at line 775.

I actually think this is a valid result. Yes, there are a lot of things happening
in between the dec_and_test and actual free on the buffer, but kernel structures
can be so complicated that it might legitimately (like in this case) take that long
for it to cleanup before the real free can be done. 

Best Regards,
Elena.

> 
> julia
> 
> On Wed, 16 Aug 2017, Elena Reshetova wrote:
> 
> > changes in v3:
> > Removed unnessesary rule 4 conditions pointed by Julia.
> >
> > changes in v2:
> > Following the suggestion from Julia the first rule is split into
> > 2. The output does not differ that much between these two versions,
> > but rule became more precise.
> >
> > Elena Reshetova (1):
> >   Coccinelle: add atomic_as_refcounter script
> >
> >  scripts/coccinelle/api/atomic_as_refcounter.cocci | 133
> ++++++++++++++++++++++
> >  1 file changed, 133 insertions(+)
> >  create mode 100644 scripts/coccinelle/api/atomic_as_refcounter.cocci
> >
> > --
> > 2.7.4
> >
> >

[Cocci] [PATCH v3] provide rule for finding refcounters

[Reshetova, Elena]

Hi, I am very sorry for the delayed reply. Finally unrigging my inbox :(

> A few more small issues:
> 
> When you deleted the disjunction, you kept the surrounding parentheses.
> you can drop them (lines 83 and 85).
> 
> I guess that the "del" regular expression is supposed to be matching
> delete.  But it also matches delayed, eg
> 
> net/batman-adv/bridge_loop_avoidance.c:1495:8-27:
> atomic_dec_and_test variation before object free at line 1507.

Actually the idea is to match them both :) "delete" because it is obvious, 
"delay", exactly because "queue_delayed_work" (in addition to "queue_work") is a common way some 
structure destruction might be scheduled. It might give false positives, since
the queued work might not be related to freeing the object, but at least
we don't miss such cases. The issue also that you do want to have "del" pattern
since I think some functions are of kind xyz_del() also and I want to catch them as well. 
Of course del then might catch some other non-queue related "delay", but I haven't seen that many
to consider it a problem. 

> 
> In the following result, the lines are at least quite far apart.  I don't
> know if there is some way to consider this to be a false positive:
> 
> fs/btrfs/disk-io.c:708:14-33: atomic_dec_and_test
> variation before object free at line 775.

I actually think this is a valid result. Yes, there are a lot of things happening
in between the dec_and_test and actual free on the buffer, but kernel structures
can be so complicated that it might legitimately (like in this case) take that long
for it to cleanup before the real free can be done. 

Best Regards,
Elena.

> 
> julia
> 
> On Wed, 16 Aug 2017, Elena Reshetova wrote:
> 
> > changes in v3:
> > Removed unnessesary rule 4 conditions pointed by Julia.
> >
> > changes in v2:
> > Following the suggestion from Julia the first rule is split into
> > 2. The output does not differ that much between these two versions,
> > but rule became more precise.
> >
> > Elena Reshetova (1):
> >   Coccinelle: add atomic_as_refcounter script
> >
> >  scripts/coccinelle/api/atomic_as_refcounter.cocci | 133
> ++++++++++++++++++++++
> >  1 file changed, 133 insertions(+)
> >  create mode 100644 scripts/coccinelle/api/atomic_as_refcounter.cocci
> >
> > --
> > 2.7.4
> >
> >

RE: [PATCH v3] provide rule for finding refcounters

[Julia Lawall]

On Tue, 29 Aug 2017, Reshetova, Elena wrote:

> Hi, I am very sorry for the delayed reply. Finally unrigging my inbox :(
>
> > A few more small issues:
> >
> > When you deleted the disjunction, you kept the surrounding parentheses.
> > you can drop them (lines 83 and 85).
> >
> > I guess that the "del" regular expression is supposed to be matching
> > delete.  But it also matches delayed, eg
> >
> > net/batman-adv/bridge_loop_avoidance.c:1495:8-27:
> > atomic_dec_and_test variation before object free at line 1507.
>
> Actually the idea is to match them both :) "delete" because it is obvious,
> "delay", exactly because "queue_delayed_work" (in addition to "queue_work") is a common way some
> structure destruction might be scheduled. It might give false positives, since
> the queued work might not be related to freeing the object, but at least
> we don't miss such cases. The issue also that you do want to have "del" pattern
> since I think some functions are of kind xyz_del() also and I want to catch them as well.
> Of course del then might catch some other non-queue related "delay", but I haven't seen that many
> to consider it a problem.
>
> >
> > In the following result, the lines are at least quite far apart.  I don't
> > know if there is some way to consider this to be a false positive:
> >
> > fs/btrfs/disk-io.c:708:14-33: atomic_dec_and_test
> > variation before object free at line 775.
>
> I actually think this is a valid result. Yes, there are a lot of things happening
> in between the dec_and_test and actual free on the buffer, but kernel structures
> can be so complicated that it might legitimately (like in this case) take that long
> for it to cleanup before the real free can be done.

OK, if you are happy with the results of the regexps, I think that the
only remaining improvement was an extra pair of () where there was no
longer a disjuction.  If you want to send it back, then I can ack it.

julia

>
> Best Regards,
> Elena.
>
> >
> > julia
> >
> > On Wed, 16 Aug 2017, Elena Reshetova wrote:
> >
> > > changes in v3:
> > > Removed unnessesary rule 4 conditions pointed by Julia.
> > >
> > > changes in v2:
> > > Following the suggestion from Julia the first rule is split into
> > > 2. The output does not differ that much between these two versions,
> > > but rule became more precise.
> > >
> > > Elena Reshetova (1):
> > >   Coccinelle: add atomic_as_refcounter script
> > >
> > >  scripts/coccinelle/api/atomic_as_refcounter.cocci | 133
> > ++++++++++++++++++++++
> > >  1 file changed, 133 insertions(+)
> > >  create mode 100644 scripts/coccinelle/api/atomic_as_refcounter.cocci
> > >
> > > --
> > > 2.7.4
> > >
> > >
>

[Cocci] [PATCH v3] provide rule for finding refcounters

[Julia Lawall]

On Tue, 29 Aug 2017, Reshetova, Elena wrote:

> Hi, I am very sorry for the delayed reply. Finally unrigging my inbox :(
>
> > A few more small issues:
> >
> > When you deleted the disjunction, you kept the surrounding parentheses.
> > you can drop them (lines 83 and 85).
> >
> > I guess that the "del" regular expression is supposed to be matching
> > delete.  But it also matches delayed, eg
> >
> > net/batman-adv/bridge_loop_avoidance.c:1495:8-27:
> > atomic_dec_and_test variation before object free at line 1507.
>
> Actually the idea is to match them both :) "delete" because it is obvious,
> "delay", exactly because "queue_delayed_work" (in addition to "queue_work") is a common way some
> structure destruction might be scheduled. It might give false positives, since
> the queued work might not be related to freeing the object, but at least
> we don't miss such cases. The issue also that you do want to have "del" pattern
> since I think some functions are of kind xyz_del() also and I want to catch them as well.
> Of course del then might catch some other non-queue related "delay", but I haven't seen that many
> to consider it a problem.
>
> >
> > In the following result, the lines are at least quite far apart.  I don't
> > know if there is some way to consider this to be a false positive:
> >
> > fs/btrfs/disk-io.c:708:14-33: atomic_dec_and_test
> > variation before object free at line 775.
>
> I actually think this is a valid result. Yes, there are a lot of things happening
> in between the dec_and_test and actual free on the buffer, but kernel structures
> can be so complicated that it might legitimately (like in this case) take that long
> for it to cleanup before the real free can be done.

OK, if you are happy with the results of the regexps, I think that the
only remaining improvement was an extra pair of () where there was no
longer a disjuction.  If you want to send it back, then I can ack it.

julia

>
> Best Regards,
> Elena.
>
> >
> > julia
> >
> > On Wed, 16 Aug 2017, Elena Reshetova wrote:
> >
> > > changes in v3:
> > > Removed unnessesary rule 4 conditions pointed by Julia.
> > >
> > > changes in v2:
> > > Following the suggestion from Julia the first rule is split into
> > > 2. The output does not differ that much between these two versions,
> > > but rule became more precise.
> > >
> > > Elena Reshetova (1):
> > >   Coccinelle: add atomic_as_refcounter script
> > >
> > >  scripts/coccinelle/api/atomic_as_refcounter.cocci | 133
> > ++++++++++++++++++++++
> > >  1 file changed, 133 insertions(+)
> > >  create mode 100644 scripts/coccinelle/api/atomic_as_refcounter.cocci
> > >
> > > --
> > > 2.7.4
> > >
> > >
>

RE: [PATCH v3] provide rule for finding refcounters

[Reshetova, Elena]

> On Tue, 29 Aug 2017, Reshetova, Elena wrote:
> 
> > Hi, I am very sorry for the delayed reply. Finally unrigging my inbox :(
> >
> > > A few more small issues:
> > >
> > > When you deleted the disjunction, you kept the surrounding parentheses.
> > > you can drop them (lines 83 and 85).
> > >
> > > I guess that the "del" regular expression is supposed to be matching
> > > delete.  But it also matches delayed, eg
> > >
> > > net/batman-adv/bridge_loop_avoidance.c:1495:8-27:
> > > atomic_dec_and_test variation before object free at line 1507.
> >
> > Actually the idea is to match them both :) "delete" because it is obvious,
> > "delay", exactly because "queue_delayed_work" (in addition to "queue_work") is a
> common way some
> > structure destruction might be scheduled. It might give false positives, since
> > the queued work might not be related to freeing the object, but at least
> > we don't miss such cases. The issue also that you do want to have "del" pattern
> > since I think some functions are of kind xyz_del() also and I want to catch them as
> well.
> > Of course del then might catch some other non-queue related "delay", but I
> haven't seen that many
> > to consider it a problem.
> >
> > >
> > > In the following result, the lines are at least quite far apart.  I don't
> > > know if there is some way to consider this to be a false positive:
> > >
> > > fs/btrfs/disk-io.c:708:14-33: atomic_dec_and_test
> > > variation before object free at line 775.
> >
> > I actually think this is a valid result. Yes, there are a lot of things happening
> > in between the dec_and_test and actual free on the buffer, but kernel structures
> > can be so complicated that it might legitimately (like in this case) take that long
> > for it to cleanup before the real free can be done.
> 
> OK, if you are happy with the results of the regexps, I think that the
> only remaining improvement was an extra pair of () where there was no
> longer a disjuction.  If you want to send it back, then I can ack it.

Sure, I will fix that extra pair of brackets and resend. Thank you very much!

Best Regards,
Elena.
> 
> julia
> 
> >
> > Best Regards,
> > Elena.
> >
> > >
> > > julia
> > >
> > > On Wed, 16 Aug 2017, Elena Reshetova wrote:
> > >
> > > > changes in v3:
> > > > Removed unnessesary rule 4 conditions pointed by Julia.
> > > >
> > > > changes in v2:
> > > > Following the suggestion from Julia the first rule is split into
> > > > 2. The output does not differ that much between these two versions,
> > > > but rule became more precise.
> > > >
> > > > Elena Reshetova (1):
> > > >   Coccinelle: add atomic_as_refcounter script
> > > >
> > > >  scripts/coccinelle/api/atomic_as_refcounter.cocci | 133
> > > ++++++++++++++++++++++
> > > >  1 file changed, 133 insertions(+)
> > > >  create mode 100644 scripts/coccinelle/api/atomic_as_refcounter.cocci
> > > >
> > > > --
> > > > 2.7.4
> > > >
> > > >
> >

[Cocci] [PATCH v3] provide rule for finding refcounters

[Reshetova, Elena]

> On Tue, 29 Aug 2017, Reshetova, Elena wrote:
> 
> > Hi, I am very sorry for the delayed reply. Finally unrigging my inbox :(
> >
> > > A few more small issues:
> > >
> > > When you deleted the disjunction, you kept the surrounding parentheses.
> > > you can drop them (lines 83 and 85).
> > >
> > > I guess that the "del" regular expression is supposed to be matching
> > > delete.  But it also matches delayed, eg
> > >
> > > net/batman-adv/bridge_loop_avoidance.c:1495:8-27:
> > > atomic_dec_and_test variation before object free at line 1507.
> >
> > Actually the idea is to match them both :) "delete" because it is obvious,
> > "delay", exactly because "queue_delayed_work" (in addition to "queue_work") is a
> common way some
> > structure destruction might be scheduled. It might give false positives, since
> > the queued work might not be related to freeing the object, but at least
> > we don't miss such cases. The issue also that you do want to have "del" pattern
> > since I think some functions are of kind xyz_del() also and I want to catch them as
> well.
> > Of course del then might catch some other non-queue related "delay", but I
> haven't seen that many
> > to consider it a problem.
> >
> > >
> > > In the following result, the lines are at least quite far apart.  I don't
> > > know if there is some way to consider this to be a false positive:
> > >
> > > fs/btrfs/disk-io.c:708:14-33: atomic_dec_and_test
> > > variation before object free at line 775.
> >
> > I actually think this is a valid result. Yes, there are a lot of things happening
> > in between the dec_and_test and actual free on the buffer, but kernel structures
> > can be so complicated that it might legitimately (like in this case) take that long
> > for it to cleanup before the real free can be done.
> 
> OK, if you are happy with the results of the regexps, I think that the
> only remaining improvement was an extra pair of () where there was no
> longer a disjuction.  If you want to send it back, then I can ack it.

Sure, I will fix that extra pair of brackets and resend. Thank you very much!

Best Regards,
Elena.
> 
> julia
> 
> >
> > Best Regards,
> > Elena.
> >
> > >
> > > julia
> > >
> > > On Wed, 16 Aug 2017, Elena Reshetova wrote:
> > >
> > > > changes in v3:
> > > > Removed unnessesary rule 4 conditions pointed by Julia.
> > > >
> > > > changes in v2:
> > > > Following the suggestion from Julia the first rule is split into
> > > > 2. The output does not differ that much between these two versions,
> > > > but rule became more precise.
> > > >
> > > > Elena Reshetova (1):
> > > >   Coccinelle: add atomic_as_refcounter script
> > > >
> > > >  scripts/coccinelle/api/atomic_as_refcounter.cocci | 133
> > > ++++++++++++++++++++++
> > > >  1 file changed, 133 insertions(+)
> > > >  create mode 100644 scripts/coccinelle/api/atomic_as_refcounter.cocci
> > > >
> > > > --
> > > > 2.7.4
> > > >
> > > >
> >

[PATCH] Coccinelle: add atomic_as_refcounter script

[Elena Reshetova]

atomic_as_refcounter.cocci script allows detecting
cases when refcount_t type and API should be used
instead of atomic_t.

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
---
 scripts/coccinelle/api/atomic_as_refcounter.cocci | 148 ++++++++++++++++++++++
 1 file changed, 148 insertions(+)
 create mode 100644 scripts/coccinelle/api/atomic_as_refcounter.cocci

diff --git a/scripts/coccinelle/api/atomic_as_refcounter.cocci b/scripts/coccinelle/api/atomic_as_refcounter.cocci
new file mode 100644
index 0000000..996d72b
--- /dev/null
+++ b/scripts/coccinelle/api/atomic_as_refcounter.cocci
@@ -0,0 +1,148 @@
+// Check if refcount_t type and API should be used
+// instead of atomic_t type when dealing with refcounters
+//
+// Copyright (c) 2016-2017, Elena Reshetova, Intel Corporation
+//
+// Confidence: Moderate
+// URL: http://coccinelle.lip6.fr/
+// Options: --include-headers --very-quiet
+
+virtual report
+
+@r1 exists@
+identifier a, x, y;
+position p1, p2;
+identifier fname =~ ".*free.*";
+identifier fname2 =~ ".*destroy.*";
+identifier fname3 =~ ".*del.*";
+identifier fname4 =~ ".*queue_work.*";
+identifier fname5 =~ ".*schedule_work.*";
+identifier fname6 =~ ".*call_rcu.*";
+
+@@
+
+(
+ atomic_dec_and_test@p1(&(a)->x)
+|
+ atomic_dec_and_lock@p1(&(a)->x, ...)
+|
+ atomic_long_dec_and_lock@p1(&(a)->x, ...)
+|
+ atomic_long_dec_and_test@p1(&(a)->x)
+|
+ atomic64_dec_and_test@p1(&(a)->x)
+|
+ local_dec_and_test@p1(&(a)->x)
+)
+...
+(
+ fname@p2(a, ...);
+|
+ fname2@p2(...);
+|
+ fname3@p2(...);
+|
+ fname4@p2(...);
+|
+ fname5@p2(...);
+|
+ fname6@p2(...);
+)
+
+
+@script:python depends on report@
+p1 << r1.p1;
+p2 << r1.p2;
+@@
+msg = "atomic_dec_and_test variation before object free at line %s."
+coccilib.report.print_report(p1[0], msg % (p2[0].line))
+
+@r4 exists@
+identifier a, x, y;
+position p1, p2;
+identifier fname =~ ".*free.*";
+identifier fname2 =~ ".*destroy.*";
+identifier fname3 =~ ".*del.*";
+identifier fname4 =~ ".*queue_work.*";
+identifier fname5 =~ ".*schedule_work.*";
+identifier fname6 =~ ".*call_rcu.*";
+
+@@
+
+(
+ atomic_dec_and_test@p1(&(a)->x)
+|
+ atomic_dec_and_lock@p1(&(a)->x, ...)
+|
+ atomic_long_dec_and_lock@p1(&(a)->x, ...)
+|
+ atomic_long_dec_and_test@p1(&(a)->x)
+|
+ atomic64_dec_and_test@p1(&(a)->x)
+|
+ local_dec_and_test@p1(&(a)->x)
+)
+...
+y=a
+...
+(
+ fname@p2(y, ...);
+|
+ fname2@p2(...);
+|
+ fname3@p2(...);
+|
+ fname4@p2(...);
+|
+ fname5@p2(...);
+|
+ fname6@p2(...);
+)
+
+
+@script:python depends on report@
+p1 << r4.p1;
+p2 << r4.p2;
+@@
+msg = "atomic_dec_and_test variation before object free at line %s."
+coccilib.report.print_report(p1[0], msg % (p2[0].line))
+
+@r2 exists@
+identifier a, x;
+position p1;
+@@
+
+(
+atomic_add_unless(&(a)->x,-1,1)@p1
+|
+atomic_long_add_unless(&(a)->x,-1,1)@p1
+|
+atomic64_add_unless(&(a)->x,-1,1)@p1
+)
+
+@script:python depends on report@
+p1 << r2.p1;
+@@
+msg = "atomic_add_unless"
+coccilib.report.print_report(p1[0], msg)
+
+@r3 exists@
+identifier x;
+position p1;
+@@
+
+(
+x = atomic_add_return@p1(-1, ...);
+|
+x = atomic_long_add_return@p1(-1, ...);
+|
+x = atomic64_add_return@p1(-1, ...);
+)
+
+@script:python depends on report@
+p1 << r3.p1;
+@@
+msg = "x = atomic_add_return(-1, ...)"
+coccilib.report.print_report(p1[0], msg)
+
+
-- 
2.7.4

[Cocci] Coccinelle: add atomic_as_refcounter script

[SF Markus Elfring]

Dear Elena,

Would you like to take another look at my development suggestion for your approach?
https://systeme.lip6.fr/pipermail/cocci/2017-August/004300.html

How do you think about to refactor another rule like the following?

@r3 exists@
expression E;
identifier I=~"^atomic(?:64|_long)?_add_return$";
position P;
@@
 E = I at P(-1, ...);


Can the specification of SmPL constraints be occasionally be more succinct
with the use of regular expressions?

Regards,
Markus

[Cocci] Coccinelle: add atomic_as_refcounter script

[Julia Lawall]

On Tue, 15 Aug 2017, SF Markus Elfring wrote:

> Dear Elena,
>
> Would you like to take another look at my development suggestion for your approach?
> https://systeme.lip6.fr/pipermail/cocci/2017-August/004300.html
>
> How do you think about to refactor another rule like the following?
>
> @r3 exists@
> expression E;
> identifier I=~"^atomic(?:64|_long)?_add_return$";
> position P;
> @@
>  E = I at P(-1, ...);
>
>
> Can the specification of SmPL constraints be occasionally be more succinct
> with the use of regular expressions?

Elena, please don't follow Markus's suggestion.  Coccinelle doesn't
interpret regular expressions when selecting code to work on, and thus the
resulting rule will be less efficient.

Markus, if you see this somehow, at least yesterday all mail sent to you
was bouncing.

julia

[Cocci] Coccinelle: add atomic_as_refcounter script

[SF Markus Elfring]

> Elena, please don't follow Markus's suggestion.

I suggest to consider additional software development possibilities.

We have got recurring different opinions around the application of advanced
regular expressions.


> Coccinelle doesn't interpret regular expressions when selecting code to work on,

This software behaviour can be fine.


> and thus the resulting rule will be less efficient.

I do not come to the same conclusion for some use cases.


> Markus, if you see this somehow,

Yes. - But I can read your response only by the web interface for your mailing
list archive at the moment.
https://systeme.lip6.fr/pipermail/cocci/2017-August/004308.html


> at least yesterday all mail sent to you was bouncing.

Thanks for this information. - I was not informed about such message exchange
problems so far.
I hope that communication difficulties will be fixed somehow.

Regards,
Markus