[PATCH] use mod_timer in scsi_add_timer

[PATCH] use mod_timer in scsi_add_timer

[Christoph Hellwig]

del_timer / modify / add_timer is racy.  mod_timer on not active
timer is fine, though.

Note that eh_timeout handling looks fishy in more places, I'll
audit it.


--- 1.62/drivers/scsi/scsi_error.c	Mon Aug 25 15:37:40 2003
+++ edited/drivers/scsi/scsi_error.c	Thu Aug 28 23:04:57 2003
@@ -105,24 +105,13 @@
 void scsi_add_timer(struct scsi_cmnd *scmd, int timeout,
 		    void (*complete)(struct scsi_cmnd *))
 {
-
-	/*
-	 * If the clock was already running for this command, then
-	 * first delete the timer.  The timer handling code gets rather
-	 * confused if we don't do this.
-	 */
-	if (scmd->eh_timeout.function)
-		del_timer(&scmd->eh_timeout);
-
 	scmd->eh_timeout.data = (unsigned long)scmd;
-	scmd->eh_timeout.expires = jiffies + timeout;
 	scmd->eh_timeout.function = (void (*)(unsigned long)) complete;
+	mod_timer(&scmd->eh_timeout, jiffies + timeout);
 
 	SCSI_LOG_ERROR_RECOVERY(5, printk("%s: scmd: %p, time:"
 					  " %d, (%p)\n", __FUNCTION__,
 					  scmd, timeout, complete));
-
-	add_timer(&scmd->eh_timeout);
 }
 
 /**

Re: [PATCH] use mod_timer in scsi_add_timer

[Alan Cox]

On Sul, 2003-08-31 at 12:28, Christoph Hellwig wrote:
> -
>  	scmd->eh_timeout.data = (unsigned long)scmd;
> -	scmd->eh_timeout.expires = jiffies + timeout;
>  	scmd->eh_timeout.function = (void (*)(unsigned long)) complete;

And your change is racy too. You might now call the timer function with
the data from the old timeout and the function of the new...

You just need the del_timer to be unconditional, the other change isn't
safe.

Re: [PATCH] use mod_timer in scsi_add_timer

[Christoph Hellwig]

On Sun, Aug 31, 2003 at 01:51:58PM +0100, Alan Cox wrote:
> On Sul, 2003-08-31 at 12:28, Christoph Hellwig wrote:
> > -
> >  	scmd->eh_timeout.data = (unsigned long)scmd;
> > -	scmd->eh_timeout.expires = jiffies + timeout;
> >  	scmd->eh_timeout.function = (void (*)(unsigned long)) complete;
> 
> And your change is racy too. You might now call the timer function with
> the data from the old timeout and the function of the new...

The data always is scmd.

Re: [PATCH] use mod_timer in scsi_add_timer

[Alan Cox]

On Sul, 2003-08-31 at 13:53, Christoph Hellwig wrote:
> > And your change is racy too. You might now call the timer function with
> > the data from the old timeout and the function of the new...
> 
> The data always is scmd.

So why are you setting that field ?

Even if it is true now its an invitation to a very hard to track down
and horrible bug.