Re: + aacraid-fix-unchecked-down_interruptible.patch added to -mm tree

Re: + aacraid-fix-unchecked-down_interruptible.patch added to -mm tree

[James Bottomley]

On Thu, 2008-04-10 at 13:26 -0700, akpm@linux-foundation.org wrote:
> The patch titled
>      aacraid: fix unchecked down_interruptible()
> has been added to the -mm tree.  Its filename is
>      aacraid-fix-unchecked-down_interruptible.patch
> 
> Before you just go and hit "reply", please:
>    a) Consider who else should be cc'ed
>    b) Prefer to cc a suitable mailing list as well
>    c) Ideally: find the original patch on the mailing list and do a
>       reply-to-all to that, adding suitable additional cc's
> 
> *** Remember to use Documentation/SubmitChecklist when testing your code ***
> 
> See http://www.zip.com.au/~akpm/linux/patches/stuff/added-to-mm.txt to find
> out what to do about this
> 
> The current -mm tree may be found at http://userweb.kernel.org/~akpm/mmotm/
> 
> ------------------------------------------------------
> Subject: aacraid: fix unchecked down_interruptible()
> From: Andrew Morton <akpm@linux-foundation.org>
> 
> drivers/scsi/aacraid/commsup.c: In function 'aac_fib_send':
> drivers/scsi/aacraid/commsup.c:519: warning: ignoring return value of 'down_interruptible', declared with attribute warn_unused_result
> 
> This is a bug.  Code *must* check whether or not down_interruptible()
> succeeded.  This driver has lost track of whether or not the lock was taken.
> 
> (I thought I already fixed this once?)

You added the patch once before, it seems to have dropped out of the -mm
tree, but it was never sent upstream.

> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

I researched this with Adaptec.  This is what they say:

> We had to use down_interruptible because if the Firmware locked up,  
> unresponsive or retiscent, and the system was being shut down, it  
> would never complete the shutdown. The user task is 'in driver' and  
> can not be shut down.
> 
> The loss of this one fib because a user is interrupting the
> management  
> applications is a small price to pay. This FIB remains in the open  
> queue and needs to since the Adapter has not completed it...

So the point about this is that if you ever hit the interrupt, you lose
a FIB, but on the other hand, there's nothing the driver can actually do
to recover it, hence it wouldn't behave differently regardless of the
return value of down_interruptible().  On the other hand, if you change
the down_interruptible() to down() we lock up the system when we hit the
wait forever condition.  I'm sympathetic, even though I agree we don't
want this bad programming example persisting for someone to cut and
paste, so I think we have to keep the interruptible, but I'm open to
other ways of making the warning go away.

> ---
> 
>  drivers/scsi/aacraid/commsup.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff -puN drivers/scsi/aacraid/commsup.c~aacraid-fix-unchecked-down_interruptible drivers/scsi/aacraid/commsup.c
> --- a/drivers/scsi/aacraid/commsup.c~aacraid-fix-unchecked-down_interruptible
> +++ a/drivers/scsi/aacraid/commsup.c
> @@ -516,7 +516,7 @@ int aac_fib_send(u16 command, struct fib
>  				udelay(5);
>  			}
>  		} else
> -			(void)down_interruptible(&fibptr->event_wait);
> +			down(&fibptr->event_wait);
>  		spin_lock_irqsave(&fibptr->event_lock, flags);
>  		if (fibptr->done == 0) {
>  			fibptr->done = 2; /* Tell interrupt we aborted */

James

Re: + aacraid-fix-unchecked-down_interruptible.patch added to -mm tree

[Mark Salyzyn]

Until the epiphany that solves this problem or someone straightens me  
out on the proper usage, I suggest a STRONG comment be added: /*  
BADCODE DO NOT COPY */ :-)

The check 'fibptr->done == 0' below is used to indirectly deal with  
the lock state, for if it is still zero (I know a race condition is  
here) then the lock was not 'taken', and the setting it to a value of  
2 acknowledges this fact. The fix may very well be that if the return  
value indicates interrupt instead of release, that we set 'done' to a  
value of 2 ...

I will need to revisit this ...

Sincerely -- Mark Salyzyn

On Apr 10, 2008, at 4:54 PM, James Bottomley wrote:

>
> On Thu, 2008-04-10 at 13:26 -0700, akpm@linux-foundation.org wrote:
>> The patch titled
>>     aacraid: fix unchecked down_interruptible()
>> has been added to the -mm tree.  Its filename is
>>     aacraid-fix-unchecked-down_interruptible.patch
>>
>> Before you just go and hit "reply", please:
>>   a) Consider who else should be cc'ed
>>   b) Prefer to cc a suitable mailing list as well
>>   c) Ideally: find the original patch on the mailing list and do a
>>      reply-to-all to that, adding suitable additional cc's
>>
>> *** Remember to use Documentation/SubmitChecklist when testing your  
>> code ***
>>
>> See http://www.zip.com.au/~akpm/linux/patches/stuff/added-to-mm.txt  
>> to find
>> out what to do about this
>>
>> The current -mm tree may be found at http://userweb.kernel.org/~akpm/mmotm/
>>
>> ------------------------------------------------------
>> Subject: aacraid: fix unchecked down_interruptible()
>> From: Andrew Morton <akpm@linux-foundation.org>
>>
>> drivers/scsi/aacraid/commsup.c: In function 'aac_fib_send':
>> drivers/scsi/aacraid/commsup.c:519: warning: ignoring return value  
>> of 'down_interruptible', declared with attribute warn_unused_result
>>
>> This is a bug.  Code *must* check whether or not down_interruptible()
>> succeeded.  This driver has lost track of whether or not the lock  
>> was taken.
>>
>> (I thought I already fixed this once?)
>
> You added the patch once before, it seems to have dropped out of the  
> -mm
> tree, but it was never sent upstream.
>
>> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
>> Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
>> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
>
> I researched this with Adaptec.  This is what they say:
>
>> We had to use down_interruptible because if the Firmware locked up,
>> unresponsive or retiscent, and the system was being shut down, it
>> would never complete the shutdown. The user task is 'in driver' and
>> can not be shut down.
>>
>> The loss of this one fib because a user is interrupting the
>> management
>> applications is a small price to pay. This FIB remains in the open
>> queue and needs to since the Adapter has not completed it...
>
> So the point about this is that if you ever hit the interrupt, you  
> lose
> a FIB, but on the other hand, there's nothing the driver can  
> actually do
> to recover it, hence it wouldn't behave differently regardless of the
> return value of down_interruptible().  On the other hand, if you  
> change
> the down_interruptible() to down() we lock up the system when we hit  
> the
> wait forever condition.  I'm sympathetic, even though I agree we don't
> want this bad programming example persisting for someone to cut and
> paste, so I think we have to keep the interruptible, but I'm open to
> other ways of making the warning go away.
>
>> ---
>>
>> drivers/scsi/aacraid/commsup.c |    2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff -puN drivers/scsi/aacraid/commsup.c~aacraid-fix-unchecked- 
>> down_interruptible drivers/scsi/aacraid/commsup.c
>> --- a/drivers/scsi/aacraid/commsup.c~aacraid-fix-unchecked- 
>> down_interruptible
>> +++ a/drivers/scsi/aacraid/commsup.c
>> @@ -516,7 +516,7 @@ int aac_fib_send(u16 command, struct fib
>>                              udelay(5);
>>                      }
>>              } else
>> -                     (void)down_interruptible(&fibptr->event_wait);
>> +                     down(&fibptr->event_wait);
>>              spin_lock_irqsave(&fibptr->event_lock, flags);
>>              if (fibptr->done == 0) {
>>                      fibptr->done = 2; /* Tell interrupt we aborted  
>> */
>
> James
>
>

Re: + aacraid-fix-unchecked-down_interruptible.patch added to -mm tree

[Andrew Morton]

On Thu, 10 Apr 2008 17:19:23 -0400
Mark Salyzyn <Mark_Salyzyn@adaptec.com> wrote:

> Until the epiphany that solves this problem or someone straightens me  
> out on the proper usage, I suggest a STRONG comment be added: /*  
> BADCODE DO NOT COPY */ :-)

There must be _some_ way of handling it which vaguely reflects what the
code is actually trying to do.

I assume that what you're saying is that *fibptr is about to be freed (or
reinitialised) whether or not the down_interruptible() succeeded.

If it gets freed with the lock held, CONFIG_DEBUG_LOCK_ALLOC could (should)
generate a runtime warning.

Even something like

	/*
	 * lavish comment goes here
	 */
	if (down_interruptible(&fibptr->event_wait) == 0)
		up(&fibptr->event_wait);

would fix the compile-time and runtime warnings.

Re: + aacraid-fix-unchecked-down_interruptible.patch added to -mm tree

[Mark Salyzyn]

	if (down_interruptible(&fibptr->event_wait) == 0) {
		fibptr->done = 2;
                up(&fibptr->event_wait);
	}

would make me feel much more comfortable ... The fib can never be  
'freed'. The fib is owned by the adapter until it completes it on it's  
own (if ever, the assumption here is that the adapter is misbehaving  
on this 'one' fib). Setting the value of 2 allows us to report back to  
the ioctl caller a failed operation without actually closing out the  
fib.

I am having a problem with my bandwidth right now to do a unit test to  
see if this gets the job done.

Sincerely -- Mark Salyzyn

On Apr 10, 2008, at 5:29 PM, Andrew Morton wrote:

> On Thu, 10 Apr 2008 17:19:23 -0400
> Mark Salyzyn <Mark_Salyzyn@adaptec.com> wrote:
>
>> Until the epiphany that solves this problem or someone straightens me
>> out on the proper usage, I suggest a STRONG comment be added: /*
>> BADCODE DO NOT COPY */ :-)
>
> There must be _some_ way of handling it which vaguely reflects what  
> the
> code is actually trying to do.
>
> I assume that what you're saying is that *fibptr is about to be  
> freed (or
> reinitialised) whether or not the down_interruptible() succeeded.
>
> If it gets freed with the lock held, CONFIG_DEBUG_LOCK_ALLOC could  
> (should)
> generate a runtime warning.
>
> Even something like
>
>        /*
>         * lavish comment goes here
>         */
>        if (down_interruptible(&fibptr->event_wait) == 0)
>                up(&fibptr->event_wait);
>
> would fix the compile-time and runtime warnings.

Re: + aacraid-fix-unchecked-down_interruptible.patch added to -mm tree

[James Bottomley]

On Thu, 2008-04-10 at 17:44 -0400, Mark Salyzyn wrote:
> 	if (down_interruptible(&fibptr->event_wait) == 0) {
> 		fibptr->done = 2;
>                 up(&fibptr->event_wait);
> 	}
> 
> would make me feel much more comfortable ... The fib can never be  
> 'freed'. The fib is owned by the adapter until it completes it on it's  
> own (if ever, the assumption here is that the adapter is misbehaving  
> on this 'one' fib). Setting the value of 2 allows us to report back to  
> the ioctl caller a failed operation without actually closing out the  
> fib.
> 
> I am having a problem with my bandwidth right now to do a unit test to  
> see if this gets the job done.

That's OK ... send the patch when you're ready; I'll make sure it gets
upstream.  We'll do the standard put it into scsi-misc and backport if
it seems stable, so we'll have time to shake out any problems.

James

Re: + aacraid-fix-unchecked-down_interruptible.patch added to -mm tree

[Mark Salyzyn]

[-- Attachment #1: aacraid_down_interruptible.patch --]
[-- Type: application/octet-stream, Size: 711 bytes --]

diff -ru a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c
--- a/drivers/scsi/aacraid/commsup.c	2008-04-14 14:08:00.737201138 -0400
+++ b/drivers/scsi/aacraid/commsup.c	2008-04-14 14:09:34.019252960 -0400
@@ -515,10 +515,12 @@
 				}
 				udelay(5);
 			}
-		} else
-			(void)down_interruptible(&fibptr->event_wait);
+		} else if (down_interruptible(&fibptr->event_wait) == 0) {
+			fibptr->done = 2;
+			up(&fibptr->event_wait);
+		}
 		spin_lock_irqsave(&fibptr->event_lock, flags);
-		if (fibptr->done == 0) {
+		if ((fibptr->done == 0) || (fibptr->done == 2)) {
 			fibptr->done = 2; /* Tell interrupt we aborted */
 			spin_unlock_irqrestore(&fibptr->event_lock, flags);
 			return -EINTR;

[-- Attachment #2: Type: text/plain, Size: 2104 bytes --]



Discussion below.

This attached patch is against current scsi-misc-2.6.

ObligatoryDisclaimer: Please accept my condolences regarding Outlook's  
handling of patch attachments (inline gets damaged, please use  
attachment).

Signed-off-by: Mark Salyzyn <aacraid@adaptec.com>

  drivers/scsi/aacraid/commsup.c |    8 +++++---
  1 file changed, 5 insertions(+), 3 deletions(-)

diff -ru a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/ 
commsup.c
--- a/drivers/scsi/aacraid/commsup.c	2008-04-14 14:08:00.737201138 -0400
+++ b/drivers/scsi/aacraid/commsup.c	2008-04-14 14:09:34.019252960 -0400
@@ -515,10 +515,12 @@
  				}
  				udelay(5);
  			}
-		} else
-			(void)down_interruptible(&fibptr->event_wait);
+		} else if (down_interruptible(&fibptr->event_wait) == 0) {
+			fibptr->done = 2;
+			up(&fibptr->event_wait);
+		}
  		spin_lock_irqsave(&fibptr->event_lock, flags);
-		if (fibptr->done == 0) {
+		if ((fibptr->done == 0) || (fibptr->done == 2)) {
  			fibptr->done = 2; /* Tell interrupt we aborted */
  			spin_unlock_irqrestore(&fibptr->event_lock, flags);
  			return -EINTR;

Sincerely - Mark Salyzyn

On Apr 10, 2008, at 5:57 PM, James Bottomley wrote:

> On Thu, 2008-04-10 at 17:44 -0400, Mark Salyzyn wrote:
>>      if (down_interruptible(&fibptr->event_wait) == 0) {
>>              fibptr->done = 2;
>>                up(&fibptr->event_wait);
>>      }
>>
>> would make me feel much more comfortable ... The fib can never be
>> 'freed'. The fib is owned by the adapter until it completes it on  
>> it's
>> own (if ever, the assumption here is that the adapter is misbehaving
>> on this 'one' fib). Setting the value of 2 allows us to report back  
>> to
>> the ioctl caller a failed operation without actually closing out the
>> fib.
>>
>> I am having a problem with my bandwidth right now to do a unit test  
>> to
>> see if this gets the job done.
>
> That's OK ... send the patch when you're ready; I'll make sure it gets
> upstream.  We'll do the standard put it into scsi-misc and backport if
> it seems stable, so we'll have time to shake out any problems.
>
> James