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

[sashiko-bot@kernel.org]

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