linux-kernel - Re: [BUG] 2.6.24-git usb reset problems

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <479F774B.60303@panasas.com>
Date:	Tue, 29 Jan 2008 20:58:19 +0200
From:	Boaz Harrosh <bharrosh@...asas.com>
To:	James Bottomley <James.Bottomley@...senPartnership.com>
CC:	Alan Stern <stern@...land.harvard.edu>, Greg KH <greg@...ah.com>,
	Jens Axboe <jens.axboe@...cle.com>,
	Matthew Dharm <mdharm-usb@...-eyed-alien.net>,
	linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
	linux-scsi@...r.kernel.org
Subject: Re: [BUG] 2.6.24-git usb reset problems

On Tue, Jan 29 2008 at 20:48 +0200, James Bottomley <James.Bottomley@...senPartnership.com> wrote:
> On Tue, 2008-01-29 at 20:27 +0200, Boaz Harrosh wrote:
>> On Tue, Jan 29 2008 at 18:34 +0200, James Bottomley <James.Bottomley@...senPartnership.com> wrote:
>>> On Tue, 2008-01-29 at 10:36 -0500, Alan Stern wrote:
>>>> On Tue, 29 Jan 2008, Boaz Harrosh wrote:
>>>>
>>>>> --- a/drivers/usb/storage/transport.c
>>>>> +++ b/drivers/usb/storage/transport.c
>>>>> @@ -462,18 +462,24 @@ static int usb_stor_bulk_transfer_sglist(struct us_data *us, unsigned int pipe,
>>>>>   * Common used function. Transfer a complete command
>>>>>   * via usb_stor_bulk_transfer_sglist() above. Set cmnd resid
>>>>>   */
>>>>> -int usb_stor_bulk_srb(struct us_data* us, unsigned int pipe,
>>>>> -		      struct scsi_cmnd* srb)
>>>>> +int usb_stor_bulk_srb_length(struct us_data* us, unsigned int pipe,
>>>>> +		      struct scsi_cmnd* srb, unsigned length)
>>>>>  {
>>>>>  	unsigned int partial;
>>>>>  	int result = usb_stor_bulk_transfer_sglist(us, pipe, scsi_sglist(srb),
>>>>> -				      scsi_sg_count(srb), scsi_bufflen(srb),
>>>>> +				      scsi_sg_count(srb), length,
>>>>>  				      &partial);
>>>>>  
>>>>>  	scsi_set_resid(srb, scsi_bufflen(srb) - partial);
>>>>>  	return result;
>>>>>  }
>>>>>  
>>>>> +int usb_stor_bulk_srb(struct us_data* us, unsigned int pipe,
>>>>> +		struct scsi_cmnd* srb)
>>>>> +{
>>>>> +	return usb_stor_bulk_srb_length(us, pipe, srb, scsi_bufflen(srb));
>>>>> +}
>>>>> +
>>>> I don't like this patch very much.  Why add another layer of 
>>>> indirection when the two subroutines do hardly any work?  Leave 
>>>> usb_stor_bulk_srb() the way it was, and add usb_stor_bulk_srb_length() 
>>>> as a separate routine that simply calls usb_stor_bulk_transfer_sglist() 
>>>> and scsi_set_resid().
>>>>
>>>> BTW, the standard coding style calls for a blank line after the list of 
>>>> local variables at the start of a function or block.
>>> There's another bug in the transport.c conversion in that the residuals
>>> are updated with bogus data in several error cases, since
>>> usb_stor_bulk_transfer_sglist() only sets the actual length if the urb
>>> is actually sent.
>>>
>>> I'm not sure if this is is the solution to the problem at hand, but it
>>> definitely fixes another bug in the code.
>>>
>>> James
>>>
>>> diff --git a/drivers/usb/storage/transport.c b/drivers/usb/storage/transport.c
>>> index d9f4912..bab0858 100644
>>> --- a/drivers/usb/storage/transport.c
>>> +++ b/drivers/usb/storage/transport.c
>>> @@ -465,7 +465,7 @@ static int usb_stor_bulk_transfer_sglist(struct us_data *us, unsigned int pipe,
>>>  int usb_stor_bulk_srb(struct us_data* us, unsigned int pipe,
>>>  		      struct scsi_cmnd* srb)
>>>  {
>>> -	unsigned int partial;
>>> +	unsigned int partial = scsi_get_resid(srb);
>>>  	int result = usb_stor_bulk_transfer_sglist(us, pipe, scsi_sglist(srb),
>>>  				      scsi_sg_count(srb), scsi_bufflen(srb),
>>>  				      &partial);
>>>
>>>
>>> -
>> But then this is weird because it is not what usb_stor_bulk_transfer_sg() is doing
>> which was the one called before.
> 
> Um, yes it was.  The original code did this
> 
> sb_stor_bulk_transfer_sg(..., &srp->resid, ...)
> 
> Which was at liberty not to touch resid, which it chose not to do in the
> error legs.
> 
> Your new code does
> 
> int partial; <- stack uninitialised
> sb_stor_bulk_transfer_sglist(..., &partial, ...);
> scsi_set_resid(srb, scsi_bufflen(srb) - partial);
> 
> If the function doesn't touch partial, as it doesn't in the error legs,
> resid now gets set with rubbish.
> 
> Actually, my code is still wrong .. we have to set it to
> scsi_bufflen(srb) - scsi_resid(srb) so that it comes back the same if
> left untouched.
> 
>> I have such a device and I get one reset but then every thing works nice.
>> This is with debug on. I'll try to make it fail.
> 
> James
> 
> 
Sorry I still don't see it.

original code did sb_stor_bulk_transfer_sg(..., &srp->resid, ...)

but sb_stor_bulk_transfer_sg does the:
 int partial; <- stack uninitialised
 sb_stor_bulk_transfer_sglist(..., &partial, ...);

and then unconditionally sets *residual = length_left;
I do not see an "error legs" case in sb_stor_bulk_transfer_sg().

I have just cut and pasted the !use_sg from sb_stor_bulk_transfer_sg names
and all

Boaz
  
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/