On 2021/08/19 23:42, Niklas Cassel wrote:
> On Fri, Aug 13, 2021 at 10:07:38AM +0900, Damien Le Moal wrote:
>> Remove unnecessary gotos in scsi_get_vpd_page() to improve the code
>> readability. Also use memchr() instead of an open coded search loop
>> and update the outdated kernel doc comment for this function.
>>
>> Signed-off-by: Damien Le Moal <damien.lemoal@xxxxxxx>
>> ---
>>
>> Changes from v1:
>> * Keep the "found" goto and use memchr() in place of the page search
>> loop, as suggested by Bart.
>> * Update the patch commit title and message
>>
>> drivers/scsi/scsi.c | 30 +++++++++++++-----------------
>> 1 file changed, 13 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
>> index d26025cf5de3..4946d8c4f298 100644
>> --- a/drivers/scsi/scsi.c
>> +++ b/drivers/scsi/scsi.c
>> @@ -339,47 +339,43 @@ static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,
>> *
>> * SCSI devices may optionally supply Vital Product Data. Each 'page'
>> * of VPD is defined in the appropriate SCSI document (eg SPC, SBC).
>> - * If the device supports this VPD page, this routine returns a pointer
>> - * to a buffer containing the data from that page. The caller is
>> - * responsible for calling kfree() on this pointer when it is no longer
>> - * needed. If we cannot retrieve the VPD page this routine returns %NULL.
>> + * If the device supports this VPD page, this routine fills @buf
>> + * with the data from that page and return 0. If the VPD page is not
>> + * supported or its content cannot be retrieved, -EINVAL is returned.
>> */
>> int scsi_get_vpd_page(struct scsi_device *sdev, u8 page, unsigned char *buf,
>> int buf_len)
>> {
>> - int i, result;
>> + int result;
>>
>> if (sdev->skip_vpd_pages)
>> - goto fail;
>> + return -EINVAL;
>>
>> /* Ask for all the pages supported by this device */
>> result = scsi_vpd_inquiry(sdev, buf, 0, buf_len);
>> if (result < 4)
>> - goto fail;
>> + return -EINVAL;
>>
>> /* If the user actually wanted this page, we can skip the rest */
>> if (page == 0)
>> return 0;
>>
>> - for (i = 4; i < min(result, buf_len); i++)
>> - if (buf[i] == page)
>> - goto found;
>> + if (memchr(&buf[4], page, min(result, buf_len)))
>
> Hello Damien,
>
> This will try to access data outside the buffer.
>
> When starting from index 4, you cannot search buf_len number of elements.
Aouch, indeed, good catch. Sending V2 to correct that.
>
>
> Kind regards,
> Niklas
>
--
Damien Le Moal
Western Digital Research
