Re: Bugs in scsi_vpd_inquiry()

Matthew Wilcox
2009-08-11T16:04:34Z

On Mon, Aug 10, 2009 at 11:32:00AM -0400, Alan Stern wrote:
> > > 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);
>
> I'm not referring to this routine; I'm talking about the code in
> scsi_vpd_inquiry() where the CDB is set.

You could have been a little clearer in your bug report.

> > > 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)
>
> Sure. But what reason could there possibly be for making a field
> non-zero when you know that the device won't be able to interpret the
> value correctly? The fact that sdev->scsi_level == SCSI_2 means
> the device follows _this_ version of the standard, not a future
> extension. Or am I missing something?

Because with my misunderstanding of the allocation length to be the page
length, we wouldn't ever request more than 255 bytes until the device
said it supported a page which is more than 255 bytes long. Since such
a device must be outside the SCSI-2 spec, it was assumed to understand
the meaning of the 'reserved' byte. So that wasn't a bug.

Now that we need to pass 259 to get 255 bytes, that would be a bug,
so we need to take care of it.

> (And there's no reason to be rude. I'm trying to hold a civil
> discussion.)

It didn't feel like it.

--
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."