[PATCH 0/7] scsi_debug: Enable lock context analysis

[PATCH 0/7] scsi_debug: Enable lock context analysis

[Bart Van Assche]

Hi Martin,

This patch series fixes a locking bug in the scsi_debug driver and enables
lock context analysis. Please consider this patch series for the next merge
window.

Thanks,

Bart.

Bart Van Assche (7):
  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: Improve lock context annotations
  scsi: core: Enable lock context analysis for the scsi_debug driver

 drivers/scsi/Makefile     |   1 +
 drivers/scsi/scsi_debug.c | 355 +++++++++++++++++++++-----------------
 2 files changed, 197 insertions(+), 159 deletions(-)

[PATCH 1/7] 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")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_debug.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 6941809dfdb7..b854102e6caa 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:

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] scsi: scsi_debug: error masked in resp_write_same()
--

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

scsi: scsi_debug: Fix a locking bug in resp_write_same()

This commit fixes a missing lock release in resp_write_same() when
fetch_to_dev_buffer() fails. It redirects the error path to a new
unlock label to ensure sdeb_data_write_unlock() is called.

> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> --- 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))

[ ... ]

> @@ -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;

[Severity: Medium]
Does this unconditionally overwrite the error code?

By jumping to the new unlock label when fetch_to_dev_buffer() fails, ret
is initially set to DID_ERROR << 16, but then it appears ret = 0 is
executed unconditionally right below the unlock label.

Could this silently mask I/O failures by returning 0 on the error path?

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

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

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

[John Garry]

On 9/23/26 00:26, Bart Van Assche wrote:
> 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")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   drivers/scsi/scsi_debug.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index 6941809dfdb7..b854102e6caa 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;

at @unlock we lose the error code in ret - is that intentional?

>   	} 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:

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

[John Garry]

On 9/23/26 08:37, John Garry wrote:
> On 9/23/26 00:26, Bart Van Assche wrote:
>> 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")
>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>> ---
>>   drivers/scsi/scsi_debug.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
>> index 6941809dfdb7..b854102e6caa 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;
> 
> at @unlock we lose the error code in ret - is that intentional?
> 

Now I notice that sashiko spotted this too. I will await until issues 
spotted by sashiko elsewhere at attended to before checking further.

>>       } 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:
> 

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

[Bart Van Assche]

On 9/23/26 12:37 AM, John Garry wrote:
>> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
>> index 6941809dfdb7..b854102e6caa 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;
> 
> at @unlock we lose the error code in ret - is that intentional?
It's a bug. I will fix this issue and the Sashiko complaints and I will
review these changes myself carefully before I repost this patch series.

Thanks,

Bart.

[PATCH 2/7] 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.

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

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index b854102e6caa..a44886a4c853 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -5360,33 +5360,26 @@ 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)
 {
 	struct scsi_device *sdp = scp->device;
 	struct sdebug_dev_info *devip = (struct sdebug_dev_info *)sdp->hostdata;
+	struct sdeb_store_info *sip = devip2sip(devip, true);
+	u32 lb_size = sdebug_sector_size;
 	unsigned long long i;
 	u64 block, lbaa;
-	u32 lb_size = sdebug_sector_size;
 	int ret;
-	struct sdeb_store_info *sip = devip2sip((struct sdebug_dev_info *)
-						scp->device->hostdata, true);
 	u8 *fs1p;
 	u8 *fsp;
-	bool meta_data_locked = false;
-
-	if (sdebug_dev_is_zoned(devip) || scsi_debug_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 && scsi_debug_lbp()) {
 		unmap_region(sip, lba, num);
-		goto out;
+		return ret;
 	}
 	lbaa = lba;
 	block = do_div(lbaa, sdebug_store_sectors);
@@ -5422,9 +5415,25 @@ static int resp_write_same(struct scsi_cmnd *scp, u64 lba, u32 num,
 unlock:
 	sdeb_data_write_unlock(sip);
 	ret = 0;
-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);
+	int ret;
+
+	if (sdebug_dev_is_zoned(devip) || scsi_debug_lbp()) {
+		sdeb_meta_write_lock(sip);
+		ret = __resp_write_same(scp, lba, num, ei_lba, unmap, ndob);
 		sdeb_meta_write_unlock(sip);
+	} else {
+		ret = __resp_write_same(scp, lba, num, ei_lba, unmap, ndob);
+	}
+
 	return ret;
 }
 

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: scsi_debug: error code clobbered in __resp_write_same()
--

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

scsi: scsi_debug: Split resp_write_same()
    
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.

> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index b854102e6caa1..a44886a4c8539 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
[ ... ]
> @@ -5422,9 +5415,25 @@ static int resp_write_same(struct scsi_cmnd *scp, u64 lba, u32 num,
>  unlock:
>  	sdeb_data_write_unlock(sip);
>  	ret = 0;
> -out:
> -	if (meta_data_locked)
> +	return ret;

[Severity: High]
If fetch_to_dev_buffer() fails and returns -1, the code earlier in
__resp_write_same() sets ret = DID_ERROR << 16 and jumps to the unlock label:

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

Does this code unconditionally overwrite the captured error code and return 0,
silently masking the write failure?

> +}

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

[PATCH 3/7] 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.

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

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index a44886a4c853..fcfb49df9ee1 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -4948,39 +4948,26 @@ 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)
 {
-	struct sdeb_store_info *sip = devip2sip(devip, false);
-	bool meta_data_locked = false;
 	u32 block, num_mapped, b, i;
-	int error = 0;
-
-	if (sdebug_dev_is_zoned(devip) ||
-	    sdebug_dix ||
-	    scsi_debug_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 (scsi_debug_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;
 	}
 
 	/*
@@ -5018,9 +5005,24 @@ 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);
+	int error;
+
+	if (sdebug_dev_is_zoned(devip) || sdebug_dix || scsi_debug_lbp()) {
+		sdeb_meta_write_lock(sip);
+		error = __corrupt_lbas(sip, lba, num, nr_bit_errors,
+				       reftag_adjust);
 		sdeb_meta_write_unlock(sip);
+	} else {
+		error = __corrupt_lbas(sip, lba, num, nr_bit_errors,
+				       reftag_adjust);
+	}
 	return error;
 }
 

[PATCH 4/7] 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 | 80 +++++++++++++++++++++------------------
 1 file changed, 43 insertions(+), 37 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index fcfb49df9ee1..ac3b9bfa7e63 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)) {
+		case 1: /* Guard tag error */
+			if (cmd[1] >> 5 != 3) { /* RDPROTECT != 3 */
+				mk_sense_buffer(scp, ABORTED_COMMAND,
+					LOGICAL_BLOCK_GUARD_CHECK_FAILED);
+				return check_condition_result;
+			} else if (scp->prot_flags & SCSI_PROT_GUARD_CHECK) {
+				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 */
+				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) {
+				mk_sense_buffer(scp, ILLEGAL_REQUEST,
+				      LOGICAL_BLOCK_REFERENCE_TAG_CHECK_FAILED);
+				return illegal_condition_result;
+			}
+			break;
+		}
+	}
+
+	return do_device_access(sip, scp, 0, lba, num, 0, false, false);
+}
+
 static int resp_read_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 {
 	bool check_prot;
@@ -4586,7 +4622,6 @@ 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;
 
 	switch (cmd[0]) {
 	case READ_16:
@@ -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);
 	}
 
-	/* 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;
-		}
-	}
+	if (ret > 0)
+		return ret;
 
-	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;
 

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

[sashiko-bot]

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

[PATCH 5/7] 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 | 107 ++++++++++++++++++++------------------
 1 file changed, 57 insertions(+), 50 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index ac3b9bfa7e63..ed30b3950c4e 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;
+
+	/* 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) {
+				mk_sense_buffer(scp, ILLEGAL_REQUEST,
+					LOGICAL_BLOCK_GUARD_CHECK_FAILED);
+				return illegal_condition_result;
+			} else if (scp->cmnd[1] >> 5 != 3) { /* WRPROTECT != 3 */
+				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) {
+				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 */
+				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(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;
+}
+
 static int resp_write_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 {
 	bool check_prot;
@@ -5042,7 +5091,6 @@ 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;
 
 	if (unlikely(sdebug_opts & SDEBUG_OPT_UNALIGNED_WRITE &&
 		     atomic_read(&sdeb_inject_pending))) {
@@ -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;
-	}
-
-	/* 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(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);
-	if (meta_data_locked)
-		sdeb_meta_write_unlock(sip);
 
 	if (unlikely(-1 == ret))
 		return DID_ERROR << 16;

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

[sashiko-bot]

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

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

[Bart Van Assche]

Prepare the scsi_debug driver for enabling compiler-based lock context
analysis:

- Define context lock structures __ctx_lock_data, __ctx_lock_sector, and
  __ctx_lock_meta, and embed them in struct sdeb_store_info so that data,
  sector, and metadata locking operations can be tracked as context locks.

- Annotate the lock and unlock helper functions with __acquires,
  __releases, __acquires_shared, and __releases_shared.

- Mark helper functions that perform conditional locking based on
  parameters (such as atomic or do_write) or configuration
  (sdebug_no_rwlock) with __context_unsafe(conditional locking).

- Remove sparse __acquire() and __release() calls from the helper
  functions when sdebug_no_rwlock is true since these mislead the lock
  context analyzer.

- Wrap the conditional metadata locking sections in resp_read_dt0() and
  resp_write_dt0() with context_unsafe().

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

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index ed30b3950c4e..ccaaf13cc243 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -382,11 +382,18 @@ struct sdebug_host_info {
 	struct list_head dev_info_list;
 };
 
+context_lock_struct(__ctx_lock_data) {};
+context_lock_struct(__ctx_lock_sector) {};
+context_lock_struct(__ctx_lock_meta) {};
+
 /* There is an xarray of pointers to this struct's objects, one per host */
 struct sdeb_store_info {
 	rwlock_t macc_data_lck;	/* for media data access on this store */
 	rwlock_t macc_meta_lck;	/* for atomic media meta access on this store */
 	rwlock_t macc_sector_lck;	/* per-sector media data access on this store */
+	struct __ctx_lock_data data_lck;
+	struct __ctx_lock_sector sector_lck;
+	struct __ctx_lock_meta meta_lck;
 	u8 *storep;		/* user data storage (ram) */
 	struct t10_pi_tuple *dif_storep; /* protection info */
 	void *map_storep;	/* provisioning map */
@@ -4042,42 +4049,44 @@ static inline struct sdeb_store_info *devip2sip(struct sdebug_dev_info *devip,
 
 static inline void
 sdeb_read_lock(rwlock_t *lock)
+	__acquires_shared(lock)
+	__context_unsafe(/*conditional locking*/)
 {
-	if (sdebug_no_rwlock)
-		__acquire(lock);
-	else
+	if (!sdebug_no_rwlock)
 		read_lock(lock);
 }
 
 static inline void
 sdeb_read_unlock(rwlock_t *lock)
+	__releases_shared(lock)
+	__context_unsafe(/*conditional locking*/)
 {
-	if (sdebug_no_rwlock)
-		__release(lock);
-	else
+	if (!sdebug_no_rwlock)
 		read_unlock(lock);
 }
 
 static inline void
 sdeb_write_lock(rwlock_t *lock)
+	__acquires(lock)
+	__context_unsafe(/*conditional locking*/)
 {
-	if (sdebug_no_rwlock)
-		__acquire(lock);
-	else
+	if (!sdebug_no_rwlock)
 		write_lock(lock);
 }
 
 static inline void
 sdeb_write_unlock(rwlock_t *lock)
+	__releases(lock)
+	__context_unsafe(/*conditional locking*/)
 {
-	if (sdebug_no_rwlock)
-		__release(lock);
-	else
+	if (!sdebug_no_rwlock)
 		write_unlock(lock);
 }
 
 static inline void
 sdeb_data_read_lock(struct sdeb_store_info *sip)
+	__acquires_shared(&sip->macc_data_lck)
+	__context_unsafe(/*conditional locking*/)
 {
 	BUG_ON(!sip);
 
@@ -4086,6 +4095,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);
 
@@ -4094,6 +4104,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);
 
@@ -4102,6 +4113,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);
 
@@ -4110,6 +4122,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);
 
@@ -4118,6 +4131,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);
 
@@ -4126,6 +4140,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);
 
@@ -4134,6 +4149,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);
 
@@ -4155,6 +4171,8 @@ 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->data_lck)
+	__context_unsafe(/*conditional locking*/)
 {
 	if (atomic)
 		sdeb_data_write_lock(sip);
@@ -4164,6 +4182,8 @@ sdeb_data_lock(struct sdeb_store_info *sip, bool atomic)
 
 static inline void
 sdeb_data_unlock(struct sdeb_store_info *sip, bool atomic)
+	__releases(&sip->data_lck)
+	__context_unsafe(/*conditional locking*/)
 {
 	if (atomic)
 		sdeb_data_write_unlock(sip);
@@ -4174,6 +4194,8 @@ sdeb_data_unlock(struct sdeb_store_info *sip, bool atomic)
 /* 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->sector_lck)
+	__context_unsafe(/*conditional locking*/)
 {
 	if (do_write)
 		sdeb_data_sector_write_lock(sip);
@@ -4183,6 +4205,8 @@ sdeb_data_sector_lock(struct sdeb_store_info *sip, bool do_write)
 
 static inline void
 sdeb_data_sector_unlock(struct sdeb_store_info *sip, bool do_write)
+	__releases(&sip->sector_lck)
+	__context_unsafe(/*conditional locking*/)
 {
 	if (do_write)
 		sdeb_data_sector_write_unlock(sip);
@@ -4192,13 +4216,10 @@ sdeb_data_sector_unlock(struct sdeb_store_info *sip, bool do_write)
 
 static inline void
 sdeb_meta_read_lock(struct sdeb_store_info *sip)
+	__acquires_shared(&sip->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
@@ -4208,13 +4229,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->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
@@ -4224,13 +4242,10 @@ sdeb_meta_read_unlock(struct sdeb_store_info *sip)
 
 static inline void
 sdeb_meta_write_lock(struct sdeb_store_info *sip)
+	__acquires(&sip->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
@@ -4240,13 +4255,10 @@ sdeb_meta_write_lock(struct sdeb_store_info *sip)
 
 static inline void
 sdeb_meta_write_unlock(struct sdeb_store_info *sip)
+	__releases(&sip->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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] scsi: error code clobbered in __resp_write_same()
- [Medium] scsi: code and commit message mismatch for context_unsafe()
- [Low] scsi: inconsistent __context_unsafe annotation on sdeb_data_read_lock()
--

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

scsi: scsi_debug: Improve lock context annotations

Prepare the scsi_debug driver for enabling compiler-based lock context
analysis by defining context lock structures, annotating lock/unlock helpers,
and wrapping conditional metadata locks.

>     - Wrap the conditional metadata locking sections in resp_read_dt0() and
>       resp_write_dt0() with context_unsafe().

[Severity: Medium]
Are resp_read_dt0() and resp_write_dt0() still supposed to be wrapped
with context_unsafe()?

This isn't a bug, but the commit message explicitly states these conditional
metadata locking sections are wrapped, whereas the patch does not add any
context_unsafe() wrappers to these functions. Could this cause static analysis
tools to incorrectly flag the conditional locking?

> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index ed30b3950c4e2..ccaaf13cc2430 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c

[ ... ]

> @@ -4042,42 +4049,44 @@ static inline struct sdeb_store_info *devip2sip(struct sdebug_dev_info *devip,

[ ... ]

>  static inline void
>  sdeb_data_read_lock(struct sdeb_store_info *sip)
> +	__acquires_shared(&sip->macc_data_lck)
> +	__context_unsafe(/*conditional locking*/)
>  {
>  	BUG_ON(!sip);

[Severity: Low]
Does sdeb_data_read_lock() actually perform any conditional locking itself?

It unconditionally calls sdeb_read_lock(), so the __context_unsafe annotation
appears to be inconsistent with its counterpart sdeb_data_read_unlock() and
other wrappers like sdeb_data_write_lock() which correctly omit this tag.

[Severity: High]
Does the error handling path in __resp_write_same() inadvertently clobber
its error code?

This appears to have been introduced in an earlier patch in this series
("scsi: scsi_debug: Fix a locking bug in resp_write_same()") and remains
unfixed in the final state of the series:

drivers/scsi/scsi_debug.c:__resp_write_same() {
    ...
	} else
		ret = fetch_to_dev_buffer(scp, fs1p, lb_size);

	if (-1 == ret) {
		ret = DID_ERROR << 16;
		goto unlock;
	} else if (sdebug_verbose && !ndob && (ret < lb_size))
    ...
unlock:
	sdeb_data_write_unlock(sip);
	ret = 0;
	return ret;
}

When fetch_to_dev_buffer() fails, ret is set to DID_ERROR << 16 and the code
jumps to the unlock label, where ret is unconditionally overwritten with 0.

Could this mask the error from the SCSI subsystem, leading it to believe
the write same command succeeded when it actually failed?

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

[PATCH 7/7] 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.

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