[patch] drivers/scsi/dpt_i2o.c: fix a NULL pointer dereference

[patch] drivers/scsi/dpt_i2o.c: fix a NULL pointer dereference

[Adrian Bunk]

The Coverity checker spotted this obvious NULL pointer dereference.


Signed-off-by: Adrian Bunk <bunk@stusta.de>
Acked-by: Mark Salyzyn <mark_salyzyn@adaptec.com>

---

This patch was already sent on:
- 23 Nov 2005
- 21 Nov 2005

 drivers/scsi/dpt_i2o.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

--- linux-2.6.15-rc1-mm2-full/drivers/scsi/dpt_i2o.c.old	2005-11-20 22:13:37.000000000 +0100
+++ linux-2.6.15-rc1-mm2-full/drivers/scsi/dpt_i2o.c	2005-11-20 22:16:57.000000000 +0100
@@ -816,7 +816,7 @@
 static void adpt_i2o_sys_shutdown(void)
 {
 	adpt_hba *pHba, *pNext;
-	struct adpt_i2o_post_wait_data *p1, *p2;
+	struct adpt_i2o_post_wait_data *p1, *old;
 
 	 printk(KERN_INFO"Shutting down Adaptec I2O controllers.\n");
 	 printk(KERN_INFO"   This could take a few minutes if there are many devices attached\n");
@@ -830,13 +830,14 @@
 	}
 
 	/* Remove any timedout entries from the wait queue.  */
-	p2 = NULL;
 //	spin_lock_irqsave(&adpt_post_wait_lock, flags);
 	/* Nothing should be outstanding at this point so just
 	 * free them 
 	 */
-	for(p1 = adpt_post_wait_queue; p1; p2 = p1, p1 = p2->next) {
-		kfree(p1);
+	for(p1 = adpt_post_wait_queue; p1;) {
+		old = p1;
+		p1 = p1->next;
+		kfree(old);
 	}
 //	spin_unlock_irqrestore(&adpt_post_wait_lock, flags);
 	adpt_post_wait_queue = NULL;

Re: [patch] drivers/scsi/dpt_i2o.c: fix a NULL pointer dereference

[Marcelo Tosatti]

On Sun, Nov 27, 2005 at 12:36:37AM +0100, Adrian Bunk wrote:
> The Coverity checker spotted this obvious NULL pointer dereference.

Hi Adrian,

Could you explain why you remove the adpt_post_wait_lock acquision? 

And if it does not belong there, why don't you remove it instead of
commeting out?

> Signed-off-by: Adrian Bunk <bunk@stusta.de>
> Acked-by: Mark Salyzyn <mark_salyzyn@adaptec.com>
> 
> ---
> 
> This patch was already sent on:
> - 23 Nov 2005
> - 21 Nov 2005
> 
>  drivers/scsi/dpt_i2o.c |    9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> --- linux-2.6.15-rc1-mm2-full/drivers/scsi/dpt_i2o.c.old	2005-11-20 22:13:37.000000000 +0100
> +++ linux-2.6.15-rc1-mm2-full/drivers/scsi/dpt_i2o.c	2005-11-20 22:16:57.000000000 +0100
> @@ -816,7 +816,7 @@
>  static void adpt_i2o_sys_shutdown(void)
>  {
>  	adpt_hba *pHba, *pNext;
> -	struct adpt_i2o_post_wait_data *p1, *p2;
> +	struct adpt_i2o_post_wait_data *p1, *old;
>  
>  	 printk(KERN_INFO"Shutting down Adaptec I2O controllers.\n");
>  	 printk(KERN_INFO"   This could take a few minutes if there are many devices attached\n");
> @@ -830,13 +830,14 @@
>  	}
>  
>  	/* Remove any timedout entries from the wait queue.  */
> -	p2 = NULL;
>  //	spin_lock_irqsave(&adpt_post_wait_lock, flags);
>  	/* Nothing should be outstanding at this point so just
>  	 * free them 
>  	 */
> -	for(p1 = adpt_post_wait_queue; p1; p2 = p1, p1 = p2->next) {
> -		kfree(p1);
> +	for(p1 = adpt_post_wait_queue; p1;) {
> +		old = p1;
> +		p1 = p1->next;
> +		kfree(old);
>  	}
>  //	spin_unlock_irqrestore(&adpt_post_wait_lock, flags);
>  	adpt_post_wait_queue = NULL;

Re: [patch] drivers/scsi/dpt_i2o.c: fix a NULL pointer dereference

[Adrian Bunk]

On Sun, Nov 27, 2005 at 10:47:38AM -0200, Marcelo Tosatti wrote:
> On Sun, Nov 27, 2005 at 12:36:37AM +0100, Adrian Bunk wrote:
> > The Coverity checker spotted this obvious NULL pointer dereference.
> 
> Hi Adrian,

Hi Marcelo,

> Could you explain why you remove the adpt_post_wait_lock acquision? 
> 
> And if it does not belong there, why don't you remove it instead of
> commeting out?
>...

my patch does remove the following not required line:

> > -	p2 = NULL;

It does not touch the following line that was already commented out:

> >  //	spin_lock_irqsave(&adpt_post_wait_lock, flags);
>...

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: [patch] drivers/scsi/dpt_i2o.c: fix a NULL pointer dereference

[Marcelo Tosatti]

On Sun, Nov 27, 2005 at 07:52:52PM +0100, Adrian Bunk wrote:
> On Sun, Nov 27, 2005 at 10:47:38AM -0200, Marcelo Tosatti wrote:
> > On Sun, Nov 27, 2005 at 12:36:37AM +0100, Adrian Bunk wrote:
> > > The Coverity checker spotted this obvious NULL pointer dereference.
> > 
> > Hi Adrian,
> 
> Hi Marcelo,
> 
> > Could you explain why you remove the adpt_post_wait_lock acquision? 
> > 
> > And if it does not belong there, why don't you remove it instead of
> > commeting out?
> >...
> 
> my patch does remove the following not required line:
> 
> > > -	p2 = NULL;
> 
> It does not touch the following line that was already commented out:
> 
> > >  //	spin_lock_irqsave(&adpt_post_wait_lock, flags);
> >...

Doh. I should _read_ the patch. 

Re: [patch] drivers/scsi/dpt_i2o.c: fix a NULL pointer dereference

[James Bottomley]

On Sun, 2005-11-27 at 00:36 +0100, Adrian Bunk wrote:
> The Coverity checker spotted this obvious NULL pointer dereference.

It's a bit late for this one, since Linus already put it in, but for
future reference, could you please try to use proper descriptions.  This
isn't an "obvious NULL pointer dereference", it's actually a use after
free of a data structure.

Thanks,

James

Re: [patch] drivers/scsi/dpt_i2o.c: fix a NULL pointer dereference

[Adrian Bunk]

On Mon, Nov 28, 2005 at 12:37:11PM -0600, James Bottomley wrote:
> On Sun, 2005-11-27 at 00:36 +0100, Adrian Bunk wrote:
> > The Coverity checker spotted this obvious NULL pointer dereference.
> 
> It's a bit late for this one, since Linus already put it in, but for
> future reference, could you please try to use proper descriptions.  This
> isn't an "obvious NULL pointer dereference", it's actually a use after
> free of a data structure.

OK sorry, I'll look better at the descriptions for future patches.

> Thanks,
> 
> James

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

[patch] drivers/scsi/dpt_i2o.c: fix a NULL pointer dereference

[Adrian Bunk]

The Coverity checker spotted this obvious NULL pointer dereference.


Signed-off-by: Adrian Bunk <bunk@stusta.de>
Acked-by: Mark Salyzyn <mark_salyzyn@adaptec.com>

---

This patch was already sent on:
- 21 Nov 2005

 drivers/scsi/dpt_i2o.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

--- linux-2.6.15-rc1-mm2-full/drivers/scsi/dpt_i2o.c.old	2005-11-20 22:13:37.000000000 +0100
+++ linux-2.6.15-rc1-mm2-full/drivers/scsi/dpt_i2o.c	2005-11-20 22:16:57.000000000 +0100
@@ -816,7 +816,7 @@
 static void adpt_i2o_sys_shutdown(void)
 {
 	adpt_hba *pHba, *pNext;
-	struct adpt_i2o_post_wait_data *p1, *p2;
+	struct adpt_i2o_post_wait_data *p1, *old;
 
 	 printk(KERN_INFO"Shutting down Adaptec I2O controllers.\n");
 	 printk(KERN_INFO"   This could take a few minutes if there are many devices attached\n");
@@ -830,13 +830,14 @@
 	}
 
 	/* Remove any timedout entries from the wait queue.  */
-	p2 = NULL;
 //	spin_lock_irqsave(&adpt_post_wait_lock, flags);
 	/* Nothing should be outstanding at this point so just
 	 * free them 
 	 */
-	for(p1 = adpt_post_wait_queue; p1; p2 = p1, p1 = p2->next) {
-		kfree(p1);
+	for(p1 = adpt_post_wait_queue; p1;) {
+		old = p1;
+		p1 = p1->next;
+		kfree(old);
 	}
 //	spin_unlock_irqrestore(&adpt_post_wait_lock, flags);
 	adpt_post_wait_queue = NULL;