All of lore.kernel.org
 help / color / mirror / Atom feed
* usb: storage: suspicious code
@ 2017-02-15  5:06 Gustavo A. R. Silva
  2017-02-15  7:01 ` [usb-storage] " Oliver Neukum
  0 siblings, 1 reply; 6+ messages in thread
From: Gustavo A. R. Silva @ 2017-02-15  5:06 UTC (permalink / raw)
  To: stern, gregkh
  Cc: linux-usb, usb-storage, linux-kernel, Peter Senna Tschudi,
	Gustavo A. R. Silva

Hello,

I ran into the following piece of code at drivers/usb/storage/jumpshot.c:305 (linux-next), and it seems a little bit suspicious:

// read the result.  apparently the bulk write can complete
// before the jumpshot drive is finished writing.  so we loop
// here until we get a good return code
waitcount = 0;
do {
	result = jumpshot_get_status(us);
	if (result != USB_STOR_TRANSPORT_GOOD) {
        	// I have not experimented to find the smallest value.
		//
		msleep(50);
	}
	} while ((result != USB_STOR_TRANSPORT_GOOD) && (waitcount < 10));

	if (result != USB_STOR_TRANSPORT_GOOD)
        	usb_stor_dbg(us, "Gah!  Waitcount = 10.  Bad write!?\n");

Variable 'waitcount' is never updated inside the do-while loop. So, either it isn't needed at all or line 316 should be modified (++waitcount < 10)

In case 'waitcount' isn't needed, lines 318 and 319 should be removed.

Can someone help me to clarify this so I can write a patch to fix this code?

Thank you
--
Gustavo A. R. Silva

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [usb-storage] usb: storage: suspicious code
  2017-02-15  5:06 usb: storage: suspicious code Gustavo A. R. Silva
@ 2017-02-15  7:01 ` Oliver Neukum
  2017-02-15  7:14   ` Gustavo A. R. Silva
  0 siblings, 1 reply; 6+ messages in thread
From: Oliver Neukum @ 2017-02-15  7:01 UTC (permalink / raw)
  To: Gustavo A. R. Silva, stern, gregkh
  Cc: linux-usb, usb-storage, linux-kernel, Peter Senna Tschudi

Am Dienstag, den 14.02.2017, 23:06 -0600 schrieb Gustavo A. R. Silva:

Hi,

> waitcount = 0;
> do {
>         result = jumpshot_get_status(us);
>         if (result != USB_STOR_TRANSPORT_GOOD) {
>                 // I have not experimented to find the smallest
> value.
>                 //
>                 msleep(50);
>         }
>         } while ((result != USB_STOR_TRANSPORT_GOOD) && (waitcount <
> 10));
> 
>         if (result != USB_STOR_TRANSPORT_GOOD)
>                 usb_stor_dbg(us, "Gah!  Waitcount = 10.  Bad
> write!?\n");
> 
> Variable 'waitcount' is never updated inside the do-while loop. So,
> either it isn't needed at all or line 316 should be modified
> (++waitcount < 10)

you are correct. Waitcount needs to be incremented.

	HTH
		Oliver

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [usb-storage] usb: storage: suspicious code
  2017-02-15  7:01 ` [usb-storage] " Oliver Neukum
@ 2017-02-15  7:14   ` Gustavo A. R. Silva
  2017-02-15  7:39     ` [PATCH] usb: storage: add missing pre-increment to variable Gustavo A. R. Silva
  0 siblings, 1 reply; 6+ messages in thread
From: Gustavo A. R. Silva @ 2017-02-15  7:14 UTC (permalink / raw)
  To: Oliver Neukum
  Cc: stern, gregkh, linux-usb, usb-storage, linux-kernel, Peter Senna Tschudi

Hi Oliver,

Quoting Oliver Neukum <oneukum@suse.com>:

> Am Dienstag, den 14.02.2017, 23:06 -0600 schrieb Gustavo A. R. Silva:
>
> Hi,
>
>> waitcount = 0;
>> do {
>>         result = jumpshot_get_status(us);
>>         if (result != USB_STOR_TRANSPORT_GOOD) {
>>                 // I have not experimented to find the smallest
>> value.
>>                 //
>>                 msleep(50);
>>         }
>>         } while ((result != USB_STOR_TRANSPORT_GOOD) && (waitcount <
>> 10));
>>
>>         if (result != USB_STOR_TRANSPORT_GOOD)
>>                 usb_stor_dbg(us, "Gah!  Waitcount = 10.  Bad
>> write!?\n");
>>
>> Variable 'waitcount' is never updated inside the do-while loop. So,
>> either it isn't needed at all or line 316 should be modified
>> (++waitcount < 10)
>
> you are correct. Waitcount needs to be incremented.
>

Thanks for clarifying, I'll send a patch shortly.

--
Gustavo A. R. Silva

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH] usb: storage: add missing pre-increment to variable
  2017-02-15  7:14   ` Gustavo A. R. Silva
@ 2017-02-15  7:39     ` Gustavo A. R. Silva
  2017-02-15 15:26       ` [usb-storage] " Alan Stern
  0 siblings, 1 reply; 6+ messages in thread
From: Gustavo A. R. Silva @ 2017-02-15  7:39 UTC (permalink / raw)
  To: oneukum, stern, gregkh
  Cc: linux-usb, usb-storage, linux-kernel, Peter Senna Tschudin,
	Gustavo A. R. Silva

Add missing pre-increment to 'waitcount' variable used in do-while loop.

Addresses-Coverity-ID: 1011631
Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
---
 drivers/usb/storage/jumpshot.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/storage/jumpshot.c b/drivers/usb/storage/jumpshot.c
index 011e527..a26c4bb 100644
--- a/drivers/usb/storage/jumpshot.c
+++ b/drivers/usb/storage/jumpshot.c
@@ -313,7 +313,7 @@ static int jumpshot_write_data(struct us_data *us,
 				//
 				msleep(50); 
 			}
-		} while ((result != USB_STOR_TRANSPORT_GOOD) && (waitcount < 10));
+		} while ((result != USB_STOR_TRANSPORT_GOOD) && (++waitcount < 10));
 
 		if (result != USB_STOR_TRANSPORT_GOOD)
 			usb_stor_dbg(us, "Gah!  Waitcount = 10.  Bad write!?\n");
-- 
2.5.0

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [usb-storage] [PATCH] usb: storage: add missing pre-increment to variable
  2017-02-15  7:39     ` [PATCH] usb: storage: add missing pre-increment to variable Gustavo A. R. Silva
@ 2017-02-15 15:26       ` Alan Stern
  2017-02-20 23:07         ` Gustavo A. R. Silva
  0 siblings, 1 reply; 6+ messages in thread
From: Alan Stern @ 2017-02-15 15:26 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: oneukum, gregkh, linux-usb, usb-storage, linux-kernel,
	Peter Senna Tschudin

On Wed, 15 Feb 2017, Gustavo A. R. Silva wrote:

> Add missing pre-increment to 'waitcount' variable used in do-while loop.
> 
> Addresses-Coverity-ID: 1011631
> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
> ---
>  drivers/usb/storage/jumpshot.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/storage/jumpshot.c b/drivers/usb/storage/jumpshot.c
> index 011e527..a26c4bb 100644
> --- a/drivers/usb/storage/jumpshot.c
> +++ b/drivers/usb/storage/jumpshot.c
> @@ -313,7 +313,7 @@ static int jumpshot_write_data(struct us_data *us,
>  				//
>  				msleep(50); 
>  			}
> -		} while ((result != USB_STOR_TRANSPORT_GOOD) && (waitcount < 10));
> +		} while ((result != USB_STOR_TRANSPORT_GOOD) && (++waitcount < 10));
>  
>  		if (result != USB_STOR_TRANSPORT_GOOD)
>  			usb_stor_dbg(us, "Gah!  Waitcount = 10.  Bad write!?\n");
> 

This has already been reported and fixed.  See

	http://marc.info/?l=linux-usb&m=148604164024557&w=2

Alan Stern

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [usb-storage] [PATCH] usb: storage: add missing pre-increment to variable
  2017-02-15 15:26       ` [usb-storage] " Alan Stern
@ 2017-02-20 23:07         ` Gustavo A. R. Silva
  0 siblings, 0 replies; 6+ messages in thread
From: Gustavo A. R. Silva @ 2017-02-20 23:07 UTC (permalink / raw)
  To: Alan Stern
  Cc: oneukum, gregkh, linux-usb, usb-storage, linux-kernel,
	Peter Senna Tschudin

Hi Alan,

Quoting Alan Stern <stern@rowland.harvard.edu>:

> On Wed, 15 Feb 2017, Gustavo A. R. Silva wrote:
>
>> Add missing pre-increment to 'waitcount' variable used in do-while loop.
>>
>> Addresses-Coverity-ID: 1011631
>> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
>> ---
>>  drivers/usb/storage/jumpshot.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/storage/jumpshot.c b/drivers/usb/storage/jumpshot.c
>> index 011e527..a26c4bb 100644
>> --- a/drivers/usb/storage/jumpshot.c
>> +++ b/drivers/usb/storage/jumpshot.c
>> @@ -313,7 +313,7 @@ static int jumpshot_write_data(struct us_data *us,
>>  				//
>>  				msleep(50);
>>  			}
>> -		} while ((result != USB_STOR_TRANSPORT_GOOD) && (waitcount < 10));
>> +		} while ((result != USB_STOR_TRANSPORT_GOOD) && (++waitcount < 10));
>>
>>  		if (result != USB_STOR_TRANSPORT_GOOD)
>>  			usb_stor_dbg(us, "Gah!  Waitcount = 10.  Bad write!?\n");
>>
>
> This has already been reported and fixed.  See
>
> 	http://marc.info/?l=linux-usb&m=148604164024557&w=2
>

Awesome. Thanks for the info.
--
Gustavo A. R. Silva

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-02-20 23:33 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-15  5:06 usb: storage: suspicious code Gustavo A. R. Silva
2017-02-15  7:01 ` [usb-storage] " Oliver Neukum
2017-02-15  7:14   ` Gustavo A. R. Silva
2017-02-15  7:39     ` [PATCH] usb: storage: add missing pre-increment to variable Gustavo A. R. Silva
2017-02-15 15:26       ` [usb-storage] " Alan Stern
2017-02-20 23:07         ` Gustavo A. R. Silva

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.