Re: Bugs in scsi_vpd_inquiry()

[James Bottomley <James.Bottomley@HansenPartnership.com>]

On Tue, 2009-08-11 at 18:18 +0300, Boaz Harrosh wrote:
> On 08/11/2009 06:13 PM, James Bottomley wrote:
> > On Tue, 2009-08-11 at 10:53 -0400, Alan Stern wrote:
> >> On Tue, 11 Aug 2009, Boaz Harrosh wrote:
> >>
> >>> This is certainly a bug. Otherwise I would get all my pages 4 bytes short
> >>> and wonder why.
> >>>
> >>> I wish the bug would explain that stupid USB device Martin was fixing.
> >>> "I die if evpd page=0 is read" is a very brain dead thing. But there
> >>> is no overflow in current code, only underflow.
> >>>
> >>> If you are at it could you please fix all the bugs in this code: ;-)
> >>
> >> The USB problem shouldn't affect anything thanks to Martin's other
> >> changes (sd won't read VPD for devices with scsi_level <= SCSI_2).  So
> >> how does this revised patch look?
> >>
> >> Alan Stern
> >>
> >>
> >> Index: usb-2.6/drivers/scsi/scsi.c
> >> ===================================================================
> >> --- usb-2.6.orig/drivers/scsi/scsi.c
> >> +++ usb-2.6/drivers/scsi/scsi.c
> >> @@ -969,7 +969,7 @@ EXPORT_SYMBOL(scsi_track_queue_full);
> >>   * @sdev: The device to ask
> >>   * @buffer: Where to put the result
> >>   * @page: Which Vital Product Data to return
> >> - * @len: The length of the buffer
> >> + * @len: The length of the data (= buffer length - 4)
> > 
> > Really, no.  The former is the correct (and universally used
> > definition).  This one you propose is asking for confusion and misuse.
> > 
> 
> This is what is done today Only documented
> 
> If you want to fix it to be the full buffer size the user code
> should be fixed

Right.

> >>   *
> >>   * This is an internal helper function.  You probably want to use
> >>   * scsi_get_vpd_page instead.
> >> @@ -980,7 +980,10 @@ static int scsi_vpd_inquiry(struct scsi_
> >>  							u8 page, unsigned len)
> >>  {
> >>  	int result;
> >> -	unsigned char cmd[16];
> >> +	int resid;
> >> +	unsigned char cmd[6];
> >> +
> >> +	len += 4;		/* Include room for the header bytes */
> >>  
> >>  	cmd[0] = INQUIRY;
> >>  	cmd[1] = 1;		/* EVPD */
> >> @@ -989,17 +992,19 @@ static int scsi_vpd_inquiry(struct scsi_
> >>  	cmd[4] = len & 0xff;
> >>  	cmd[5] = 0;		/* Control byte */
> >>  
> >> +	buffer[1] = ~page;
> > 
> > This is pointless and dangerous:  Some architectures will invalidate
> > caches for DMA not flush them, so it might not do what you think it
> > does.
> > 
> 
> Then you can't do that "after check" either. It's a simple minefield.
> What do you suggest?

Well, nothing really, like the original code.  Byzantine checking is
never a good idea.  You always assume that if a device told you it did
what you told it, it actually did.  If we find devices that fail this
simple premise, then we get into blacklists and other forms of
nastiness.

James