Re: [PATCH v9 05/11] scsi: scsi_debug: Report the residual of a write

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] scsi: scsi_debug: incorrect residual for WRITE SCATTERED due to 32-bit overflow
--

commit 8bc6d8e408815c5b6d6afe8b3e8dbdaabb79d3d5
Author: Niklas Cassel <cassel@kernel.org>
scsi: scsi_debug: Report the residual of a write

This commit adds residual reporting for writes in scsi_debug, ensuring
that writes with data buffers larger than their transfer length report
the residual correctly, similar to reads. It updates resp_write_dt0(),
resp_write_scat(), and resp_atomic_write() to calculate and set the
residual based on what the command consumed.

> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index b64ae3ad300d8..0c66a3118ee89 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c

[ ... ]

> @@ -5352,6 +5358,13 @@ static int resp_write_scat(struct scsi_cmnd *scp,
>  	}
>  	ret = 0;
>  err_out_unlock:
> +	/*
> +	 * sg_off counts what the command asked for, which can exceed the
> +	 * buffer: lbdof is not validated against it, and a range is counted
> +	 * in full even if do_device_access() copied less.
> +	 */
> +	if (scsi_bufflen(scp) > sg_off)
> +		scsi_set_resid(scp, scsi_bufflen(scp) - sg_off);

[Severity: Medium]
Will this calculation produce an incorrect residual if the command includes
a very large LBA range?

In resp_write_scat(), the variables num_by, num, and lb_size are all 32-bit
integers, and num_by is calculated as:

    num_by = num * lb_size;

If num is sufficiently large (e.g., > 8.38M blocks), does this 32-bit
multiplication overflow and wrap around to a small value?

If num_by wraps, it appears the sg_off accumulator will also wrap below the
actual buffer length, causing this new guard if (scsi_bufflen(scp) > sg_off)
to erroneously evaluate to true. Could this result in a bogus residual being
set for a write that actually fully consumed the buffer, potentially
confusing the initiator or midlayer into failing or retrying completed I/Os?

>  	sdeb_meta_write_unlock(sip);
>  err_out:
>  	kfree(lrdp);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260927052650.567035-13-cassel@kernel.org?part=5