mirror of https://lore.kernel.org/lkml/
 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
> > >
> > >
>

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

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-08-16 11:52 Elena Reshetova
2017-08-16 11:52 ` [PATCH] Coccinelle: add atomic_as_refcounter script Elena Reshetova
2017-08-17 11:50   ` Julia Lawall
2017-08-29  9:01     ` Reshetova, Elena
2017-08-16 14:16 ` [PATCH v3] provide rule for finding refcounters Julia Lawall
2017-08-29  8:54   ` Reshetova, Elena
2017-08-29  9:23     ` Julia Lawall [this message]
2017-08-29 10:57       ` 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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®