Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1539800 > unrolled thread

[patch] nvme-fabrics: correct some printk information

Back to article view | Back to linux.kernel

Contents

  [patch] nvme-fabrics: correct some printk information Dan Carpenter <dan.carpenter@oracle.com> - 2016-12-10 10:20 +0100
    Re: [patch] nvme-fabrics: correct some printk information Joe Perches <joe@perches.com> - 2016-12-10 12:30 +0100
      Re: [patch] nvme-fabrics: correct some printk information Dan Carpenter <dan.carpenter@oracle.com> - 2016-12-10 20:00 +0100
        Re: [patch] nvme-fabrics: correct some printk information Julia Lawall <julia.lawall@lip6.fr> - 2016-12-10 21:10 +0100
          Re: [patch] nvme-fabrics: correct some printk information Dan Carpenter <dan.carpenter@oracle.com> - 2016-12-10 21:30 +0100
          Re: [patch] nvme-fabrics: correct some printk information Joe Perches <joe@perches.com> - 2016-12-10 22:00 +0100
            Re: [patch] nvme-fabrics: correct some printk information Dan Carpenter <dan.carpenter@oracle.com> - 2016-12-10 22:10 +0100
              Re: [patch] nvme-fabrics: correct some printk information Joe Perches <joe@perches.com> - 2016-12-10 23:30 +0100
                Re: [patch] nvme-fabrics: correct some printk information Dan Carpenter <dan.carpenter@oracle.com> - 2016-12-12 10:40 +0100
                  Re: [patch] nvme-fabrics: correct some printk information Julia Lawall <julia.lawall@lip6.fr> - 2016-12-12 16:50 +0100
                    Re: [patch] nvme-fabrics: correct some printk information Joe Perches <joe@perches.com> - 2016-12-12 17:00 +0100
            Re: [patch] nvme-fabrics: correct some printk information Julia Lawall <julia.lawall@lip6.fr> - 2016-12-10 23:10 +0100
              Re: [patch] nvme-fabrics: correct some printk information Joe Perches <joe@perches.com> - 2016-12-10 23:30 +0100
          Re: [patch] nvme-fabrics: correct some printk information Joe Perches <joe@perches.com> - 2016-12-11 01:40 +0100
    Re: [patch] nvme-fabrics: correct some printk information James Smart <james.smart@broadcom.com> - 2016-12-20 01:50 +0100
      Re: [patch] nvme-fabrics: correct some printk information Dan Carpenter <dan.carpenter@oracle.com> - 2016-12-20 10:20 +0100

#1539800 — [patch] nvme-fabrics: correct some printk information

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

[toc] | [next] | [standalone]

#1539812

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.

[toc] | [prev] | [next] | [standalone]

#1539890

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

[toc] | [prev] | [next] | [standalone]

#1539898

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
>

[toc] | [prev] | [next] | [standalone]

#1539900

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

[toc] | [prev] | [next] | [standalone]

#1539909

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 "\("  

[toc] | [prev] | [next] | [standalone]

#1539914

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

[toc] | [prev] | [next] | [standalone]

#1539921

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;

[toc] | [prev] | [next] | [standalone]

#1540210

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

[toc] | [prev] | [next] | [standalone]

#1540400

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

[toc] | [prev] | [next] | [standalone]

#1540409

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.

[toc] | [prev] | [next] | [standalone]

#1539918

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

[toc] | [prev] | [next] | [standalone]

#1539922

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

[toc] | [prev] | [next] | [standalone]

#1539936

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

[toc] | [prev] | [next] | [standalone]

#1544845

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

[toc] | [prev] | [next] | [standalone]

#1544988

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web