All of lore.kernel.org
 help / color / mirror / Atom feed
From: Julia Lawall <julia.lawall@lip6.fr>
To: "Reshetova, Elena" <elena.reshetova@intel.com>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"cocci@systeme.lip6.fr" <cocci@systeme.lip6.fr>,
	"Gilles.Muller@lip6.fr" <Gilles.Muller@lip6.fr>,
	"nicolas.palix@imag.fr" <nicolas.palix@imag.fr>,
	"mmarek@suse.com" <mmarek@suse.com>,
	"keescook@chromium.org" <keescook@chromium.org>,
	"ishkamiel@gmail.com" <ishkamiel@gmail.com>
Subject: RE: [PATCH v3] provide rule for finding refcounters
Date: Tue, 29 Aug 2017 11:23:01 +0200 (CEST)	[thread overview]
Message-ID: <alpine.DEB.2.20.1708291121480.2398@hadrien> (raw)
In-Reply-To: <2236FBA76BA1254E88B949DDB74E612B6FF5F9AD@IRSMSX102.ger.corp.intel.com>



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

WARNING: multiple messages have this Message-ID (diff)
From: julia.lawall@lip6.fr (Julia Lawall)
To: cocci@systeme.lip6.fr
Subject: [Cocci] [PATCH v3] provide rule for finding refcounters
Date: Tue, 29 Aug 2017 11:23:01 +0200 (CEST)	[thread overview]
Message-ID: <alpine.DEB.2.20.1708291121480.2398@hadrien> (raw)
In-Reply-To: <2236FBA76BA1254E88B949DDB74E612B6FF5F9AD@IRSMSX102.ger.corp.intel.com>



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

  reply	other threads:[~2017-08-29  9:23 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-08-16 11:52 [PATCH v3] provide rule for finding refcounters Elena Reshetova
2017-08-16 11:52 ` [Cocci] " Elena Reshetova
2017-08-16 11:52 ` [PATCH] Coccinelle: add atomic_as_refcounter script Elena Reshetova
2017-08-16 11:52   ` [Cocci] " Elena Reshetova
2017-08-16 17:00   ` [Cocci] " SF Markus Elfring
2017-08-17  7:22     ` Reshetova, Elena
2017-08-17 11:31       ` SF Markus Elfring
2017-08-17 11:50   ` [PATCH] " Julia Lawall
2017-08-17 11:50     ` [Cocci] " Julia Lawall
2017-08-29  9:01     ` Reshetova, Elena
2017-08-29  9:01       ` [Cocci] " Reshetova, Elena
2017-08-16 14:16 ` [PATCH v3] provide rule for finding refcounters Julia Lawall
2017-08-16 14:16   ` [Cocci] " Julia Lawall
2017-08-29  8:54   ` Reshetova, Elena
2017-08-29  8:54     ` [Cocci] " Reshetova, Elena
2017-08-29  9:23     ` Julia Lawall [this message]
2017-08-29  9:23       ` Julia Lawall
2017-08-29 10:57       ` Reshetova, Elena
2017-08-29 10:57         ` [Cocci] " Reshetova, Elena

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alpine.DEB.2.20.1708291121480.2398@hadrien \
    --to=julia.lawall@lip6.fr \
    --cc=Gilles.Muller@lip6.fr \
    --cc=cocci@systeme.lip6.fr \
    --cc=elena.reshetova@intel.com \
    --cc=ishkamiel@gmail.com \
    --cc=keescook@chromium.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mmarek@suse.com \
    --cc=nicolas.palix@imag.fr \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.