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
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 <[email protected]> and the Coverity checker.
Signed-off-by: Rene Herman <[email protected]>
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;
}
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 <[email protected]>
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)) {
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 <[email protected]>
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.
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 <[email protected]>
>
> 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.
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 <[email protected]>
> >
> > 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
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.