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

[Niklas Cassel <cassel@kernel.org>]

On Thu, Sep 17, 2026 at 01:12:50PM +0000, sashiko-bot@kernel.org wrote:
> 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);

Well, this seems to be a pre-existing problem, since resp_write_dt0() takes
the locks in the same order as resp_atomic_write().

It looks like resp_comp_write() is the only one who takes the locks in opposite
order, so the fix is probably to swap the order in resp_comp_write().


> 
> 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?

Sure, but this is also a pre-existing problem, since resp_write_dt0() looks
exactly the same.

But sure, can introduce a local variable in all functions that call
scsi_debug_lbp(), so that it is never evaluated twice.


Kind regards,
Niklas