* [PATCH v2] scsi: simplify scsi_get_vpd_page()
@ 2021-08-13 1:07 Damien Le Moal
2021-08-19 14:42 ` Niklas Cassel
0 siblings, 1 reply; 5+ messages in thread
From: Damien Le Moal @ 2021-08-13 1:07 UTC (permalink / raw)
To: linux-scsi, Martin K . Petersen
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@wdc.com>
---
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)))
+ goto found;
- if (i < result && i >= buf_len)
- /* ran off the end of the buffer, give us benefit of doubt */
+ /* If we ran off the end of the buffer, give us benefit of doubt */
+ if (result > buf_len)
goto found;
+
/* The device claims it doesn't support the requested page */
- goto fail;
+ return -EINVAL;
found:
result = scsi_vpd_inquiry(sdev, buf, page, buf_len);
if (result < 0)
- goto fail;
+ return -EINVAL;
return 0;
-
- fail:
- return -EINVAL;
}
EXPORT_SYMBOL_GPL(scsi_get_vpd_page);
--
2.31.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH v2] scsi: simplify scsi_get_vpd_page()
2021-08-13 1:07 [PATCH v2] scsi: simplify scsi_get_vpd_page() Damien Le Moal
@ 2021-08-19 14:42 ` Niklas Cassel
2021-08-19 23:59 ` Damien Le Moal
0 siblings, 1 reply; 5+ messages in thread
From: Niklas Cassel @ 2021-08-19 14:42 UTC (permalink / raw)
To: Damien Le Moal; +Cc: linux-scsi, Martin K . Petersen
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@wdc.com>
> ---
>
> 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.
Kind regards,
Niklas
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v2] scsi: simplify scsi_get_vpd_page()
2021-08-19 14:42 ` Niklas Cassel
@ 2021-08-19 23:59 ` Damien Le Moal
0 siblings, 0 replies; 5+ messages in thread
From: Damien Le Moal @ 2021-08-19 23:59 UTC (permalink / raw)
To: Niklas Cassel; +Cc: linux-scsi, Martin K . Petersen
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@wdc.com>
>> ---
>>
>> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2] scsi: simplify scsi_get_vpd_page()
@ 2021-08-20 3:21 Damien Le Moal
2021-08-20 13:13 ` Niklas Cassel
0 siblings, 1 reply; 5+ messages in thread
From: Damien Le Moal @ 2021-08-20 3:21 UTC (permalink / raw)
To: linux-scsi, Martin K . Petersen
Remove unnecessary gotos in scsi_get_vpd_page() to improve the code
readability and use memchr() instead of an open coded search loop.
Also update the outdated kernel doc comment for this function.
Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
Changes from v1:
* Fix memchr() use to avoid accesses beyond the buffer and result size.
drivers/scsi/scsi.c | 31 ++++++++++++++-----------------
1 file changed, 14 insertions(+), 17 deletions(-)
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index b241f9e3885c..6be68b3427a0 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -339,47 +339,44 @@ 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, len;
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;
+ len = min(result, buf_len);
+ if (len > 4 && memchr(&buf[4], page, len - 4))
+ goto found;
- if (i < result && i >= buf_len)
- /* ran off the end of the buffer, give us benefit of doubt */
+ /* If we ran off the end of the buffer, give us benefit of doubt */
+ if (result > buf_len)
goto found;
+
/* The device claims it doesn't support the requested page */
- goto fail;
+ return -EINVAL;
found:
result = scsi_vpd_inquiry(sdev, buf, page, buf_len);
if (result < 0)
- goto fail;
+ return -EINVAL;
return 0;
-
- fail:
- return -EINVAL;
}
EXPORT_SYMBOL_GPL(scsi_get_vpd_page);
--
2.31.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH v2] scsi: simplify scsi_get_vpd_page()
2021-08-20 3:21 Damien Le Moal
@ 2021-08-20 13:13 ` Niklas Cassel
0 siblings, 0 replies; 5+ messages in thread
From: Niklas Cassel @ 2021-08-20 13:13 UTC (permalink / raw)
To: Damien Le Moal; +Cc: linux-scsi, Martin K . Petersen
On Fri, Aug 20, 2021 at 12:21:54PM +0900, Damien Le Moal wrote:
> Remove unnecessary gotos in scsi_get_vpd_page() to improve the code
> readability and use memchr() instead of an open coded search loop.
> Also update the outdated kernel doc comment for this function.
>
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> ---
> Changes from v1:
> * Fix memchr() use to avoid accesses beyond the buffer and result size.
>
> drivers/scsi/scsi.c | 31 ++++++++++++++-----------------
> 1 file changed, 14 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index b241f9e3885c..6be68b3427a0 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -339,47 +339,44 @@ 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, len;
>
> 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;
> + len = min(result, buf_len);
> + if (len > 4 && memchr(&buf[4], page, len - 4))
> + goto found;
>
> - if (i < result && i >= buf_len)
> - /* ran off the end of the buffer, give us benefit of doubt */
> + /* If we ran off the end of the buffer, give us benefit of doubt */
> + if (result > buf_len)
> goto found;
> +
> /* The device claims it doesn't support the requested page */
> - goto fail;
> + return -EINVAL;
>
> found:
> result = scsi_vpd_inquiry(sdev, buf, page, buf_len);
> if (result < 0)
> - goto fail;
> + return -EINVAL;
>
> return 0;
> -
> - fail:
> - return -EINVAL;
> }
> EXPORT_SYMBOL_GPL(scsi_get_vpd_page);
>
(Nit: I think you forgot to bump the version. Subject for this is v2, which
is the same as the previous "scsi: simplify scsi_get_vpd_page()" patch.)
Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-08-20 13:13 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-13 1:07 [PATCH v2] scsi: simplify scsi_get_vpd_page() Damien Le Moal
2021-08-19 14:42 ` Niklas Cassel
2021-08-19 23:59 ` Damien Le Moal
2021-08-20 3:21 Damien Le Moal
2021-08-20 13:13 ` Niklas Cassel
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.