pnp_bus_resume(): inconsequent NULL checking

pnp_bus_resume(): inconsequent NULL checking

[Adrian Bunk]

The Coverity checker spotted the following inconsequent NULL checking 
introduced by commit 5d38998ed15b31f524bde9a193d60150af30d916:

<--  snip  -->

...
static int pnp_bus_resume(struct device *dev)
{
...
        if (pnp_dev->protocol && pnp_dev->protocol->resume)
                pnp_dev->protocol->resume(pnp_dev);

        if (pnp_can_write(pnp_dev)) {
...

<--  snip  -->

pnp_can_write(pnp_dev) dereferences pnp_dev->protocol.

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: pnp_bus_resume(): inconsequent NULL checking

[Rene Herman]

[-- Attachment #1: Type: text/plain, Size: 1066 bytes --]

On 19-02-08 23:49, Adrian Bunk wrote:

> The Coverity checker spotted the following inconsequent NULL checking 
> introduced by commit 5d38998ed15b31f524bde9a193d60150af30d916:
> 
> <--  snip  -->
> 
> ...
> static int pnp_bus_resume(struct device *dev)
> {
> ...
>         if (pnp_dev->protocol && pnp_dev->protocol->resume)
>                 pnp_dev->protocol->resume(pnp_dev);
> 
>         if (pnp_can_write(pnp_dev)) {
> ...
> 
> <--  snip  -->
> 
> pnp_can_write(pnp_dev) dereferences pnp_dev->protocol.

I see, thanks. pnp_bus_suspend() has the same problem (I added this test to 
complete the mirror in fact) and/but is not a real problem since the tests 
are also the first things done inside the blocks they protect -- if 
pnp_dev->protocol isn't set here, we're dead anyway therefore.

That probably means we can just delete the pnp_dev->protocol tests but this 
  would need an ack from for example Bjorn Helgaas who might have an idea 
about how generically useful this is designed to be. The no brain thing to 
do would be just as per attached.

Bjorn?

[-- Attachment #2: coverity-pnp_bus_suspend_resume.diff --]
[-- Type: text/plain, Size: 1681 bytes --]

pnp: be consistent about testing pnp_dev->protocol in pnp_bus_{suspend,resume}

pnp_can_{disable,write}() dereference pnp_dev->protocol. So do the
pnp_{stop,start}_dev() the tests protect, but let's be consistent
at least.

Spotted by Adrian Bunk <bunk@kernel.org> and the Coverity checker.

Signed-off-by: Rene Herman <rene.herman@gmail.com>

diff --git a/drivers/pnp/driver.c b/drivers/pnp/driver.c
index 12a1645..170af61 100644
--- a/drivers/pnp/driver.c
+++ b/drivers/pnp/driver.c
@@ -160,15 +160,15 @@ static int pnp_bus_suspend(struct device *dev, pm_message_t state)
 		if (error)
 			return error;
 	}
-
-	if (pnp_can_disable(pnp_dev)) {
-		error = pnp_stop_dev(pnp_dev);
-		if (error)
-			return error;
+	if (pnp_dev->protocol) {
+		if (pnp_can_disable(pnp_dev)) {
+			error = pnp_stop_dev(pnp_dev);
+			if (error)
+				return error;
+		}
+		if (pnp_dev->protocol->suspend)
+			pnp_dev->protocol->suspend(pnp_dev, state);
 	}
-
-	if (pnp_dev->protocol && pnp_dev->protocol->suspend)
-		pnp_dev->protocol->suspend(pnp_dev, state);
 	return 0;
 }
 
@@ -181,21 +181,21 @@ static int pnp_bus_resume(struct device *dev)
 	if (!pnp_drv)
 		return 0;
 
-	if (pnp_dev->protocol && pnp_dev->protocol->resume)
-		pnp_dev->protocol->resume(pnp_dev);
+	if (pnp_dev->protocol) {
+		if (pnp_dev->protocol->resume)
+			pnp_dev->protocol->resume(pnp_dev);
 
-	if (pnp_can_write(pnp_dev)) {
-		error = pnp_start_dev(pnp_dev);
-		if (error)
-			return error;
+		if (pnp_can_write(pnp_dev)) {
+			error = pnp_start_dev(pnp_dev);
+			if (error)
+				return error;
+		}
 	}
-
 	if (pnp_drv->resume) {
 		error = pnp_drv->resume(pnp_dev);
 		if (error)
 			return error;
 	}
-
 	return 0;
 }
 

Re: pnp_bus_resume(): inconsequent NULL checking

[Bjorn Helgaas]

On Tuesday 19 February 2008 05:00:43 pm Rene Herman wrote:
> On 19-02-08 23:49, Adrian Bunk wrote:
> 
> > The Coverity checker spotted the following inconsequent NULL checking 
> > introduced by commit 5d38998ed15b31f524bde9a193d60150af30d916:
> > 
> > <--  snip  -->
> > 
> > ...
> > static int pnp_bus_resume(struct device *dev)
> > {
> > ...
> >         if (pnp_dev->protocol && pnp_dev->protocol->resume)
> >                 pnp_dev->protocol->resume(pnp_dev);
> > 
> >         if (pnp_can_write(pnp_dev)) {
> > ...
> > 
> > <--  snip  -->
> > 
> > pnp_can_write(pnp_dev) dereferences pnp_dev->protocol.
> 
> I see, thanks. pnp_bus_suspend() has the same problem (I added this test to 
> complete the mirror in fact) and/but is not a real problem since the tests 
> are also the first things done inside the blocks they protect -- if 
> pnp_dev->protocol isn't set here, we're dead anyway therefore.
> 
> That probably means we can just delete the pnp_dev->protocol tests but this 
>   would need an ack from for example Bjorn Helgaas who might have an idea 
> about how generically useful this is designed to be. The no brain thing to 
> do would be just as per attached.

I agree with you that we can just delete the dev->protocol tests
completely. So I'd rather see something like this (built but untested):


PNP: remove dev->protocol NULL checks

Every PNP device should have a valid protocol pointer.  If it doesn't,
something's wrong and we should oops so we can find and fix the problem.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>

Index: work6/drivers/pnp/driver.c
===================================================================
--- work6.orig/drivers/pnp/driver.c	2008-02-20 09:46:01.000000000 -0700
+++ work6/drivers/pnp/driver.c	2008-02-20 09:46:28.000000000 -0700
@@ -167,7 +167,7 @@
 			return error;
 	}
 
-	if (pnp_dev->protocol && pnp_dev->protocol->suspend)
+	if (pnp_dev->protocol->suspend)
 		pnp_dev->protocol->suspend(pnp_dev, state);
 	return 0;
 }
@@ -181,7 +181,7 @@
 	if (!pnp_drv)
 		return 0;
 
-	if (pnp_dev->protocol && pnp_dev->protocol->resume)
+	if (pnp_dev->protocol->resume)
 		pnp_dev->protocol->resume(pnp_dev);
 
 	if (pnp_can_write(pnp_dev)) {

Re: pnp_bus_resume(): inconsequent NULL checking

[Rene Herman]

On 20-02-08 17:59, Bjorn Helgaas wrote:

> I agree with you that we can just delete the dev->protocol tests
> completely. So I'd rather see something like this (built but untested):
> 
> 
> PNP: remove dev->protocol NULL checks
> 
> Every PNP device should have a valid protocol pointer.  If it doesn't,
> something's wrong and we should oops so we can find and fix the problem.
> 
> Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>

Ack from a functional standpoint: we are oopsing in pnp_start/stop_dev 
_anyway_ if the protocol pointer isn't set.

Will you coach this upstream? A 2.6.25-rc1 change from me made the coverity 
checker pick up on it which might be considered enough of an excuse to call 
it a regression and submit this as a fix...

> Index: work6/drivers/pnp/driver.c
> ===================================================================
> --- work6.orig/drivers/pnp/driver.c	2008-02-20 09:46:01.000000000 -0700
> +++ work6/drivers/pnp/driver.c	2008-02-20 09:46:28.000000000 -0700
> @@ -167,7 +167,7 @@
>  			return error;
>  	}
>  
> -	if (pnp_dev->protocol && pnp_dev->protocol->suspend)
> +	if (pnp_dev->protocol->suspend)
>  		pnp_dev->protocol->suspend(pnp_dev, state);
>  	return 0;
>  }
> @@ -181,7 +181,7 @@
>  	if (!pnp_drv)
>  		return 0;
>  
> -	if (pnp_dev->protocol && pnp_dev->protocol->resume)
> +	if (pnp_dev->protocol->resume)
>  		pnp_dev->protocol->resume(pnp_dev);
>  
>  	if (pnp_can_write(pnp_dev)) {
> 

Rene.

Re: pnp_bus_resume(): inconsequent NULL checking

[Bjorn Helgaas]

On Wednesday 20 February 2008 10:47:21 pm Rene Herman wrote:
> On 20-02-08 17:59, Bjorn Helgaas wrote:
> > I agree with you that we can just delete the dev->protocol tests
> > completely. So I'd rather see something like this (built but untested):
> >
> >
> > PNP: remove dev->protocol NULL checks
> >
> > Every PNP device should have a valid protocol pointer.  If it doesn't,
> > something's wrong and we should oops so we can find and fix the problem.
> >
> > Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
>
> Ack from a functional standpoint: we are oopsing in pnp_start/stop_dev
> _anyway_ if the protocol pointer isn't set.
>
> Will you coach this upstream? A 2.6.25-rc1 change from me made the coverity
> checker pick up on it which might be considered enough of an excuse to call
> it a regression and submit this as a fix...

I'll push it upstream, but a coverity warning seems like a marginal
excuse for putting it in 2.6.25.  Is there any real reason it can't
wait until 2.6.26?

> > Index: work6/drivers/pnp/driver.c
> > ===================================================================
> > --- work6.orig/drivers/pnp/driver.c	2008-02-20 09:46:01.000000000 -0700
> > +++ work6/drivers/pnp/driver.c	2008-02-20 09:46:28.000000000 -0700
> > @@ -167,7 +167,7 @@
> >  			return error;
> >  	}
> >
> > -	if (pnp_dev->protocol && pnp_dev->protocol->suspend)
> > +	if (pnp_dev->protocol->suspend)
> >  		pnp_dev->protocol->suspend(pnp_dev, state);
> >  	return 0;
> >  }
> > @@ -181,7 +181,7 @@
> >  	if (!pnp_drv)
> >  		return 0;
> >
> > -	if (pnp_dev->protocol && pnp_dev->protocol->resume)
> > +	if (pnp_dev->protocol->resume)
> >  		pnp_dev->protocol->resume(pnp_dev);
> >
> >  	if (pnp_can_write(pnp_dev)) {
>
> Rene.

Re: pnp_bus_resume(): inconsequent NULL checking

[Adrian Bunk]

On Thu, Feb 21, 2008 at 08:26:53AM -0700, Bjorn Helgaas wrote:
> On Wednesday 20 February 2008 10:47:21 pm Rene Herman wrote:
> > On 20-02-08 17:59, Bjorn Helgaas wrote:
> > > I agree with you that we can just delete the dev->protocol tests
> > > completely. So I'd rather see something like this (built but untested):
> > >
> > >
> > > PNP: remove dev->protocol NULL checks
> > >
> > > Every PNP device should have a valid protocol pointer.  If it doesn't,
> > > something's wrong and we should oops so we can find and fix the problem.
> > >
> > > Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
> >
> > Ack from a functional standpoint: we are oopsing in pnp_start/stop_dev
> > _anyway_ if the protocol pointer isn't set.
> >
> > Will you coach this upstream? A 2.6.25-rc1 change from me made the coverity
> > checker pick up on it which might be considered enough of an excuse to call
> > it a regression and submit this as a fix...
> 
> I'll push it upstream, but a coverity warning seems like a marginal
> excuse for putting it in 2.6.25.  Is there any real reason it can't
> wait until 2.6.26?

The main purpose of my mail was to get an answer whether the NULL check 
should be removed or whether there's a NULL dereference that could 
happen in practice (which would have been a real bug).

A NULL check too much is not a real bug and therefore it can't count as 
a regression, so from my side it doesn't matter whether you push it as 
"trivial enough" for 2.6.25 or as "not urgent" for 2.6.26.

> > Rene.

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: pnp_bus_resume(): inconsequent NULL checking

[Rene Herman]

On 21-02-08 16:26, Bjorn Helgaas wrote:

> I'll push it upstream, but a coverity warning seems like a marginal
> excuse for putting it in 2.6.25.  Is there any real reason it can't
> wait until 2.6.26?

No, but we're only at -rc2. Dumb little things such as this needn't wait an 
entire development cycle I'd feel but I obviously don't feel strongly about 
the issue itself.

Rene.