[PATCH 0/2] scsi: scsi_debug: fix protection information for writes

[PATCH 0/2] scsi: scsi_debug: fix protection information for writes

[Tal Zussman]

With DIX enabled, blocks written by WRITE SAME or WRITE ATOMIC(16) on
scsi_debug fail the guard check when they are read back. A filesystem
made by mkfs.xfs on a previously formatted device can't be mounted, and
xfstests generic/770 fails on XFS.

The first patch makes WRITE ATOMIC(16) verify and store the protection
information it is sent, like WRITE. The second patch generates
protection information for writes that come without it, such as WRITE
SAME, as a device formatted with protection information does.

This was found while testing the atomic write bounce series [1]. The
fixes were proposed by an LLM.

[1] https://lore.kernel.org/r/20261107-blkdev-atomic-bounce-v1-0-f66344262096@columbia.edu

---
Tal Zussman (2):
      scsi: scsi_debug: handle protection information for WRITE ATOMIC(16)
      scsi: scsi_debug: generate protection information for writes without it

 drivers/scsi/scsi_debug.c | 61 +++++++++++++++++++++++++++++++++++++----------
 1 file changed, 49 insertions(+), 12 deletions(-)
---
base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
change-id: 20261107-scsi-debug-pi-f51019c898a0

Best regards,
--  
Tal Zussman <tz2294@columbia.edu>

[PATCH 1/2] scsi: scsi_debug: handle protection information for WRITE ATOMIC(16)

[Tal Zussman]

resp_atomic_write() writes the data but ignores the protection
information sent with the command, so the blocks keep the tags of their
previous contents and reading them back fails the guard check.

With scsi_debug loaded with dif=1 dix=1 atomic_wr=1, every RWF_ATOMIC
write that overwrites a block makes it unreadable, so xfstests
generic/770 fails on XFS.

Make resp_atomic_write() write through __resp_write_dt0() like WRITE
does, passing a new atomic argument down to do_device_access(). The
protection information is then verified and stored under the meta write
lock before the data is copied, as it is for WRITE.

Fixes: 84f3a3c01d70 ("scsi: scsi_debug: Atomic write support")
Signed-off-by: Tal Zussman <tz2294@columbia.edu>
---
 drivers/scsi/scsi_debug.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index a922c8318a2f..7d5065039637 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -5175,7 +5175,8 @@ static int __resp_write_dt0_dix(struct scsi_cmnd *scp,
 static int __resp_write_dt0(struct scsi_cmnd *scp,
 			    struct sdebug_dev_info *devip,
 			    struct sdeb_store_info *sip, u64 lba, u32 num,
-			    u32 ei_lba, u8 group, bool lbp, int *scsi_status)
+			    u32 ei_lba, u8 group, bool lbp, bool atomic,
+			    int *scsi_status)
 {
 	int ret;
 
@@ -5191,7 +5192,7 @@ static int __resp_write_dt0(struct scsi_cmnd *scp,
 			return ret;
 	}
 
-	ret = do_device_access(sip, scp, 0, lba, num, group, true, false);
+	ret = do_device_access(sip, scp, 0, lba, num, group, true, atomic);
 	if (ret < 0)
 		*scsi_status = DID_ERROR << 16;
 
@@ -5286,11 +5287,11 @@ static int resp_write_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 	    (sdebug_dix && scsi_prot_sg_count(scp)) || lbp) {
 		sdeb_meta_write_lock(sip);
 		ret = __resp_write_dt0(scp, devip, sip, lba, num, ei_lba, group,
-				       lbp, &scsi_status);
+				       lbp, false, &scsi_status);
 		sdeb_meta_write_unlock(sip);
 	} else {
 		ret = __resp_write_dt0(scp, devip, sip, lba, num, ei_lba, group,
-				       lbp, &scsi_status);
+				       lbp, false, &scsi_status);
 	}
 
 	if (unlikely(-1 == ret))
@@ -6328,6 +6329,7 @@ static int resp_atomic_write(struct scsi_cmnd *scp,
 	u16 boundary, len;
 	u64 lba, lba_tmp;
 	bool lbp = scsi_debug_lbp();
+	int scsi_status = DID_ERROR << 16;
 	int ret;
 
 	if (!scsi_debug_atomic_write()) {
@@ -6384,17 +6386,18 @@ static int resp_atomic_write(struct scsi_cmnd *scp,
 	if (scsi_bufflen(scp) < len * sdebug_sector_size)
 		return DID_ERROR << 16;
 
-	if (lbp) {
+	if ((sdebug_dix && scsi_prot_sg_count(scp)) || lbp) {
 		sdeb_meta_write_lock(sip);
-		ret = do_device_access(sip, scp, 0, lba, len, 0, true, true);
-		map_region(sip, lba, len);
+		ret = __resp_write_dt0(scp, devip, sip, lba, len, 0, 0, lbp,
+				       true, &scsi_status);
 		sdeb_meta_write_unlock(sip);
 	} else {
-		ret = do_device_access(sip, scp, 0, lba, len, 0, true, true);
+		ret = __resp_write_dt0(scp, devip, sip, lba, len, 0, 0, lbp,
+				       true, &scsi_status);
 	}
 
 	if (unlikely(ret < 0))
-		return DID_ERROR << 16;
+		return scsi_status;
 	if (unlikely(ret != len * sdebug_sector_size))
 		return DID_ERROR << 16;
 

-- 
2.39.5

[PATCH 2/2] scsi: scsi_debug: generate protection information for writes without it

[Tal Zussman]

With DIX enabled, scsi_debug only updates the protection information of
blocks written by a command that carries it. Any other write leaves the
tags of the blocks' previous contents in place, so reading them back
fails the guard check. A real device formatted with protection
information generates it for such writes.

sd never sends protection information with WRITE SAME, so a BLKZEROOUT
over previously written blocks makes them unreadable. mkfs.xfs zeroes
the log this way, so formatting a device a second time leaves a
filesystem that can't be mounted:

  # modprobe scsi_debug sector_size=4096 dev_size_mb=1024 dif=1 dix=1
  # mkfs.xfs -f /dev/sda && mount /dev/sda /mnt && umount /mnt
  # mkfs.xfs -f /dev/sda && mount /dev/sda /mnt
  mount: /mnt: mount(2) system call failed: Invalid or incomplete multibyte or wide character.

The guard tag is still that of the log the first filesystem wrote,
while the block now holds zeroes:

scsi_debug:dif_verify: GUARD check failed on sector 131593 rcvd 0xe313, data 0x0000
XFS (sda): log recovery read I/O error at daddr 0xff8 len 8 error -84
XFS (sda): log mount failed

COMPARE AND WRITE has the same problem, as do WRITE, WRITE SCATTERED
and WRITE ATOMIC(16) when they are sent without protection
information.

Generate the protection information for these writes: the guard tag
from the data, the LBA as the reference tag and an application tag of
FFFFh. scsi_debug does not check a block with that application tag when
it is read back.

WRITE SAME writes the same data to every block, so compute its guard
tag only once. Take the meta write lock whenever DIX is enabled, so
generating the tags is serialized with the reads that verify and return
them.

Fixes: c6a44287417d ("[SCSI] scsi_debug: DIF/DIX support")
Signed-off-by: Tal Zussman <tz2294@columbia.edu>
---
 drivers/scsi/scsi_debug.c | 42 ++++++++++++++++++++++++++++++++++++++----
 1 file changed, 38 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 7d5065039637..2c7afdd2846b 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -4434,6 +4434,32 @@ static __be16 dif_compute_csum(const void *buf, int len)
 	return csum;
 }
 
+/*
+ * Generate the protection information for blocks written without any, as a
+ * device formatted with protection information does. The guard tag is only
+ * computed once if @same says that the blocks all hold the same data.
+ */
+static void dif_generate(struct sdeb_store_info *sip, u64 lba, u32 num,
+			 bool same)
+{
+	__be16 guard = 0;
+	u32 i;
+
+	if (!sip->dif_storep)
+		return;
+
+	for (i = 0; i < num; i++) {
+		struct t10_pi_tuple *sdt = dif_store(sip, lba + i);
+
+		if (!same || !i)
+			guard = dif_compute_csum(lba2fake_store(sip, lba + i),
+						 sdebug_sector_size);
+		sdt->guard_tag = guard;
+		sdt->app_tag = T10_PI_APP_ESCAPE;
+		sdt->ref_tag = cpu_to_be32(lower_32_bits(lba + i));
+	}
+}
+
 static int dif_verify(struct t10_pi_tuple *sdt, const void *data,
 		      sector_t sector, u32 ei_lba)
 {
@@ -5195,6 +5221,9 @@ static int __resp_write_dt0(struct scsi_cmnd *scp,
 	ret = do_device_access(sip, scp, 0, lba, num, group, true, atomic);
 	if (ret < 0)
 		*scsi_status = DID_ERROR << 16;
+	else if (!scsi_prot_sg_count(scp))
+		dif_generate(sip, lba, DIV_ROUND_UP(ret, sdebug_sector_size),
+			     false);
 
 	if (unlikely(lbp))
 		map_region(sip, lba, num);
@@ -5283,8 +5312,7 @@ static int resp_write_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 				    "to DIF device\n");
 	}
 
-	if (sdebug_dev_is_zoned(devip) ||
-	    (sdebug_dix && scsi_prot_sg_count(scp)) || lbp) {
+	if (sdebug_dev_is_zoned(devip) || sdebug_dix || lbp) {
 		sdeb_meta_write_lock(sip);
 		ret = __resp_write_dt0(scp, devip, sip, lba, num, ei_lba, group,
 				       lbp, false, &scsi_status);
@@ -5473,6 +5501,10 @@ static int resp_write_scat(struct scsi_cmnd *scp,
 			sdev_printk(KERN_INFO, scp->device,
 			    "%s: write: cdb indicated=%llu, IO sent=%d bytes\n",
 			    my_name, num_by, ret);
+		if (!scsi_prot_sg_count(scp))
+			dif_generate(sip, lba,
+				     DIV_ROUND_UP(ret, sdebug_sector_size),
+				     false);
 
 		if (unlikely((sdebug_opts & SDEBUG_OPT_RECOV_DIF_DIX) &&
 			     atomic_read(&sdeb_inject_pending))) {
@@ -5564,6 +5596,7 @@ static int __resp_write_same(struct scsi_cmnd *scp, u64 lba, u32 num,
 		block = do_div(lbaa, sdebug_store_sectors);
 		memmove(fsp + (block * lb_size), fs1p, lb_size);
 	}
+	dif_generate(sip, lba, num, true);
 	if (lbp)
 		map_region(sip, lba, num);
 	/* If ZBC zone then bump its write pointer */
@@ -5584,7 +5617,7 @@ static int resp_write_same(struct scsi_cmnd *scp, u64 lba, u32 num, u32 ei_lba,
 	const bool lbp = scsi_debug_lbp();
 	int ret;
 
-	if (sdebug_dev_is_zoned(devip) || lbp) {
+	if (sdebug_dev_is_zoned(devip) || sdebug_dix || lbp) {
 		sdeb_meta_write_lock(sip);
 		ret = __resp_write_same(scp, lba, num, ei_lba, unmap, ndob,
 					lbp);
@@ -5753,6 +5786,7 @@ static int resp_comp_write(struct scsi_cmnd *scp,
 		retval = check_condition_result;
 		goto cleanup_unlock;
 	}
+	dif_generate(sip, lba, num, false);
 
 	/* Cover sip->map_storep (which map_region() sets) with the meta lock */
 	if (scsi_debug_lbp())
@@ -6386,7 +6420,7 @@ static int resp_atomic_write(struct scsi_cmnd *scp,
 	if (scsi_bufflen(scp) < len * sdebug_sector_size)
 		return DID_ERROR << 16;
 
-	if ((sdebug_dix && scsi_prot_sg_count(scp)) || lbp) {
+	if (sdebug_dix || lbp) {
 		sdeb_meta_write_lock(sip);
 		ret = __resp_write_dt0(scp, devip, sip, lba, len, 0, 0, lbp,
 				       true, &scsi_status);

-- 
2.39.5

Re: [PATCH 1/2] scsi: scsi_debug: handle protection information for WRITE ATOMIC(16)

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Medium] scsi: scsi_debug: incorrect CDB byte offset in
  resp_atomic_write() [1]
--

commit 48252d9d72a1d4d7e89b4f529cfc1af6aee082f9
Author: Tal Zussman <tz2294@columbia.edu>

scsi: scsi_debug: handle protection information for WRITE ATOMIC(16)

This commit routes resp_atomic_write() through __resp_write_dt0() so that
protection information is verified and stored for WRITE ATOMIC(16)
commands.

> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index a922c8318a2fb..7d50650396377 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
[ ... ]
> @@ -6328,6 +6329,7 @@ static int resp_atomic_write(struct scsi_cmnd *scp,
>  	u16 boundary, len;
>  	u64 lba, lba_tmp;
>  	bool lbp = scsi_debug_lbp();
> +	int scsi_status = DID_ERROR << 16;
>  	int ret;
>  
>  	if (!scsi_debug_atomic_write()) {

[Severity: Medium]
This problem wasn't introduced by this patch, but does resp_atomic_write()
report the wrong CDB byte offset [1] when boundary exceeds
sdebug_atomic_wr_max_bndry?

drivers/scsi/scsi_debug.c:resp_atomic_write() {
	...
	boundary = get_unaligned_be16(cmd + 10);
	len = get_unaligned_be16(cmd + 12);
	...
	if (boundary > 0) {
		if (boundary > sdebug_atomic_wr_max_bndry) {
			mk_sense_invalid_fld(scp, SDEB_IN_CDB, 12, -1);
			return check_condition_result;
		}
	...
}

boundary is parsed from CDB byte 10, while byte 12 is the transfer length
field, so the sense data points the initiator at the transfer length
instead of the atomic boundary field.

[1] https://sashiko.dev/#/bug/linux-0276b62a-b78e-4b07-bc67-b1a9f88dc08e

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-scsi-debug-pi-v1-0-539829902b3a@columbia.edu?part=1

Re: [PATCH 2/2] scsi: scsi_debug: generate protection information for writes without it

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 2 related pre-existing issues it does not
introduce:
- [Critical] scsi: scsi_debug: LBP and protection state corruption on
  partial writes [1]
- [Critical] scsi: scsi_debug: write protection bypass in
  resp_comp_write() [2]
--

commit 6d076bfd0f944479df3df26a58ea22111f72afc1
Author: Tal Zussman <tz2294@columbia.edu>

scsi: scsi_debug: generate protection information for writes without it

This commit generates T10 protection information in scsi_debug for write
commands submitted without protection scatterlists, and extends the metadata
write lock in write handlers to cover DIX updates.

> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index 7d50650396377..2c7afdd2846b1 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
[ ... ]
> @@ -5195,6 +5221,9 @@ static int __resp_write_dt0(struct scsi_cmnd *scp,
>  	ret = do_device_access(sip, scp, 0, lba, num, group, true, atomic);
>  	if (ret < 0)
>  		*scsi_status = DID_ERROR << 16;
> +	else if (!scsi_prot_sg_count(scp))
> +		dif_generate(sip, lba, DIV_ROUND_UP(ret, sdebug_sector_size),
> +			     false);
>  
>  	if (unlikely(lbp))
>  		map_region(sip, lba, num);

[Severity: Critical]
This problem wasn't introduced by this patch, but can a failed or short
write in __resp_write_dt0() and resp_write_scat() corrupt the LBP and DIX
metadata [1]?

While dif_generate() uses the actual byte count returned by
do_device_access(), map_region() right below it (and __resp_write_dt0_dix()
or prot_verify_write() before the transfer) still operates on the full
requested sector count num.  If do_device_access() fails or transfers fewer
bytes than requested, unwritten sectors are marked as mapped in
sip->map_storep and have their protection tags overwritten in
sip->dif_storep, desynchronizing the metadata from the backing store and
causing subsequent reads of those sectors to fail verification.

[ ... ]
> @@ -5753,6 +5786,7 @@ static int resp_comp_write(struct scsi_cmnd *scp,
>  		retval = check_condition_result;
>  		goto cleanup_unlock;
>  	}
> +	dif_generate(sip, lba, num, false);
>  
>  	/* Cover sip->map_storep (which map_region() sets) with the meta lock */
>  	if (scsi_debug_lbp())

[Severity: Critical]
This problem wasn't introduced by this patch, but does resp_comp_write()
bypass device write protection by passing false for the write argument to
check_device_access_params() [2]?

drivers/scsi/scsi_debug.c:resp_comp_write() {
	...
	ret = check_device_access_params(scp, lba, num, false);
	if (ret)
		return ret;
	...
}

Because write is false, check_device_access_params() skips the sdebug_wp
check, allowing COMPARE AND WRITE to modify data and protection metadata on
a write-protected device.

[1] https://sashiko.dev/#/bug/linux-5ff253b7-5155-4709-ae8b-55ff4a1eb98f
[2] https://sashiko.dev/#/bug/linux-0b24b1f8-28c0-4efa-915b-66147893a0f1

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-scsi-debug-pi-v1-0-539829902b3a@columbia.edu?part=2