[PATCH v2 0/4] target: Fix several problems related to T10-PI support

[PATCH v2 0/4] target: Fix several problems related to T10-PI support

[Akinobu Mita]

This patchset aims to fix several problems related to T10-PI support.

These patches can be applied on top of Sagi's "[v1] Simlify dif_verify
routines and fixup fileio protection information code" patchset.

* Changes from v1:
- Reduce code duplication a bit in target_read_prot_action()
- Fix sbc_dif_verify() for WRITE_SAME command
- Fix inverted rw argument for fd_do_rw()
- Perform DIF verify before write for WRITE_SAME

Akinobu Mita (4):
  target: ensure se_cmd->t_prot_sg is allocated when required
  target: Abandon odd SG mapping for data transfer memory
  target: Fix sbc_dif_generate() and sbc_dif_verify() for WRITE SAME
  target/file: enable WRITE SAME when protection info is enabled

 drivers/target/target_core_file.c      | 18 ++++++++----
 drivers/target/target_core_sbc.c       | 49 +++++++++++++++++++++++++++++--
 drivers/target/target_core_transport.c | 53 +++++++++++++++++++---------------
 include/target/target_core_backend.h   |  2 +-
 include/target/target_core_base.h      |  1 +
 5 files changed, 91 insertions(+), 32 deletions(-)

Cc: Nicholas Bellinger <nab@linux-iscsi.org>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
Cc: target-devel@vger.kernel.org
Cc: linux-scsi@vger.kernel.org

-- 
1.9.1

[PATCH v2 1/4] target: ensure se_cmd->t_prot_sg is allocated when required

[Akinobu Mita]

Even if the device backend is initialized with protection info is
enabled, some requests don't have the protection info attached for
WRITE SAME command issued by block device helpers, WRITE command with
WRPROTECT=0 by SG_IO ioctl, etc.

So when TCM loopback fabric module is used, se_cmd->t_prot_sg is NULL
for these requests and performing WRITE_INSERT of PI using software
emulation by sbc_dif_generate() causes kernel crash.

To fix this, introduce SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC for
se_cmd_flags, which is used to determine that se_cmd->t_prot_sg needs
to be allocated or use pre-allocated protection information by scsi
mid-layer.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Nicholas Bellinger <nab@linux-iscsi.org>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
Cc: target-devel@vger.kernel.org
Cc: linux-scsi@vger.kernel.org
---
* No change from v1

 drivers/target/target_core_transport.c | 30 ++++++++++++++++++------------
 include/target/target_core_base.h      |  1 +
 2 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 7a9e7e2..fe52883 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1450,6 +1450,7 @@ int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess
 	if (sgl_prot_count) {
 		se_cmd->t_prot_sg = sgl_prot;
 		se_cmd->t_prot_nents = sgl_prot_count;
+		se_cmd->se_cmd_flags |= SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC;
 	}
 
 	/*
@@ -2181,6 +2182,12 @@ static inline void transport_reset_sgl_orig(struct se_cmd *cmd)
 
 static inline void transport_free_pages(struct se_cmd *cmd)
 {
+	if (!(cmd->se_cmd_flags & SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC)) {
+		transport_free_sgl(cmd->t_prot_sg, cmd->t_prot_nents);
+		cmd->t_prot_sg = NULL;
+		cmd->t_prot_nents = 0;
+	}
+
 	if (cmd->se_cmd_flags & SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC) {
 		/*
 		 * Release special case READ buffer payload required for
@@ -2204,10 +2211,6 @@ static inline void transport_free_pages(struct se_cmd *cmd)
 	transport_free_sgl(cmd->t_bidi_data_sg, cmd->t_bidi_data_nents);
 	cmd->t_bidi_data_sg = NULL;
 	cmd->t_bidi_data_nents = 0;
-
-	transport_free_sgl(cmd->t_prot_sg, cmd->t_prot_nents);
-	cmd->t_prot_sg = NULL;
-	cmd->t_prot_nents = 0;
 }
 
 /**
@@ -2346,6 +2349,17 @@ transport_generic_new_cmd(struct se_cmd *cmd)
 	int ret = 0;
 	bool zero_flag = !(cmd->se_cmd_flags & SCF_SCSI_DATA_CDB);
 
+	if (!(cmd->se_cmd_flags & SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC)) {
+		if (cmd->prot_op != TARGET_PROT_NORMAL) {
+			ret = target_alloc_sgl(&cmd->t_prot_sg,
+					       &cmd->t_prot_nents,
+					       cmd->prot_length, true);
+			if (ret < 0)
+				return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+		}
+
+	}
+
 	/*
 	 * Determine is the TCM fabric module has already allocated physical
 	 * memory, and is directly calling transport_generic_map_mem_to_cmd()
@@ -2371,14 +2385,6 @@ transport_generic_new_cmd(struct se_cmd *cmd)
 				return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
 		}
 
-		if (cmd->prot_op != TARGET_PROT_NORMAL) {
-			ret = target_alloc_sgl(&cmd->t_prot_sg,
-					       &cmd->t_prot_nents,
-					       cmd->prot_length, true);
-			if (ret < 0)
-				return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
-		}
-
 		ret = target_alloc_sgl(&cmd->t_data_sg, &cmd->t_data_nents,
 				       cmd->data_length, zero_flag);
 		if (ret < 0)
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 480e9f8..13efcdd 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -167,6 +167,7 @@ enum se_cmd_flags_table {
 	SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC = 0x00020000,
 	SCF_COMPARE_AND_WRITE		= 0x00080000,
 	SCF_COMPARE_AND_WRITE_POST	= 0x00100000,
+	SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC = 0x00200000,
 };
 
 /* struct se_dev_entry->lun_flags and struct se_lun->lun_access */
-- 
1.9.1

[PATCH v2 2/4] target: Abandon odd SG mapping for data transfer memory

[Akinobu Mita]

sbc_dif_generate() and sbc_dif_verify() assume that each SG element
for data transfer memory doesn't straddle the block size boundary.

However, when using SG_IO ioctl, we can choose the data transfer memory
which doesn't satisfy that alignment requirement.

This change detects such cases and makes those functions return failure
to stop continuing the operation.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Nicholas Bellinger <nab@linux-iscsi.org>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
Cc: target-devel@vger.kernel.org
Cc: linux-scsi@vger.kernel.org
---
* Changes from v1:
- Reduce code duplication a bit in target_read_prot_action()

 drivers/target/target_core_sbc.c       | 10 +++++++++-
 drivers/target/target_core_transport.c | 23 +++++++++++------------
 include/target/target_core_backend.h   |  2 +-
 3 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index edba39f..982836b 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -1177,7 +1177,7 @@ err:
 }
 EXPORT_SYMBOL(sbc_execute_unmap);
 
-void
+sense_reason_t
 sbc_dif_generate(struct se_cmd *cmd)
 {
 	struct se_device *dev = cmd->se_dev;
@@ -1188,6 +1188,9 @@ sbc_dif_generate(struct se_cmd *cmd)
 	int i, j, offset = 0;
 
 	for_each_sg(cmd->t_data_sg, dsg, cmd->t_data_nents, i) {
+		if (dsg->length % dev->dev_attrib.block_size)
+			return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+
 		daddr = kmap_atomic(sg_page(dsg)) + dsg->offset;
 		paddr = kmap_atomic(sg_page(psg)) + psg->offset;
 
@@ -1221,6 +1224,8 @@ sbc_dif_generate(struct se_cmd *cmd)
 		kunmap_atomic(paddr);
 		kunmap_atomic(daddr);
 	}
+
+	return 0;
 }
 
 static sense_reason_t
@@ -1323,6 +1328,9 @@ sbc_dif_verify(struct se_cmd *cmd, sector_t start, unsigned int sectors,
 	sense_reason_t rc;
 
 	for_each_sg(cmd->t_data_sg, dsg, cmd->t_data_nents, i) {
+		if (dsg->length % dev->dev_attrib.block_size)
+			return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+
 		daddr = kmap_atomic(sg_page(dsg)) + dsg->offset;
 		paddr = kmap_atomic(sg_page(psg)) + psg->offset;
 
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index fe52883..fd89f57 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1760,7 +1760,7 @@ static int target_write_prot_action(struct se_cmd *cmd)
 	switch (cmd->prot_op) {
 	case TARGET_PROT_DOUT_INSERT:
 		if (!(cmd->se_sess->sup_prot_ops & TARGET_PROT_DOUT_INSERT))
-			sbc_dif_generate(cmd);
+			cmd->pi_err = sbc_dif_generate(cmd);
 		break;
 	case TARGET_PROT_DOUT_STRIP:
 		if (cmd->se_sess->sup_prot_ops & TARGET_PROT_DOUT_STRIP)
@@ -1769,18 +1769,19 @@ static int target_write_prot_action(struct se_cmd *cmd)
 		sectors = cmd->data_length >> ilog2(cmd->se_dev->dev_attrib.block_size);
 		cmd->pi_err = sbc_dif_verify(cmd, cmd->t_task_lba,
 					     sectors, 0, cmd->t_prot_sg, 0);
-		if (unlikely(cmd->pi_err)) {
-			spin_lock_irq(&cmd->t_state_lock);
-			cmd->transport_state &= ~CMD_T_BUSY|CMD_T_SENT;
-			spin_unlock_irq(&cmd->t_state_lock);
-			transport_generic_request_failure(cmd, cmd->pi_err);
-			return -1;
-		}
 		break;
 	default:
 		break;
 	}
 
+	if (unlikely(cmd->pi_err)) {
+		spin_lock_irq(&cmd->t_state_lock);
+		cmd->transport_state &= ~CMD_T_BUSY|CMD_T_SENT;
+		spin_unlock_irq(&cmd->t_state_lock);
+		transport_generic_request_failure(cmd, cmd->pi_err);
+		return -1;
+	}
+
 	return 0;
 }
 
@@ -2002,21 +2003,19 @@ static bool target_read_prot_action(struct se_cmd *cmd)
 			cmd->pi_err = sbc_dif_verify(cmd, cmd->t_task_lba,
 						     sectors, 0, cmd->t_prot_sg,
 						     0);
-			if (cmd->pi_err)
-				return true;
 		}
 		break;
 	case TARGET_PROT_DIN_INSERT:
 		if (cmd->se_sess->sup_prot_ops & TARGET_PROT_DIN_INSERT)
 			break;
 
-		sbc_dif_generate(cmd);
+		cmd->pi_err = sbc_dif_generate(cmd);
 		break;
 	default:
 		break;
 	}
 
-	return false;
+	return cmd->pi_err ? true : false;
 }
 
 static void target_complete_ok_work(struct work_struct *work)
diff --git a/include/target/target_core_backend.h b/include/target/target_core_backend.h
index ab8ed4c..41dfde0 100644
--- a/include/target/target_core_backend.h
+++ b/include/target/target_core_backend.h
@@ -85,7 +85,7 @@ sense_reason_t sbc_execute_unmap(struct se_cmd *cmd,
 	sense_reason_t (*do_unmap_fn)(struct se_cmd *cmd, void *priv,
 				      sector_t lba, sector_t nolb),
 	void *priv);
-void	sbc_dif_generate(struct se_cmd *);
+sense_reason_t	sbc_dif_generate(struct se_cmd *);
 sense_reason_t	sbc_dif_verify(struct se_cmd *, sector_t, unsigned int,
 				     unsigned int, struct scatterlist *, int);
 void sbc_dif_copy_prot(struct se_cmd *, unsigned int, bool,
-- 
1.9.1

[PATCH v2 3/4] target: Fix sbc_dif_generate() and sbc_dif_verify() for WRITE SAME

[Akinobu Mita]

For WRITE SAME, data transfer memory only contains a single block but
protection information is required for all blocks that are written by
the command.

This makes sbc_dif_generate() and sbc_dif_verify() work for WRITE_SAME.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Nicholas Bellinger <nab@linux-iscsi.org>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
Cc: target-devel@vger.kernel.org
Cc: linux-scsi@vger.kernel.org
---
* Changes from v1:
- Fix sbc_dif_verify() for WRITE_SAME command

 drivers/target/target_core_sbc.c | 39 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 982836b..5577829 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -1177,6 +1177,24 @@ err:
 }
 EXPORT_SYMBOL(sbc_execute_unmap);
 
+static bool sbc_is_write_same(struct se_cmd *cmd)
+{
+	u16 service_action;
+
+	switch (cmd->t_task_cdb[0]) {
+	case WRITE_SAME:
+	case WRITE_SAME_16:
+		return true;
+	case VARIABLE_LENGTH_CMD:
+		service_action = get_unaligned_be16(&cmd->t_task_cdb[8]);
+		if (service_action == WRITE_SAME_32)
+			return true;
+		break;
+	}
+
+	return false;
+}
+
 sense_reason_t
 sbc_dif_generate(struct se_cmd *cmd)
 {
@@ -1185,8 +1203,14 @@ sbc_dif_generate(struct se_cmd *cmd)
 	struct scatterlist *dsg, *psg = cmd->t_prot_sg;
 	sector_t sector = cmd->t_task_lba;
 	void *daddr, *paddr;
-	int i, j, offset = 0;
+	int i, j, offset = 0, left = cmd->prot_length;
+	bool is_write_same = sbc_is_write_same(cmd);
 
+	if (is_write_same && (cmd->t_data_nents != 1 ||
+	    cmd->t_data_sg->length != dev->dev_attrib.block_size))
+		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+
+repeat:
 	for_each_sg(cmd->t_data_sg, dsg, cmd->t_data_nents, i) {
 		if (dsg->length % dev->dev_attrib.block_size)
 			return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
@@ -1219,11 +1243,14 @@ sbc_dif_generate(struct se_cmd *cmd)
 
 			sector++;
 			offset += sizeof(struct se_dif_v1_tuple);
+			left -= sizeof(struct se_dif_v1_tuple);
 		}
 
 		kunmap_atomic(paddr);
 		kunmap_atomic(daddr);
 	}
+	if (is_write_same && left)
+		goto repeat;
 
 	return 0;
 }
@@ -1326,7 +1353,14 @@ sbc_dif_verify(struct se_cmd *cmd, sector_t start, unsigned int sectors,
 	void *daddr, *paddr;
 	int i, j;
 	sense_reason_t rc;
+	int left = cmd->prot_length;
+	bool is_write_same = sbc_is_write_same(cmd);
+
+	if (is_write_same && (cmd->t_data_nents != 1 ||
+	    cmd->t_data_sg->length != dev->dev_attrib.block_size))
+		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
 
+repeat:
 	for_each_sg(cmd->t_data_sg, dsg, cmd->t_data_nents, i) {
 		if (dsg->length % dev->dev_attrib.block_size)
 			return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
@@ -1368,11 +1402,14 @@ sbc_dif_verify(struct se_cmd *cmd, sector_t start, unsigned int sectors,
 			sector++;
 			ei_lba++;
 			psg_off += sizeof(struct se_dif_v1_tuple);
+			left -= sizeof(struct se_dif_v1_tuple);
 		}
 
 		kunmap_atomic(paddr - psg->offset);
 		kunmap_atomic(daddr - dsg->offset);
 	}
+	if (is_write_same && left)
+		goto repeat;
 
 	return 0;
 }
-- 
1.9.1

[PATCH v2 4/4] target/file: enable WRITE SAME when protection info is enabled

[Akinobu Mita]

Now we can generate correct PI for WRITE SAME command, so it is
unnecessary to disallow WRITE SAME when protection info is enabled.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Nicholas Bellinger <nab@linux-iscsi.org>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
Cc: target-devel@vger.kernel.org
Cc: linux-scsi@vger.kernel.org
---
* Changes from v1:
- Fix inverted rw argument for fd_do_rw()
- Perform DIF verify before write for WRITE_SAME

 drivers/target/target_core_file.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
index 829817a..fe98f58 100644
--- a/drivers/target/target_core_file.c
+++ b/drivers/target/target_core_file.c
@@ -376,16 +376,12 @@ fd_execute_write_same(struct se_cmd *cmd)
 	struct bio_vec *bvec;
 	unsigned int len = 0, i;
 	ssize_t ret;
+	sense_reason_t rc;
 
 	if (!nolb) {
 		target_complete_cmd(cmd, SAM_STAT_GOOD);
 		return 0;
 	}
-	if (cmd->prot_op) {
-		pr_err("WRITE_SAME: Protection information with FILEIO"
-		       " backends not supported\n");
-		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
-	}
 
 	if (cmd->t_data_nents > 1 ||
 	    cmd->t_data_sg[0].length != cmd->se_dev->dev_attrib.block_size) {
@@ -397,6 +393,10 @@ fd_execute_write_same(struct se_cmd *cmd)
 		return TCM_INVALID_CDB_FIELD;
 	}
 
+	rc = sbc_dif_verify(cmd, cmd->t_task_lba, nolb, 0, cmd->t_prot_sg, 0);
+	if (rc)
+		return rc;
+
 	bvec = kcalloc(nolb, sizeof(struct bio_vec), GFP_KERNEL);
 	if (!bvec)
 		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
@@ -418,6 +418,14 @@ fd_execute_write_same(struct se_cmd *cmd)
 		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
 	}
 
+	if (cmd->prot_op) {
+		ret = fd_do_rw(cmd, fd_dev->fd_prot_file, se_dev->prot_length,
+				cmd->t_prot_sg, cmd->t_prot_nents,
+				cmd->prot_length, 1);
+		if (ret < 0)
+			return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+	}
+
 	target_complete_cmd(cmd, SAM_STAT_GOOD);
 	return 0;
 }
-- 
1.9.1