Re: [PATCH] zfcp: Test kmalloc failure in scsi_get_vpd_page()

[James Bottomley <James.Bottomley@suse.de>]

On Wed, 2009-11-04 at 18:18 +0200, Boaz Harrosh wrote:
> On 11/04/2009 05:09 PM, James Bottomley wrote:
> > On Wed, 2009-11-04 at 10:54 +0200, Boaz Harrosh wrote:
> >>> +
> >>> +	if (i < buf[3] && i > buf_len)
> >>> +		/* ran off the end of the buffer, give us benefit of doubt */
> >>> +		goto found;
> >>
> >> Some cheep devices are known to break when asked for pages they do not support
> >> better return an -ETOOSMALL the user can check for. (And the comment above should
> >> also take care of it)
> > 
> > OK, so I struggled with this.  The reason for the behaviour is  that
> > only USB devices with incredibly small page lists are likely to exhibit
> > the problem.  Whereas the page lists in array type devices are likely to
> > grow.  I don't want to run into the situation that your new $10m array
> > suddenly gives an error with DIF/DIX because the page list is too big
> > and the problem this is checking for only afflicts cheap and badly
> > implemented HW.
> > 
> 
> If I remember the standard correctly and from inspecting the code 260 is the
> maximum size for page 0. (And the maximum we supported until today for page
> 0 was 256) so there is no speculations here. 256 it should be. If you want
> you can do an:

The standard actually lists only two mandatory pages and about eight
optional ones (plus the 127 vendor specific ones that no-one seems to
use).

The point isn't to exercise the standard to the maximum, it's to
generate working code.  The point is that problem devices likely only
list a couple of VPD pages, so we'll correctly not probe them.
Conforming devices don't matter because they'll do the right thing when
asked for a VPD page they don't have.

> 	if (i < buf[3] && i > buf_len) {
> 		if (likely(buf_len >= 255))
> 			/* ran off the end of the buffer, give us benefit of doubt */
> 			goto found;
> 		else
> 			return -ETOOSMALL;
> 	}
> 
> which is only theoretical because we only check against buf[3] that cannot
> be more then 255 so all is left is return -ETOOSMALL;
> 
> Callers that allocate smaller then 256 for this (hence the comment)
> must check for -ETOOSMALL;. Or should call with >= 256.
> 
> <snip>
> >>> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> >>> index 9093c72..fd1bd8f 100644
> >>> --- a/drivers/scsi/sd.c
> >>> +++ b/drivers/scsi/sd.c
> >>> @@ -1864,19 +1864,20 @@ void sd_read_app_tag_own(struct scsi_disk *sdkp, unsigned char *buffer)
> >>>  static void sd_read_block_limits(struct scsi_disk *sdkp)
> >>>  {
> >>>  	unsigned int sector_sz = sdkp->device->sector_size;
> >>> -	char *buffer;
> >>> +	const int vpd_len = 32;
> >>> +	unsigned char *buffer = kmalloc(vpd_len, GFP_KERNEL);
> >>>  
> >>
> >> 32 sounds two small for me. Not because of the page but because of the
> >> first pass. Why not just 255 as a rule. We kmalloc it anyway.
> > 
> > It only needs 16 if you look at the code.  32 is actually the smallest
> > kmalloc slab on 32 bit systems.
> > 
> 
> >> We used to allocate 255 here, for some devices out there this is surly
> >> a regression.
> > 
> > We did 255 because we were concerned about space for the page list ...
> > hopefully I answered that one above.
> > 
> 
> No, I still disagree. Whats the point of checking at all? Surly there is a device
> out there that has more then 12 pages but will breaks with wrong page. So what's
> the limit? the standards say 260 why not stick with that. It's not at any hot path.

The point of checking is not to send a VPD inquiry to USB devices that
don't support it.  These have a very limited range of supported VPD
pages.

James