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

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

[Roel Kluin]

kmalloc() may fail, so test whether it succeeded.

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 2de5f3a..34fdde0 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -1056,6 +1056,9 @@ unsigned char *scsi_get_vpd_page(struct scsi_device *sdev, u8 page)
 
 	kfree(buf);
 	buf = kmalloc(len + 4, GFP_KERNEL);
+	if (!buf)
+		return NULL;
+
 	result = scsi_vpd_inquiry(sdev, buf, page, len);
 	if (result)
 		goto fail;

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

[James Bottomley]

On Mon, 2009-08-24 at 18:21 +0200, Roel Kluin wrote:
> kmalloc() may fail, so test whether it succeeded.
> 
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> ---
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index 2de5f3a..34fdde0 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -1056,6 +1056,9 @@ unsigned char *scsi_get_vpd_page(struct scsi_device *sdev, u8 page)
>  
>  	kfree(buf);
>  	buf = kmalloc(len + 4, GFP_KERNEL);
> +	if (!buf)
> +		return NULL;
> +

Firstly, this won't actually apply ... you should be developing against
either the SCSI trees or linux-next to get the latest versions in git.

Secondly it's not really right for the most common use cases, which
don't usually want the whole vpd buffer anyway.  I don't really see a
simple way of fixing it without altering the interface, though.

James

---

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

[Boaz Harrosh]

On 08/28/2009 02:45 AM, James Bottomley wrote:
> On Mon, 2009-08-24 at 18:21 +0200, Roel Kluin wrote:
>> kmalloc() may fail, so test whether it succeeded.
>>
>> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
>> ---
>> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
>> index 2de5f3a..34fdde0 100644
>> --- a/drivers/scsi/scsi.c
>> +++ b/drivers/scsi/scsi.c
>> @@ -1056,6 +1056,9 @@ unsigned char *scsi_get_vpd_page(struct scsi_device *sdev, u8 page)
>>  
>>  	kfree(buf);
>>  	buf = kmalloc(len + 4, GFP_KERNEL);
>> +	if (!buf)
>> +		return NULL;
>> +
> 
> Firstly, this won't actually apply ... you should be developing against
> either the SCSI trees or linux-next to get the latest versions in git.
> 
> Secondly it's not really right for the most common use cases, which
> don't usually want the whole vpd buffer anyway.  I don't really see a
> simple way of fixing it without altering the interface, though.
> 

"most common use cases" do not have pages bigger then 255. Can you think
of any? For these few places that anticipate pages bigger then 255 I don't
see much choice, we just freed 255 bytes and tried a new size, say 317, which
failed with GFP_KERNEL, In such a busy system, better fail with NULL then BUG,
No? In any way caller must check for NULL because of the first allocation.

> James
> 

Boaz

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

[James Bottomley]

On Sun, 2009-08-30 at 14:45 +0300, Boaz Harrosh wrote:
> On 08/28/2009 02:45 AM, James Bottomley wrote:
> > On Mon, 2009-08-24 at 18:21 +0200, Roel Kluin wrote:
> >> kmalloc() may fail, so test whether it succeeded.
> >>
> >> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> >> ---
> >> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> >> index 2de5f3a..34fdde0 100644
> >> --- a/drivers/scsi/scsi.c
> >> +++ b/drivers/scsi/scsi.c
> >> @@ -1056,6 +1056,9 @@ unsigned char *scsi_get_vpd_page(struct scsi_device *sdev, u8 page)
> >>  
> >>  	kfree(buf);
> >>  	buf = kmalloc(len + 4, GFP_KERNEL);
> >> +	if (!buf)
> >> +		return NULL;
> >> +
> > 
> > Firstly, this won't actually apply ... you should be developing against
> > either the SCSI trees or linux-next to get the latest versions in git.
> > 
> > Secondly it's not really right for the most common use cases, which
> > don't usually want the whole vpd buffer anyway.  I don't really see a
> > simple way of fixing it without altering the interface, though.
> > 
> 
> "most common use cases" do not have pages bigger then 255. Can you think
> of any? For these few places that anticipate pages bigger then 255 I don't
> see much choice, we just freed 255 bytes and tried a new size, say 317, which
> failed with GFP_KERNEL, In such a busy system, better fail with NULL then BUG,
> No? In any way caller must check for NULL because of the first allocation.

Other way around: common use cases means the callers.  What I'm saying
is that regardless of the size of the VPD page, the caller usually only
wants a few bytes into it.  Once we have all the information the caller
ever needs, there's no point in trying again with a larger buffer just
because the VPD page is larger ... to code this into the interface,
though, the caller would need to specify the max length it is looking
for.

James

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

[James Bottomley]

On Sun, 2009-08-30 at 09:35 -0500, James Bottomley wrote:
> On Sun, 2009-08-30 at 14:45 +0300, Boaz Harrosh wrote:
> > On 08/28/2009 02:45 AM, James Bottomley wrote:
> > > On Mon, 2009-08-24 at 18:21 +0200, Roel Kluin wrote:
> > >> kmalloc() may fail, so test whether it succeeded.
> > >>
> > >> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> > >> ---
> > >> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> > >> index 2de5f3a..34fdde0 100644
> > >> --- a/drivers/scsi/scsi.c
> > >> +++ b/drivers/scsi/scsi.c
> > >> @@ -1056,6 +1056,9 @@ unsigned char *scsi_get_vpd_page(struct scsi_device *sdev, u8 page)
> > >>  
> > >>  	kfree(buf);
> > >>  	buf = kmalloc(len + 4, GFP_KERNEL);
> > >> +	if (!buf)
> > >> +		return NULL;
> > >> +
> > > 
> > > Firstly, this won't actually apply ... you should be developing against
> > > either the SCSI trees or linux-next to get the latest versions in git.
> > > 
> > > Secondly it's not really right for the most common use cases, which
> > > don't usually want the whole vpd buffer anyway.  I don't really see a
> > > simple way of fixing it without altering the interface, though.
> > > 
> > 
> > "most common use cases" do not have pages bigger then 255. Can you think
> > of any? For these few places that anticipate pages bigger then 255 I don't
> > see much choice, we just freed 255 bytes and tried a new size, say 317, which
> > failed with GFP_KERNEL, In such a busy system, better fail with NULL then BUG,
> > No? In any way caller must check for NULL because of the first allocation.
> 
> Other way around: common use cases means the callers.  What I'm saying
> is that regardless of the size of the VPD page, the caller usually only
> wants a few bytes into it.  Once we have all the information the caller
> ever needs, there's no point in trying again with a larger buffer just
> because the VPD page is larger ... to code this into the interface,
> though, the caller would need to specify the max length it is looking
> for.

OK, it's been a couple of months and  no patch has been forthcoming on
this, so how about the attached?  It does the query and only adjusts the
size where the caller actually wants to bother.

James

---

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index a60da55..513661f 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -1026,55 +1026,39 @@ static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,
  * responsible for calling kfree() on this pointer when it is no longer
  * needed.  If we cannot retrieve the VPD page this routine returns %NULL.
  */
-unsigned char *scsi_get_vpd_page(struct scsi_device *sdev, u8 page)
+int scsi_get_vpd_page(struct scsi_device *sdev, u8 page, unsigned char *buf,
+		      int buf_len)
 {
 	int i, result;
-	unsigned int len;
-	const unsigned int init_vpd_len = 255;
-	unsigned char *buf = kmalloc(init_vpd_len, GFP_KERNEL);
-
-	if (!buf)
-		return NULL;
 
 	/* Ask for all the pages supported by this device */
-	result = scsi_vpd_inquiry(sdev, buf, 0, init_vpd_len);
+	result = scsi_vpd_inquiry(sdev, buf, 0, buf_len);
 	if (result)
 		goto fail;
 
 	/* If the user actually wanted this page, we can skip the rest */
 	if (page == 0)
-		return buf;
+		return -EINVAL;
 
-	for (i = 0; i < buf[3]; i++)
+	for (i = 0; i < min((int)buf[3], buf_len - 4); i++)
 		if (buf[i + 4] == page)
 			goto found;
+
+	if (i < buf[3] && i > buf_len)
+		/* ran off the end of the buffer, give us benefit of doubt */
+		goto found;
 	/* The device claims it doesn't support the requested page */
 	goto fail;
 
  found:
-	result = scsi_vpd_inquiry(sdev, buf, page, 255);
+	result = scsi_vpd_inquiry(sdev, buf, page, buf_len);
 	if (result)
 		goto fail;
 
-	/*
-	 * Some pages are longer than 255 bytes.  The actual length of
-	 * the page is returned in the header.
-	 */
-	len = ((buf[2] << 8) | buf[3]) + 4;
-	if (len <= init_vpd_len)
-		return buf;
-
-	kfree(buf);
-	buf = kmalloc(len, GFP_KERNEL);
-	result = scsi_vpd_inquiry(sdev, buf, page, len);
-	if (result)
-		goto fail;
-
-	return buf;
+	return 0;
 
  fail:
-	kfree(buf);
-	return NULL;
+	return -EINVAL;
 }
 EXPORT_SYMBOL_GPL(scsi_get_vpd_page);
 
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);
 
-	/* Block Limits VPD */
-	buffer = scsi_get_vpd_page(sdkp->device, 0xb0);
-
-	if (buffer == NULL)
-		return;
+	if (!buffer ||
+	    /* Block Limits VPD */
+	    scsi_get_vpd_page(sdkp->device, 0xb0, buffer, vpd_len))
+		goto out;
 
 	blk_queue_io_min(sdkp->disk->queue,
 			 get_unaligned_be16(&buffer[6]) * sector_sz);
 	blk_queue_io_opt(sdkp->disk->queue,
 			 get_unaligned_be32(&buffer[12]) * sector_sz);
 
+ out:
 	kfree(buffer);
 }
 
@@ -1886,20 +1887,23 @@ static void sd_read_block_limits(struct scsi_disk *sdkp)
  */
 static void sd_read_block_characteristics(struct scsi_disk *sdkp)
 {
-	char *buffer;
+	unsigned char *buffer;
 	u16 rot;
+	const int vpd_len = 32;
 
-	/* Block Device Characteristics VPD */
-	buffer = scsi_get_vpd_page(sdkp->device, 0xb1);
+	buffer = kmalloc(vpd_len, GFP_KERNEL);
 
-	if (buffer == NULL)
-		return;
+	if (!buffer ||
+	    /* Block Device Characteristics VPD */
+	    scsi_get_vpd_page(sdkp->device, 0xb1, buffer, vpd_len))
+		goto out;
 
 	rot = get_unaligned_be16(&buffer[4]);
 
 	if (rot == 1)
 		queue_flag_set_unlocked(QUEUE_FLAG_NONROT, sdkp->disk->queue);
 
+ out:
 	kfree(buffer);
 }
 
diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index 55b034b..1d7a878 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -448,13 +448,17 @@ static void ses_match_to_enclosure(struct enclosure_device *edev,
 		.addr = 0,
 	};
 
-	buf = scsi_get_vpd_page(sdev, 0x83);
-	if (!buf)
-		return;
+	buf = kmalloc(INIT_ALLOC_SIZE, GFP_KERNEL);
+	if (!buf || scsi_get_vpd_page(sdev, 0x83, buf, INIT_ALLOC_SIZE))
+		goto free;
 
 	ses_enclosure_data_process(edev, to_scsi_device(edev->edev.parent), 0);
 
 	vpd_len = ((buf[2] << 8) | buf[3]) + 4;
+	kfree(buf);
+	buf = kmalloc(vpd_len, GFP_KERNEL);
+	if (!buf ||scsi_get_vpd_page(sdev, 0x83, buf, vpd_len))
+		goto free;
 
 	desc = buf + 4;
 	while (desc < buf + vpd_len) {
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 68d185c..fe66d34 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -347,7 +347,8 @@ extern int scsi_mode_select(struct scsi_device *sdev, int pf, int sp,
 			    struct scsi_sense_hdr *);
 extern int scsi_test_unit_ready(struct scsi_device *sdev, int timeout,
 				int retries, struct scsi_sense_hdr *sshdr);
-extern unsigned char *scsi_get_vpd_page(struct scsi_device *, u8 page);
+extern int scsi_get_vpd_page(struct scsi_device *, u8 page, unsigned char *buf,
+			     int buf_len);
 extern int scsi_device_set_state(struct scsi_device *sdev,
 				 enum scsi_device_state state);
 extern struct scsi_event *sdev_evt_alloc(enum scsi_device_event evt_type,

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

[Boaz Harrosh]

On 11/03/2009 08:33 PM, James Bottomley wrote:
> On Sun, 2009-08-30 at 09:35 -0500, James Bottomley wrote:
>> On Sun, 2009-08-30 at 14:45 +0300, Boaz Harrosh wrote:
>>> On 08/28/2009 02:45 AM, James Bottomley wrote:
>>>> On Mon, 2009-08-24 at 18:21 +0200, Roel Kluin wrote:
>>>>> kmalloc() may fail, so test whether it succeeded.
>>>>>
>>>>> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
>>>>> ---
>>>>> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
>>>>> index 2de5f3a..34fdde0 100644
>>>>> --- a/drivers/scsi/scsi.c
>>>>> +++ b/drivers/scsi/scsi.c
>>>>> @@ -1056,6 +1056,9 @@ unsigned char *scsi_get_vpd_page(struct scsi_device *sdev, u8 page)
>>>>>  
>>>>>  	kfree(buf);
>>>>>  	buf = kmalloc(len + 4, GFP_KERNEL);
>>>>> +	if (!buf)
>>>>> +		return NULL;
>>>>> +
>>>>
>>>> Firstly, this won't actually apply ... you should be developing against
>>>> either the SCSI trees or linux-next to get the latest versions in git.
>>>>
>>>> Secondly it's not really right for the most common use cases, which
>>>> don't usually want the whole vpd buffer anyway.  I don't really see a
>>>> simple way of fixing it without altering the interface, though.
>>>>
>>>
>>> "most common use cases" do not have pages bigger then 255. Can you think
>>> of any? For these few places that anticipate pages bigger then 255 I don't
>>> see much choice, we just freed 255 bytes and tried a new size, say 317, which
>>> failed with GFP_KERNEL, In such a busy system, better fail with NULL then BUG,
>>> No? In any way caller must check for NULL because of the first allocation.
>>
>> Other way around: common use cases means the callers.  What I'm saying
>> is that regardless of the size of the VPD page, the caller usually only
>> wants a few bytes into it.  Once we have all the information the caller
>> ever needs, there's no point in trying again with a larger buffer just
>> because the VPD page is larger ... to code this into the interface,
>> though, the caller would need to specify the max length it is looking
>> for.
> 
> OK, it's been a couple of months and  no patch has been forthcoming on
> this, so how about the attached?  It does the query and only adjusts the
> size where the caller actually wants to bother.
> 
> James
> 
> ---
> 
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index a60da55..513661f 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -1026,55 +1026,39 @@ static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,
>   * responsible for calling kfree() on this pointer when it is no longer
>   * needed.  If we cannot retrieve the VPD page this routine returns %NULL.
>   */

The comment is wrong now, I would also say something about buf_len is recommended
to be 255 or more since other wise we might miss out on pages in the first inquiry.

> -unsigned char *scsi_get_vpd_page(struct scsi_device *sdev, u8 page)
> +int scsi_get_vpd_page(struct scsi_device *sdev, u8 page, unsigned char *buf,
> +		      int buf_len)
>  {
>  	int i, result;
> -	unsigned int len;
> -	const unsigned int init_vpd_len = 255;
> -	unsigned char *buf = kmalloc(init_vpd_len, GFP_KERNEL);
> -
> -	if (!buf)
> -		return NULL;
>  
>  	/* Ask for all the pages supported by this device */
> -	result = scsi_vpd_inquiry(sdev, buf, 0, init_vpd_len);
> +	result = scsi_vpd_inquiry(sdev, buf, 0, buf_len);
>  	if (result)
>  		goto fail;
>  
>  	/* If the user actually wanted this page, we can skip the rest */
>  	if (page == 0)
> -		return buf;
> +		return -EINVAL;
>  

Why -EINVAL; Look at the comment above, return 0 would be better.

> -	for (i = 0; i < buf[3]; i++)
> +	for (i = 0; i < min((int)buf[3], buf_len - 4); i++)
>  		if (buf[i + 4] == page)
>  			goto found;
> +
> +	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)

>  	/* The device claims it doesn't support the requested page */
>  	goto fail;
>  
>   found:
> -	result = scsi_vpd_inquiry(sdev, buf, page, 255);
> +	result = scsi_vpd_inquiry(sdev, buf, page, buf_len);
>  	if (result)
>  		goto fail;
>  
> -	/*
> -	 * Some pages are longer than 255 bytes.  The actual length of
> -	 * the page is returned in the header.
> -	 */
> -	len = ((buf[2] << 8) | buf[3]) + 4;
> -	if (len <= init_vpd_len)
> -		return buf;
> -
> -	kfree(buf);
> -	buf = kmalloc(len, GFP_KERNEL);
> -	result = scsi_vpd_inquiry(sdev, buf, page, len);
> -	if (result)
> -		goto fail;
> -
> -	return buf;
> +	return 0;
>  
>   fail:
> -	kfree(buf);
> -	return NULL;
> +	return -EINVAL;

why -EINVAL? should we not return result, as received from scsi_vpd_inquiry?

>  }
>  EXPORT_SYMBOL_GPL(scsi_get_vpd_page);
>  
> 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.

We used to allocate 255 here, for some devices out there this is surly
a regression.

Boaz
> -	/* Block Limits VPD */
> -	buffer = scsi_get_vpd_page(sdkp->device, 0xb0);
> -
> -	if (buffer == NULL)
> -		return;
> +	if (!buffer ||
> +	    /* Block Limits VPD */
> +	    scsi_get_vpd_page(sdkp->device, 0xb0, buffer, vpd_len))
> +		goto out;
>  
>  	blk_queue_io_min(sdkp->disk->queue,
>  			 get_unaligned_be16(&buffer[6]) * sector_sz);
>  	blk_queue_io_opt(sdkp->disk->queue,
>  			 get_unaligned_be32(&buffer[12]) * sector_sz);
>  
> + out:
>  	kfree(buffer);
>  }
>  
> @@ -1886,20 +1887,23 @@ static void sd_read_block_limits(struct scsi_disk *sdkp)
>   */
>  static void sd_read_block_characteristics(struct scsi_disk *sdkp)
>  {
> -	char *buffer;
> +	unsigned char *buffer;
>  	u16 rot;
> +	const int vpd_len = 32;
>  
> -	/* Block Device Characteristics VPD */
> -	buffer = scsi_get_vpd_page(sdkp->device, 0xb1);
> +	buffer = kmalloc(vpd_len, GFP_KERNEL);
>  
> -	if (buffer == NULL)
> -		return;
> +	if (!buffer ||
> +	    /* Block Device Characteristics VPD */
> +	    scsi_get_vpd_page(sdkp->device, 0xb1, buffer, vpd_len))
> +		goto out;
>  
>  	rot = get_unaligned_be16(&buffer[4]);
>  
>  	if (rot == 1)
>  		queue_flag_set_unlocked(QUEUE_FLAG_NONROT, sdkp->disk->queue);
>  
> + out:
>  	kfree(buffer);
>  }
>  
> diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
> index 55b034b..1d7a878 100644
> --- a/drivers/scsi/ses.c
> +++ b/drivers/scsi/ses.c
> @@ -448,13 +448,17 @@ static void ses_match_to_enclosure(struct enclosure_device *edev,
>  		.addr = 0,
>  	};
>  
> -	buf = scsi_get_vpd_page(sdev, 0x83);
> -	if (!buf)
> -		return;
> +	buf = kmalloc(INIT_ALLOC_SIZE, GFP_KERNEL);
> +	if (!buf || scsi_get_vpd_page(sdev, 0x83, buf, INIT_ALLOC_SIZE))
> +		goto free;
>  
>  	ses_enclosure_data_process(edev, to_scsi_device(edev->edev.parent), 0);
>  
>  	vpd_len = ((buf[2] << 8) | buf[3]) + 4;
> +	kfree(buf);
> +	buf = kmalloc(vpd_len, GFP_KERNEL);
> +	if (!buf ||scsi_get_vpd_page(sdev, 0x83, buf, vpd_len))
> +		goto free;
>  
>  	desc = buf + 4;
>  	while (desc < buf + vpd_len) {
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index 68d185c..fe66d34 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -347,7 +347,8 @@ extern int scsi_mode_select(struct scsi_device *sdev, int pf, int sp,
>  			    struct scsi_sense_hdr *);
>  extern int scsi_test_unit_ready(struct scsi_device *sdev, int timeout,
>  				int retries, struct scsi_sense_hdr *sshdr);
> -extern unsigned char *scsi_get_vpd_page(struct scsi_device *, u8 page);
> +extern int scsi_get_vpd_page(struct scsi_device *, u8 page, unsigned char *buf,
> +			     int buf_len);
>  extern int scsi_device_set_state(struct scsi_device *sdev,
>  				 enum scsi_device_state state);
>  extern struct scsi_event *sdev_evt_alloc(enum scsi_device_event evt_type,
> 
> 
> 

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

[James Bottomley]

On Wed, 2009-11-04 at 10:54 +0200, Boaz Harrosh wrote:
> On 11/03/2009 08:33 PM, James Bottomley wrote:
> > On Sun, 2009-08-30 at 09:35 -0500, James Bottomley wrote:
> >> On Sun, 2009-08-30 at 14:45 +0300, Boaz Harrosh wrote:
> >>> On 08/28/2009 02:45 AM, James Bottomley wrote:
> >>>> On Mon, 2009-08-24 at 18:21 +0200, Roel Kluin wrote:
> >>>>> kmalloc() may fail, so test whether it succeeded.
> >>>>>
> >>>>> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> >>>>> ---
> >>>>> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> >>>>> index 2de5f3a..34fdde0 100644
> >>>>> --- a/drivers/scsi/scsi.c
> >>>>> +++ b/drivers/scsi/scsi.c
> >>>>> @@ -1056,6 +1056,9 @@ unsigned char *scsi_get_vpd_page(struct scsi_device *sdev, u8 page)
> >>>>>  
> >>>>>  	kfree(buf);
> >>>>>  	buf = kmalloc(len + 4, GFP_KERNEL);
> >>>>> +	if (!buf)
> >>>>> +		return NULL;
> >>>>> +
> >>>>
> >>>> Firstly, this won't actually apply ... you should be developing against
> >>>> either the SCSI trees or linux-next to get the latest versions in git.
> >>>>
> >>>> Secondly it's not really right for the most common use cases, which
> >>>> don't usually want the whole vpd buffer anyway.  I don't really see a
> >>>> simple way of fixing it without altering the interface, though.
> >>>>
> >>>
> >>> "most common use cases" do not have pages bigger then 255. Can you think
> >>> of any? For these few places that anticipate pages bigger then 255 I don't
> >>> see much choice, we just freed 255 bytes and tried a new size, say 317, which
> >>> failed with GFP_KERNEL, In such a busy system, better fail with NULL then BUG,
> >>> No? In any way caller must check for NULL because of the first allocation.
> >>
> >> Other way around: common use cases means the callers.  What I'm saying
> >> is that regardless of the size of the VPD page, the caller usually only
> >> wants a few bytes into it.  Once we have all the information the caller
> >> ever needs, there's no point in trying again with a larger buffer just
> >> because the VPD page is larger ... to code this into the interface,
> >> though, the caller would need to specify the max length it is looking
> >> for.
> > 
> > OK, it's been a couple of months and  no patch has been forthcoming on
> > this, so how about the attached?  It does the query and only adjusts the
> > size where the caller actually wants to bother.
> > 
> > James
> > 
> > ---
> > 
> > diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> > index a60da55..513661f 100644
> > --- a/drivers/scsi/scsi.c
> > +++ b/drivers/scsi/scsi.c
> > @@ -1026,55 +1026,39 @@ static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,
> >   * responsible for calling kfree() on this pointer when it is no longer
> >   * needed.  If we cannot retrieve the VPD page this routine returns %NULL.
> >   */
> 
> The comment is wrong now, I would also say something about buf_len is recommended
> to be 255 or more since other wise we might miss out on pages in the first inquiry.

Will fix.

> > -unsigned char *scsi_get_vpd_page(struct scsi_device *sdev, u8 page)
> > +int scsi_get_vpd_page(struct scsi_device *sdev, u8 page, unsigned char *buf,
> > +		      int buf_len)
> >  {
> >  	int i, result;
> > -	unsigned int len;
> > -	const unsigned int init_vpd_len = 255;
> > -	unsigned char *buf = kmalloc(init_vpd_len, GFP_KERNEL);
> > -
> > -	if (!buf)
> > -		return NULL;
> >  
> >  	/* Ask for all the pages supported by this device */
> > -	result = scsi_vpd_inquiry(sdev, buf, 0, init_vpd_len);
> > +	result = scsi_vpd_inquiry(sdev, buf, 0, buf_len);
> >  	if (result)
> >  		goto fail;
> >  
> >  	/* If the user actually wanted this page, we can skip the rest */
> >  	if (page == 0)
> > -		return buf;
> > +		return -EINVAL;
> >  
> 
> Why -EINVAL; Look at the comment above, return 0 would be better.

Bug in code (well spotted), will fix.

> > -	for (i = 0; i < buf[3]; i++)
> > +	for (i = 0; i < min((int)buf[3], buf_len - 4); i++)
> >  		if (buf[i + 4] == page)
> >  			goto found;
> > +
> > +	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.

> >  	/* The device claims it doesn't support the requested page */
> >  	goto fail;
> >  
> >   found:
> > -	result = scsi_vpd_inquiry(sdev, buf, page, 255);
> > +	result = scsi_vpd_inquiry(sdev, buf, page, buf_len);
> >  	if (result)
> >  		goto fail;
> >  
> > -	/*
> > -	 * Some pages are longer than 255 bytes.  The actual length of
> > -	 * the page is returned in the header.
> > -	 */
> > -	len = ((buf[2] << 8) | buf[3]) + 4;
> > -	if (len <= init_vpd_len)
> > -		return buf;
> > -
> > -	kfree(buf);
> > -	buf = kmalloc(len, GFP_KERNEL);
> > -	result = scsi_vpd_inquiry(sdev, buf, page, len);
> > -	if (result)
> > -		goto fail;
> > -
> > -	return buf;
> > +	return 0;
> >  
> >   fail:
> > -	kfree(buf);
> > -	return NULL;
> > +	return -EINVAL;
> 
> why -EINVAL? should we not return result, as received from scsi_vpd_inquiry?

Yes, was in two minds about that.  Since zero is success, we can return
result easily ... I just like the idea of negative error numbers.

> >  }
> >  EXPORT_SYMBOL_GPL(scsi_get_vpd_page);
> >  
> > 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.

James

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

[Boaz Harrosh]

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:

	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.

> James
> 
> 

Boaz

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

[James Bottomley]

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

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

[Boaz Harrosh]

On 11/04/2009 07:50 PM, James Bottomley wrote:
> On Wed, 2009-11-04 at 18:18 +0200, Boaz Harrosh wrote:
> 
> 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.
> 

OK thanks. But maybe just define a MIN_INQUIRY_SIZE instead of hard
coded 32 everywhere, and use that. So in future if such a device is
found we can easily change it.

(Never say never ;-))


> James
> 
> 

Boaz

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

[James Bottomley]

On Thu, 2009-11-05 at 10:29 +0200, Boaz Harrosh wrote:
> On 11/04/2009 07:50 PM, James Bottomley wrote:
> > On Wed, 2009-11-04 at 18:18 +0200, Boaz Harrosh wrote:
> > 
> > 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.
> > 
> 
> OK thanks. But maybe just define a MIN_INQUIRY_SIZE instead of hard
> coded 32 everywhere, and use that. So in future if such a device is
> found we can easily change it.

So the minimum inquiry size would actually be 36 ... from
scsi_scan.c ... and that's hard coded to a value too.

> (Never say never ;-))

I'm hoping that by the time USB devices get complex enough to need more
than 28 VPD pages, they've actually discovered what conforming to the
standards means.

James

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

[Boaz Harrosh]

On 11/05/2009 09:41 PM, James Bottomley wrote:
> On Thu, 2009-11-05 at 10:29 +0200, Boaz Harrosh wrote:
>> On 11/04/2009 07:50 PM, James Bottomley wrote:
>>> On Wed, 2009-11-04 at 18:18 +0200, Boaz Harrosh wrote:
>>>
>>> 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.
>>>
>>
>> OK thanks. But maybe just define a MIN_INQUIRY_SIZE instead of hard
>> coded 32 everywhere, and use that. So in future if such a device is
>> found we can easily change it.
> 
> So the minimum inquiry size would actually be 36 ... from
> scsi_scan.c ... and that's hard coded to a value too.
> 

two wrongs don't make a right ;-) But yes, sorry, that's a wrong name.
I meant minimum-vpd-page-inquiry, a specifc constant for that function
we are talking about.

>> (Never say never ;-))
> 
> I'm hoping that by the time USB devices get complex enough to need more
> than 28 VPD pages, they've actually discovered what conforming to the
> standards means.
> 

I don't understand what your saying. I'm making a simple and sane point.
A new comer, from looking at the API, will say "Haa I only need 12 bytes
lets set that". But no the buffer has dual purpose, one get the page I want,
but zero check these crap USB devices. (And all those other subtle things)
So you are actually going to argue that a patch should introduce an
hard coded 32 unexplained, and refuse a constant that actually communicates
the issue, which also covers our asses from the hopes we make.
Because I sure hope so too, but I'm also 48 years old.

> James
> 
> 

Boaz