[PATCH] scsi: core: Consult supported VPD page list prior to fetching page

[PATCH] scsi: core: Consult supported VPD page list prior to fetching page

[Martin K. Petersen]

Commit c92a6b5d6335 ("scsi: core: Query VPD size before getting full
page") removed the logic which checks whether a VPD page is present on
the supported pages list before asking for the page itself. That was
done because SPC helpfully states "The Supported VPD Pages VPD page
list may or may not include all the VPD pages that are able to be
returned by the device server". Testing had revealed a few devices
that supported some of the 0xBn pages but didn't actually list them in
page 0.

Julian Sikorski bisected a problem with his drive resetting during
discovery to the commit above. As it turns out, this particular drive
firmware will crash if we attempt to fetch page 0xB9.

Various approaches were attempted to work around this. In the end,
reinstating the logic that consults VPD page 0 before fetching any
other page was the path of least resistance. A firmware update for the
devices which originally compelled us to remove the check has since
been released.

Cc: <stable@vger.kernel.org>
Fixes: c92a6b5d6335 ("scsi: core: Query VPD size before getting full page")
Reported-by: Julian Sikorski <belegdol@gmail.com>
Tested-by: Julian Sikorski <belegdol@gmail.com>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
---
 drivers/scsi/scsi.c        | 21 +++++++++++++++++++--
 include/scsi/scsi_device.h |  1 +
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 76d369343c7a..5ef5fcf022ed 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -330,19 +330,36 @@ static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,
 
 static int scsi_get_vpd_size(struct scsi_device *sdev, u8 page)
 {
-	unsigned char vpd_header[SCSI_VPD_HEADER_SIZE] __aligned(4);
+	unsigned char vpd[SCSI_VPD_LIST_SIZE] __aligned(4);
 	int result;
 
 	if (sdev->no_vpd_size)
 		return SCSI_DEFAULT_VPD_LEN;
 
+	/*
+	 * Fetch the supported pages VPD and validate that the requested page
+	 * number is present.
+	 */
+	if (page != 0) {
+		result = scsi_vpd_inquiry(sdev, vpd, 0, sizeof(vpd));
+		if (result < SCSI_VPD_HEADER_SIZE)
+			return 0;
+
+		for (unsigned int i = SCSI_VPD_HEADER_SIZE ; i < result ; i++) {
+			if (vpd[i] == page)
+				goto found;
+		}
+
+		return 0;
+	}
+found:
 	/*
 	 * Fetch the VPD page header to find out how big the page
 	 * is. This is done to prevent problems on legacy devices
 	 * which can not handle allocation lengths as large as
 	 * potentially requested by the caller.
 	 */
-	result = scsi_vpd_inquiry(sdev, vpd_header, page, sizeof(vpd_header));
+	result = scsi_vpd_inquiry(sdev, vpd, page, SCSI_VPD_HEADER_SIZE);
 	if (result < 0)
 		return 0;
 
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index cb019c80763b..6673885565e3 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -102,6 +102,7 @@ struct scsi_vpd {
 
 enum scsi_vpd_parameters {
 	SCSI_VPD_HEADER_SIZE = 4,
+	SCSI_VPD_LIST_SIZE = 36,
 };
 
 struct scsi_device {
-- 
2.42.1

Re: [PATCH] scsi: core: Consult supported VPD page list prior to fetching page

[Bart Van Assche]

On 2/14/24 10:25, Martin K. Petersen wrote:
> +		for (unsigned int i = SCSI_VPD_HEADER_SIZE ; i < result ; i++) {
> +			if (vpd[i] == page)
> +				goto found;
> +		}

Can this loop be changed into a memchr() call?

> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index cb019c80763b..6673885565e3 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -102,6 +102,7 @@ struct scsi_vpd {
>   
>   enum scsi_vpd_parameters {
>   	SCSI_VPD_HEADER_SIZE = 4,
> +	SCSI_VPD_LIST_SIZE = 36,
>   };
>   
>   struct scsi_device {

Since these constants are only used inside drivers/scsi/scsi.c, how about
moving these constants into the drivers/scsi/scsi.c file?

Thanks,

Bart.

Re: [PATCH] scsi: core: Consult supported VPD page list prior to fetching page

[Martin K. Petersen]

Hi Bart!

> On 2/14/24 10:25, Martin K. Petersen wrote:
>> +		for (unsigned int i = SCSI_VPD_HEADER_SIZE ; i < result ; i++) {
>> +			if (vpd[i] == page)
>> +				goto found;
>> +		}
>
> Can this loop be changed into a memchr() call?

Would you prefer the following?

	/* Look for page number in the returned list of supported VPDs */
	result -= SCSI_VPD_HEADER_SIZE;
	if (!memchr(&vpd[SCSI_VPD_HEADER_SIZE], page, result))
		return 0;

I find that the idiomatic for loop is easy to understand whereas the
memchr() requires a bit of squinting. But I don't really have a strong
preference. I do like that the memchr() gets rid of the goto.

>> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
>> index cb019c80763b..6673885565e3 100644
>> --- a/include/scsi/scsi_device.h
>> +++ b/include/scsi/scsi_device.h
>> @@ -102,6 +102,7 @@ struct scsi_vpd {
>>     enum scsi_vpd_parameters {
>>   	SCSI_VPD_HEADER_SIZE = 4,
>> +	SCSI_VPD_LIST_SIZE = 36,
>>   };
>>     struct scsi_device {
>
> Since these constants are only used inside drivers/scsi/scsi.c, how about
> moving these constants into the drivers/scsi/scsi.c file?

Sure!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] scsi: core: Consult supported VPD page list prior to fetching page

[Bart Van Assche]

On 2/14/24 12:20, Martin K. Petersen wrote:
>> On 2/14/24 10:25, Martin K. Petersen wrote:
>>> +		for (unsigned int i = SCSI_VPD_HEADER_SIZE ; i < result ; i++) {
>>> +			if (vpd[i] == page)
>>> +				goto found;
>>> +		}
>>
>> Can this loop be changed into a memchr() call?
> 
> Would you prefer the following?
> 
> 	/* Look for page number in the returned list of supported VPDs */
> 	result -= SCSI_VPD_HEADER_SIZE;
> 	if (!memchr(&vpd[SCSI_VPD_HEADER_SIZE], page, result))
> 		return 0;
> 
> I find that the idiomatic for loop is easy to understand whereas the
> memchr() requires a bit of squinting. But I don't really have a strong
> preference. I do like that the memchr() gets rid of the goto.

Although I slightly prefer the memchr() variant, I'm also fine with keeping the
for-loop.

Thanks,

Bart.