dpt_i2o.c memleak/incorrectness

dpt_i2o.c memleak/incorrectness

[Oleg Drokin]

Hello!

   There is something strange going on in drivers/scsi/dpt_i2o.c in both
   2.4 and 2.5. adpt_i2o_reset_hba() function allocates 4 bytes 
   for "status" stuff, then tries to reset controller, then 
   if timeout on first reset stage is reached, frees "status" and returns,
   otherwise it proceeds to monitor "status" (which is modified by hardware
   now, btw), and if timeout is reached, just exits.
   On the first thought I just thought it is trivial memleak that can be
   fixed with patch below, but after some more thining I just thought
   "what if after some time controller awakes and modifies status,
   but it is already allocated for other purposes", scary eh?
   So may be we shold not free those four bytes on timeout at all just
   for safeness reasons?

   Found with help of smatch + enhanced unfree script.

Bye,
    Oleg

===== drivers/scsi/dpt_i2o.c 1.9 vs edited =====
--- 1.9/drivers/scsi/dpt_i2o.c	Wed Jan  8 18:26:13 2003
+++ edited/drivers/scsi/dpt_i2o.c	Thu Mar 13 21:17:10 2003
@@ -1336,6 +1336,7 @@
 			}
 			if(time_after(jiffies,timeout)){
 				printk(KERN_ERR "%s:Timeout waiting for IOP Reset.\n",pHba->name);
+				kfree(status);
 				return -ETIMEDOUT;
 			}
 		} while (m == EMPTY_QUEUE);

Re: dpt_i2o.c memleak/incorrectness

[Alan Cox]

On Thu, 2003-03-13 at 18:28, Oleg Drokin wrote:
> Hello!
> 
>    There is something strange going on in drivers/scsi/dpt_i2o.c in both
>    2.4 and 2.5. adpt_i2o_reset_hba() function allocates 4 bytes 
>    for "status" stuff, then tries to reset controller, then 
>    if timeout on first reset stage is reached, frees "status" and returns,
>    otherwise it proceeds to monitor "status" (which is modified by hardware
>    now, btw), and if timeout is reached, just exits.

Correctly - I2O does the same thing in this case. Its just better to
throw a few bytes away than risk corruption

Re: dpt_i2o.c fix for possibly memory corruption on reset timeout

[Oleg Drokin]

Hello!

On Thu, Mar 13, 2003 at 07:44:23PM +0000, Alan Cox wrote:
> >    if timeout on first reset stage is reached, frees "status" and returns,
> >    otherwise it proceeds to monitor "status" (which is modified by hardware
> >    now, btw), and if timeout is reached, just exits.
> Correctly - I2O does the same thing in this case. Its just better to
> throw a few bytes away than risk corruption

Ok, so please consider applying this patch instead (appies to both
2.4 and 2.5)

Bye,
    Oleg

===== drivers/scsi/dpt_i2o.c 1.9 vs edited =====
--- 1.9/drivers/scsi/dpt_i2o.c	Wed Jan  8 18:26:13 2003
+++ edited/drivers/scsi/dpt_i2o.c	Thu Mar 13 21:39:07 2003
@@ -1318,7 +1318,9 @@
 	while(*status == 0){
 		if(time_after(jiffies,timeout)){
 			printk(KERN_WARNING"%s: IOP Reset Timeout\n",pHba->name);
-			kfree(status);
+			/* We loose 4 bytes of "status" here, but we cannot
+			   free these because controller may awake and corrupt
+			   those bytes at any time */
 			return -ETIMEDOUT;
 		}
 		rmb();
@@ -1336,6 +1338,9 @@
 			}
 			if(time_after(jiffies,timeout)){
 				printk(KERN_ERR "%s:Timeout waiting for IOP Reset.\n",pHba->name);
+			/* We loose 4 bytes of "status" here, but we cannot
+			   free these because controller may awake and corrupt
+			   those bytes at any time */
 				return -ETIMEDOUT;
 			}
 		} while (m == EMPTY_QUEUE);

Re: dpt_i2o.c fix for possibly memory corruption on reset timeout

[Randy.Dunlap]

On Thu, 13 Mar 2003 21:41:07 +0300 Oleg Drokin <green@linuxhacker.ru> wrote:

| Ok, so please consider applying this patch instead (appies to both
| 2.4 and 2.5)

| +			/* We loose 4 bytes of "status" here, but we cannot
                              lose

| @@ -1336,6 +1338,9 @@
| +			/* We loose 4 bytes of "status" here, but we cannot
                              lose

--
~Randy

Re: dpt_i2o.c fix for possibly memory corruption on reset timeout

[Oleg Drokin]

Hello!

On Thu, Mar 13, 2003 at 10:51:25AM -0800, Randy.Dunlap wrote:
> | Ok, so please consider applying this patch instead (appies to both
> | 2.4 and 2.5)

Ok, here's the one with spelling fix from Randy ;)

Bye,
    Oleg

===== drivers/scsi/dpt_i2o.c 1.9 vs edited =====
--- 1.9/drivers/scsi/dpt_i2o.c	Wed Jan  8 18:26:13 2003
+++ edited/drivers/scsi/dpt_i2o.c	Thu Mar 13 21:55:08 2003
@@ -1318,7 +1318,9 @@
 	while(*status == 0){
 		if(time_after(jiffies,timeout)){
 			printk(KERN_WARNING"%s: IOP Reset Timeout\n",pHba->name);
-			kfree(status);
+			/* We lose 4 bytes of "status" here, but we cannot
+			   free these because controller may awake and corrupt
+			   those bytes at any time */
 			return -ETIMEDOUT;
 		}
 		rmb();
@@ -1336,6 +1338,9 @@
 			}
 			if(time_after(jiffies,timeout)){
 				printk(KERN_ERR "%s:Timeout waiting for IOP Reset.\n",pHba->name);
+			/* We lose 4 bytes of "status" here, but we cannot
+			   free these because controller may awake and corrupt
+			   those bytes at any time */
 				return -ETIMEDOUT;
 			}
 		} while (m == EMPTY_QUEUE);

Re: dpt_i2o.c fix for possibly memory corruption on reset timeout

[Denis Vlasenko]

On 13 March 2003 20:56, Oleg Drokin wrote:
> Hello!
>
> On Thu, Mar 13, 2003 at 10:51:25AM -0800, Randy.Dunlap wrote:
> > | Ok, so please consider applying this patch instead (appies to
> > | both 2.4 and 2.5)
>
> Ok, here's the one with spelling fix from Randy ;)
>
> Bye,
>     Oleg
>
> ===== drivers/scsi/dpt_i2o.c 1.9 vs edited =====
> --- 1.9/drivers/scsi/dpt_i2o.c	Wed Jan  8 18:26:13 2003
> +++ edited/drivers/scsi/dpt_i2o.c	Thu Mar 13 21:55:08 2003
> @@ -1318,7 +1318,9 @@
>  	while(*status == 0){
>  		if(time_after(jiffies,timeout)){
>  			printk(KERN_WARNING"%s: IOP Reset Timeout\n",pHba->name);
> -			kfree(status);
> +			/* We lose 4 bytes of "status" here, but we cannot
> +			   free these because controller may awake and corrupt
> +			   those bytes at any time */

I'd leave kfree() inside the comment - less effort for those
operating under -ENOCAFFEINE condition

			/* do NOT do kfree(status): we lose ....

I don't like the whole idea that mem leak is tolerable.

Can we just add a 4 byte scratch pad status to
struct _adpt_hba? Let it scribble there...
--
vda

Re: dpt_i2o.c fix for possibly memory corruption on reset timeout

[Joern Engel]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=unknown-8bit, Size: 614 bytes --]

On Fri, 14 March 2003 11:18:49 +0200, Denis Vlasenko wrote:
> 
> I don't like the whole idea that mem leak is tolerable.
> 
> Can we just add a 4 byte scratch pad status to
> struct _adpt_hba? Let it scribble there...

I've had the same idea, but there might be some pitfalls around.
The problem boils down to the users of the buffer. Is it per-device,
per-process, per-whatever?

Once this is known I'm all for putting the buffer into a per-whatever
data structure. But someone has to understand the driver first. :)

Jörn

-- 
Everything should be made as simple as possible, but not simpler.
-- Albert Einstein

Re: dpt_i2o.c fix for possibly memory corruption on reset timeout

[Alan Cox]

On Fri, 2003-03-14 at 09:18, Denis Vlasenko wrote:
> I don't like the whole idea that mem leak is tolerable.
> 
> Can we just add a 4 byte scratch pad status to
> struct _adpt_hba? Let it scribble there...

Its 4 bytes (+ slab overhead), its far safer if this happens to say
its gone forever. Its owned by the I2O controller now and it never
gave it back

Re: dpt_i2o.c fix for possibly memory corruption on reset timeout

[Joern Engel]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=unknown-8bit, Size: 890 bytes --]

On Fri, 14 March 2003 14:19:58 +0000, Alan Cox wrote:
> On Fri, 2003-03-14 at 09:18, Denis Vlasenko wrote:
> > I don't like the whole idea that mem leak is tolerable.
> > 
> > Can we just add a 4 byte scratch pad status to
> > struct _adpt_hba? Let it scribble there...
> 
> Its 4 bytes (+ slab overhead), its far safer if this happens to say
> its gone forever. Its owned by the I2O controller now and it never
> gave it back

How about an (optional) counter then? If you can show that this case
is hit zero times during operation, noone will complain. On the other
hand, if some hardware hits this problem 1000+ times, we have a good
reason to find another solution.

I'd volunteer to create the patch, if the idea is accepted.

Jörn

-- 
Mundie uses a textbook tactic of manipulation: start with some
reasonable talk, and lead the audience to an unreasonable conclusion.
-- Bruce Perens

Re: dpt_i2o.c fix for possibly memory corruption on reset timeout

[Oleg Drokin]

Hello!

On Fri, Mar 14, 2003 at 02:39:42PM +0100, Joern Engel wrote:
> > > Can we just add a 4 byte scratch pad status to
> > > struct _adpt_hba? Let it scribble there...
> > Its 4 bytes (+ slab overhead), its far safer if this happens to say
> > its gone forever. Its owned by the I2O controller now and it never
> > gave it back
> How about an (optional) counter then? If you can show that this case
> is hit zero times during operation, noone will complain. On the other
> hand, if some hardware hits this problem 1000+ times, we have a good
> reason to find another solution.
> I'd volunteer to create the patch, if the idea is accepted.

Well, if some hardware would do so, then users would go here and complain
about kernel being noisy on certain hardware. (message is printed each
time this happens). Have not happened so far.

Bye,
    Oleg

Re: dpt_i2o.c fix for possibly memory corruption on reset timeout

[Alan Cox]

On Fri, 2003-03-14 at 13:43, Oleg Drokin wrote:
> Well, if some hardware would do so, then users would go here and complain
> about kernel being noisy on certain hardware. (message is printed each
> time this happens). Have not happened so far.

Its run once per controller on board setup, and if it fails that controller
is history until reboot

Re: dpt_i2o.c memleak/incorrectness

[Bryan Andersen]

>>   There is something strange going on in drivers/scsi/dpt_i2o.c in both
>>   2.4 and 2.5. adpt_i2o_reset_hba() function allocates 4 bytes 
>>   for "status" stuff, then tries to reset controller, then 
>>   if timeout on first reset stage is reached, frees "status" and returns,
>>   otherwise it proceeds to monitor "status" (which is modified by hardware
>>   now, btw), and if timeout is reached, just exits.
> 
> Correctly - I2O does the same thing in this case. Its just better to
> throw a few bytes away than risk corruption

Better document it in the comments or it will get "corrected" by some 
mem leak detector.  If possible try to use a static for the pointer to 
the status block, but that may not work.  Re-enterant code and multi CPU 
situations likely won't allow for that.  Also it might not be worth the 
effort to properly determin if it is safe to use only one location.

- Bryan

Re: dpt_i2o.c memleak/incorrectness

[Horst von Brand]

Bryan Andersen <bryan@bogonomicon.net> dijo:
> >>   There is something strange going on in drivers/scsi/dpt_i2o.c in both
> >>   2.4 and 2.5. adpt_i2o_reset_hba() function allocates 4 bytes 
> >>   for "status" stuff, then tries to reset controller, then 
> >>   if timeout on first reset stage is reached, frees "status" and returns,
> >>   otherwise it proceeds to monitor "status" (which is modified by hardware
> >>   now, btw), and if timeout is reached, just exits.
> > 
> > Correctly - I2O does the same thing in this case. Its just better to
> > throw a few bytes away than risk corruption
> 
> Better document it in the comments or it will get "corrected" by some 
> mem leak detector.  If possible try to use a static for the pointer to 
> the status block, but that may not work.  Re-enterant code and multi CPU 
> situations likely won't allow for that.  Also it might not be worth the 
> effort to properly determin if it is safe to use only one location.

A device owned area, perhaps? To set up an area that can be messed around
without proper locking will get you problems down the line.
-- 
Dr. Horst H. von Brand                   User #22616 counter.li.org
Departamento de Informatica                     Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria              +56 32 654239
Casilla 110-V, Valparaiso, Chile                Fax:  +56 32 797513

Now i2o_core.c memleak/incorrectness?

[Oleg Drokin]

Hello!

On Thu, Mar 13, 2003 at 07:44:23PM +0000, Alan Cox wrote:
> >    There is something strange going on in drivers/scsi/dpt_i2o.c in both
> >    2.4 and 2.5. adpt_i2o_reset_hba() function allocates 4 bytes 
> >    for "status" stuff, then tries to reset controller, then 
> >    if timeout on first reset stage is reached, frees "status" and returns,
> >    otherwise it proceeds to monitor "status" (which is modified by hardware
> >    now, btw), and if timeout is reached, just exits.
> Correctly - I2O does the same thing in this case. Its just better to
> throw a few bytes away than risk corruption

Well, it seems that i2o does not always follow this rule.
Also i2o_init_outbound_q() seems not free this "status" thing if everything
went ok, is this intentional?
Or perhaps something like this patch is needed?

Bye,
    Oleg

===== drivers/message/i2o/i2o_core.c 1.12 vs edited =====
--- 1.12/drivers/message/i2o/i2o_core.c	Tue Aug  6 18:42:18 2002
+++ edited/drivers/message/i2o/i2o_core.c	Thu Mar 13 22:36:40 2003
@@ -2217,7 +2217,7 @@
 			else  
 				printk(KERN_ERR "%s: Outbound queue initialize timeout.\n",
 					c->name);
-			kfree(status);
+			// Better leak this for safety: kfree(status);
 			return -ETIMEDOUT;
 		}  
 		schedule();
@@ -2231,6 +2231,7 @@
 		return -ETIMEDOUT;
 	}
 
+	kfree(status);
 	return 0;
 }
 

Re: Now i2o_core.c memleak/incorrectness?

[Alan Cox]

On Thu, 2003-03-13 at 19:38, Oleg Drokin wrote:
> Well, it seems that i2o does not always follow this rule.
> Also i2o_init_outbound_q() seems not free this "status" thing if everything
> went ok, is this intentional?
> Or perhaps something like this patch is needed?

I'll take a look. I'm doing a bunch of i2o cleanup in 2.5 right now