Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1722246 > unrolled thread

RE: [PATCH v3] provide rule for finding refcounters

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  RE: [PATCH v3] provide rule for finding refcounters "Reshetova, Elena" <elena.reshetova@intel.com> - 2017-08-29 11:00 +0200
    RE: [PATCH v3] provide rule for finding refcounters Julia Lawall <julia.lawall@lip6.fr> - 2017-08-29 11:30 +0200
      RE: [PATCH v3] provide rule for finding refcounters "Reshetova, Elena" <elena.reshetova@intel.com> - 2017-08-29 13:00 +0200

#1722246 — RE: [PATCH v3] provide rule for finding refcounters

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

[toc] | [next] | [standalone]

#1722266

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

[toc] | [prev] | [next] | [standalone]

#1722321

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web