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@xxxxxxxxx>
>>>>> ---
>>>>> 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,
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
