From: Joe Perches <joe@perches.com>
To: Julia Lawall <julia.lawall@lip6.fr>
Cc: Dan Carpenter <dan.carpenter@oracle.com>,
James Smart <james.smart@broadcom.com>,
Keith Busch <keith.busch@intel.com>, Jens Axboe <axboe@fb.com>,
linux-nvme@lists.infradead.org, linux-kernel@vger.kernel.org,
kernel-janitors@vger.kernel.org
Subject: Re: [patch] nvme-fabrics: correct some printk information
Date: Sat, 10 Dec 2016 14:27:53 -0800 [thread overview]
Message-ID: <1481408873.1764.4.camel@perches.com> (raw)
In-Reply-To: <alpine.DEB.2.20.1612102304390.1986@hadrien>
On Sat, 2016-12-10 at 23:07 +0100, Julia Lawall wrote:
>
> On Sat, 10 Dec 2016, Joe Perches wrote:
>
> > On Sat, 2016-12-10 at 21:06 +0100, Julia Lawall wrote:
> > >
> > > On Sat, 10 Dec 2016, Dan Carpenter wrote:
> > >
> > > > On Sat, Dec 10, 2016 at 03:27:50AM -0800, Joe Perches wrote:
> > > > > On Sat, 2016-12-10 at 12:06 +0300, Dan Carpenter wrote:
> > > > > > We really don't care where "ctrl" is on the stack since we're just
> > > > > > returning soon what we want is the actual ctrl pointer itself.
> > > > > >
> > > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > > > >
> > > > > > diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
> > > > >
> > > > > []
> > > > > > @@ -2402,7 +2402,7 @@ enum blk_eh_timer_return
> > > > > >
> > > > > > dev_info(ctrl->ctrl.device,
> > > > > > "NVME-FC{%d}: new ctrl: NQN \"%s\" (%p)\n",
> > > > > > - ctrl->cnum, ctrl->ctrl.opts->subsysnqn, &ctrl);
> > > > > > + ctrl->cnum, ctrl->ctrl.opts->subsysnqn, ctrl);
> > > > >
> > > > > Found by script or inspection?
> > > > >
> > > > > If by script, it seems unlikely there's only 1 instance
> > > > > where an address of an automatic pointer type is used
> > > > > incorrectly.
> > > >
> > > > Script. But it's using a pretty specific heuristic where we kmalloc a
> > > > pointer and then pass the address. It prints few warnings. Probably
> > > > 40% false positives, but the remaining examples of course are 100% false
> > > > positives.
> > >
> > > I tried anything that looks like a print, ie has a format string argument,
> > > and was taking the address of a local variable as another argument. But
> > > there are lots of weird format designators in the kernel that Coccinelle
> > > doesn't know about for which passing the address of a local variable is
> > > reasonable. So for the moment, there are, as far as I can see, just a lot
> > > of false positives. I did add improving the support for format strings to
> > > my TODO list.
> >
> > I think there's probably a class of defects that could
> > be found something like this in coccinelle:
> >
> > @@
> > type T;
> > T *t;
> > @@
> >
> > * \(netdev_emerg\|netdev_crit\|netdev_alert\|netdev_err\|netdev_notice\|netdev_warn\|netdev_warn\|netdev_info\|netdev_dbg\|dev_emerg\|dev_crit\|dev_alert\|dev_err\|dev_notice\|dev_warn\|dev_warn\|dev_info\|dev_dbg\|pr_emerg\|pr_crit\|pr_alert\|pr_err\|pr_notice\|pr_warn\|pr_warning\|pr_warn\|pr_info\|pr_debug\|printk\|vsprintf\|vscnprintf\|vsprintf\)(..., &t, ...);
> >
> > This finds a few like:
> >
> > diff -u -p drivers//dma/pxa_dma.c /tmp/nothing//dma/pxa_dma.c
> > --- drivers//dma/pxa_dma.c
> > +++ /tmp/nothing//dma/pxa_dma.c
> > @@ -640,9 +640,6 @@ static unsigned int clear_chan_irq(struc
> > dcsr = phy_readl_relaxed(phy, DCSR);
> > phy_writel(phy, dcsr, DCSR);
> > if ((dcsr & PXA_DCSR_BUSERR) && (phy->vchan))
> > - dev_warn(&phy->vchan->vc.chan.dev->device,
> > - "%s(chan=%p): PXA_DCSR_BUSERR\n",
> > - __func__, &phy->vchan);
> >
> > return dcsr & ~PXA_DCSR_RUN;
> > }
> >
> > btw: It'd be nice if coccinelle could use multiple nested "\("
>
> What exactly didn't work? It should be possible.
>
> My rule was:
>
> @@
> format d;
> local idexpression l;
> identifier f != {sscanf,fscanf};
> @@
>
> f(...,"...%@d@...",...,
> *&l
> ,...)
>
> But there are many false positives, with things like %pV.
>
> julia
I think local idexpression is not constrained
to just a pointer type..
Basically, anything that's taking an address of a
pointer should be a misuse.
I think instead of local idexpression l, using
type L;
L *l;
would be better.
My version of spatch seems to be too old to
support this syntax though.
I'll upgrade eventually and try it again later.
$ spatch --version
spatch version 1.0.5-00102-gd8ee7a6 compiled with OCaml version 4.02.3
Flags passed to the configure script: [none]
Python scripting support: yes
Syntax of regular expresssions: PCRE
next prev parent reply other threads:[~2016-12-10 22:27 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-12-10 9:06 [patch] nvme-fabrics: correct some printk information Dan Carpenter
2016-12-10 11:27 ` Joe Perches
2016-12-10 18:55 ` Dan Carpenter
2016-12-10 20:06 ` Julia Lawall
2016-12-10 20:24 ` Dan Carpenter
2016-12-10 20:54 ` Joe Perches
2016-12-10 21:07 ` Dan Carpenter
2016-12-10 22:24 ` Joe Perches
2016-12-12 9:33 ` Dan Carpenter
2016-12-12 15:47 ` Julia Lawall
2016-12-12 15:55 ` Joe Perches
2016-12-10 22:07 ` Julia Lawall
2016-12-10 22:27 ` Joe Perches [this message]
2016-12-11 0:36 ` Joe Perches
2016-12-20 0:40 ` James Smart
2016-12-20 9:16 ` Dan Carpenter
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=1481408873.1764.4.camel@perches.com \
--to=joe@perches.com \
--cc=axboe@fb.com \
--cc=dan.carpenter@oracle.com \
--cc=james.smart@broadcom.com \
--cc=julia.lawall@lip6.fr \
--cc=keith.busch@intel.com \
--cc=kernel-janitors@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-nvme@lists.infradead.org \
/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;
as well as URLs for NNTP newsgroup(s).