* [patch] nvme-fabrics: correct some printk information @ 2016-12-10 9:06 Dan Carpenter 2016-12-10 11:27 ` Joe Perches 2016-12-20 0:40 ` James Smart 0 siblings, 2 replies; 16+ messages in thread From: Dan Carpenter @ 2016-12-10 9:06 UTC (permalink / raw) To: James Smart Cc: Keith Busch, Jens Axboe, linux-nvme, linux-kernel, kernel-janitors 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 index 771e2e761872..e6395ed2f562 100644 --- 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); kref_get(&ctrl->ctrl.kref); ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [patch] nvme-fabrics: correct some printk information 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-20 0:40 ` James Smart 1 sibling, 1 reply; 16+ messages in thread From: Joe Perches @ 2016-12-10 11:27 UTC (permalink / raw) To: Dan Carpenter, James Smart Cc: Keith Busch, Jens Axboe, linux-nvme, linux-kernel, kernel-janitors 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [patch] nvme-fabrics: correct some printk information 2016-12-10 11:27 ` Joe Perches @ 2016-12-10 18:55 ` Dan Carpenter 2016-12-10 20:06 ` Julia Lawall 0 siblings, 1 reply; 16+ messages in thread From: Dan Carpenter @ 2016-12-10 18:55 UTC (permalink / raw) To: Joe Perches Cc: James Smart, Keith Busch, Jens Axboe, linux-nvme, linux-kernel, kernel-janitors 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. regards, dan carpenter ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [patch] nvme-fabrics: correct some printk information 2016-12-10 18:55 ` Dan Carpenter @ 2016-12-10 20:06 ` Julia Lawall 2016-12-10 20:24 ` Dan Carpenter ` (2 more replies) 0 siblings, 3 replies; 16+ messages in thread From: Julia Lawall @ 2016-12-10 20:06 UTC (permalink / raw) To: Dan Carpenter Cc: Joe Perches, James Smart, Keith Busch, Jens Axboe, linux-nvme, linux-kernel, kernel-janitors 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. julia > > regards, > dan carpenter > > -- > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [patch] nvme-fabrics: correct some printk information 2016-12-10 20:06 ` Julia Lawall @ 2016-12-10 20:24 ` Dan Carpenter 2016-12-10 20:54 ` Joe Perches 2016-12-11 0:36 ` Joe Perches 2 siblings, 0 replies; 16+ messages in thread From: Dan Carpenter @ 2016-12-10 20:24 UTC (permalink / raw) To: Julia Lawall Cc: Joe Perches, James Smart, Keith Busch, Jens Axboe, linux-nvme, linux-kernel, kernel-janitors For my check, most of the results fall into three categories. 1) False positives (40% of results) 2) Badly designed interfaces that take a pointer to a pointer for no reason and can be cleaned up. (5%) 3) Bugs where we modified the code, but haven't tested it. Most of the time passing the wrong pointer will be detected right away during testing so it's not like this is a super common type of bug. (55%) I haven't pushed the check because 40% false positives is probably enough to make people complain. regards, dan carpenter ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [patch] nvme-fabrics: correct some printk information 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:07 ` Julia Lawall 2016-12-11 0:36 ` Joe Perches 2 siblings, 2 replies; 16+ messages in thread From: Joe Perches @ 2016-12-10 20:54 UTC (permalink / raw) To: Julia Lawall, Dan Carpenter Cc: James Smart, Keith Busch, Jens Axboe, linux-nvme, linux-kernel, kernel-janitors 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 "\(" ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [patch] nvme-fabrics: correct some printk information 2016-12-10 20:54 ` Joe Perches @ 2016-12-10 21:07 ` Dan Carpenter 2016-12-10 22:24 ` Joe Perches 2016-12-10 22:07 ` Julia Lawall 1 sibling, 1 reply; 16+ messages in thread From: Dan Carpenter @ 2016-12-10 21:07 UTC (permalink / raw) To: Joe Perches Cc: Julia Lawall, James Smart, Keith Busch, Jens Axboe, linux-nvme, linux-kernel, kernel-janitors On Sat, Dec 10, 2016 at 12:54:50PM -0800, Joe Perches wrote: > 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); That's not a defect. We're getting the address of vchan. I don't get it? regards, dan carpenter ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [patch] nvme-fabrics: correct some printk information 2016-12-10 21:07 ` Dan Carpenter @ 2016-12-10 22:24 ` Joe Perches 2016-12-12 9:33 ` Dan Carpenter 0 siblings, 1 reply; 16+ messages in thread From: Joe Perches @ 2016-12-10 22:24 UTC (permalink / raw) To: Dan Carpenter Cc: Julia Lawall, James Smart, Keith Busch, Jens Axboe, linux-nvme, linux-kernel, kernel-janitors On Sun, 2016-12-11 at 00:07 +0300, Dan Carpenter wrote: > On Sat, Dec 10, 2016 at 12:54:50PM -0800, Joe Perches wrote: > > 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); > > That's not a defect. We're getting the address of vchan. I don't get > it? $ git grep -n -w vchan drivers/dma/pxa* drivers/dma/pxa_dma.c:103: struct pxad_chan *vchan; ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [patch] nvme-fabrics: correct some printk information 2016-12-10 22:24 ` Joe Perches @ 2016-12-12 9:33 ` Dan Carpenter 2016-12-12 15:47 ` Julia Lawall 0 siblings, 1 reply; 16+ messages in thread From: Dan Carpenter @ 2016-12-12 9:33 UTC (permalink / raw) To: Joe Perches Cc: Julia Lawall, James Smart, Keith Busch, Jens Axboe, linux-nvme, linux-kernel, kernel-janitors On Sat, Dec 10, 2016 at 02:24:22PM -0800, Joe Perches wrote: > On Sun, 2016-12-11 at 00:07 +0300, Dan Carpenter wrote: > > On Sat, Dec 10, 2016 at 12:54:50PM -0800, Joe Perches wrote: > > > 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); > > > > That's not a defect. We're getting the address of vchan. I don't get > > it? > > $ git grep -n -w vchan drivers/dma/pxa* > drivers/dma/pxa_dma.c:103: struct pxad_chan *vchan; I'm not sure what you're saying here still. This code works as intended. We're not printing a stack address. regards, dan carpenter ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [patch] nvme-fabrics: correct some printk information 2016-12-12 9:33 ` Dan Carpenter @ 2016-12-12 15:47 ` Julia Lawall 2016-12-12 15:55 ` Joe Perches 0 siblings, 1 reply; 16+ messages in thread From: Julia Lawall @ 2016-12-12 15:47 UTC (permalink / raw) To: Dan Carpenter Cc: Joe Perches, James Smart, Keith Busch, Jens Axboe, linux-nvme, linux-kernel, kernel-janitors On Mon, 12 Dec 2016, Dan Carpenter wrote: > On Sat, Dec 10, 2016 at 02:24:22PM -0800, Joe Perches wrote: > > On Sun, 2016-12-11 at 00:07 +0300, Dan Carpenter wrote: > > > On Sat, Dec 10, 2016 at 12:54:50PM -0800, Joe Perches wrote: > > > > 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); > > > > > > That's not a defect. We're getting the address of vchan. I don't get > > > it? > > > > $ git grep -n -w vchan drivers/dma/pxa* > > drivers/dma/pxa_dma.c:103: struct pxad_chan *vchan; > > I'm not sure what you're saying here still. This code works as > intended. We're not printing a stack address. I guess that the point is that one would like to print the channel, not the address of the channel? julia ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [patch] nvme-fabrics: correct some printk information 2016-12-12 15:47 ` Julia Lawall @ 2016-12-12 15:55 ` Joe Perches 0 siblings, 0 replies; 16+ messages in thread From: Joe Perches @ 2016-12-12 15:55 UTC (permalink / raw) To: Julia Lawall, Dan Carpenter Cc: James Smart, Keith Busch, Jens Axboe, linux-nvme, linux-kernel, kernel-janitors On Mon, 2016-12-12 at 16:47 +0100, Julia Lawall wrote: > > On Mon, 12 Dec 2016, Dan Carpenter wrote: > > > On Sat, Dec 10, 2016 at 02:24:22PM -0800, Joe Perches wrote: > > > On Sun, 2016-12-11 at 00:07 +0300, Dan Carpenter wrote: > > > > On Sat, Dec 10, 2016 at 12:54:50PM -0800, Joe Perches wrote: > > > > > 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); > > > > > > > > That's not a defect. We're getting the address of vchan. I don't get > > > > it? > > > > > > $ git grep -n -w vchan drivers/dma/pxa* > > > drivers/dma/pxa_dma.c:103: struct pxad_chan *vchan; > > > > I'm not sure what you're saying here still. This code works as > > intended. We're not printing a stack address. > > I guess that the point is that one would like to print the channel, not > the address of the channel? Generally, printing the address of a pointer _can_ be useful, but it's likely a defect with a low false positive rate. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [patch] nvme-fabrics: correct some printk information 2016-12-10 20:54 ` Joe Perches 2016-12-10 21:07 ` Dan Carpenter @ 2016-12-10 22:07 ` Julia Lawall 2016-12-10 22:27 ` Joe Perches 1 sibling, 1 reply; 16+ messages in thread From: Julia Lawall @ 2016-12-10 22:07 UTC (permalink / raw) To: Joe Perches Cc: Dan Carpenter, James Smart, Keith Busch, Jens Axboe, linux-nvme, linux-kernel, kernel-janitors 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [patch] nvme-fabrics: correct some printk information 2016-12-10 22:07 ` Julia Lawall @ 2016-12-10 22:27 ` Joe Perches 0 siblings, 0 replies; 16+ messages in thread From: Joe Perches @ 2016-12-10 22:27 UTC (permalink / raw) To: Julia Lawall Cc: Dan Carpenter, James Smart, Keith Busch, Jens Axboe, linux-nvme, linux-kernel, kernel-janitors 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [patch] nvme-fabrics: correct some printk information 2016-12-10 20:06 ` Julia Lawall 2016-12-10 20:24 ` Dan Carpenter 2016-12-10 20:54 ` Joe Perches @ 2016-12-11 0:36 ` Joe Perches 2 siblings, 0 replies; 16+ messages in thread From: Joe Perches @ 2016-12-11 0:36 UTC (permalink / raw) To: Julia Lawall, Dan Carpenter, Rasmus Villemoes Cc: James Smart, Keith Busch, Jens Axboe, linux-nvme, linux-kernel, kernel-janitors 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. fyi: A message from Rasmus awhile ago on the smatch list sent to me and Dan that's relevant. (AFAIK: this list isn't archived anywhere) On Wed, 2015-02-11 at 11:34 +0100, Rasmus Villemoes wrote: > Hi, > > As mentioned, I've been working on getting smatch to do type checking of > the various %p format extensions. The code is now on github > (https://github.com/Villemoes/smatch). > > Note that this work revealed a bug in sparse's handling of string > literals coming from macro expansions > (http://thread.gmane.org/gmane.comp.parsers.sparse/4080). I've applied > one of the suggested fixes, but it's still not clear to me what the > final fix will be in sparse upstream. Anyway, this was good enough to > get the ball rolling. > > While developing this, I found it useful to only enable that specific > check (both to get smatch run faster and to get less noise in the > output), so there's also a few unrelated patches in the printf branch > implementing that feature. > > sparse currently ignores attribute((format)), so the list of printf functions > has been extracted with a perl script and hard-coded. Even if sparse > understood attribute((format)), I wouldn't know how to set up a hook for > 'call of function with this or that attribute'. > > I don't think it's ready to be merged upstream (and whether that will > even happen is of course entirely up to Dan), but now it's out there for > people to play with. I have already sent patches for the four %p bugs > found, but there may be a few more lurking in arch/<not x86>/ - I don't > know how to pursuade the build system to go there. > > Rasmus ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [patch] nvme-fabrics: correct some printk information 2016-12-10 9:06 [patch] nvme-fabrics: correct some printk information Dan Carpenter 2016-12-10 11:27 ` Joe Perches @ 2016-12-20 0:40 ` James Smart 2016-12-20 9:16 ` Dan Carpenter 1 sibling, 1 reply; 16+ messages in thread From: James Smart @ 2016-12-20 0:40 UTC (permalink / raw) To: Dan Carpenter Cc: Keith Busch, Jens Axboe, linux-nvme, linux-kernel, kernel-janitors Dan, Mind if I solve this a different way ? I really don't know why knowing the ptr value is even meaningful -- james On 12/10/2016 1:06 AM, 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 > index 771e2e761872..e6395ed2f562 100644 > --- 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); > > kref_get(&ctrl->ctrl.kref); > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [patch] nvme-fabrics: correct some printk information 2016-12-20 0:40 ` James Smart @ 2016-12-20 9:16 ` Dan Carpenter 0 siblings, 0 replies; 16+ messages in thread From: Dan Carpenter @ 2016-12-20 9:16 UTC (permalink / raw) To: James Smart Cc: Keith Busch, Jens Axboe, linux-nvme, linux-kernel, kernel-janitors On Mon, Dec 19, 2016 at 04:40:30PM -0800, James Smart wrote: > Dan, > > Mind if I solve this a different way ? Not at all. Could you give me a Reported-by tag? regards, dan carpenter ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2016-12-20 9:17 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 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 2016-12-11 0:36 ` Joe Perches 2016-12-20 0:40 ` James Smart 2016-12-20 9:16 ` Dan Carpenter
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).