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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <1201624455.3069.10.camel@localhost.localdomain>
Date:	Tue, 29 Jan 2008 11:34:15 -0500
From:	James Bottomley <James.Bottomley@...senPartnership.com>
To:	Alan Stern <stern@...land.harvard.edu>
Cc:	Boaz Harrosh <bharrosh@...asas.com>, 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, 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);


--
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/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ