Re: [PATCH 4/7] scsi: scsi_debug: Split resp_read_dt0()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi_debug: early return of byte count in resp_read_dt0 fails IO
--

commit 17ea9421189072b59343f9ec94fa5ff956adb17f
Author: Bart Van Assche <bvanassche@acm.org>

scsi: scsi_debug: Split resp_read_dt0()

This commit splits the `resp_read_dt0()` function to eliminate conditional
locking and prepare the code for context analysis.

> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index fcfb49df9ee17..ac3b9bfa7e634 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -4577,6 +4577,42 @@ static int resp_read_tape(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
>  	return 0;
>  }
>  
> +static int __resp_read_dt0(struct scsi_cmnd *scp, struct sdeb_store_info *sip,
> +			   u64 lba, u32 num, u32 ei_lba)
> +{
> +	u8 *cmd = scp->cmnd;
> +
> +	/* DIX + T10 DIF */
> +	if (unlikely(sdebug_dix && scsi_prot_sg_count(scp))) {
> +		switch (prot_verify_read(scp, lba, num, ei_lba)) {

[ ... ]

> +		}
> +	}
> +
> +	return do_device_access(sip, scp, 0, lba, num, 0, false, false);
> +}

[Severity: High]
Does returning the output of do_device_access() here cause successful reads
to return their byte count instead of zero?

Looking at do_device_access(), it returns the positive number of bytes copied
on success. Before this patch, the positive byte count was returned into
'ret', but it was ignored for successful reads.

>  static int resp_read_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
>  {
>  	bool check_prot;
> @@ -4668,46 +4703,17 @@ static int resp_read_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
>  	}
>  
>  	if (sdebug_dev_is_zoned(devip) ||
> -	    (sdebug_dix && scsi_prot_sg_count(scp)))  {
> +	    (sdebug_dix && scsi_prot_sg_count(scp))) {
>  		sdeb_meta_read_lock(sip);
> -		meta_data_locked = true;
> +		ret = __resp_read_dt0(scp, sip, lba, num, ei_lba);
> +		sdeb_meta_read_unlock(sip);
> +	} else {
> +		ret = __resp_read_dt0(scp, sip, lba, num, ei_lba);
>  	}
>  
> +	if (ret > 0)
> +		return ret;

[Severity: High]
Could this new check inadvertently cause successful IOs to fail?

Since __resp_read_dt0() returns the positive byte count on success
(e.g., 65536 bytes), this condition will evaluate to true. The function
will then return 65536 (0x10000) directly to the caller, bypassing
scsi_set_resid().

The SCSI midlayer appears to interpret a positive return value like 0x10000
as a SCSI result where the host byte is DID_NO_CONNECT (1), leading to a
complete failure of the IO operation.

(I verified that this issue remains present in the final commit of the series,
and the subsequent patch for writes also introduces the identical behavior in
resp_write_dt0().)

>  
> -	ret = do_device_access(sip, scp, 0, lba, num, 0, false, false);
> -	if (meta_data_locked)
> -		sdeb_meta_read_unlock(sip);
>  	if (unlikely(ret == -1))
>  		return DID_ERROR << 16;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1790119506.git.bvanassche@acm.org?part=4