Re: [PATCH v5 2/8] scsi: scsi_debug: Split resp_write_same()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] scsi_debug: data race on sip->storep between unmap_region() and do_device_access()
--

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

scsi: scsi_debug: Split resp_write_same()

This commit splits resp_write_same() into two functions to ensure all locking
statements end up in the same branch of an if-statement. This change is
intended to support the Clang thread-safety analyzer.

> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index 44b4587b53c66..cadd94e17109a 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
[ ... ]
> -static int resp_write_same(struct scsi_cmnd *scp, u64 lba, u32 num,
> -			   u32 ei_lba, bool unmap, bool ndob)
> +static int __resp_write_same(struct scsi_cmnd *scp, u64 lba, u32 num,
> +			     u32 ei_lba, bool unmap, bool ndob, bool lbp)
>  {
>  	struct scsi_device *sdp = scp->device;
[ ... ]
>  	ret = check_device_access_params(scp, lba, num, true);
>  	if (ret)
> -		goto out;
> +		return ret;
>  
>  	if (unmap && lbp) {
>  		unmap_region(sip, lba, num);
> -		goto out;
> +		return ret;

[Severity: Medium]
Does this early return result in a data race on the sip->storep buffer?

When a SCSI UNMAP command (or WRITE SAME with the unmap bit set) and a READ
command execute concurrently on the same LBA range of an LBP-enabled device,
unmap_region() modifies the data store via memset().

By returning early here, __resp_write_same() only holds the
sdeb_meta_write_lock (acquired in the resp_write_same() caller) and bypasses
acquiring the sdeb_data_write_lock.

Concurrently, read operations like resp_read_dt0() -> do_device_access()
acquire only the sdeb_data_read_lock and skip the sdeb_meta_read_lock entirely
when running on non-zoned/non-DIX devices. 

Could this concurrent access to sip->storep result in torn or mixed reads?

[ ... ]

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