[02/14] scsi: core: Query VPD size before getting full page

We currently default to 255 bytes when fetching VPD pages during discovery.
However, we have had a few devices that are known to wedge if the requested
buffer exceeds a certain size. See commit af73623f5f10 ("[SCSI] sd: Reduce
buffer size for vpd request") which works around one example of this
problem in the SCSI disk driver.

With commit d188b0675b21 ("scsi: core: Add sysfs attributes for VPD pages
0h and 89h") we now risk triggering the same issue in the generic midlayer
code.

The problem with the ATA VPD page in particular is that the SCSI portion of
the page is trailed by 512 bytes of verbatim ATA Identify Device
information.  However, not all controllers actually provide the additional
512 bytes and will lock up if one asks for more than the 64 bytes
containing the SCSI protocol fields.

Instead of picking a new, somewhat arbitrary, number of bytes for the VPD
buffer size, start fetching the 4-byte header for each page. The header
contains the size of the page as far as the device is concerned. We can use
the reported size to specify the correct allocation length when
subsequently fetching the full page.

The header validation is done by a new helper function scsi_get_vpd_size()
and both scsi_get_vpd_page() and scsi_get_vpd_buf() now rely on this to
query the page size.

In addition, scsi_get_vpd_page() is simplified to mirror the logic in
scsi_get_vpd_page(). This involves removing the Supported VPD Pages lookup
prior to attempting to query a page. There does not appear any evidence,
even in the oldest SCSI specs, that this step is required. We already rely
on scsi_get_vpd_page() throughout the stack and this function never
consulted the Supported VPD Pages. Since this has not caused any problems
it should be safe to remove the precondition from scsi_get_vpd_page().

Instrumented runs also revealed that the Supported VPD Pages lookup had
little effect since the device page index often was larger than the
supplied buffer size. As a result, inquiries frequently bypassed the index
check and went through the "If we ran off the end of the buffer, give us
the benefit of the doubt" code path which assumed the page was present
despite not being listed. The revised code takes both the page size
reported by the device as well as the size of the buffer provided by the
scsi_get_vpd_page() caller into account.

Fixes: d188b0675b21 ("scsi: core: Add sysfs attributes for VPD pages 0h and 89h")
Reported-by: Maciej W. Rozycki <macro@orcam.me.uk>
Tested-by: Maciej W. Rozycki <macro@orcam.me.uk>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
---
 drivers/scsi/scsi.c        | 94 +++++++++++++++++++++++++-------------
 include/scsi/scsi_device.h |  5 +-
 2 files changed, 67 insertions(+), 32 deletions(-)

On 3/1/22 21:35, Martin K. Petersen wrote:
> +static int scsi_get_vpd_size(struct scsi_device *sdev, u8 page)
> +{
> +	unsigned char vpd_header[SCSI_VPD_HEADER_SIZE];
> +	int result;

The SCSI core sets the minimum DMA alignment to 4 bytes. How about 
aligning the vpd_header[] array explicitly to a four-byte boundary such 
that the block layer does not have to allocate a temporary buffer?

> +	vpd_len = min(vpd_len, buf_len);
>   
> - found:
> -	result = scsi_vpd_inquiry(sdev, buf, page, buf_len);
> +retry_pg:
> +	/*
> +	 * Fetch the actual page. Since the appropriate size was reported
> +	 * by the device it is now safe to ask for something bigger.
> +	 */
> +	memset(buf, 0, buf_len);
> +	result = scsi_vpd_inquiry(sdev, buf, page, vpd_len);
>   	if (result < 0)
> -		goto fail;
> +		return -EINVAL;
> +	else if (result > vpd_len) {
> +		dev_warn_once(&sdev->sdev_gendev,
> +			      "%s: VPD page 0x%02x result %d > %d bytes\n",
> +			      __func__, page, result, vpd_len);
> +		vpd_len = min(result, buf_len);
> +		goto retry_pg;
> +	}

Will an endless loop be triggered if the VPD page length is larger than 
'buf_len'?

Thanks,

Bart.

Bart,

>> +	vpd_len = min(vpd_len, buf_len);
>>   - found:
>> -	result = scsi_vpd_inquiry(sdev, buf, page, buf_len);
>> +retry_pg:
>> +	/*
>> +	 * Fetch the actual page. Since the appropriate size was reported
>> +	 * by the device it is now safe to ask for something bigger.
>> +	 */
>> +	memset(buf, 0, buf_len);
>> +	result = scsi_vpd_inquiry(sdev, buf, page, vpd_len);
>>   	if (result < 0)
>> -		goto fail;
>> +		return -EINVAL;
>> +	else if (result > vpd_len) {
>> +		dev_warn_once(&sdev->sdev_gendev,
>> +			      "%s: VPD page 0x%02x result %d > %d bytes\n",
>> +			      __func__, page, result, vpd_len);
>> +		vpd_len = min(result, buf_len);
>> +		goto retry_pg;
>> +	}
>
> Will an endless loop be triggered if the VPD page length is larger
> than 'buf_len'?

Ah, transplant thinko from scsi_get_vpd_buf() which reallocates the
buffer on mismatch. Will fix.

Thanks!

Johannes,

> Two minior nits below:
>
>> +	result = scsi_vpd_inquiry(sdev, vpd_header, page, sizeof(vpd_header));
>> +
>> +	if (result < 0)
>> +		return 0;
>
> can we remove the empty line?

Sure!

>> +	memset(buf, 0, buf_len);
>> +	result = scsi_vpd_inquiry(sdev, buf, page, vpd_len);
>>  	if (result < 0)
>> -		goto fail;
>> +		return -EINVAL;
>> +	else if (result > vpd_len) {
>> +		dev_warn_once(&sdev->sdev_gendev,
>> +			      "%s: VPD page 0x%02x result %d > %d bytes\n",
>> +			      __func__, page, result, vpd_len);
>> +		vpd_len = min(result, buf_len);
>> +		goto retry_pg;
>> +	}
>>  
>
> Adding {} to the if block as well makes it look nicer IMHO

Roger.