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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: deadlock or oops due to unprotected sdebug_no_rwlock modification
--

commit c129c8944c668498a44ba3a38c88f4214c8204f7
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:

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

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

> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index 244250ba460bc..2edb70013cbc2 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -4052,42 +4052,43 @@ 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);
>  }

[Severity: High]
Does this conditional lock acquisition create a potential deadlock or oops?

The lock acquisition in sdeb_read_lock() is conditional on the unprotected
runtime value of sdebug_no_rwlock. If a user writes to the no_rwlock
attribute while I/O operations are actively being queued or processed, the
variable can change mid-flight.

drivers/scsi/scsi_debug.c:no_rwlock_store() {
	...
	if (kstrtobool(buf, &v))
		return -EINVAL;

	sdebug_no_rwlock = v;
	return count;
}

This sysfs store function modifies the global sdebug_no_rwlock variable
concurrently without any locking or quiescent state wait, allowing the race
condition to occur.

>  
>  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);
>  }

[Severity: High]
Does this lock release rely on the same unprotected global variable?

If sdebug_no_rwlock changed to true since acquisition, the unlock is
skipped, causing a permanent deadlock. If changed to false while inside
a lockless critical section, read_unlock() is called without a prior
read_lock(), causing a kernel oops.

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