[PATCH 0/2] scsi: target: core: Fix and convert kmap_atomic() in SBC emulation

[PATCH 0/2] scsi: target: core: Fix and convert kmap_atomic() in SBC emulation

[Danish Khateeb]

Patch 1 fixes the address sbc_dif_copy_prot() passes to kunmap_atomic(),
which has been off by one page since v4.2. It keeps kmap_atomic() so it
can be backported on its own, which is why checkpatch warns about the
deprecated call there. Patch 2 then converts the kmap_atomic() calls in
target_core_sbc.c to kmap_local_page().

The rd backend's protection pages are lowmem, so the bug only shows on
32-bit x86 with CONFIG_DEBUG_HIGHMEM, which gives lowmem pages a real
kmap slot too. I have not Cc'd stable for that reason.

Testing, in QEMU/KVM, with PREEMPT_LAZY and DEBUG_PREEMPT:

- i386 with HIGHMEM and DEBUG_HIGHMEM. On fe2ec83746e5 (v7.3-rc4+),
  kunmap_local_indexed() warns from sbc_dif_copy_prot(), the log has 103
  "DIFv1 checksum failed" errors, and 9 of the 23 checks fail with
  EILSEQ or EIO on the rd PI disk. With patch 1, and with both patches,
  all 23 pass with no warnings.
- x86-64 with KASAN. All 23 checks pass before and after the series,
  with no reports.

A static init sets up three rd_mcp devices through configfs and exports
them through tcm_loop: one with pi_prot_type=1, one without PI behind a
TPG with fabric_prot_type=1, and a plain one. It then runs O_DIRECT and
buffered writes and read-backs on the two PI disks, SG_IO WRITE(10) and
READ(10) without PI so the target inserts and strips it, and COMPARE AND
WRITE with a match and with a miscompare at byte 403. Kprobes show that
sbc_dif_generate(), sbc_dif_verify(), sbc_dif_copy_prot() and
compare_and_write_callback(), which inlines compare_and_write_do_cmp(),
all ran. W=1 and sparse are clean for target_core_sbc.c. I can post the
init program if it is useful.

Danish Khateeb (2):
  scsi: target: core: Fix kunmap_atomic() address in sbc_dif_copy_prot()
  scsi: target: core: Use kmap_local_page() in SBC emulation

 drivers/target/target_core_sbc.c | 58 ++++++++++++++++----------------
 1 file changed, 29 insertions(+), 29 deletions(-)


base-commit: fe2ec83746e501645709761605c2464a44fd2929
-- 
2.55.0

[PATCH 1/2] scsi: target: core: Fix kunmap_atomic() address in sbc_dif_copy_prot()

[Danish Khateeb]

sbc_dif_copy_prot() copies protection information between the command's
protection SGL and the backend's SGL "sg". It maps each page of "sg" with
kmap_atomic() and unmaps it with kunmap_atomic(addr - sg->offset - offset).
The unmap runs after offset has been advanced by len, so it is passed an
address len bytes below the start of the mapped page, inside the page
below the mapping.

The only caller, rd_do_prot_rw(), passes the rd backend's protection
pages, which come from alloc_pages(GFP_KERNEL). kmap_atomic() of a lowmem
page returns its linear address, and kunmap_atomic() of a linear address
has nothing to unmap, so the wrong address has gone unnoticed. It matters
on 32-bit x86 with CONFIG_DEBUG_HIGHMEM, which selects
CONFIG_DEBUG_KMAP_LOCAL_FORCE_MAP so that lowmem pages get a real
temporary mapping too. kunmap_local_indexed() then warns

  WARNING: mm/highmem.c:623 at kunmap_local_indexed+0x148/0x190
  Workqueue: target_submission target_queued_submit_work
  Call Trace:
   sbc_dif_copy_prot+0xef/0x310
   rd_do_prot_rw+0x115/0x140
   rd_execute_rw+0x354/0x3b0
   sbc_execute_rw+0x2b/0x40
   __target_execute_cmd+0x22/0xb0

and clears the right PTE, but x86 flushes the TLB entry of the wrong
address. The stale entry keeps the slot pointing at the old page, so the
next page mapped there is not the one accessed. With an rd device with
pi_prot_type=1 exported through tcm_loop, reads and writes then fail
with "DIFv1 checksum failed" errors.

Unmap the page before advancing offset, so kunmap_atomic() gets the
address kmap_atomic() returned.

Fixes: 57636388af32 ("target: Fix inconsistent address passed to kunmap_atomic() in sbc_dif_copy_prot()")
Assisted-by: LLM
Signed-off-by: Danish Khateeb <danishkhateeb03@gmail.com>
---
 drivers/target/target_core_sbc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 21f5cb86d70c..55a8c2f0a286 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -1347,13 +1347,13 @@ void sbc_dif_copy_prot(struct se_cmd *cmd, unsigned int sectors, bool read,
 			else
 				memcpy(addr, paddr + copied, len);
 
+			kunmap_atomic(addr - sg->offset - offset);
+
 			left -= len;
 			offset += len;
 			copied += len;
 			psg_len -= len;
 
-			kunmap_atomic(addr - sg->offset - offset);
-
 			if (offset >= sg->length) {
 				sg = sg_next(sg);
 				offset = 0;
-- 
2.55.0

[PATCH 2/2] scsi: target: core: Use kmap_local_page() in SBC emulation

[Danish Khateeb]

kmap_atomic() is deprecated in favour of kmap_local_page(). Convert the
kmap_atomic() and kunmap_atomic() calls in sbc_dif_generate(),
sbc_dif_verify(), sbc_dif_copy_prot() and compare_and_write_do_cmp().

The mappings already nest the way kmap_local_page() requires. The
protection page is mapped first and unmapped last, and the data page (the
rd protection page in sbc_dif_copy_prot()) is mapped and unmapped inside
it, including on the early returns. compare_and_write_do_cmp() maps one
page at a time. The code between map and unmap only works on kernel
memory with memcpy(), memcmp(), the CRC helpers and pr_*(), so it does
not rely on page faults or preemption being disabled.

Apart from dropping the deprecated calls, this lets preemptible kernels
preempt the guard tag CRCs in sbc_dif_generate() and sbc_dif_verify(),
which run inside the mapping of a protection SGL entry: up to 512 blocks
per page of protection information, 2 MiB of data with 4 KiB blocks.

Assisted-by: LLM
Signed-off-by: Danish Khateeb <danishkhateeb03@gmail.com>
---
 drivers/target/target_core_sbc.c | 56 ++++++++++++++++----------------
 1 file changed, 28 insertions(+), 28 deletions(-)

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 55a8c2f0a286..2c79bca5f6e4 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -406,7 +406,7 @@ compare_and_write_do_cmp(struct scatterlist *read_sgl, unsigned int read_nents,
 	ret = TCM_NO_SENSE;
 	for_each_sg(read_sgl, sg, read_nents, sg_cnt) {
 		unsigned int len = min(sg->length, cmp_len);
-		unsigned char *addr = kmap_atomic(sg_page(sg));
+		unsigned char *addr = kmap_local_page(sg_page(sg));
 
 		if (memcmp(addr, buf + offset, len)) {
 			unsigned int i;
@@ -418,7 +418,7 @@ compare_and_write_do_cmp(struct scatterlist *read_sgl, unsigned int read_nents,
 				*miscmp_off);
 			ret = TCM_MISCOMPARE_VERIFY;
 		}
-		kunmap_atomic(addr);
+		kunmap_local(addr);
 		if (ret != TCM_NO_SENSE)
 			goto out;
 
@@ -1222,8 +1222,8 @@ sbc_dif_generate(struct se_cmd *cmd)
 	unsigned int block_size = dev->dev_attrib.block_size;
 
 	for_each_sg(cmd->t_prot_sg, psg, cmd->t_prot_nents, i) {
-		paddr = kmap_atomic(sg_page(psg)) + psg->offset;
-		daddr = kmap_atomic(sg_page(dsg)) + dsg->offset;
+		paddr = kmap_local_page(sg_page(psg)) + psg->offset;
+		daddr = kmap_local_page(sg_page(dsg)) + dsg->offset;
 
 		for (j = 0; j < psg->length;
 				j += sizeof(*sdt)) {
@@ -1232,26 +1232,26 @@ sbc_dif_generate(struct se_cmd *cmd)
 
 			if (offset >= dsg->length) {
 				offset -= dsg->length;
-				kunmap_atomic(daddr - dsg->offset);
+				kunmap_local(daddr - dsg->offset);
 				dsg = sg_next(dsg);
 				if (!dsg) {
-					kunmap_atomic(paddr - psg->offset);
+					kunmap_local(paddr - psg->offset);
 					return;
 				}
-				daddr = kmap_atomic(sg_page(dsg)) + dsg->offset;
+				daddr = kmap_local_page(sg_page(dsg)) + dsg->offset;
 			}
 
 			sdt = paddr + j;
 			avail = min(block_size, dsg->length - offset);
 			crc = crc_t10dif(daddr + offset, avail);
 			if (avail < block_size) {
-				kunmap_atomic(daddr - dsg->offset);
+				kunmap_local(daddr - dsg->offset);
 				dsg = sg_next(dsg);
 				if (!dsg) {
-					kunmap_atomic(paddr - psg->offset);
+					kunmap_local(paddr - psg->offset);
 					return;
 				}
-				daddr = kmap_atomic(sg_page(dsg)) + dsg->offset;
+				daddr = kmap_local_page(sg_page(dsg)) + dsg->offset;
 				offset = block_size - avail;
 				crc = crc_t10dif_update(crc, daddr, offset);
 			} else {
@@ -1273,8 +1273,8 @@ sbc_dif_generate(struct se_cmd *cmd)
 			sector++;
 		}
 
-		kunmap_atomic(daddr - dsg->offset);
-		kunmap_atomic(paddr - psg->offset);
+		kunmap_local(daddr - dsg->offset);
+		kunmap_local(paddr - psg->offset);
 	}
 }
 
@@ -1336,18 +1336,18 @@ void sbc_dif_copy_prot(struct se_cmd *cmd, unsigned int sectors, bool read,
 	for_each_sg(cmd->t_prot_sg, psg, cmd->t_prot_nents, i) {
 		unsigned int psg_len, copied = 0;
 
-		paddr = kmap_atomic(sg_page(psg)) + psg->offset;
+		paddr = kmap_local_page(sg_page(psg)) + psg->offset;
 		psg_len = min(left, psg->length);
 		while (psg_len) {
 			len = min(psg_len, sg->length - offset);
-			addr = kmap_atomic(sg_page(sg)) + sg->offset + offset;
+			addr = kmap_local_page(sg_page(sg)) + sg->offset + offset;
 
 			if (read)
 				memcpy(paddr + copied, addr, len);
 			else
 				memcpy(addr, paddr + copied, len);
 
-			kunmap_atomic(addr - sg->offset - offset);
+			kunmap_local(addr - sg->offset - offset);
 
 			left -= len;
 			offset += len;
@@ -1359,7 +1359,7 @@ void sbc_dif_copy_prot(struct se_cmd *cmd, unsigned int sectors, bool read,
 				offset = 0;
 			}
 		}
-		kunmap_atomic(paddr - psg->offset);
+		kunmap_local(paddr - psg->offset);
 	}
 }
 EXPORT_SYMBOL(sbc_dif_copy_prot);
@@ -1379,8 +1379,8 @@ sbc_dif_verify(struct se_cmd *cmd, sector_t start, unsigned int sectors,
 	unsigned int block_size = dev->dev_attrib.block_size;
 
 	for (; psg && sector < start + sectors; psg = sg_next(psg)) {
-		paddr = kmap_atomic(sg_page(psg)) + psg->offset;
-		daddr = kmap_atomic(sg_page(dsg)) + dsg->offset;
+		paddr = kmap_local_page(sg_page(psg)) + psg->offset;
+		daddr = kmap_local_page(sg_page(dsg)) + dsg->offset;
 
 		for (i = psg_off; i < psg->length &&
 				sector < start + sectors;
@@ -1390,13 +1390,13 @@ sbc_dif_verify(struct se_cmd *cmd, sector_t start, unsigned int sectors,
 
 			if (dsg_off >= dsg->length) {
 				dsg_off -= dsg->length;
-				kunmap_atomic(daddr - dsg->offset);
+				kunmap_local(daddr - dsg->offset);
 				dsg = sg_next(dsg);
 				if (!dsg) {
-					kunmap_atomic(paddr - psg->offset);
+					kunmap_local(paddr - psg->offset);
 					return 0;
 				}
-				daddr = kmap_atomic(sg_page(dsg)) + dsg->offset;
+				daddr = kmap_local_page(sg_page(dsg)) + dsg->offset;
 			}
 
 			sdt = paddr + i;
@@ -1414,13 +1414,13 @@ sbc_dif_verify(struct se_cmd *cmd, sector_t start, unsigned int sectors,
 			avail = min(block_size, dsg->length - dsg_off);
 			crc = crc_t10dif(daddr + dsg_off, avail);
 			if (avail < block_size) {
-				kunmap_atomic(daddr - dsg->offset);
+				kunmap_local(daddr - dsg->offset);
 				dsg = sg_next(dsg);
 				if (!dsg) {
-					kunmap_atomic(paddr - psg->offset);
+					kunmap_local(paddr - psg->offset);
 					return 0;
 				}
-				daddr = kmap_atomic(sg_page(dsg)) + dsg->offset;
+				daddr = kmap_local_page(sg_page(dsg)) + dsg->offset;
 				dsg_off = block_size - avail;
 				crc = crc_t10dif_update(crc, daddr, dsg_off);
 			} else {
@@ -1429,8 +1429,8 @@ sbc_dif_verify(struct se_cmd *cmd, sector_t start, unsigned int sectors,
 
 			rc = sbc_dif_v1_verify(cmd, sdt, crc, sector, ei_lba);
 			if (rc) {
-				kunmap_atomic(daddr - dsg->offset);
-				kunmap_atomic(paddr - psg->offset);
+				kunmap_local(daddr - dsg->offset);
+				kunmap_local(paddr - psg->offset);
 				cmd->sense_info = sector;
 				return rc;
 			}
@@ -1440,8 +1440,8 @@ sbc_dif_verify(struct se_cmd *cmd, sector_t start, unsigned int sectors,
 		}
 
 		psg_off = 0;
-		kunmap_atomic(daddr - dsg->offset);
-		kunmap_atomic(paddr - psg->offset);
+		kunmap_local(daddr - dsg->offset);
+		kunmap_local(paddr - psg->offset);
 	}
 
 	return 0;
-- 
2.55.0