From: "Reshetova, Elena" <elena.reshetova@intel.com> To: Julia Lawall <julia.lawall@lip6.fr> 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 08:54:25 +0000 [thread overview] Message-ID: <2236FBA76BA1254E88B949DDB74E612B6FF5F9AD@IRSMSX102.ger.corp.intel.com> (raw) In-Reply-To: <alpine.DEB.2.20.1708161609050.3169@hadrien> 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 > > > >
WARNING: multiple messages have this Message-ID (diff)
From: elena.reshetova@intel.com (Reshetova, Elena) To: cocci@systeme.lip6.fr Subject: [Cocci] [PATCH v3] provide rule for finding refcounters Date: Tue, 29 Aug 2017 08:54:25 +0000 [thread overview] Message-ID: <2236FBA76BA1254E88B949DDB74E612B6FF5F9AD@IRSMSX102.ger.corp.intel.com> (raw) In-Reply-To: <alpine.DEB.2.20.1708161609050.3169@hadrien> 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 > > > >
next prev parent reply other threads:[~2017-08-29 8:54 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 [this message] 2017-08-29 8:54 ` Reshetova, Elena 2017-08-29 9:23 ` Julia Lawall 2017-08-29 9:23 ` [Cocci] " 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=2236FBA76BA1254E88B949DDB74E612B6FF5F9AD@IRSMSX102.ger.corp.intel.com \ --to=elena.reshetova@intel.com \ --cc=Gilles.Muller@lip6.fr \ --cc=cocci@systeme.lip6.fr \ --cc=ishkamiel@gmail.com \ --cc=julia.lawall@lip6.fr \ --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: linkBe 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.