[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

[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

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
>
>

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

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
> >
> >

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

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
> > >
> > >
>

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
> > > >
> > > >
> >