Re: [PATCH v3 02/57] scsi: scsi_debug: Prepare for enabling lock context analysis

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] The `__context_unsafe(conditional locking)` annotation was applied to `resp_write_same` but missed on `resp_read_dt0` and `resp_write_dt0`, which use identical conditional locking logic.

Pre-existing issues:
- [High] The function `resp_write_same()` leaks a write lock on the data lock (`sip->macc_data_lck`) when `fetch_to_dev_buffer()` fails.
- [High] AB-BA deadlock between `sip->macc_meta_lck` and `sip->macc_data_lck` due to inconsistent lock acquisition order.
--

--- Patch [2]: [PATCH v3 02/57] scsi: scsi_debug: Prepare for enabling lock context analysis ---
commit ad9c0fb7632790d874892a1d6261443b39766d7d
Author: Bart Van Assche <bvanassche@acm.org>

scsi: scsi_debug: Prepare for enabling lock context analysis

Suppress lock context analysis for the functions that perform conditional
locking to prevent that the Clang thread-safety analyzer complains about
these functions. Remove incorrect __acquire() and __release() annotations.

> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index 4a08f0cfae08d..cb6663e24c225 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c

[ ... ]

> @@ -5193,6 +5185,7 @@ static int resp_write_scat(struct scsi_cmnd *scp,
>  
>  static int resp_write_same(struct scsi_cmnd *scp, u64 lba, u32 num,
>  			   u32 ei_lba, bool unmap, bool ndob)
> +	__context_unsafe(conditional locking)
>  {
>  	struct scsi_device *sdp = scp->device;
>  	struct sdebug_dev_info *devip = (struct sdebug_dev_info *)sdp->hostdata;

[Severity: Medium]
Should drivers/scsi/scsi_debug.c:resp_read_dt0() and resp_write_dt0() also
get this annotation?

They appear to use the same conditional locking pattern with
meta_data_locked, conditionally acquiring sip->macc_meta_lck.

[Severity: High]
This is a pre-existing issue, but does drivers/scsi/scsi_debug.c:resp_write_same()
leak the sip->macc_data_lck write lock when fetch_to_dev_buffer() fails?

If fetch_to_dev_buffer() returns -1, the code jumps to the out label:

	sdeb_data_write_lock(sip);
	if (ndob) {
		memset(fs1p, 0, lb_size);
		ret = 0;
	} else
		ret = fetch_to_dev_buffer(scp, fs1p, lb_size);

	if (-1 == ret) {
		ret = DID_ERROR << 16;
		goto out;
	}

But the out label only unlocks sip->macc_meta_lck and skips
sdeb_data_write_unlock():

out:
	if (meta_data_locked)
		sdeb_meta_write_unlock(sip);
	return ret;

[Severity: High]
This is also a pre-existing issue, but could drivers/scsi/scsi_debug.c:resp_write_same()
cause a lock ordering deadlock with resp_comp_write()?

In resp_write_same(), the meta lock is acquired first, followed by the
data lock:

	if (sdebug_dev_is_zoned(devip) || scsi_debug_lbp()) {
		sdeb_meta_write_lock(sip);
		meta_data_locked = true;
	}
...
	sdeb_data_write_lock(sip);

However, in resp_comp_write(), the acquisition order is reversed:

	sdeb_data_write_lock(sip);
	sdeb_meta_write_lock(sip);

Could this lead to a deadlock if a compare and write command and a write same
command are processed concurrently on the same store?

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