[PATCH] sd: Fix REQ_OP_ZONE_REPORT completion handling

[PATCH] sd: Fix REQ_OP_ZONE_REPORT completion handling

[Masato Suzuki]

ZBC/ZAC report zones command may return less bytes than requested if the
number of matching zones for the report request is small. However, unlike
read or write commands, the remainder of incomplete report zones commands
cannot be automatically requested by the block layer: the start sector of
the next report cannot be known, and the report reply may not be 512B
aligned for SAS drives (a report zone reply size is always a multiple of
64B). The regular request completion code executing bio_advance() and
restart of the command remainder part currently causes invalid zone
descriptor data to be reported to the caller if the report zone size is
smaller than 512B (a case that can happen easily for a report of the last
zones of a SAS drive for example).

Since blkdev_report_zones() handles report zone command processing in a
loop until completion (no more zones are being reported), we can safely
avoid that the block layer performs an incorrect bio_advance() call and
restart of the remainder of incomplete report zone BIOs. To do so, always
indicate a full completion of REQ_OP_ZONE_REPORT by setting good_bytes to
the request buffer size and by setting the command resid to 0. This does
not affect the post processing of the report zone reply done by
sd_zbc_complete() since the reply header indicates the number of zones
reported.

Fixes: 89d947561077 ("sd: Implement support for ZBC devices")
Cc: <stable@vger.kernel.org> # 4.19
Cc: <stable@vger.kernel.org> # 4.14
Signed-off-by: Masato Suzuki <masato.suzuki@wdc.com>
---
 drivers/scsi/sd.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 2955b856e9ec..e8c2afbb82e9 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1981,9 +1981,13 @@ static int sd_done(struct scsi_cmnd *SCpnt)
 		}
 		break;
 	case REQ_OP_ZONE_REPORT:
+		/* To avoid that the block layer performs an incorrect
+		 * bio_advance() call and restart of the remainder of
+		 * incomplete report zone BIOs, always indicate a full
+		 * completion of REQ_OP_ZONE_REPORT.
+		 */
 		if (!result) {
-			good_bytes = scsi_bufflen(SCpnt)
-				- scsi_get_resid(SCpnt);
+			good_bytes = scsi_bufflen(SCpnt);
 			scsi_set_resid(SCpnt, 0);
 		} else {
 			good_bytes = 0;
-- 
2.24.1

Re: [PATCH] sd: Fix REQ_OP_ZONE_REPORT completion handling

[Damien Le Moal]

On 2020/01/27 14:07, Masato Suzuki wrote:
> ZBC/ZAC report zones command may return less bytes than requested if the
> number of matching zones for the report request is small. However, unlike
> read or write commands, the remainder of incomplete report zones commands
> cannot be automatically requested by the block layer: the start sector of
> the next report cannot be known, and the report reply may not be 512B
> aligned for SAS drives (a report zone reply size is always a multiple of
> 64B). The regular request completion code executing bio_advance() and
> restart of the command remainder part currently causes invalid zone
> descriptor data to be reported to the caller if the report zone size is
> smaller than 512B (a case that can happen easily for a report of the last
> zones of a SAS drive for example).
> 
> Since blkdev_report_zones() handles report zone command processing in a
> loop until completion (no more zones are being reported), we can safely
> avoid that the block layer performs an incorrect bio_advance() call and
> restart of the remainder of incomplete report zone BIOs. To do so, always
> indicate a full completion of REQ_OP_ZONE_REPORT by setting good_bytes to
> the request buffer size and by setting the command resid to 0. This does
> not affect the post processing of the report zone reply done by
> sd_zbc_complete() since the reply header indicates the number of zones
> reported.
> 
> Fixes: 89d947561077 ("sd: Implement support for ZBC devices")
> Cc: <stable@vger.kernel.org> # 4.19
> Cc: <stable@vger.kernel.org> # 4.14
> Signed-off-by: Masato Suzuki <masato.suzuki@wdc.com>

This bug exists since the beginning of SMR support in 4.10, until report
zones was changed to a device file method in kernel 4.20. It fell through
the cracks the entire time and was caught only recently with improvements
to our test suite.

Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>

> ---
>  drivers/scsi/sd.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 2955b856e9ec..e8c2afbb82e9 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -1981,9 +1981,13 @@ static int sd_done(struct scsi_cmnd *SCpnt)
>  		}
>  		break;
>  	case REQ_OP_ZONE_REPORT:
> +		/* To avoid that the block layer performs an incorrect
> +		 * bio_advance() call and restart of the remainder of
> +		 * incomplete report zone BIOs, always indicate a full
> +		 * completion of REQ_OP_ZONE_REPORT.
> +		 */
>  		if (!result) {
> -			good_bytes = scsi_bufflen(SCpnt)
> -				- scsi_get_resid(SCpnt);
> +			good_bytes = scsi_bufflen(SCpnt);
>  			scsi_set_resid(SCpnt, 0);
>  		} else {
>  			good_bytes = 0;
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] sd: Fix REQ_OP_ZONE_REPORT completion handling

[Martin K. Petersen]

Damien,

>> Fixes: 89d947561077 ("sd: Implement support for ZBC devices")
>> Cc: <stable@vger.kernel.org> # 4.19
>> Cc: <stable@vger.kernel.org> # 4.14
>> Signed-off-by: Masato Suzuki <masato.suzuki@wdc.com>
>
> This bug exists since the beginning of SMR support in 4.10, until report
> zones was changed to a device file method in kernel 4.20. It fell through
> the cracks the entire time and was caught only recently with improvements
> to our test suite.

Looks good to me. Obviously only applies to stable since, as you point
out, this code has been substantially reworked.

Acked-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] sd: Fix REQ_OP_ZONE_REPORT completion handling

[Greg Kroah-Hartman]

On Mon, Jan 27, 2020 at 11:01:50PM -0500, Martin K. Petersen wrote:
> 
> Damien,
> 
> >> Fixes: 89d947561077 ("sd: Implement support for ZBC devices")
> >> Cc: <stable@vger.kernel.org> # 4.19
> >> Cc: <stable@vger.kernel.org> # 4.14
> >> Signed-off-by: Masato Suzuki <masato.suzuki@wdc.com>
> >
> > This bug exists since the beginning of SMR support in 4.10, until report
> > zones was changed to a device file method in kernel 4.20. It fell through
> > the cracks the entire time and was caught only recently with improvements
> > to our test suite.
> 
> Looks good to me. Obviously only applies to stable since, as you point
> out, this code has been substantially reworked.
> 
> Acked-by: Martin K. Petersen <martin.petersen@oracle.com>

Thanks for the review, now queued up.

greg k-h