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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] scsi_debug: unconditional exclusive lock annotation for conditional lock
- [Low] scsi_debug: omitted fallback lock tracking in lock annotations
--

commit 2f1a5cf89b239b8a5043e8baea7eb6d404bcc911
Author: Bart Van Assche <bvanassche@acm.org>

scsi: scsi_debug: Improve lock context annotations

This commit prepares the scsi_debug driver for enabling compiler-based lock
context analysis. It annotates lock and unlock helper functions with
acquires and releases macros while removing sparse acquire/release calls
that mislead the analyzer.

> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index 44840ba0c5b58..a6cd958855411 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c

[ ... ]

> @@ -4155,6 +4163,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->macc_data_lck)
> +	__context_unsafe(/*conditional locking*/)
>  {
>  	if (atomic)
>  		sdeb_data_write_lock(sip);
>  	else
>  		sdeb_data_read_lock(sip);
>  }

[Severity: Low]
Does unconditionally declaring an exclusive lock capability here cause 
incorrect compile-time static analysis tracking? 

When atomic is false, sdeb_data_read_lock() is called which acquires a 
shared lock, but the __acquires() annotation marks it as exclusively 
locked. Could this suppress valid compile-time warnings for data races 
by tracking shared locks as exclusive?

[ ... ]

>  /* 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)
> +	__context_unsafe(/*conditional locking*/)
>  {
>  	if (do_write)
>  		sdeb_data_sector_write_lock(sip);
>  	else
>  		sdeb_data_sector_read_lock(sip);
>  }

[Severity: Low]
Similar to sdeb_data_lock() above, does this annotation incorrectly track
a shared lock as an exclusive lock when do_write is false?

[ ... ]

>  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
>  			read_lock(&sdeb_fake_rw_lck);

[Severity: Low]
Does the annotation completely omit tracking for the fallback 
sdeb_fake_rw_lck?

If sip is NULL, the lock tracking seems to fall back to a dummy 
capability rather than the actual sdeb_fake_rw_lck. Could this 
prevent the analyzer from properly tracking accesses to data 
protected by the fallback lock?

[ ... ]

>  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
>  			write_lock(&sdeb_fake_rw_lck);

[Severity: Low]
Does this also omit tracking for the sdeb_fake_rw_lck when sip is NULL?

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