[PATCH v5 0/8] scsi_debug: Enable lock context analysis

[PATCH v5 0/8] scsi_debug: Enable lock context analysis

[Bart Van Assche]

Hi Martin,

This patch series enables compiler-based lock context analysis for the
scsi_debug driver. Conditional locking is eliminated across command response
and error injection functions so that static analysis can verify lock
acquisitions and releases. Internal locking helper functions are annotated
with __acquires(),__releases(), __acquires_shared() and __releases_shared()
attributes. An error-path lock leak in resp_write_same() is fixed. Finally,
lock context analysis is enabled in drivers/scsi/Makefile.

Please consider this patch series for the next merge window.

Thanks,

Bart.

Changes compared to v4:
- Add a Reviewed-by tag from Christoph Hellwig to patch 6/8.
- Drop __context_unsafe() from sdeb_read_lock(), sdeb_read_unlock(),
  sdeb_write_lock(), and sdeb_write_unlock() in patch 7/8. Preserve the
  original if (sdebug_no_rwlock) conditional structure and annotate with
  __acquire_shared(), __release_shared(), __acquire(), and __release().
- Drop __context_unsafe() from sdeb_data_lock(), sdeb_data_unlock(),
  sdeb_data_sector_lock(), and sdeb_data_sector_unlock() in patch 7/8,
  and add explicit context capability transitions on the read paths.
- Retain __context_unsafe() exclusively for
  sdeb_meta_{read,write}_{,un}lock() to prevent undefined pointer
  arithmetic when sip is NULL under fake_rw.
- Update the commit description of patch 7/8 to reflect these changes.

Changes compared to v3:
- Add a new patch to split resp_atomic_write() and eliminate conditional
  locking in atomic write handling.
- Pass the LBP state boolean down into __resp_write_same(),
  __corrupt_lbas(), and __resp_write_dt0() to avoid repeated calls to
  scsi_debug_lbp().
- Drop unused function arguments from __resp_read_dt0_dix() and
  __resp_write_dt0_dix().
- Correct the return value descriptions in the header comments of
  __resp_read_dt0_dix() and __resp_write_dt0_dix().

Changes compared to v2:
- Move DIX/DIF verification into a helper function (__resp_read_dt0_dix())
  to reduce indentation.
- Remove unnecessary "else" branches after "return".
- Set the SCSI status code only if do_device_access() fails instead of
  unconditionally setting it to DID_ERROR << 16 beforehand.

Changes compared to v1:
- Fix an issue where the error code was clobbered in resp_write_same() and
  __resp_write_same() when fetch_to_dev_buffer() fails (John Garry and
  sashiko-bot).
- Fix return value handling in resp_read_dt0() and resp_write_dt0():
  previously a positive byte count returned by do_device_access() was
  incorrectly treated as a SCSI status code and returned to the caller.
  Now, pass the SCSI status via an output pointer and only trigger an error
  return on negative return values (sashiko-bot).
- Improve lock context annotations:
  * Annotate existing rwlock_t members (&sip->macc_data_lck,
    &sip->macc_sector_lck, &sip->macc_meta_lck) directly with __acquires /
    __releases instead of defining and embedding dummy context lock structs
    in struct sdeb_store_info.
  * Drop the unwarranted __context_unsafe() annotation from
    sdeb_data_read_lock().
  * Update the commit descriptions to match the code changes.

Bart Van Assche (8):
  scsi: scsi_debug: Fix a locking bug in resp_write_same()
  scsi: scsi_debug: Split resp_write_same()
  scsi: scsi_debug: Split corrupt_lbas()
  scsi: scsi_debug: Split resp_read_dt0()
  scsi: scsi_debug: Split resp_write_dt0()
  scsi: scsi_debug: Split resp_atomic_write()
  scsi: scsi_debug: Improve lock context annotations
  scsi: core: Enable lock context analysis for the scsi_debug driver

 drivers/scsi/Makefile     |   1 +
 drivers/scsi/scsi_debug.c | 426 +++++++++++++++++++++++---------------
 2 files changed, 264 insertions(+), 163 deletions(-)

[PATCH v5 1/8] scsi: scsi_debug: Fix a locking bug in resp_write_same()

[Bart Van Assche]

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. This bug has been
discovered by building the scsi_debug driver with Clang and modified
lock context annotations. The modified lock context annotations are
available in patch "scsi: scsi_debug: Improve lock context annotations".

Fixes: 84f3a3c01d70 ("scsi: scsi_debug: Atomic write support")
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: John Garry <john.garry@linux.dev>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_debug.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index e679df1993aa..44b4587b53c6 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -5455,7 +5455,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",
@@ -5472,8 +5472,9 @@ 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);
-	sdeb_data_write_unlock(sip);
 	ret = 0;
+unlock:
+	sdeb_data_write_unlock(sip);
 out:
 	if (meta_data_locked)
 		sdeb_meta_write_unlock(sip);

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

[Bart Van Assche]

Since the Clang thread-safety analyzer does not support conditional
locking, split the function resp_write_same() such that all locking
statements end up in the same branch of an if-statement.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: John Garry <john.garry@linux.dev>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_debug.c | 38 +++++++++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 13 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 44b4587b53c6..cadd94e17109 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -5412,8 +5412,8 @@ static int resp_write_scat(struct scsi_cmnd *scp,
 	return ret;
 }
 
-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;
 	struct sdebug_dev_info *devip = (struct sdebug_dev_info *)sdp->hostdata;
@@ -5425,21 +5425,14 @@ static int resp_write_same(struct scsi_cmnd *scp, u64 lba, u32 num,
 						scp->device->hostdata, true);
 	u8 *fs1p;
 	u8 *fsp;
-	bool meta_data_locked = false;
-	bool lbp = scsi_debug_lbp();
-
-	if (sdebug_dev_is_zoned(devip) || lbp) {
-		sdeb_meta_write_lock(sip);
-		meta_data_locked = true;
-	}
 
 	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;
 	}
 	lbaa = lba;
 	block = do_div(lbaa, sdebug_store_sectors);
@@ -5475,9 +5468,28 @@ static int resp_write_same(struct scsi_cmnd *scp, u64 lba, u32 num,
 	ret = 0;
 unlock:
 	sdeb_data_write_unlock(sip);
-out:
-	if (meta_data_locked)
+	return ret;
+}
+
+static int resp_write_same(struct scsi_cmnd *scp, u64 lba, u32 num, u32 ei_lba,
+			   bool unmap, bool ndob)
+{
+	struct scsi_device *sdp = scp->device;
+	struct sdebug_dev_info *devip = sdp->hostdata;
+	struct sdeb_store_info *sip = devip2sip(devip, true);
+	const bool lbp = scsi_debug_lbp();
+	int ret;
+
+	if (sdebug_dev_is_zoned(devip) || lbp) {
+		sdeb_meta_write_lock(sip);
+		ret = __resp_write_same(scp, lba, num, ei_lba, unmap, ndob,
+					lbp);
 		sdeb_meta_write_unlock(sip);
+	} else {
+		ret = __resp_write_same(scp, lba, num, ei_lba, unmap, ndob,
+					lbp);
+	}
+
 	return ret;
 }
 

[PATCH v5 3/8] scsi: scsi_debug: Split corrupt_lbas()

[Bart Van Assche]

Move the inner logic of corrupt_lbas() into a new helper function
__corrupt_lbas() and call it from separate locked and unlocked branches
in corrupt_lbas(). This patch prepares for enabling context analysis.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: John Garry <john.garry@linux.dev>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_debug.c | 42 +++++++++++++++++++++------------------
 1 file changed, 23 insertions(+), 19 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index cadd94e17109..eafcadc6ef07 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -4984,37 +4984,25 @@ static int resp_write_tape(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 	return 0;
 }
 
-static int corrupt_lbas(struct sdebug_dev_info *devip, u64 lba, u32 num,
-		u32 nr_bit_errors, s32 reftag_adjust)
+static int __corrupt_lbas(struct sdeb_store_info *sip, u64 lba, u32 num,
+			  u32 nr_bit_errors, s32 reftag_adjust, bool lbp)
 {
-	struct sdeb_store_info *sip = devip2sip(devip, false);
-	bool meta_data_locked = false;
-	bool lbp = scsi_debug_lbp();
 	u32 block, num_mapped, b, i;
-	int error = 0;
-
-	if (sdebug_dev_is_zoned(devip) || sdebug_dix || lbp) {
-		sdeb_meta_write_lock(sip);
-		meta_data_locked = true;
-	}
 
 	if (!sip) {
 		pr_err("can't corrupt with fake_rw\n");
-		error = -EINVAL;
-		goto out_unlock;
+		return -EINVAL;
 	}
 
 	if (num > sdebug_capacity || lba > sdebug_capacity - num) {
 		pr_err("logical blocks out of bounds: %llu:%u", lba, num);
-		error = -EINVAL;
-		goto out_unlock;
+		return -EINVAL;
 	}
 
 	if (lbp && (!map_state(sip, lba, &num_mapped) || num > num_mapped)) {
 		pr_err("can't modify unmapped logical blocks: %llu:%u",
 			lba, num);
-		error = -EINVAL;
-		goto out_unlock;
+		return -EINVAL;
 	}
 
 	/*
@@ -5052,9 +5040,25 @@ static int corrupt_lbas(struct sdebug_dev_info *devip, u64 lba, u32 num,
 	}
 	sdeb_data_unlock(sip, false);
 
-out_unlock:
-	if (meta_data_locked)
+	return 0;
+}
+
+static int corrupt_lbas(struct sdebug_dev_info *devip, u64 lba, u32 num,
+			u32 nr_bit_errors, s32 reftag_adjust)
+{
+	struct sdeb_store_info *sip = devip2sip(devip, false);
+	const bool lbp = scsi_debug_lbp();
+	int error = 0;
+
+	if (sdebug_dev_is_zoned(devip) || sdebug_dix || lbp) {
+		sdeb_meta_write_lock(sip);
+		error = __corrupt_lbas(sip, lba, num, nr_bit_errors,
+				       reftag_adjust, lbp);
 		sdeb_meta_write_unlock(sip);
+	} else {
+		error = __corrupt_lbas(sip, lba, num, nr_bit_errors,
+				       reftag_adjust, lbp);
+	}
 	return error;
 }
 

[PATCH v5 4/8] scsi: scsi_debug: Split resp_read_dt0()

[Bart Van Assche]

Split this function to remove conditional locking. This patch prepares
for enabling context analysis.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_debug.c | 110 ++++++++++++++++++++++++--------------
 1 file changed, 71 insertions(+), 39 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index eafcadc6ef07..c66f8556896d 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -4608,6 +4608,70 @@ static int resp_read_tape(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 	return 0;
 }
 
+/*
+ * Returns 0 upon success or -1 with *scsi_status set to a SCSI status code upon
+ * failure.
+ */
+static int __resp_read_dt0_dix(struct scsi_cmnd *scp, u64 lba, u32 num,
+			       u32 ei_lba, int *scsi_status)
+{
+	const u8 *const cmd = scp->cmnd;
+
+	switch (prot_verify_read(scp, lba, num, ei_lba)) {
+	case 1: /* Guard tag error */
+		if (cmd[1] >> 5 != 3) { /* RDPROTECT != 3 */
+			mk_sense_buffer(scp, ABORTED_COMMAND,
+					LOGICAL_BLOCK_GUARD_CHECK_FAILED);
+			*scsi_status = check_condition_result;
+			return -1;
+		}
+		if (scp->prot_flags & SCSI_PROT_GUARD_CHECK) {
+			mk_sense_buffer(scp, ILLEGAL_REQUEST,
+					LOGICAL_BLOCK_GUARD_CHECK_FAILED);
+			*scsi_status = illegal_condition_result;
+			return -1;
+		}
+		break;
+	case 3: /* Reference tag error */
+		if (cmd[1] >> 5 != 3) { /* RDPROTECT != 3 */
+			mk_sense_buffer(scp, ABORTED_COMMAND,
+				LOGICAL_BLOCK_REFERENCE_TAG_CHECK_FAILED);
+			*scsi_status = check_condition_result;
+			return -1;
+		}
+		if (scp->prot_flags & SCSI_PROT_REF_CHECK) {
+			mk_sense_buffer(scp, ILLEGAL_REQUEST,
+				LOGICAL_BLOCK_REFERENCE_TAG_CHECK_FAILED);
+			*scsi_status = illegal_condition_result;
+			return -1;
+		}
+		break;
+	}
+	return 0;
+}
+
+/*
+ * Returns the number of bytes transferred, or -1 with *scsi_status set to a
+ * SCSI status code on failure.
+ */
+static int __resp_read_dt0(struct scsi_cmnd *scp, struct sdeb_store_info *sip,
+			   u64 lba, u32 num, u32 ei_lba, int *scsi_status)
+{
+	int ret;
+
+	/* DIX + T10 DIF */
+	if (unlikely(sdebug_dix && scsi_prot_sg_count(scp))) {
+		ret = __resp_read_dt0_dix(scp, lba, num, ei_lba, scsi_status);
+		if (ret < 0)
+			return ret;
+	}
+
+	ret = do_device_access(sip, scp, 0, lba, num, 0, false, false);
+	if (ret < 0)
+		*scsi_status = DID_ERROR << 16;
+	return ret;
+}
+
 static int resp_read_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 {
 	bool check_prot;
@@ -4617,7 +4681,7 @@ static int resp_read_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 	u64 lba;
 	struct sdeb_store_info *sip = devip2sip(devip, true);
 	u8 *cmd = scp->cmnd;
-	bool meta_data_locked = false;
+	int scsi_status;
 
 	switch (cmd[0]) {
 	case READ_16:
@@ -4699,48 +4763,16 @@ 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;
-	}
-
-	/* DIX + T10 DIF */
-	if (unlikely(sdebug_dix && scsi_prot_sg_count(scp))) {
-		switch (prot_verify_read(scp, lba, num, ei_lba)) {
-		case 1: /* Guard tag error */
-			if (cmd[1] >> 5 != 3) { /* RDPROTECT != 3 */
-				sdeb_meta_read_unlock(sip);
-				mk_sense_buffer(scp, ABORTED_COMMAND,
-					LOGICAL_BLOCK_GUARD_CHECK_FAILED);
-				return check_condition_result;
-			} else if (scp->prot_flags & SCSI_PROT_GUARD_CHECK) {
-				sdeb_meta_read_unlock(sip);
-				mk_sense_buffer(scp, ILLEGAL_REQUEST,
-					LOGICAL_BLOCK_GUARD_CHECK_FAILED);
-				return illegal_condition_result;
-			}
-			break;
-		case 3: /* Reference tag error */
-			if (cmd[1] >> 5 != 3) { /* RDPROTECT != 3 */
-				sdeb_meta_read_unlock(sip);
-				mk_sense_buffer(scp, ABORTED_COMMAND,
-				      LOGICAL_BLOCK_REFERENCE_TAG_CHECK_FAILED);
-				return check_condition_result;
-			} else if (scp->prot_flags & SCSI_PROT_REF_CHECK) {
-				sdeb_meta_read_unlock(sip);
-				mk_sense_buffer(scp, ILLEGAL_REQUEST,
-				      LOGICAL_BLOCK_REFERENCE_TAG_CHECK_FAILED);
-				return illegal_condition_result;
-			}
-			break;
-		}
+		ret = __resp_read_dt0(scp, sip, lba, num, ei_lba, &scsi_status);
+		sdeb_meta_read_unlock(sip);
+	} else {
+		ret = __resp_read_dt0(scp, sip, lba, num, ei_lba, &scsi_status);
 	}
 
-	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;
+		return scsi_status;
 
 	scsi_set_resid(scp, scsi_bufflen(scp) - ret);
 

[PATCH v5 5/8] scsi: scsi_debug: Split resp_write_dt0()

[Bart Van Assche]

Prepare for enabling lock context analysis by eliminating conditional
locking.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_debug.c | 139 ++++++++++++++++++++++++--------------
 1 file changed, 87 insertions(+), 52 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index c66f8556896d..685949aedf9a 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -5094,6 +5094,85 @@ static int corrupt_lbas(struct sdebug_dev_info *devip, u64 lba, u32 num,
 	return error;
 }
 
+/*
+ * Returns 0 upon success or -1 with *scsi_status set to a SCSI status code upon
+ * failure.
+ */
+static int __resp_write_dt0_dix(struct scsi_cmnd *scp,
+				u64 lba, u32 num,
+				u32 ei_lba, int *scsi_status)
+{
+	switch (prot_verify_write(scp, lba, num, ei_lba)) {
+	case 1: /* Guard tag error */
+		if (scp->prot_flags & SCSI_PROT_GUARD_CHECK) {
+			mk_sense_buffer(scp, ILLEGAL_REQUEST,
+					LOGICAL_BLOCK_GUARD_CHECK_FAILED);
+			*scsi_status = illegal_condition_result;
+			return -1;
+		}
+		if (scp->cmnd[1] >> 5 != 3) { /* WRPROTECT != 3 */
+			mk_sense_buffer(scp, ABORTED_COMMAND,
+					LOGICAL_BLOCK_GUARD_CHECK_FAILED);
+			*scsi_status = check_condition_result;
+			return -1;
+		}
+		break;
+	case 3: /* Reference tag error */
+		if (scp->prot_flags & SCSI_PROT_REF_CHECK) {
+			mk_sense_buffer(scp, ILLEGAL_REQUEST,
+				LOGICAL_BLOCK_REFERENCE_TAG_CHECK_FAILED);
+			*scsi_status = illegal_condition_result;
+			return -1;
+		}
+		if (scp->cmnd[1] >> 5 != 3) { /* WRPROTECT != 3 */
+			mk_sense_buffer(scp, ABORTED_COMMAND,
+				LOGICAL_BLOCK_REFERENCE_TAG_CHECK_FAILED);
+			*scsi_status = check_condition_result;
+			return -1;
+		}
+		break;
+	}
+
+	return 0;
+}
+
+/*
+ * Returns the number of bytes transferred, or -1 with *scsi_status set to a
+ * SCSI status code on failure.
+ */
+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, bool lbp, int *scsi_status)
+{
+	int ret;
+
+	*scsi_status = check_device_access_params(scp, lba, num, true);
+	if (*scsi_status)
+		return -1;
+
+	/* DIX + T10 DIF */
+	if (unlikely(sdebug_dix && scsi_prot_sg_count(scp))) {
+		ret = __resp_write_dt0_dix(scp, lba, num, ei_lba,
+					   scsi_status);
+		if (ret < 0)
+			return ret;
+	}
+
+	ret = do_device_access(sip, scp, 0, lba, num, group, true, false);
+	if (ret < 0)
+		*scsi_status = DID_ERROR << 16;
+
+	if (unlikely(lbp))
+		map_region(sip, lba, num);
+
+	/* If ZBC zone then bump its write pointer */
+	if (sdebug_dev_is_zoned(devip) && ret > 0)
+		zbc_inc_wp(devip, lba, ret >> ilog2(sdebug_sector_size));
+
+	return ret;
+}
+
 static int resp_write_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 {
 	bool check_prot;
@@ -5104,7 +5183,7 @@ static int resp_write_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 	u64 lba;
 	struct sdeb_store_info *sip = devip2sip(devip, true);
 	u8 *cmd = scp->cmnd;
-	bool meta_data_locked = false;
+	int scsi_status = DID_ERROR << 16;
 	bool lbp = scsi_debug_lbp();
 
 	if (unlikely(sdebug_opts & SDEBUG_OPT_UNALIGNED_WRITE &&
@@ -5174,60 +5253,16 @@ 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)) || lbp) {
 		sdeb_meta_write_lock(sip);
-		meta_data_locked = true;
-	}
-
-	ret = check_device_access_params(scp, lba, num, true);
-	if (ret) {
-		if (meta_data_locked)
-			sdeb_meta_write_unlock(sip);
-		return ret;
-	}
-
-	/* DIX + T10 DIF */
-	if (unlikely(sdebug_dix && scsi_prot_sg_count(scp))) {
-		switch (prot_verify_write(scp, lba, num, ei_lba)) {
-		case 1: /* Guard tag error */
-			if (scp->prot_flags & SCSI_PROT_GUARD_CHECK) {
-				sdeb_meta_write_unlock(sip);
-				mk_sense_buffer(scp, ILLEGAL_REQUEST,
-					LOGICAL_BLOCK_GUARD_CHECK_FAILED);
-				return illegal_condition_result;
-			} else if (scp->cmnd[1] >> 5 != 3) { /* WRPROTECT != 3 */
-				sdeb_meta_write_unlock(sip);
-				mk_sense_buffer(scp, ABORTED_COMMAND,
-					LOGICAL_BLOCK_GUARD_CHECK_FAILED);
-				return check_condition_result;
-			}
-			break;
-		case 3: /* Reference tag error */
-			if (scp->prot_flags & SCSI_PROT_REF_CHECK) {
-				sdeb_meta_write_unlock(sip);
-				mk_sense_buffer(scp, ILLEGAL_REQUEST,
-					LOGICAL_BLOCK_REFERENCE_TAG_CHECK_FAILED);
-				return illegal_condition_result;
-			} else if (scp->cmnd[1] >> 5 != 3) { /* WRPROTECT != 3 */
-				sdeb_meta_write_unlock(sip);
-				mk_sense_buffer(scp, ABORTED_COMMAND,
-					LOGICAL_BLOCK_REFERENCE_TAG_CHECK_FAILED);
-				return check_condition_result;
-			}
-			break;
-		}
-	}
-
-	ret = do_device_access(sip, scp, 0, lba, num, group, true, false);
-	if (unlikely(lbp))
-		map_region(sip, lba, num);
-
-	/* If ZBC zone then bump its write pointer over the data written */
-	if (sdebug_dev_is_zoned(devip) && ret > 0)
-		zbc_inc_wp(devip, lba, ret >> ilog2(sdebug_sector_size));
-	if (meta_data_locked)
+		ret = __resp_write_dt0(scp, devip, sip, lba, num, ei_lba, group,
+				       lbp, &scsi_status);
 		sdeb_meta_write_unlock(sip);
+	} else {
+		ret = __resp_write_dt0(scp, devip, sip, lba, num, ei_lba, group,
+				       lbp, &scsi_status);
+	}
 
 	if (unlikely(-1 == ret))
-		return DID_ERROR << 16;
+		return scsi_status;
 	else if (unlikely(sdebug_verbose &&
 			  (ret < (num * sdebug_sector_size))))
 		sdev_printk(KERN_INFO, scp->device,

[PATCH v5 6/8] scsi: scsi_debug: Split resp_atomic_write()

[Bart Van Assche]

Prepare for enabling lock context analysis by eliminating conditional
locking.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_debug.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 685949aedf9a..244250ba460b 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -6352,13 +6352,13 @@ static int resp_atomic_write(struct scsi_cmnd *scp,
 	if (scsi_bufflen(scp) < len * sdebug_sector_size)
 		return DID_ERROR << 16;
 
-	if (lbp)
-		sdeb_meta_write_lock(sip);
-
-	ret = do_device_access(sip, scp, 0, lba, len, 0, true, true);
 	if (lbp) {
+		sdeb_meta_write_lock(sip);
+		ret = do_device_access(sip, scp, 0, lba, len, 0, true, true);
 		map_region(sip, lba, len);
 		sdeb_meta_write_unlock(sip);
+	} else {
+		ret = do_device_access(sip, scp, 0, lba, len, 0, true, true);
 	}
 
 	if (unlikely(ret == -1))

[PATCH v5 7/8] scsi: scsi_debug: Improve lock context annotations

[Bart Van Assche]

Prepare the scsi_debug driver for enabling compiler-based lock context
analysis:
- Annotate the lock and unlock helper functions with __acquires,
  __releases, __acquires_shared, and __releases_shared.
- Add __acquire() and __release() calls in the paths that do not match
  the __acquires() / __releases() annotations.
- Annotate sdeb_meta_{read,write}_{,un}lock() with __context_unsafe()
  because adding __acquire() and __release() calls in these functions is
  not possible without triggering undefined behavior (taking the address
  of a member of a structure with a NULL pointer as base address).

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_debug.c | 84 +++++++++++++++++++++++----------------
 1 file changed, 50 insertions(+), 34 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 244250ba460b..1ca3a2dc7358 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -4052,24 +4052,27 @@ static inline struct sdeb_store_info *devip2sip(struct sdebug_dev_info *devip,
 
 static inline void
 sdeb_read_lock(rwlock_t *lock)
+	__acquires_shared(lock)
 {
 	if (sdebug_no_rwlock)
-		__acquire(lock);
+		__acquire_shared(lock);
 	else
 		read_lock(lock);
 }
 
 static inline void
 sdeb_read_unlock(rwlock_t *lock)
+	__releases_shared(lock)
 {
 	if (sdebug_no_rwlock)
-		__release(lock);
+		__release_shared(lock);
 	else
 		read_unlock(lock);
 }
 
 static inline void
 sdeb_write_lock(rwlock_t *lock)
+	__acquires(lock)
 {
 	if (sdebug_no_rwlock)
 		__acquire(lock);
@@ -4079,6 +4082,7 @@ sdeb_write_lock(rwlock_t *lock)
 
 static inline void
 sdeb_write_unlock(rwlock_t *lock)
+	__releases(lock)
 {
 	if (sdebug_no_rwlock)
 		__release(lock);
@@ -4088,6 +4092,7 @@ sdeb_write_unlock(rwlock_t *lock)
 
 static inline void
 sdeb_data_read_lock(struct sdeb_store_info *sip)
+	__acquires_shared(&sip->macc_data_lck)
 {
 	BUG_ON(!sip);
 
@@ -4096,6 +4101,7 @@ sdeb_data_read_lock(struct sdeb_store_info *sip)
 
 static inline void
 sdeb_data_read_unlock(struct sdeb_store_info *sip)
+	__releases_shared(&sip->macc_data_lck)
 {
 	BUG_ON(!sip);
 
@@ -4104,6 +4110,7 @@ sdeb_data_read_unlock(struct sdeb_store_info *sip)
 
 static inline void
 sdeb_data_write_lock(struct sdeb_store_info *sip)
+	__acquires(&sip->macc_data_lck)
 {
 	BUG_ON(!sip);
 
@@ -4112,6 +4119,7 @@ sdeb_data_write_lock(struct sdeb_store_info *sip)
 
 static inline void
 sdeb_data_write_unlock(struct sdeb_store_info *sip)
+	__releases(&sip->macc_data_lck)
 {
 	BUG_ON(!sip);
 
@@ -4120,6 +4128,7 @@ sdeb_data_write_unlock(struct sdeb_store_info *sip)
 
 static inline void
 sdeb_data_sector_read_lock(struct sdeb_store_info *sip)
+	__acquires_shared(&sip->macc_sector_lck)
 {
 	BUG_ON(!sip);
 
@@ -4128,6 +4137,7 @@ sdeb_data_sector_read_lock(struct sdeb_store_info *sip)
 
 static inline void
 sdeb_data_sector_read_unlock(struct sdeb_store_info *sip)
+	__releases_shared(&sip->macc_sector_lck)
 {
 	BUG_ON(!sip);
 
@@ -4136,6 +4146,7 @@ sdeb_data_sector_read_unlock(struct sdeb_store_info *sip)
 
 static inline void
 sdeb_data_sector_write_lock(struct sdeb_store_info *sip)
+	__acquires(&sip->macc_sector_lck)
 {
 	BUG_ON(!sip);
 
@@ -4144,6 +4155,7 @@ sdeb_data_sector_write_lock(struct sdeb_store_info *sip)
 
 static inline void
 sdeb_data_sector_write_unlock(struct sdeb_store_info *sip)
+	__releases(&sip->macc_sector_lck)
 {
 	BUG_ON(!sip);
 
@@ -4165,50 +4177,63 @@ sdeb_data_sector_write_unlock(struct sdeb_store_info *sip)
 
 static inline void
 sdeb_data_lock(struct sdeb_store_info *sip, bool atomic)
+	__acquires(&sip->macc_data_lck)
 {
-	if (atomic)
+	if (atomic) {
 		sdeb_data_write_lock(sip);
-	else
+	} else {
 		sdeb_data_read_lock(sip);
+		__release_shared(&sip->macc_data_lck);
+		__acquire(&sip->macc_data_lck);
+	}
 }
 
 static inline void
 sdeb_data_unlock(struct sdeb_store_info *sip, bool atomic)
+	__releases(&sip->macc_data_lck)
 {
-	if (atomic)
+	if (atomic) {
 		sdeb_data_write_unlock(sip);
-	else
+	} else {
+		__release(&sip->macc_data_lck);
+		__acquire_shared(&sip->macc_data_lck);
 		sdeb_data_read_unlock(sip);
+	}
 }
 
 /* Allow many reads but only 1x write per sector */
 static inline void
 sdeb_data_sector_lock(struct sdeb_store_info *sip, bool do_write)
+	__acquires(&sip->macc_sector_lck)
 {
-	if (do_write)
+	if (do_write) {
 		sdeb_data_sector_write_lock(sip);
-	else
+	} else {
 		sdeb_data_sector_read_lock(sip);
+		__release_shared(&sip->macc_sector_lck);
+		__acquire(&sip->macc_sector_lck);
+	}
 }
 
 static inline void
 sdeb_data_sector_unlock(struct sdeb_store_info *sip, bool do_write)
+	__releases(&sip->macc_sector_lck)
 {
-	if (do_write)
+	if (do_write) {
 		sdeb_data_sector_write_unlock(sip);
-	else
+	} else {
+		__release(&sip->macc_sector_lck);
+		__acquire_shared(&sip->macc_sector_lck);
 		sdeb_data_sector_read_unlock(sip);
+	}
 }
 
 static inline void
 sdeb_meta_read_lock(struct sdeb_store_info *sip)
+	__acquires_shared(&sip->macc_meta_lck)
+	__context_unsafe(/*conditional locking*/)
 {
-	if (sdebug_no_rwlock) {
-		if (sip)
-			__acquire(&sip->macc_meta_lck);
-		else
-			__acquire(&sdeb_fake_rw_lck);
-	} else {
+	if (!sdebug_no_rwlock) {
 		if (sip)
 			read_lock(&sip->macc_meta_lck);
 		else
@@ -4218,13 +4243,10 @@ sdeb_meta_read_lock(struct sdeb_store_info *sip)
 
 static inline void
 sdeb_meta_read_unlock(struct sdeb_store_info *sip)
+	__releases_shared(&sip->macc_meta_lck)
+	__context_unsafe(/*conditional locking*/)
 {
-	if (sdebug_no_rwlock) {
-		if (sip)
-			__release(&sip->macc_meta_lck);
-		else
-			__release(&sdeb_fake_rw_lck);
-	} else {
+	if (!sdebug_no_rwlock) {
 		if (sip)
 			read_unlock(&sip->macc_meta_lck);
 		else
@@ -4234,13 +4256,10 @@ sdeb_meta_read_unlock(struct sdeb_store_info *sip)
 
 static inline void
 sdeb_meta_write_lock(struct sdeb_store_info *sip)
+	__acquires(&sip->macc_meta_lck)
+	__context_unsafe(/*conditional locking*/)
 {
-	if (sdebug_no_rwlock) {
-		if (sip)
-			__acquire(&sip->macc_meta_lck);
-		else
-			__acquire(&sdeb_fake_rw_lck);
-	} else {
+	if (!sdebug_no_rwlock) {
 		if (sip)
 			write_lock(&sip->macc_meta_lck);
 		else
@@ -4250,13 +4269,10 @@ sdeb_meta_write_lock(struct sdeb_store_info *sip)
 
 static inline void
 sdeb_meta_write_unlock(struct sdeb_store_info *sip)
+	__releases(&sip->macc_meta_lck)
+	__context_unsafe(/*conditional locking*/)
 {
-	if (sdebug_no_rwlock) {
-		if (sip)
-			__release(&sip->macc_meta_lck);
-		else
-			__release(&sdeb_fake_rw_lck);
-	} else {
+	if (!sdebug_no_rwlock) {
 		if (sip)
 			write_unlock(&sip->macc_meta_lck);
 		else

[PATCH v5 8/8] scsi: core: Enable lock context analysis for the scsi_debug driver

[Bart Van Assche]

Enable lock context analysis now that all lock context analysis warnings
have been fixed.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: John Garry <john.garry@linux.dev>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile
index 533623382eca..32d816e942d6 100644
--- a/drivers/scsi/Makefile
+++ b/drivers/scsi/Makefile
@@ -18,6 +18,7 @@ CONTEXT_ANALYSIS_constants.o := y
 CONTEXT_ANALYSIS_hosts.o := y
 CONTEXT_ANALYSIS_scsi.o := y
 CONTEXT_ANALYSIS_scsi_common.o := y
+CONTEXT_ANALYSIS_scsi_debug.o := y
 CONTEXT_ANALYSIS_scsi_devinfo.o := y
 CONTEXT_ANALYSIS_scsi_dh.o := y
 CONTEXT_ANALYSIS_scsi_error.o := y

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

[sashiko-bot]

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

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

[Bart Van Assche]

On 10/8/26 3:52 PM, sashiko-bot@kernel.org wrote:
> [Severity: Medium]
> Does this early return result in a data race on the sip->storep buffer?

I think this counts as a preexisting issue that has not been introduced
by this patch series.

Bart.

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

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v5 7/8] scsi: scsi_debug: Improve lock context annotations

[Christoph Hellwig]

On Thu, Oct 08, 2026 at 03:42:18PM -0700, Bart Van Assche wrote:
>  static inline void
>  sdeb_meta_read_lock(struct sdeb_store_info *sip)
> +	__acquires_shared(&sip->macc_meta_lck)
> +	__context_unsafe(/*conditional locking*/)
>  {
> -	if (sdebug_no_rwlock) {
> -		if (sip)
> -			__acquire(&sip->macc_meta_lck);
> -		else
> -			__acquire(&sdeb_fake_rw_lck);
> -	} else {
> +	if (!sdebug_no_rwlock) {

Why can't this be modelled using a __acquire variant?  And if you
add __context_unsafe(/*conditional locking*/) please add a detailed
comment why that is needed, otherwise no one is going to understand
this in the future.

Re: [PATCH v5 7/8] scsi: scsi_debug: Improve lock context annotations

[Bart Van Assche]

On 10/12/26 5:24 AM, Christoph Hellwig wrote:
> On Thu, Oct 08, 2026 at 03:42:18PM -0700, Bart Van Assche wrote:
>>   static inline void
>>   sdeb_meta_read_lock(struct sdeb_store_info *sip)
>> +	__acquires_shared(&sip->macc_meta_lck)
>> +	__context_unsafe(/*conditional locking*/)
>>   {
>> -	if (sdebug_no_rwlock) {
>> -		if (sip)
>> -			__acquire(&sip->macc_meta_lck);
>> -		else
>> -			__acquire(&sdeb_fake_rw_lck);
>> -	} else {
>> +	if (!sdebug_no_rwlock) {
> 
> Why can't this be modelled using a __acquire variant?  And if you
> add __context_unsafe(/*conditional locking*/) please add a detailed
> comment why that is needed, otherwise no one is going to understand
> this in the future.

Hi Christoph,

I will change the comment into "can't be annotated with
__{acquire,release}(,_shared}() without triggering undefined behavior".

What Sashiko reported on a Google-internal server for an
earlier attempt to annotate this and similar functions with
__{acquire,release}(,_shared}() has been included below. I think that
Sashiko is right:
=======================================================================
 >  static inline void
 >  sdeb_meta_read_lock(struct sdeb_store_info *sip)
 > +	__acquires_shared(&sip->macc_meta_lck)
 >  {
 >  	if (sdebug_no_rwlock) {
 > -		if (sip)
 > -			__acquire(&sip->macc_meta_lck);
 > -		else
 > -			__acquire(&sdeb_fake_rw_lck);
 > +		__acquire_shared(&sip->macc_meta_lck);
 >  	} else {
 > -		if (sip)
 > +		if (sip) {
 >  			read_lock(&sip->macc_meta_lck);
 > -		else
 > +		} else {
 >  			read_lock(&sdeb_fake_rw_lck);
 > +			__release_shared(&sdeb_fake_rw_lck);
 > +			__acquire_shared(&sip->macc_meta_lck);
                                          ^^^^^^^^^^^^^^^^^^^
Can this regression trigger an undefined behavior splat when sip is NULL?
When the scsi_debug driver is used with the module parameter fake_rw=1,
devip2sip() returns NULL. In this else branch, sip is explicitly NULL,
so evaluating &sip->macc_meta_lck means taking the address of a member
from a NULL pointer.
When compiled with Context Analysis and UBSAN (-fsanitize=null or
-fsanitize=pointer-overflow), the inline function call evaluates the
argument before the call and will trigger a runtime splat. Without UBSAN,
it calculates an invalid offset from a NULL pointer.
=======================================================================

Bart.

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

[John Garry]

On 10/8/26 23:42, Bart Van Assche wrote:

Reviewed-by: John Garry <john.garry@linux.dev>

> +		ret = __resp_read_dt0(scp, sip, lba, num, ei_lba, &scsi_status);
> +		sdeb_meta_read_unlock(sip);
> +	} else {
> +		ret = __resp_read_dt0(scp, sip, lba, num, ei_lba, &scsi_status);
>   	}
>   
> -	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))

I think that ret < 0 is a better check, but this is pre-existing code 
and this is not too important.

> -		return DID_ERROR << 16;
> +		return scsi_status;