Re: [PATCH 5/7] scsi: scsi_debug: Split resp_write_dt0()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] scsi: scsi_debug: resp_write_dt0() incorrectly returns byte count instead of SCSI status
- [High] scsi: scsi_debug: resp_read_dt0() incorrectly returns byte count instead of SCSI status
- [Medium] scsi: scsi_debug: __resp_write_same() overwrites error return value with 0
--

commit 57d85f04864ae775754845a9070f7d8499a51930
Author: Bart Van Assche <bvanassche@acm.org>

scsi: scsi_debug: Split resp_write_dt0()
    
Prepare for enabling lock context analysis by eliminating conditional locking.

> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index ac3b9bfa7e634..ed30b3950c4e2 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -5032,6 +5032,55 @@ static int corrupt_lbas(struct sdebug_dev_info *devip, u64 lba, u32 num,
>  	return error;
>  }
>  
> +static int __resp_write_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip,
> +			    struct sdeb_store_info *sip, u64 lba, u32 num,
> +			    u32 ei_lba, u8 group)
> +{
> +	int ret;
> +
> +	ret = check_device_access_params(scp, lba, num, true);
> +	if (ret)
> +		return ret;
[ ... ]
> +	ret = do_device_access(sip, scp, 0, lba, num, group, true, false);
> +	if (unlikely(scsi_debug_lbp()))
> +		map_region(sip, lba, num);
> +
> +	/* If ZBC zone then bump its write pointer */
> +	if (sdebug_dev_is_zoned(devip))
> +		zbc_inc_wp(devip, lba, num);
> +
> +	return ret;
> +}
[ ... ]
> @@ -5110,59 +5158,18 @@ static int resp_write_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
>  
>  	if (sdebug_dev_is_zoned(devip) ||
>  	    (sdebug_dix && scsi_prot_sg_count(scp)) ||
> -	    scsi_debug_lbp())  {
> +	    scsi_debug_lbp()) {
>  		sdeb_meta_write_lock(sip);
> -		meta_data_locked = true;
> +		ret = __resp_write_dt0(scp, devip, sip, lba, num, ei_lba,
> +				       group);
> +		sdeb_meta_write_unlock(sip);
> +	} else {
> +		ret = __resp_write_dt0(scp, devip, sip, lba, num, ei_lba,
> +				       group);
>  	}
>  
> -	ret = check_device_access_params(scp, lba, num, true);
> -	if (ret) {
> -		if (meta_data_locked)
> -			sdeb_meta_write_unlock(sip);
> +	if (ret > 0)
>  		return ret;

[Severity: High]
Can this cause normal completion and error injection to be skipped?

In __resp_write_dt0(), do_device_access() returns a positive byte count on
success, which __resp_write_dt0() then returns.

By adding 'if (ret > 0) return ret;', resp_write_dt0() will return this byte
count rather than a valid SCSI status tuple, which skips the remaining
successful path logic.

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

scsi: scsi_debug: Split resp_read_dt0()
    
Split this function to remove conditional locking. This patch prepares
for enabling 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)
> +{
[ ... ]
> +	return do_device_access(sip, scp, 0, lba, num, 0, false, false);
> +}
[ ... ]
> @@ -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]
Is it possible that this intercepts the positive byte count and returns it
prematurely, similar to resp_write_dt0()?

The helper __resp_read_dt0() propagates the positive byte count out of
do_device_access() on success.

The caller's 'if (ret > 0) return ret;' condition intercepts this count and
returns it directly, which skips scsi_set_resid() and error injection.

commit 9e547db0a77591a35683cc5b28aeb6cb6d633c73
Author: Bart Van Assche <bvanassche@acm.org>

scsi: scsi_debug: Fix a locking bug in resp_write_same()
    
If fetch_to_dev_buffer() fails in resp_write_same(), the function jumps
to 'out' without releasing the data write lock acquired earlier via
sdeb_data_write_lock(). Jump to 'unlock' instead so that
sdeb_data_write_unlock() is called on the error path.

> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index 6941809dfdb75..b854102e6caa1 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -5402,7 +5402,7 @@ static int resp_write_same(struct scsi_cmnd *scp, u64 lba, u32 num,
>  
>  	if (-1 == ret) {
>  		ret = DID_ERROR << 16;
> -		goto out;
> +		goto unlock;
>  	} else if (sdebug_verbose && !ndob && (ret < lb_size))
>  		sdev_printk(KERN_INFO, scp->device,
>  			    "%s: %s: lb size=%u, IO sent=%d bytes\n",
> @@ -5419,6 +5419,7 @@ static int resp_write_same(struct scsi_cmnd *scp, u64 lba, u32 num,
>  	/* If ZBC zone then bump its write pointer */
>  	if (sdebug_dev_is_zoned(devip))
>  		zbc_inc_wp(devip, lba, num);
> +unlock:
>  	sdeb_data_write_unlock(sip);
>  	ret = 0;
>  out:

[Severity: Medium]
Will this change unintentionally overwrite the error return value with 0?

When fetch_to_dev_buffer() fails and returns -1, the error handling block sets
ret = DID_ERROR << 16 and jumps to unlock.

However, at the unlock label, ret is overwritten with 0 before returning,
masking the error.

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