Re: [PATCH 1/2] scsi: scsi_debug: wrap around the store when zeroing a reset zone

[Tal Zussman]

On 11/11/26 4:04 AM, Damien Le Moal wrote:
> On 2026/11/11 6:23, Tal Zussman wrote:
>> zbc_rwp_zone() zeroes the blocks of a zone that is reset, using the
>> start of the zone as an offset into the store. With virtual_gb set, the
>> device and its zones can extend past the end of the store, which is
>> only dev_size_mb in size. Writes wrap around in do_device_access(), but
>> the memset() doesn't, so resetting a zone that has been written to can
>> write past the end of the store.
>> 
>> On a device whose fifth zone starts exactly at the end of the store,
>> write to that zone and reset it:
>> 
>>   # modprobe scsi_debug zbc=managed sector_size=4096 dev_size_mb=16 \
>>         virtual_gb=1 zone_size_mb=4
>>   # dd if=/dev/urandom of=/dev/sda bs=1M count=1 seek=16 oflag=direct
>>   # blkzone reset -o 32768 -c 1 /dev/sda
>> 
>> The memset() starts at the first byte past the store, in the guard page
>> after the vmalloc'ed area, for the 1 MiB that was written:
>> 
>> BUG: unable to handle page fault for address: ffffc90002c00000
>> Oops: Oops: 0002 [#1] SMP KASAN NOPTI
>> RIP: 0010:memset_orig+0x33/0xb0
>> RDX: 0000000000100000 RSI: 0000000000000000 RDI: ffffc90002c00000
>> Call Trace:
>>  zbc_rwp_zone+0x1a7/0x2a0 [scsi_debug]
>>  resp_rwp_zone+0x19c/0x450 [scsi_debug]
>>  schedule_resp+0x2dd/0xbc0 [scsi_debug]
>>  scsi_debug_queuecommand+0x1286/0x2950 [scsi_debug]
>>  scsi_queue_rq+0x1d74/0x3200
>>  blk_mq_dispatch_rq_list+0x3ad/0x23c0
>> 
>> The command never completes, so the reset returns EIO to userspace and
>> the device is unusable afterwards.
>> 
>> Zero the blocks in a helper that wraps around at the end of the store,
>> and that stops after one pass over it for a zone that is larger than
>> the store.
>> 
>> Fixes: f0d1cf9378bd ("scsi: scsi_debug: Add ZBC zone commands")
>> Signed-off-by: Tal Zussman <tz2294@xxxxxxxxxxxx>
>> ---
>>  drivers/scsi/scsi_debug.c | 26 ++++++++++++++++++++++++--
>>  1 file changed, 24 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
>> index a922c8318a2f..1c27ea0f9b69 100644
>> --- a/drivers/scsi/scsi_debug.c
>> +++ b/drivers/scsi/scsi_debug.c
>> @@ -6612,6 +6612,29 @@ static int resp_finish_zone(struct scsi_cmnd *scp,
>>  	return res;
>>  }
>>  
>> +/*
>> + * Zero the blocks of a zone that is being reset. The store can be smaller
>> + * than the device, so wrap around at its end like do_device_access() does.
>> + */
>> +static void zbc_zero_blocks(struct sdeb_store_info *sip, sector_t lba,
>> +			    sector_t num)
>> +{
>> +	/* One pass over the store zeroes all of it */
>> +	num = min_t(sector_t, num, sdebug_store_sectors);
> 
> Why do you need to do this in a loop?
> You can certainly calculate the total number of blocks that need zeroing and do
> a single memset, no? What am I missing here?

With virtual_gb the device can be bigger than the store, and writes
wrap around, so the reset has to zero wherever the writes actually went,
which isn't always one contiguous range.

For example, with dev_size_mb=10 and zone_size_mb=4 the third zone
covers 8M to 12M, but the store ends at 10M. Writes to the second half
of that zone are done at 0 to 2M, so we need to zero two ranges.

But yes, this can be done without a loop since we have at most two ranges.
I'll switch to two memset() calls in v2, like comp_write_worker() does.
I'll also change the fixes tags as Sashiko suggested.

>> +
>> +	while (num) {
>> +		sector_t block = lba;
>> +		u32 n;
>> +
>> +		block = sector_div(block, sdebug_store_sectors);
>> +		n = min_t(sector_t, num, sdebug_store_sectors - block);
>> +		memset(sip->storep + block * sdebug_sector_size, 0,
>> +		       (size_t)n * sdebug_sector_size);
>> +		lba += n;
>> +		num -= n;
>> +	}
>> +}
>> +
>>  static void zbc_rwp_zone(struct sdebug_dev_info *devip,
>>  			 struct sdeb_zone_state *zsp)
>>  {
>> @@ -6629,8 +6652,7 @@ static void zbc_rwp_zone(struct sdebug_dev_info *devip,
>>  		devip->nr_closed--;
>>  
>>  	if (zsp->z_wp > zsp->z_start)
>> -		memset(sip->storep + zsp->z_start * sdebug_sector_size, 0,
>> -		       (zsp->z_wp - zsp->z_start) * sdebug_sector_size);
>> +		zbc_zero_blocks(sip, zsp->z_start, zsp->z_wp - zsp->z_start);
>>  
>>  	zsp->z_non_seq_resource = false;
>>  	zsp->z_wp = zsp->z_start;
>> 
> 
> 
> -- 
> Damien Le Moal
> Western Digital Research
>