[PATCH v2 0/4] scsi: scsi_debug: fix zoned write validation

[PATCH v2 0/4] scsi: scsi_debug: fix zoned write validation

[Niklas Cassel]

This series fixes scsi_debug with regards to writes to a ZBC drive.

The series is based on 7.4/scsi-staging rather than 7.3/scsi-fixes (as
Damien suggested on v1) to avoid a build warning that would have happened
if this series was simply merged with linux-next (7.4/scsi-staging):

  error: too many arguments to function 'mk_sense_buffer'

Tested on a zoned scsi_debug device with 512 byte logical blocks and a
4096 byte physical block; the notes below each patch describe what was
observed before and after it.

Changes since v1:
- Based on 7.4/scsi-staging instead of 7.3/scsi-fixes, with patch 2
  using the combined sense code introduced there.
- Added patch 1, so that none of the write paths advance the write
  pointer when do_device_access() did not write all of the data.
- Patch 2 is unchanged, and has picked up Damien's Reviewed-by.
- Added patch 3, which marks the region that WRITE ATOMIC (16) wrote in
  the logical block provisioning map.
- Patch 4 advances the write pointer only when all of the data was
  written.

Niklas Cassel (4):
  scsi: scsi_debug: Do not advance the write pointer of a failed write
  scsi: scsi_debug: Enforce physical block alignment of zoned writes
  scsi: scsi_debug: Map the region written by WRITE ATOMIC (16)
  scsi: scsi_debug: Validate zone access for WRITE ATOMIC (16)

 drivers/scsi/scsi_debug.c | 38 ++++++++++++++++++++++++++++++++++----
 1 file changed, 34 insertions(+), 4 deletions(-)


base-commit: c3cff7fac01638ab58e85fe7df41a04fa25c5bae
-- 
2.55.0

[PATCH v2 1/4] scsi: scsi_debug: Do not advance the write pointer of a failed write

[Niklas Cassel]

resp_write_dt0() and resp_write_scat() advance the write pointer of a
sequential write required zone by the transfer length of the command
before looking at what do_device_access() returned. It returns -1 when
the data direction of the command does not match the operation, and a
short byte count when the data-out buffer is smaller than the transfer
length, both of which an initiator can produce with SG_IO.

The zone state then describes more data than is on the medium, and a
write at the position where the data really ends is terminated with
UNALIGNED WRITE COMMAND, because the write pointer has moved beyond it.
The zone has to be reset before it can be written to again.

Leave the write pointer alone unless all of the data was written. A host
that retries the command writes at the write pointer, where whatever the
failed command did write begins, so the retry overwrites it.

Advancing over the data that was written instead would be no better,
because do_device_access() copies one logical block at a time and stops
at the first short copy, so it can report a byte count that is not a
multiple of the physical block size. That would leave the write pointer
in the middle of a physical block, and ZBC requires a write to a
sequential write required zone to end on a physical block boundary, so
no write can leave a real device with such a write pointer.

resp_write_same() does not need the same treatment, as it writes with
memmove() and cannot fail part way through.

Assisted-by: LLM
Fixes: f0d1cf9378bd ("scsi: scsi_debug: Add ZBC zone commands")
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
Tested with:

  modprobe scsi_debug zbc=managed sector_size=512 physblk_exp=3 \
      zone_size_mb=8 dev_size_mb=128 zone_nr_conv=2

issuing WRITE(16) with sg_raw so that the data-out buffer is shorter
than the transfer length and do_device_access() returns a short count.
Sixteen logical blocks with a buffer of twelve, and eight blocks with a
buffer of one, both leave the write pointer where it was, and the full
sixteen block write then succeeds from there. Before this patch both
advanced the write pointer by the full transfer length, and a write at
the end of the data that had actually been written was then terminated
with UNALIGNED WRITE COMMAND.
---
 drivers/scsi/scsi_debug.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 6941809dfdb7..3ce2dbdff177 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -5150,8 +5150,8 @@ static int resp_write_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 	if (unlikely(scsi_debug_lbp()))
 		map_region(sip, lba, num);
 
-	/* If ZBC zone then bump its write pointer */
-	if (sdebug_dev_is_zoned(devip))
+	/* If ZBC zone then bump its write pointer, if all data was written */
+	if (sdebug_dev_is_zoned(devip) && ret == num * sdebug_sector_size)
 		zbc_inc_wp(devip, lba, num);
 	if (meta_data_locked)
 		sdeb_meta_write_unlock(sip);
@@ -5314,8 +5314,8 @@ static int resp_write_scat(struct scsi_cmnd *scp,
 		 * writes behaviour as possible.
 		 */
 		ret = do_device_access(sip, scp, sg_off, lba, num, group, true, true);
-		/* If ZBC zone then bump its write pointer */
-		if (sdebug_dev_is_zoned(devip))
+		/* If ZBC zone then bump its write pointer, if all data was written */
+		if (sdebug_dev_is_zoned(devip) && ret == num_by)
 			zbc_inc_wp(devip, lba, num);
 		if (unlikely(scsi_debug_lbp()))
 			map_region(sip, lba, num);
-- 
2.55.0

[PATCH v2 2/4] scsi: scsi_debug: Enforce physical block alignment of zoned writes

[Niklas Cassel]

ZBC-3 r06 (T10/BSR INCITS 579), 4.5.3.3.2 Write access pattern
requirements for sequential write required zones, states:

  The device server terminates with CHECK CONDITION status, with the
  sense key set to ILLEGAL REQUEST, and the additional sense code set
  to UNALIGNED WRITE COMMAND a write command, other than an entire
  medium write same command, that specifies:
    a) the starting LBA in a sequential write required zone set to a
       value that is not equal to the write pointer for that sequential
       write required zone; or
    b) an ending LBA that is not equal to the last logical block within
       a physical block (see SBC-5).

That is why sd_zbc_read_zones() sets the zone_write_granularity queue
limit to the physical block size of a host-managed device, exposing the
constraint to user space.

check_zbc_access_params() implements condition a) but not condition b):
it verifies that a write to a sequential write required zone starts at
the write pointer of the zone, but never validates the ending LBA. As a
consequence, when scsi_debug emulates a host-managed device whose
physical block size is larger than its logical block size, for instance
with zbc=managed sector_size=512 physblk_exp=3, a write of a single
logical block at the write pointer of a sequential zone is accepted and
advances the write pointer by one logical block. The write pointer is
then no longer a multiple of the zone_write_granularity reported for the
device, so nothing can write at it at the granularity that was
advertised, and the zone can only be used again after being reset.

Implement condition b) as well, with the same sense data as the write
pointer check, as the standard gives both conditions the same sense key
and additional sense code. The exclusion of an entire medium write same
command needs no special case: such a command spans the whole medium, so
it is already terminated with WRITE BOUNDARY VIOLATION by the preceding
check.

The check is placed in check_zbc_access_params(), which every command
that advances a zone write pointer reaches first: WRITE, WRITE SCATTERED
and WRITE SAME. Reads return earlier in the function and are unaffected.

Sequential write preferred zones, which are emulated for host-aware
devices with zbc=aware, are left alone: writes to them are not required
to be sequential, and Linux does not restrict the write granularity of
host-aware devices.

With the default physblk_exp=0, the physical block size equals the
logical block size and the new check is a no-op.

Assisted-by: LLM
Fixes: f0d1cf9378bd ("scsi: scsi_debug: Add ZBC zone commands")
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
Tested with:

  modprobe scsi_debug zbc=managed sector_size=512 physblk_exp=3 \
      zone_size_mb=8 dev_size_mb=128 zone_nr_conv=2

Before this patch, a one logical block WRITE(16) at the write pointer of
an empty sequential write required zone completes with GOOD status and
leaves the write pointer at LBA 0x18001, which is not a multiple of the
4096 byte zone_write_granularity reported for the device. After it, the
same command is terminated with ILLEGAL REQUEST / UNALIGNED WRITE
COMMAND and the zone is left EMPTY, while an aligned eight block write
is still accepted.

Compared to the version reviewed in v1, the only change is that the new
mk_sense_buffer() call uses the combined UNALIGNED_WRITE_COMMAND sense
code, as this is based on 7.4/scsi-staging.
---
 drivers/scsi/scsi_debug.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 3ce2dbdff177..ddcc7a920089 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -3980,6 +3980,16 @@ static int check_zbc_access_params(struct scsi_cmnd *scp,
 					UNALIGNED_WRITE_COMMAND);
 			return check_condition_result;
 		}
+		/*
+		 * Writes must end on a physical block boundary, that is, the
+		 * transfer length must be a multiple of the physical block
+		 * size.
+		 */
+		if (!IS_ALIGNED(lba + num, 1U << sdebug_physblk_exp)) {
+			mk_sense_buffer(scp, ILLEGAL_REQUEST,
+					UNALIGNED_WRITE_COMMAND);
+			return check_condition_result;
+		}
 	}
 
 	/* Handle implicit open of closed and empty zones */
-- 
2.55.0

[PATCH v2 3/4] scsi: scsi_debug: Map the region written by WRITE ATOMIC (16)

[Niklas Cassel]

When logical block provisioning is enabled, a command that writes user
data marks the region that it wrote in the provisioning map, so that
GET LBA STATUS reports the region as mapped. resp_write_dt0(),
resp_write_scat() and resp_write_same() all call map_region() for that.

resp_atomic_write() does not, so a WRITE ATOMIC (16) leaves the
provisioning map untouched, and GET LBA STATUS keeps reporting the
region as deallocated after it has been written. map_state(), which
GET LBA STATUS uses, is the only reader of the map, so that is the whole
of the effect.

Call map_region() the way resp_write_dt0() does.

Assisted-by: LLM
Fixes: 84f3a3c01d70 ("scsi: scsi_debug: Atomic write support")
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
Tested with lbpu=1 added to the configuration used for the rest of the
series. GET LBA STATUS reports a region that has never been written as
deallocated, and reports eight blocks as mapped after either an ordinary
WRITE(16) or a WRITE ATOMIC(16) of eight blocks at the same LBA. Only
the WRITE(16) case did so before this patch.
---
 drivers/scsi/scsi_debug.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index ddcc7a920089..5a08f02130ef 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -6213,6 +6213,9 @@ static int resp_atomic_write(struct scsi_cmnd *scp,
 	}
 
 	ret = do_device_access(sip, scp, 0, lba, len, 0, true, true);
+	if (unlikely(scsi_debug_lbp()))
+		map_region(sip, lba, len);
+
 	if (unlikely(ret == -1))
 		return DID_ERROR << 16;
 	if (unlikely(ret != len * sdebug_sector_size))
-- 
2.55.0

[PATCH v2 4/4] scsi: scsi_debug: Validate zone access for WRITE ATOMIC (16)

[Niklas Cassel]

WRITE ATOMIC (16) is a write command, so on a zoned device it is subject
to the access requirements of the zone that it addresses, and it advances
the write pointer of a sequential write required zone. See ZBC-3 r06
(T10/BSR INCITS 579), 4.5.3.3.2 Write access pattern requirements for
sequential write required zones.

resp_atomic_write() calls do_device_access() directly, without calling
check_device_access_params() first and without advancing the write
pointer afterwards. It is the only command that writes user data which
does not; WRITE, WRITE SCATTERED and WRITE SAME all go through
check_device_access_params(), which validates zone access for a zoned
device.

As a consequence, with zbc=managed atomic_wr=1, a WRITE ATOMIC (16) can
write anywhere within a sequential write required zone regardless of its
write pointer and zone condition, into a gap zone, or across a zone
boundary, and none of it is reflected in the zone state. The write
pointer is left where it was, so a subsequent REPORT ZONES does not
describe the data on the medium, and the next write at that write
pointer overwrites data that was written without error.

Validate the access and advance the write pointer the way
resp_write_dt0() does, holding the zone metadata write lock across both,
since the write pointer has to be read and updated atomically with
respect to other commands, and advancing it only when all of the data
was written.

Assisted-by: LLM
Fixes: 84f3a3c01d70 ("scsi: scsi_debug: Atomic write support")
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
Tested with:

  modprobe scsi_debug zbc=managed sector_size=512 physblk_exp=3 \
      zone_size_mb=8 dev_size_mb=128 zone_nr_conv=2 atomic_wr=1

issuing WRITE ATOMIC (16) with sg_raw. Before this patch, an eight block
atomic write at the write pointer of an empty sequential write required
zone completes with GOOD status and leaves the write pointer unchanged,
and one issued past the write pointer is accepted as well. After it, the
former advances the write pointer by eight blocks and the latter is
terminated with ILLEGAL REQUEST / UNALIGNED WRITE COMMAND.

A two block atomic write at the write pointer, which satisfies
atomic_wr_gran and atomic_wr_align but is smaller than the 4096 byte
physical block, is now rejected by the physical block alignment check
added earlier in this series. An atomic write to a conventional zone is
unaffected.

A WRITE ATOMIC (16) of eight blocks with a data-out buffer of one leaves
the write pointer where it was, as an atomic write that does not
complete has no effect.
---
 drivers/scsi/scsi_debug.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 5a08f02130ef..546a21fb342d 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -6212,10 +6212,27 @@ static int resp_atomic_write(struct scsi_cmnd *scp,
 		}
 	}
 
+	if (sdebug_dev_is_zoned(devip))
+		sdeb_meta_write_lock(sip);
+
+	ret = check_device_access_params(scp, lba, len, true);
+	if (ret) {
+		if (sdebug_dev_is_zoned(devip))
+			sdeb_meta_write_unlock(sip);
+		return ret;
+	}
+
 	ret = do_device_access(sip, scp, 0, lba, len, 0, true, true);
 	if (unlikely(scsi_debug_lbp()))
 		map_region(sip, lba, len);
 
+	/* If ZBC zone then bump its write pointer, if all data was written */
+	if (sdebug_dev_is_zoned(devip)) {
+		if (ret == len * sdebug_sector_size)
+			zbc_inc_wp(devip, lba, len);
+		sdeb_meta_write_unlock(sip);
+	}
+
 	if (unlikely(ret == -1))
 		return DID_ERROR << 16;
 	if (unlikely(ret != len * sdebug_sector_size))
-- 
2.55.0

Re: [PATCH v2 1/4] scsi: scsi_debug: Do not advance the write pointer of a failed write

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: scsi_debug: ZBC specification violation by returning GOOD status without advancing WP
- [Medium] scsi: scsi_debug: integer overflow in short write check allows incorrect WP advancement
--

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

scsi: scsi_debug: Do not advance the write pointer of a failed write

This commit modifies the write pointer advancement logic for ZBC sequential
write required zones. It ensures the write pointer is only incremented when
all requested data is successfully written.

> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index 6941809dfdb75..3ce2dbdff177c 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -5150,8 +5150,8 @@ static int resp_write_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
>  	if (unlikely(scsi_debug_lbp()))
>  		map_region(sip, lba, num);
>  
> -	/* If ZBC zone then bump its write pointer */
> -	if (sdebug_dev_is_zoned(devip))
> +	/* If ZBC zone then bump its write pointer, if all data was written */
> +	if (sdebug_dev_is_zoned(devip) && ret == num * sdebug_sector_size)

[Severity: Medium]
Could the multiplication num * sdebug_sector_size overflow 32-bit arithmetic
for large num? If the module is loaded with a capacity >= 4GB, an extremely
large num and an empty data-out buffer might cause do_device_access() to
safely return 0. If the multiplication overflows and evaluates to 0, this
check might erroneously pass and incorrectly advance the write pointer.

[Severity: High]
If do_device_access() returns a short count due to a small data-out buffer,
this check prevents advancing the write pointer. However, the function then
proceeds to return 0 (GOOD status) without calling scsi_set_resid(). Does
this violate the ZBC specification, which requires that if a write terminates
with GOOD status, the write pointer must be advanced by the transfer length?

Could this lead to silent data corruption or UNALIGNED WRITE COMMAND errors
on subsequent sequential writes?

>  		zbc_inc_wp(devip, lba, num);
>  	if (meta_data_locked)
>  		sdeb_meta_write_unlock(sip);

[ ... ]

> @@ -5314,8 +5314,8 @@ static int resp_write_scat(struct scsi_cmnd *scp,
>  		 * writes behaviour as possible.
>  		 */
>  		ret = do_device_access(sip, scp, sg_off, lba, num, group, true, true);
> -		/* If ZBC zone then bump its write pointer */
> -		if (sdebug_dev_is_zoned(devip))
> +		/* If ZBC zone then bump its write pointer, if all data was written */
> +		if (sdebug_dev_is_zoned(devip) && ret == num_by)

[Severity: Medium]
Does the num_by variable suffer from a similar 32-bit overflow issue?

Earlier in resp_write_scat(), num_by is calculated as num * lb_size. If
this truncates to 0 on a massive transfer, could this condition falsely
match a 0-byte return from do_device_access(), tricking the driver into
advancing the write pointer anyway?

[Severity: High]
Similar to resp_write_dt0(), if this skips the write pointer advancement for
scattered writes on short buffers, it still continues and eventually returns
0 (GOOD status) at the end of the function. Should this set a residual count
when skipping the write pointer increment to avoid ZBC specification
violations?

>  			zbc_inc_wp(devip, lba, num);
>  		if (unlikely(scsi_debug_lbp()))
>  			map_region(sip, lba, num);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260917110316.1104863-6-cassel@kernel.org?part=1

Re: [PATCH v2 3/4] scsi: scsi_debug: Map the region written by WRITE ATOMIC (16)

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] scsi_debug: data race between WRITE ATOMIC and UNMAP
--

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

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

This commit updates resp_atomic_write() to call map_region() when logical
block provisioning is enabled. This ensures GET LBA STATUS correctly
reports the region as mapped after an atomic write, mirroring the behavior
of standard writes.

> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index ddcc7a920089e..5a08f02130ef6 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -6213,6 +6213,9 @@ static int resp_atomic_write(struct scsi_cmnd *scp,
>  	}
>  
>  	ret = do_device_access(sip, scp, 0, lba, len, 0, true, true);
> +	if (unlikely(scsi_debug_lbp()))
> +		map_region(sip, lba, len);

[Severity: Medium]
Could this introduce a data race with concurrent unmap operations?

In resp_unmap(), unmap_region() modifies the data buffer via memset() and
clears map bits while protected only by sdeb_meta_write_lock(sip), never
acquiring the data lock macc_data_lck. Normal writes like resp_write_dt0()
explicitly acquire sdeb_meta_write_lock(sip) when scsi_debug_lbp() is true
to safely serialize data access with concurrent unmap operations.

However, resp_atomic_write() does not acquire sdeb_meta_write_lock(sip)
when scsi_debug_lbp() is true. It only holds the data lock via
sdeb_data_lock(sip, atomic) during do_device_access(). Since unmap holds
macc_meta_lck and the atomic write holds macc_data_lck, they appear to be
unsynchronized, leading to potential data corruption in the scsi_debug
simulated RAM disk buffer and provisioning map.

While a later patch in this series adds sdeb_meta_write_lock(sip) to
resp_atomic_write(), it appears to do so only for the
sdebug_dev_is_zoned(devip) condition, leaving the scsi_debug_lbp() race
unmitigated in the final state of the series.

Should sdeb_meta_write_lock(sip) be acquired to protect the provisioning
map modifications?

> +
>  	if (unlikely(ret == -1))
>  		return DID_ERROR << 16;
>  	if (unlikely(ret != len * sdebug_sector_size))

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260917110316.1104863-6-cassel@kernel.org?part=3

Re: [PATCH v2 1/4] scsi: scsi_debug: Do not advance the write pointer of a failed write

[Niklas Cassel]

On Thu, Sep 17, 2026 at 11:17:38AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] scsi: scsi_debug: ZBC specification violation by returning GOOD status without advancing WP
> - [Medium] scsi: scsi_debug: integer overflow in short write check allows incorrect WP advancement
> --
> 
> commit 2b6fea07514157218eac4ea45ff1ba17e05b5125
> Author: Niklas Cassel <cassel@kernel.org>
> 
> scsi: scsi_debug: Do not advance the write pointer of a failed write
> 
> This commit modifies the write pointer advancement logic for ZBC sequential
> write required zones. It ensures the write pointer is only incremented when
> all requested data is successfully written.
> 
> > diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> > index 6941809dfdb75..3ce2dbdff177c 100644
> > --- a/drivers/scsi/scsi_debug.c
> > +++ b/drivers/scsi/scsi_debug.c
> > @@ -5150,8 +5150,8 @@ static int resp_write_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
> >  	if (unlikely(scsi_debug_lbp()))
> >  		map_region(sip, lba, num);
> >  
> > -	/* If ZBC zone then bump its write pointer */
> > -	if (sdebug_dev_is_zoned(devip))
> > +	/* If ZBC zone then bump its write pointer, if all data was written */
> > +	if (sdebug_dev_is_zoned(devip) && ret == num * sdebug_sector_size)
> 
> [Severity: Medium]
> Could the multiplication num * sdebug_sector_size overflow 32-bit arithmetic
> for large num? If the module is loaded with a capacity >= 4GB, an extremely
> large num and an empty data-out buffer might cause do_device_access() to
> safely return 0. If the multiplication overflows and evaluates to 0, this
> check might erroneously pass and incorrectly advance the write pointer.
> 
> [Severity: High]
> If do_device_access() returns a short count due to a small data-out buffer,
> this check prevents advancing the write pointer. However, the function then
> proceeds to return 0 (GOOD status) without calling scsi_set_resid(). Does
> this violate the ZBC specification, which requires that if a write terminates
> with GOOD status, the write pointer must be advanced by the transfer length?
> 
> Could this lead to silent data corruption or UNALIGNED WRITE COMMAND errors
> on subsequent sequential writes?

So it turns out that this driver is missing calls to:
scsi_set_resid() in:

resp_write_dt0(), resp_write_scat(), resp_atomic_write().

Interestingly, the driver does correctly call scsi_set_resid()
for read commands.

Will fix in v3, and will improve the WP advancement code to only
advance the blocks that was actually written (rather than leaving
the WP alone when not all blocks were written).


Kind regards,
Niklas

Re: [PATCH v2 1/4] scsi: scsi_debug: Do not advance the write pointer of a failed write

[Damien Le Moal]

On 2026/09/17 18:03, Niklas Cassel wrote:
> resp_write_dt0() and resp_write_scat() advance the write pointer of a
> sequential write required zone by the transfer length of the command
> before looking at what do_device_access() returned. It returns -1 when
> the data direction of the command does not match the operation, and a
> short byte count when the data-out buffer is smaller than the transfer
> length, both of which an initiator can produce with SG_IO.
> 
> The zone state then describes more data than is on the medium, and a
> write at the position where the data really ends is terminated with
> UNALIGNED WRITE COMMAND, because the write pointer has moved beyond it.
> The zone has to be reset before it can be written to again.
> 
> Leave the write pointer alone unless all of the data was written. A host
> that retries the command writes at the write pointer, where whatever the
> failed command did write begins, so the retry overwrites it.
> 
> Advancing over the data that was written instead would be no better,
> because do_device_access() copies one logical block at a time and stops
> at the first short copy, so it can report a byte count that is not a
> multiple of the physical block size. That would leave the write pointer
> in the middle of a physical block, and ZBC requires a write to a
> sequential write required zone to end on a physical block boundary, so
> no write can leave a real device with such a write pointer.
> 
> resp_write_same() does not need the same treatment, as it writes with
> memmove() and cannot fail part way through.
> 
> Assisted-by: LLM
> Fixes: f0d1cf9378bd ("scsi: scsi_debug: Add ZBC zone commands")
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
> Tested with:
> 
>   modprobe scsi_debug zbc=managed sector_size=512 physblk_exp=3 \
>       zone_size_mb=8 dev_size_mb=128 zone_nr_conv=2
> 
> issuing WRITE(16) with sg_raw so that the data-out buffer is shorter
> than the transfer length and do_device_access() returns a short count.
> Sixteen logical blocks with a buffer of twelve, and eight blocks with a
> buffer of one, both leave the write pointer where it was, and the full
> sixteen block write then succeeds from there. Before this patch both
> advanced the write pointer by the full transfer length, and a write at
> the end of the data that had actually been written was then terminated
> with UNALIGNED WRITE COMMAND.
> ---
>  drivers/scsi/scsi_debug.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index 6941809dfdb7..3ce2dbdff177 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -5150,8 +5150,8 @@ static int resp_write_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
>  	if (unlikely(scsi_debug_lbp()))
>  		map_region(sip, lba, num);
>  
> -	/* If ZBC zone then bump its write pointer */
> -	if (sdebug_dev_is_zoned(devip))
> +	/* If ZBC zone then bump its write pointer, if all data was written */
> +	if (sdebug_dev_is_zoned(devip) && ret == num * sdebug_sector_size)
>  		zbc_inc_wp(devip, lba, num);
>  	if (meta_data_locked)
>  		sdeb_meta_write_unlock(sip);
> @@ -5314,8 +5314,8 @@ static int resp_write_scat(struct scsi_cmnd *scp,
>  		 * writes behaviour as possible.
>  		 */
>  		ret = do_device_access(sip, scp, sg_off, lba, num, group, true, true);
> -		/* If ZBC zone then bump its write pointer */
> -		if (sdebug_dev_is_zoned(devip))
> +		/* If ZBC zone then bump its write pointer, if all data was written */
> +		if (sdebug_dev_is_zoned(devip) && ret == num_by)
>  			zbc_inc_wp(devip, lba, num);

This is *not* correct. As you note in the commit message, the buffer may be
smaller than the command length, in which case, do_device_access() returns the
smaller value, doing a partial write. And that is a thing in general with SCSI.
A command may not be processed entirely and completed with a non-zero residual.
For SMR, that translates into the WP advancing by the amount of data written, so
less than the command size. Such partial write with the WP being advanced may
also happen if a bad sector is encountered along the way and the drive fails to
remap it. An error is returned in such case, but the WP may still have advanced.
So here, the correct call should be:

		if (sdebug_dev_is_zoned(devip) && ret)
  			zbc_inc_wp(devip, lba, ret);

(the unit is wrong here since ret is in bytes, but you get the idea)

>  		if (unlikely(scsi_debug_lbp()))
>  			map_region(sip, lba, num);

And I am not sure about this... For now, I would leave it alone.


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 4/4] scsi: scsi_debug: Validate zone access for WRITE ATOMIC (16)

[Damien Le Moal]

On 2026/09/17 18:03, Niklas Cassel wrote:
> WRITE ATOMIC (16) is a write command, so on a zoned device it is subject
> to the access requirements of the zone that it addresses, and it advances
> the write pointer of a sequential write required zone. See ZBC-3 r06
> (T10/BSR INCITS 579), 4.5.3.3.2 Write access pattern requirements for
> sequential write required zones.
> 
> resp_atomic_write() calls do_device_access() directly, without calling
> check_device_access_params() first and without advancing the write
> pointer afterwards. It is the only command that writes user data which
> does not; WRITE, WRITE SCATTERED and WRITE SAME all go through
> check_device_access_params(), which validates zone access for a zoned
> device.
> 
> As a consequence, with zbc=managed atomic_wr=1, a WRITE ATOMIC (16) can
> write anywhere within a sequential write required zone regardless of its
> write pointer and zone condition, into a gap zone, or across a zone
> boundary, and none of it is reflected in the zone state. The write
> pointer is left where it was, so a subsequent REPORT ZONES does not
> describe the data on the medium, and the next write at that write
> pointer overwrites data that was written without error.
> 
> Validate the access and advance the write pointer the way
> resp_write_dt0() does, holding the zone metadata write lock across both,
> since the write pointer has to be read and updated atomically with
> respect to other commands, and advancing it only when all of the data
> was written.
> 
> Assisted-by: LLM
> Fixes: 84f3a3c01d70 ("scsi: scsi_debug: Atomic write support")
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
> Tested with:
> 
>   modprobe scsi_debug zbc=managed sector_size=512 physblk_exp=3 \
>       zone_size_mb=8 dev_size_mb=128 zone_nr_conv=2 atomic_wr=1
> 
> issuing WRITE ATOMIC (16) with sg_raw. Before this patch, an eight block
> atomic write at the write pointer of an empty sequential write required
> zone completes with GOOD status and leaves the write pointer unchanged,
> and one issued past the write pointer is accepted as well. After it, the
> former advances the write pointer by eight blocks and the latter is
> terminated with ILLEGAL REQUEST / UNALIGNED WRITE COMMAND.
> 
> A two block atomic write at the write pointer, which satisfies
> atomic_wr_gran and atomic_wr_align but is smaller than the 4096 byte
> physical block, is now rejected by the physical block alignment check
> added earlier in this series. An atomic write to a conventional zone is
> unaffected.
> 
> A WRITE ATOMIC (16) of eight blocks with a data-out buffer of one leaves
> the write pointer where it was, as an atomic write that does not
> complete has no effect.

Given that there are no SMR drives that support atomic writes that I know of, I
think we should not allow this at all, to mimic what user can actually encouter
instead of emulating the "perfect SMR drives that supports everything".
So rather the write pointer fix, I think we should have atomic write support and
ZBC emulation be mutually exclusive.

We can change that later if SMR drives supporting atomic writes ever become a thing.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 3/4] scsi: scsi_debug: Map the region written by WRITE ATOMIC (16)

[Damien Le Moal]

On 2026/09/17 18:03, Niklas Cassel wrote:
> When logical block provisioning is enabled, a command that writes user
> data marks the region that it wrote in the provisioning map, so that
> GET LBA STATUS reports the region as mapped. resp_write_dt0(),
> resp_write_scat() and resp_write_same() all call map_region() for that.
> 
> resp_atomic_write() does not, so a WRITE ATOMIC (16) leaves the
> provisioning map untouched, and GET LBA STATUS keeps reporting the
> region as deallocated after it has been written. map_state(), which
> GET LBA STATUS uses, is the only reader of the map, so that is the whole
> of the effect.
> 
> Call map_region() the way resp_write_dt0() does.
> 
> Assisted-by: LLM
> Fixes: 84f3a3c01d70 ("scsi: scsi_debug: Atomic write support")
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Looks OK to me.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research