Re: [PATCH v2 7/9] scsi: sd: Check for and retry in case of READ_CAPCITY(10)/(16) returning no data

[Damien Le Moal <dlemoal@kernel.org>]

On 8/15/25 03:29, Ewan D. Milne wrote:
> sd_read_capacity_10() and sd_read_capacity_16() do not check for underflow
> and can extract invalid (e.g. zero) data when a malfunctioning device does
> not actually transfer any data, but returnes a good status otherwise.
> Check for this and retry, and log a message and return -EINVAL if we can't
> get the capacity information.
> 
> We encountered a device that did this once but returned good data afterwards.
> 
> See similar commit 5cd3bbfad088 ("[SCSI] retry with missing data for INQUIRY")
> 
> Signed-off-by: Ewan D. Milne <emilne@redhat.com>
> ---
>  drivers/scsi/sd.c | 56 ++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 48 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index f1ab2409ea3e..20b5eebba968 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -2639,6 +2639,7 @@ static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
>  		[13] = RC16_LEN,
>  	};
>  	struct scsi_sense_hdr sshdr;
> +	int count, resid;
>  	struct scsi_failure failure_defs[] = {
>  		/*
>  		 * Do not retry Invalid Command Operation Code or Invalid
> @@ -2689,6 +2690,7 @@ static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
>  	};
>  	const struct scsi_exec_args exec_args = {
>  		.sshdr = &sshdr,
> +		.resid = &resid,
>  		.failures = &failures,
>  	};
>  	int sense_valid = 0;
> @@ -2700,11 +2702,23 @@ static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
>  	if (sdp->no_read_capacity_16)
>  		return -EINVAL;
>  
> -	memset(buffer, 0, buflen);
> +	for (count = 0; count < 3; ++count) {
> +		memset(buffer, 0, buflen);
>  
> -	the_result = scsi_execute_cmd(sdp, cmd, REQ_OP_DRV_IN, buffer,
> -				      RC16_LEN, SD_TIMEOUT, sdkp->max_retries,
> -				      &exec_args);
> +		the_result = scsi_execute_cmd(sdp, cmd, REQ_OP_DRV_IN,
> +					      buffer, RC16_LEN, SD_TIMEOUT,
> +					      sdkp->max_retries, &exec_args);
> +
> +		if ((the_result == 0) && (resid == RC16_LEN)) {

You do not need the inner parenthesis. Also, it seems to me that this check
should simply be:

		if (resid)

Because any incomplete read capacity buffer is bound to be invalid.

> +			/*
> +			 * if nothing was transferred, we try
> +			 * again. It's a workaround for a broken
> +			 * device.
> +			 */
> +			continue;
> +		}
> +		break;
> +	}
>  
>  	if (the_result > 0) {
>  		if (media_not_present(sdkp, &sshdr))
> @@ -2728,6 +2742,12 @@ static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
>  		return -EINVAL;
>  	}
>  
> +	if (resid == RC16_LEN) {
> +		sd_printk(KERN_ERR, sdkp,
> +			  "Read Capacity(16) returned good status but no data");

Shouldn't this be a warning instead of error ? After all, there was no error...
And I would prefer seeing a warning for a bad device. The message would also be
better mentioning that this is the device fault.

> +		return -EINVAL;
> +	}
> +
>  	sector_size = get_unaligned_be32(&buffer[8]);
>  	lba = get_unaligned_be64(&buffer[0]);
>  
> @@ -2770,6 +2790,7 @@ static int read_capacity_10(struct scsi_disk *sdkp, struct scsi_device *sdp,
>  {
>  	static const u8 cmd[10] = { READ_CAPACITY };
>  	struct scsi_sense_hdr sshdr;
> +	int count, resid;
>  	struct scsi_failure failure_defs[] = {
>  		/* Do not retry Medium Not Present */
>  		{
> @@ -2804,17 +2825,30 @@ static int read_capacity_10(struct scsi_disk *sdkp, struct scsi_device *sdp,
>  	};
>  	const struct scsi_exec_args exec_args = {
>  		.sshdr = &sshdr,
> +		.resid = &resid,
>  		.failures = &failures,
>  	};
>  	int the_result;
>  	sector_t lba;
>  	unsigned sector_size;
>  
> -	memset(buffer, 0, buflen);
> +	for (count = 0; count < 3; ++count) {
> +		memset(buffer, 0, buflen);
>  
> -	the_result = scsi_execute_cmd(sdp, cmd, REQ_OP_DRV_IN, buffer,
> -				      8, SD_TIMEOUT, sdkp->max_retries,
> -				      &exec_args);
> +		the_result = scsi_execute_cmd(sdp, cmd, REQ_OP_DRV_IN,
> +					      buffer, RC10_LEN, SD_TIMEOUT,
> +					      sdkp->max_retries, &exec_args);
> +
> +		if ((the_result == 0) && (resid == RC16_LEN)) {

Same comment here: if (resid) ?

> +			/*
> +			 * if nothing was transferred, we try
> +			 * again. It's a workaround for a broken
> +			 * device.
> +			 */
> +			continue;
> +		}
> +		break;
> +	}
>  
>  	if (the_result > 0) {
>  		if (media_not_present(sdkp, &sshdr))
> @@ -2827,6 +2861,12 @@ static int read_capacity_10(struct scsi_disk *sdkp, struct scsi_device *sdp,
>  		return -EINVAL;
>  	}
>  
> +	if (resid == RC10_LEN) {
> +		sd_printk(KERN_ERR, sdkp,
> +			  "Read Capacity(10) returned good status but no data");
> +		return -EINVAL;
> +	}
> +
>  	sector_size = get_unaligned_be32(&buffer[4]);
>  	lba = get_unaligned_be32(&buffer[0]);
>  


-- 
Damien Le Moal
Western Digital Research