Re: Bugs in scsi_vpd_inquiry()

[Matthew Wilcox <matthew@wil.cx>]

On Mon, Aug 10, 2009 at 10:41:42AM -0400, Alan Stern wrote:
> Martin and Matthew:
> 
> Since you guys added scsi_vpd_inquiry() and scsi_get_vpd_page() plus
> sd_read_block_limits() and sd_read_block_characteristics(), I'm
> directing these questions to you.
> 
> Is there some reason for not accounting for the 4 header bytes in the 
> allocation length value stored in the CDB?  Or is this simply a bug?

Um, we do.

unsigned char *scsi_get_vpd_page(struct scsi_device *sdev, u8 page)
        unsigned char *buf = kmalloc(259, GFP_KERNEL);
        result = scsi_vpd_inquiry(sdev, buf, 0, 255);
        for (i = 0; i < buf[3]; i++)
                if (buf[i + 4] == page)
                        goto found;
        buf = kmalloc(len + 4, GFP_KERNEL);
        result = scsi_vpd_inquiry(sdev, buf, page, len);

Now ... we do seem to be passing the len instead of len + 4 to the
device as the buffer size, so there does appear to be a minor bug here,
but it's not as horrific as you make it out to be.

> Were you aware that SCSI-2 defines the allocation length to be a single 
> byte?  cmd[3] is specified as "Reserved" in the spec.  Hence the value 
> of "len" should be capped at 255 if sdev->scsi_level <= SCSI_2, right?

and 'Reserved' in SCSI-2 means:

"A reserved bit, field, or byte shall be set to zero, or in accordance
with a future extension to this standard." (7.1.1)

> Why does scsi_get_vpd_page() retrieve page 0 first, rather than 
> directly asking for the page in question?  Is this some sort of 
> play-it-safe approach, to avoid sending devices commands they may not 
> support?

I think we had an example of a device which crashed when asked for pages
that it didn't support.

> Have you considered that plenty of low-budget USB mass-storage devices
> don't implement VPD properly?  I've got a flash drive right here which

I've noticed you whining about it, yes.

> totally ignores the "page" byte in the INQUIRY command; it always
> responds with the normal INQUIRY data.  Thus expecting the response

I don't think you mean the 'page' byte.  I think you mean the 'EVPD'
bit.

> data always to be accurate may not be a good idea.  I'm considering
> adding a "restrict_to_MS_usb" flag to the host template, to indicate
> that commands shouldn't be sent unless some version of Windows uses
> them when talking to USB devices -- do you think that could work?

Not really my area of expertise.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."