Re: [PATCH v6 09/11] scsi: scsi_debug: Refuse a short WRITE ATOMIC (16) before writing it

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

On 9/24/26 20:21, Niklas Cassel wrote:
> A write whose data-out buffer is shorter than its transfer length is
> short. An ordinary write transfers what the buffer holds and reports the
> rest as a residual, but an atomic write cannot be short: SBC-6 r02
> (T10/BSR INCITS 587), 4.28.1, requires each atomic write operation to
> write either all of its data or none of it, and 4.28.2 requires one that
> cannot complete to leave the LBAs that it specifies unaltered.
> 
> resp_atomic_write() does fail a short WRITE ATOMIC (16) with DID_ERROR,
> but only after do_device_access() has returned, and do_device_access()
> copies one logical block at a time, so by then the blocks that the
> buffer did hold have been written. An eight block WRITE ATOMIC (16) with
> a buffer of four fails having overwritten the first four.
> 
> Check the length of the buffer before writing anything, and fail the
> command with the same DID_ERROR as before.
> 
> Assisted-by: LLM
> Fixes: 84f3a3c01d70 ("scsi: scsi_debug: Atomic write support")
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
> Tested with:
> 
>   modprobe scsi_debug sector_size=512 physblk_exp=3 dev_size_mb=128 \
>       atomic_wr=1 lbpu=1
> 
> issuing a WRITE ATOMIC (16) of eight blocks through SG_IO with a buffer
> of four blocks of 0xaa. It fails with DID_ERROR before and after this
> patch, but before it the first four blocks read back as 0xaa afterwards,
> and after it all eight read back as they were. A WRITE ATOMIC (16) with
> a full buffer writes all eight blocks, as before.
> 
> New in v6.
> ---
>  drivers/scsi/scsi_debug.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index af1b27b48867..3b112ad58489 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -6242,6 +6242,10 @@ static int resp_atomic_write(struct scsi_cmnd *scp,
>  		}
>  	}
>  
> +	/* A failed atomic write must not have written any of its data */

At this point, the write was not yet done, right? So this comment seems
incorrect, and does not really match the commit message explanation. What about
simply:

	/* Short atomic writes are not allowed. */

With that fixed,

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

> +	if (scsi_bufflen(scp) < len * sdebug_sector_size)
> +		return DID_ERROR << 16;
> +
>  	ret = do_device_access(sip, scp, 0, lba, len, 0, true, true);
>  	if (unlikely(ret == -1))
>  		return DID_ERROR << 16;


-- 
Damien Le Moal
Western Digital Research