Re: [PATCH v3 5/6] scsi: scsi_debug: Map the region written by WRITE ATOMIC (16)

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi_debug: ABBA deadlock between resp_atomic_write() and resp_comp_write()
- [Medium] scsi_debug: TOCTOU race in resp_atomic_write() via double evaluation
--

commit d00845f77d0b11824b6bd723b3a99d9e2026b838
Author: Niklas Cassel <cassel@kernel.org>

scsi: scsi_debug: Map the region written by WRITE ATOMIC (16)

When logical block provisioning is enabled, commands that write user data mark
the region in the provisioning map. WRITE ATOMIC (16) leaves the map untouched,
so this patch calls map_region() and takes the metadata write lock across the
access.

> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index 64f32d97455d6..33f2df26e5af7 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
[ ... ]
> @@ -6228,7 +6229,18 @@ static int resp_atomic_write(struct scsi_cmnd *scp,
>  		}
>  	}
>  
> +	if (scsi_debug_lbp()) {
> +		sdeb_meta_write_lock(sip);
> +		meta_data_locked = true;
> +	}
> +
>  	ret = do_device_access(sip, scp, 0, lba, len, 0, true, true);

[Severity: High]
Does this code introduce a lock inversion deadlock with resp_comp_write()?

Here in resp_atomic_write(), the metadata lock is acquired before calling
do_device_access(), which acquires the data lock:

resp_atomic_write()
  sdeb_meta_write_lock(sip);
  do_device_access()
    sdeb_data_lock(sip, true)
      sdeb_data_write_lock(sip);

However, in resp_comp_write(), the locks are acquired in the reverse order:

resp_comp_write()
  sdeb_data_write_lock(sip);
  sdeb_meta_write_lock(sip);

Could concurrent execution of write atomic (16) and compare and write
commands to the same target cause the scsi processing kworkers to deadlock?

> +	if (unlikely(scsi_debug_lbp()))
> +		map_region(sip, lba, len);

[Severity: Medium]
Could this result in a race condition due to evaluating scsi_debug_lbp() twice?

If an administrator modifies /sys/bus/pseudo/drivers/scsi_debug/fake_rw while
a write atomic (16) command is executing, the first evaluation could return
false, bypassing the lock acquisition. If the second evaluation then returns
true, map_region() would be executed without the metadata lock being held.

Could this be fixed by caching the result of the first evaluation,
or checking meta_data_locked instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260917125445.1376493-8-cassel@kernel.org?part=5