[PATCH 0/8] target: Fixes for SPC/SBC device emulation

[PATCH 0/8] target: Fixes for SPC/SBC device emulation

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

Hi all,

The following patch series contains a number of target SPC/SBC
related emulation fixes that address failures occurring while
using Ronnie Sahlberg's excellent libiscsi test suite.

The first two are the important ones.  Patch #1 adds a missing
end-of-device sanity check for WRITE_SAME emulation, that Bart
had reported earlier.  Patch #2 fixes a case where the existing
end-of-device check did not take into account the potential of
LBA + sector wrapping overflowing sector_t.

The remaining patches address other minor test failures.  This
includes failing I/O + WRITE_SAME ops when PROTECT bit is set,
if the backend device is not configured to support protection
information.

Also included are sanity checks for DPO/FUA, WRITE_SAME w/ UNMAP=1
and UNMAP that will fail the command if the associated feature
bits are not being advertised by the backend device via existing
device attributes.

Please review.

Thank you,

--nab

Nicholas Bellinger (8):
  target: Add missing WRITE_SAME end-of-device sanity check
  target: Check for LBA + sectors wrap-around in sbc_parse_cdb
  target: Fail I/O with PROTECT bit when protection is unsupported
  target: Perform PROTECT sanity checks for WRITE_SAME
  target: Add sanity checks for DPO/FUA bit usage
  target: Fail WRITE_SAME w/ UNMAP=1 when emulate_tpws=0
  target: Fail UNMAP when emulate_tpu=0
  target: Set LBPWS10 bit in Logical Block Provisioning EVPD

 drivers/target/target_core_file.c   |   5 ++
 drivers/target/target_core_iblock.c |   5 ++
 drivers/target/target_core_sbc.c    | 140 ++++++++++++++++++++++++++++--------
 drivers/target/target_core_spc.c    |   2 +-
 4 files changed, 121 insertions(+), 31 deletions(-)

-- 
1.9.1

[PATCH 1/8] target: Add missing WRITE_SAME end-of-device sanity check

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch adds a check to sbc_setup_write_same() to verify
the incoming WRITE_SAME LBA + number of blocks does not exceed
past the end-of-device.

Also check for potential LBA wrap-around as well.

Reported-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Martin Petersen <martin.petersen@oracle.com>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_sbc.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 11bea19..b26b52f 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -251,6 +251,8 @@ static inline unsigned long long transport_lba_64_ext(unsigned char *cdb)
 static sense_reason_t
 sbc_setup_write_same(struct se_cmd *cmd, unsigned char *flags, struct sbc_ops *ops)
 {
+	struct se_device *dev = cmd->se_dev;
+	sector_t end_lba = dev->transport->get_blocks(dev) + 1;
 	unsigned int sectors = sbc_get_write_same_sectors(cmd);
 
 	if ((flags[0] & 0x04) || (flags[0] & 0x02)) {
@@ -264,6 +266,16 @@ sbc_setup_write_same(struct se_cmd *cmd, unsigned char *flags, struct sbc_ops *o
 			sectors, cmd->se_dev->dev_attrib.max_write_same_len);
 		return TCM_INVALID_CDB_FIELD;
 	}
+	/*
+	 * Sanity check for LBA wrap and request past end of device.
+	 */
+	if (((cmd->t_task_lba + sectors) < cmd->t_task_lba) ||
+	    ((cmd->t_task_lba + sectors) > end_lba)) {
+		pr_err("WRITE_SAME exceeds last lba %llu (lba %llu, sectors %u)\n",
+		       (unsigned long long)end_lba, cmd->t_task_lba, sectors);
+		return TCM_ADDRESS_OUT_OF_RANGE;
+	}
+
 	/* We always have ANC_SUP == 0 so setting ANCHOR is always an error */
 	if (flags[0] & 0x10) {
 		pr_warn("WRITE SAME with ANCHOR not supported\n");
-- 
1.9.1

[PATCH 2/8] target: Check for LBA + sectors wrap-around in sbc_parse_cdb

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch adds a check to sbc_parse_cdb() in order to detect when
an LBA + sector vs. end-of-device calculation wraps when the LBA is
sufficently large enough (eg: 0xFFFFFFFFFFFFFFFF).

Cc: Martin Petersen <martin.petersen@oracle.com>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_sbc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index b26b52f..17259c0 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -982,7 +982,8 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		}
 check_lba:
 		end_lba = dev->transport->get_blocks(dev) + 1;
-		if (cmd->t_task_lba + sectors > end_lba) {
+		if (((cmd->t_task_lba + sectors) < cmd->t_task_lba) ||
+		    ((cmd->t_task_lba + sectors) > end_lba)) {
 			pr_err("cmd exceeds last lba %llu "
 				"(lba %llu, sectors %u)\n",
 				end_lba, cmd->t_task_lba, sectors);
-- 
1.9.1

Re: [PATCH 2/8] target: Check for LBA + sectors wrap-around in sbc_parse_cdb

[Sagi Grimberg]

On 2/14/2015 5:27 AM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
>
> This patch adds a check to sbc_parse_cdb() in order to detect when
> an LBA + sector vs. end-of-device calculation wraps when the LBA is
> sufficently large enough (eg: 0xFFFFFFFFFFFFFFFF).
>
> Cc: Martin Petersen <martin.petersen@oracle.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
>   drivers/target/target_core_sbc.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
> index b26b52f..17259c0 100644
> --- a/drivers/target/target_core_sbc.c
> +++ b/drivers/target/target_core_sbc.c
> @@ -982,7 +982,8 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
>   		}
>   check_lba:
>   		end_lba = dev->transport->get_blocks(dev) + 1;
> -		if (cmd->t_task_lba + sectors > end_lba) {
> +		if (((cmd->t_task_lba + sectors) < cmd->t_task_lba) ||
> +		    ((cmd->t_task_lba + sectors) > end_lba)) {
>   			pr_err("cmd exceeds last lba %llu "
>   				"(lba %llu, sectors %u)\n",
>   				end_lba, cmd->t_task_lba, sectors);
>

Can you also please modify the err message as well?

Sagi.

[PATCH 3/8] target: Fail I/O with PROTECT bit when protection is unsupported

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch adds an explicit check for WRPROTECT + RDPROTECT bit usage
within sbc_check_prot(), and fails with TCM_INVALID_CDB_FIELD if the
backend device does not have protection enabled.

Also, update sbc_check_prot() to return sense_reason_t in order to
propigate up the correct sense ASQ.

Cc: Martin Petersen <martin.petersen@oracle.com>
Cc: Sagi Grimberg <sagig@mellanox.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_sbc.c | 51 +++++++++++++++++++++++++---------------
 1 file changed, 32 insertions(+), 19 deletions(-)

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 17259c0..1bb8b4a 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -626,14 +626,21 @@ sbc_set_prot_op_checks(u8 protect, enum target_prot_type prot_type,
 	return 0;
 }
 
-static bool
+static sense_reason_t
 sbc_check_prot(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb,
 	       u32 sectors, bool is_write)
 {
 	u8 protect = cdb[1] >> 5;
 
-	if ((!cmd->t_prot_sg || !cmd->t_prot_nents) && cmd->prot_pto)
-		return true;
+	if (!cmd->t_prot_sg || !cmd->t_prot_nents) {
+		if (protect && !dev->dev_attrib.pi_prot_type) {
+			pr_err("CDB contains protect bit, but device does not"
+			       " advertise PROTECT=1 feature bit\n");
+			return TCM_INVALID_CDB_FIELD;
+		}
+		if (cmd->prot_pto)
+			return TCM_NO_SENSE;
+	}
 
 	switch (dev->dev_attrib.pi_prot_type) {
 	case TARGET_DIF_TYPE3_PROT:
@@ -641,7 +648,7 @@ sbc_check_prot(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb,
 		break;
 	case TARGET_DIF_TYPE2_PROT:
 		if (protect)
-			return false;
+			return TCM_INVALID_CDB_FIELD;
 
 		cmd->reftag_seed = cmd->t_task_lba;
 		break;
@@ -650,12 +657,12 @@ sbc_check_prot(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb,
 		break;
 	case TARGET_DIF_TYPE0_PROT:
 	default:
-		return true;
+		return TCM_NO_SENSE;
 	}
 
 	if (sbc_set_prot_op_checks(protect, dev->dev_attrib.pi_prot_type,
 				   is_write, cmd))
-		return false;
+		return TCM_INVALID_CDB_FIELD;
 
 	cmd->prot_type = dev->dev_attrib.pi_prot_type;
 	cmd->prot_length = dev->prot_length * sectors;
@@ -674,7 +681,7 @@ sbc_check_prot(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb,
 		 __func__, cmd->prot_type, cmd->data_length, cmd->prot_length,
 		 cmd->prot_op, cmd->prot_checks);
 
-	return true;
+	return TCM_NO_SENSE;
 }
 
 sense_reason_t
@@ -698,8 +705,9 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		sectors = transport_get_sectors_10(cdb);
 		cmd->t_task_lba = transport_lba_32(cdb);
 
-		if (!sbc_check_prot(dev, cmd, cdb, sectors, false))
-			return TCM_UNSUPPORTED_SCSI_OPCODE;
+		ret = sbc_check_prot(dev, cmd, cdb, sectors, false);
+		if (ret)
+			return ret;
 
 		cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
 		cmd->execute_rw = ops->execute_rw;
@@ -709,8 +717,9 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		sectors = transport_get_sectors_12(cdb);
 		cmd->t_task_lba = transport_lba_32(cdb);
 
-		if (!sbc_check_prot(dev, cmd, cdb, sectors, false))
-			return TCM_UNSUPPORTED_SCSI_OPCODE;
+		ret = sbc_check_prot(dev, cmd, cdb, sectors, false);
+		if (ret)
+			return ret;
 
 		cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
 		cmd->execute_rw = ops->execute_rw;
@@ -720,8 +729,9 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		sectors = transport_get_sectors_16(cdb);
 		cmd->t_task_lba = transport_lba_64(cdb);
 
-		if (!sbc_check_prot(dev, cmd, cdb, sectors, false))
-			return TCM_UNSUPPORTED_SCSI_OPCODE;
+		ret = sbc_check_prot(dev, cmd, cdb, sectors, false);
+		if (ret)
+			return ret;
 
 		cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
 		cmd->execute_rw = ops->execute_rw;
@@ -739,8 +749,9 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		sectors = transport_get_sectors_10(cdb);
 		cmd->t_task_lba = transport_lba_32(cdb);
 
-		if (!sbc_check_prot(dev, cmd, cdb, sectors, true))
-			return TCM_UNSUPPORTED_SCSI_OPCODE;
+		ret = sbc_check_prot(dev, cmd, cdb, sectors, true);
+		if (ret)
+			return ret;
 
 		if (cdb[1] & 0x8)
 			cmd->se_cmd_flags |= SCF_FUA;
@@ -752,8 +763,9 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		sectors = transport_get_sectors_12(cdb);
 		cmd->t_task_lba = transport_lba_32(cdb);
 
-		if (!sbc_check_prot(dev, cmd, cdb, sectors, true))
-			return TCM_UNSUPPORTED_SCSI_OPCODE;
+		ret = sbc_check_prot(dev, cmd, cdb, sectors, true);
+		if (ret)
+			return ret;
 
 		if (cdb[1] & 0x8)
 			cmd->se_cmd_flags |= SCF_FUA;
@@ -765,8 +777,9 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		sectors = transport_get_sectors_16(cdb);
 		cmd->t_task_lba = transport_lba_64(cdb);
 
-		if (!sbc_check_prot(dev, cmd, cdb, sectors, true))
-			return TCM_UNSUPPORTED_SCSI_OPCODE;
+		ret = sbc_check_prot(dev, cmd, cdb, sectors, true);
+		if (ret)
+			return ret;
 
 		if (cdb[1] & 0x8)
 			cmd->se_cmd_flags |= SCF_FUA;
-- 
1.9.1

Re: [PATCH 3/8] target: Fail I/O with PROTECT bit when protection is unsupported

[Sagi Grimberg]

On 2/14/2015 5:27 AM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
>
> This patch adds an explicit check for WRPROTECT + RDPROTECT bit usage
> within sbc_check_prot(), and fails with TCM_INVALID_CDB_FIELD if the
> backend device does not have protection enabled.
>
> Also, update sbc_check_prot() to return sense_reason_t in order to
> propigate up the correct sense ASQ.
>
> Cc: Martin Petersen <martin.petersen@oracle.com>
> Cc: Sagi Grimberg <sagig@mellanox.com>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
>   drivers/target/target_core_sbc.c | 51 +++++++++++++++++++++++++---------------
>   1 file changed, 32 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
> index 17259c0..1bb8b4a 100644
> --- a/drivers/target/target_core_sbc.c
> +++ b/drivers/target/target_core_sbc.c
> @@ -626,14 +626,21 @@ sbc_set_prot_op_checks(u8 protect, enum target_prot_type prot_type,
>   	return 0;
>   }
>
> -static bool
> +static sense_reason_t
>   sbc_check_prot(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb,
>   	       u32 sectors, bool is_write)
>   {
>   	u8 protect = cdb[1] >> 5;
>
> -	if ((!cmd->t_prot_sg || !cmd->t_prot_nents) && cmd->prot_pto)
> -		return true;
> +	if (!cmd->t_prot_sg || !cmd->t_prot_nents) {
> +		if (protect && !dev->dev_attrib.pi_prot_type) {
> +			pr_err("CDB contains protect bit, but device does not"
> +			       " advertise PROTECT=1 feature bit\n");
> +			return TCM_INVALID_CDB_FIELD;
> +		}
> +		if (cmd->prot_pto)
> +			return TCM_NO_SENSE;
> +	}

I think this should indicate a strip operation rather than a failure.
Depends on what we reported in readcapacity wrt backend/frontend
support & setting.

>
>   	switch (dev->dev_attrib.pi_prot_type) {
>   	case TARGET_DIF_TYPE3_PROT:
> @@ -641,7 +648,7 @@ sbc_check_prot(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb,
>   		break;
>   	case TARGET_DIF_TYPE2_PROT:
>   		if (protect)
> -			return false;
> +			return TCM_INVALID_CDB_FIELD;
>
>   		cmd->reftag_seed = cmd->t_task_lba;
>   		break;
> @@ -650,12 +657,12 @@ sbc_check_prot(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb,
>   		break;
>   	case TARGET_DIF_TYPE0_PROT:
>   	default:
> -		return true;
> +		return TCM_NO_SENSE;
>   	}
>
>   	if (sbc_set_prot_op_checks(protect, dev->dev_attrib.pi_prot_type,
>   				   is_write, cmd))
> -		return false;
> +		return TCM_INVALID_CDB_FIELD;
>
>   	cmd->prot_type = dev->dev_attrib.pi_prot_type;
>   	cmd->prot_length = dev->prot_length * sectors;
> @@ -674,7 +681,7 @@ sbc_check_prot(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb,
>   		 __func__, cmd->prot_type, cmd->data_length, cmd->prot_length,
>   		 cmd->prot_op, cmd->prot_checks);
>
> -	return true;
> +	return TCM_NO_SENSE;
>   }
>
>   sense_reason_t
> @@ -698,8 +705,9 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
>   		sectors = transport_get_sectors_10(cdb);
>   		cmd->t_task_lba = transport_lba_32(cdb);
>
> -		if (!sbc_check_prot(dev, cmd, cdb, sectors, false))
> -			return TCM_UNSUPPORTED_SCSI_OPCODE;
> +		ret = sbc_check_prot(dev, cmd, cdb, sectors, false);
> +		if (ret)
> +			return ret;
>
>   		cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
>   		cmd->execute_rw = ops->execute_rw;
> @@ -709,8 +717,9 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
>   		sectors = transport_get_sectors_12(cdb);
>   		cmd->t_task_lba = transport_lba_32(cdb);
>
> -		if (!sbc_check_prot(dev, cmd, cdb, sectors, false))
> -			return TCM_UNSUPPORTED_SCSI_OPCODE;
> +		ret = sbc_check_prot(dev, cmd, cdb, sectors, false);
> +		if (ret)
> +			return ret;
>
>   		cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
>   		cmd->execute_rw = ops->execute_rw;
> @@ -720,8 +729,9 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
>   		sectors = transport_get_sectors_16(cdb);
>   		cmd->t_task_lba = transport_lba_64(cdb);
>
> -		if (!sbc_check_prot(dev, cmd, cdb, sectors, false))
> -			return TCM_UNSUPPORTED_SCSI_OPCODE;
> +		ret = sbc_check_prot(dev, cmd, cdb, sectors, false);
> +		if (ret)
> +			return ret;
>
>   		cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
>   		cmd->execute_rw = ops->execute_rw;
> @@ -739,8 +749,9 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
>   		sectors = transport_get_sectors_10(cdb);
>   		cmd->t_task_lba = transport_lba_32(cdb);
>
> -		if (!sbc_check_prot(dev, cmd, cdb, sectors, true))
> -			return TCM_UNSUPPORTED_SCSI_OPCODE;
> +		ret = sbc_check_prot(dev, cmd, cdb, sectors, true);
> +		if (ret)
> +			return ret;
>
>   		if (cdb[1] & 0x8)
>   			cmd->se_cmd_flags |= SCF_FUA;
> @@ -752,8 +763,9 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
>   		sectors = transport_get_sectors_12(cdb);
>   		cmd->t_task_lba = transport_lba_32(cdb);
>
> -		if (!sbc_check_prot(dev, cmd, cdb, sectors, true))
> -			return TCM_UNSUPPORTED_SCSI_OPCODE;
> +		ret = sbc_check_prot(dev, cmd, cdb, sectors, true);
> +		if (ret)
> +			return ret;
>
>   		if (cdb[1] & 0x8)
>   			cmd->se_cmd_flags |= SCF_FUA;
> @@ -765,8 +777,9 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
>   		sectors = transport_get_sectors_16(cdb);
>   		cmd->t_task_lba = transport_lba_64(cdb);
>
> -		if (!sbc_check_prot(dev, cmd, cdb, sectors, true))
> -			return TCM_UNSUPPORTED_SCSI_OPCODE;
> +		ret = sbc_check_prot(dev, cmd, cdb, sectors, true);
> +		if (ret)
> +			return ret;
>
>   		if (cdb[1] & 0x8)
>   			cmd->se_cmd_flags |= SCF_FUA;
>

[PATCH 4/8] target: Perform PROTECT sanity checks for WRITE_SAME

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch adds a call to sbc_check_prot() within sbc_setup_write_same()
code to perform the various protection releated sanity checks, including
failing if WRPROTECT or RDPROTECT is set for a backend device that has
not advertised support for T10-PI.

Also, since WRITE_SAME + T10-PI is currently not supported by IBLOCK +
FILEIO backends, go ahead and fail if ->execute_write_same() is invoked
with a non zero cmd->prot_op.

Cc: Martin Petersen <martin.petersen@oracle.com>
Cc: Sagi Grimberg <sagig@mellanox.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_file.c   | 5 +++++
 drivers/target/target_core_iblock.c | 5 +++++
 drivers/target/target_core_sbc.c    | 8 ++++++++
 3 files changed, 18 insertions(+)

diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
index c2aea09..9f1ed77 100644
--- a/drivers/target/target_core_file.c
+++ b/drivers/target/target_core_file.c
@@ -494,6 +494,11 @@ fd_execute_write_same(struct se_cmd *cmd)
 		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;
+	}
 	sg = &cmd->t_data_sg[0];
 
 	if (cmd->t_data_nents > 1 ||
diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c
index 3efff94..303299e 100644
--- a/drivers/target/target_core_iblock.c
+++ b/drivers/target/target_core_iblock.c
@@ -464,6 +464,11 @@ iblock_execute_write_same(struct se_cmd *cmd)
 	sector_t block_lba = cmd->t_task_lba;
 	sector_t sectors = sbc_get_write_same_sectors(cmd);
 
+	if (cmd->prot_op) {
+		pr_err("WRITE_SAME: Protection information with IBLOCK"
+		       " backends not supported\n");
+		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+	}
 	sg = &cmd->t_data_sg[0];
 
 	if (cmd->t_data_nents > 1 ||
diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 1bb8b4a..87cbbe2 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -37,6 +37,9 @@
 #include "target_core_alua.h"
 
 static sense_reason_t
+sbc_check_prot(struct se_device *, struct se_cmd *, unsigned char *, u32, bool);
+
+static sense_reason_t
 sbc_emulate_readcapacity(struct se_cmd *cmd)
 {
 	struct se_device *dev = cmd->se_dev;
@@ -254,6 +257,7 @@ sbc_setup_write_same(struct se_cmd *cmd, unsigned char *flags, struct sbc_ops *o
 	struct se_device *dev = cmd->se_dev;
 	sector_t end_lba = dev->transport->get_blocks(dev) + 1;
 	unsigned int sectors = sbc_get_write_same_sectors(cmd);
+	sense_reason_t ret;
 
 	if ((flags[0] & 0x04) || (flags[0] & 0x02)) {
 		pr_err("WRITE_SAME PBDATA and LBDATA"
@@ -295,6 +299,10 @@ sbc_setup_write_same(struct se_cmd *cmd, unsigned char *flags, struct sbc_ops *o
 	if (!ops->execute_write_same)
 		return TCM_UNSUPPORTED_SCSI_OPCODE;
 
+	ret = sbc_check_prot(dev, cmd, &cmd->t_task_cdb[0], sectors, true);
+	if (ret)
+		return ret;
+
 	cmd->execute_cmd = ops->execute_write_same;
 	return 0;
 }
-- 
1.9.1

Re: [PATCH 4/8] target: Perform PROTECT sanity checks for WRITE_SAME

[Sagi Grimberg]

On 2/14/2015 5:27 AM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
>
> This patch adds a call to sbc_check_prot() within sbc_setup_write_same()
> code to perform the various protection releated sanity checks, including
> failing if WRPROTECT or RDPROTECT is set for a backend device that has
> not advertised support for T10-PI.
>
> Also, since WRITE_SAME + T10-PI is currently not supported by IBLOCK +
> FILEIO backends, go ahead and fail if ->execute_write_same() is invoked
> with a non zero cmd->prot_op.

So this is OK, for now... But I don't see any reason why we shouldn't
support T10-PI in write_same as well...

>
> Cc: Martin Petersen <martin.petersen@oracle.com>
> Cc: Sagi Grimberg <sagig@mellanox.com>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
>   drivers/target/target_core_file.c   | 5 +++++
>   drivers/target/target_core_iblock.c | 5 +++++
>   drivers/target/target_core_sbc.c    | 8 ++++++++
>   3 files changed, 18 insertions(+)
>
> diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
> index c2aea09..9f1ed77 100644
> --- a/drivers/target/target_core_file.c
> +++ b/drivers/target/target_core_file.c
> @@ -494,6 +494,11 @@ fd_execute_write_same(struct se_cmd *cmd)
>   		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;
> +	}
>   	sg = &cmd->t_data_sg[0];
>
>   	if (cmd->t_data_nents > 1 ||
> diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c
> index 3efff94..303299e 100644
> --- a/drivers/target/target_core_iblock.c
> +++ b/drivers/target/target_core_iblock.c
> @@ -464,6 +464,11 @@ iblock_execute_write_same(struct se_cmd *cmd)
>   	sector_t block_lba = cmd->t_task_lba;
>   	sector_t sectors = sbc_get_write_same_sectors(cmd);
>
> +	if (cmd->prot_op) {
> +		pr_err("WRITE_SAME: Protection information with IBLOCK"
> +		       " backends not supported\n");
> +		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> +	}
>   	sg = &cmd->t_data_sg[0];
>
>   	if (cmd->t_data_nents > 1 ||
> diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
> index 1bb8b4a..87cbbe2 100644
> --- a/drivers/target/target_core_sbc.c
> +++ b/drivers/target/target_core_sbc.c
> @@ -37,6 +37,9 @@
>   #include "target_core_alua.h"
>
>   static sense_reason_t
> +sbc_check_prot(struct se_device *, struct se_cmd *, unsigned char *, u32, bool);
> +
> +static sense_reason_t
>   sbc_emulate_readcapacity(struct se_cmd *cmd)
>   {
>   	struct se_device *dev = cmd->se_dev;
> @@ -254,6 +257,7 @@ sbc_setup_write_same(struct se_cmd *cmd, unsigned char *flags, struct sbc_ops *o
>   	struct se_device *dev = cmd->se_dev;
>   	sector_t end_lba = dev->transport->get_blocks(dev) + 1;
>   	unsigned int sectors = sbc_get_write_same_sectors(cmd);
> +	sense_reason_t ret;
>
>   	if ((flags[0] & 0x04) || (flags[0] & 0x02)) {
>   		pr_err("WRITE_SAME PBDATA and LBDATA"
> @@ -295,6 +299,10 @@ sbc_setup_write_same(struct se_cmd *cmd, unsigned char *flags, struct sbc_ops *o
>   	if (!ops->execute_write_same)
>   		return TCM_UNSUPPORTED_SCSI_OPCODE;
>
> +	ret = sbc_check_prot(dev, cmd, &cmd->t_task_cdb[0], sectors, true);
> +	if (ret)
> +		return ret;
> +
>   	cmd->execute_cmd = ops->execute_write_same;
>   	return 0;
>   }
>

[PATCH 5/8] target: Add sanity checks for DPO/FUA bit usage

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch adds a sbc_check_dpofua() function that performs sanity
checks for DPO/FUA command bits.

It introduces checks to fail when either bit is set, but the backend
device is not advertising support for them.

It also moves the existing cmd->se_cmd_flags |= SCF_FUA assignement
into the new helper function.

Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_sbc.c | 56 +++++++++++++++++++++++++++++++++-------
 1 file changed, 46 insertions(+), 10 deletions(-)

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 87cbbe2..856e800 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -692,6 +692,29 @@ sbc_check_prot(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb,
 	return TCM_NO_SENSE;
 }
 
+static int
+sbc_check_dpofua(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb)
+{
+	if (cdb[1] & 0x10) {
+		if (!dev->dev_attrib.emulate_dpo) {
+			pr_err("Got CDB: 0x%02x with DPO bit set, but device"
+			       " does not advertise support for DPO\n", cdb[0]);
+			return -EINVAL;
+		}
+	}
+	if (cdb[1] & 0x8) {
+		if (!dev->dev_attrib.emulate_fua_write ||
+		    !dev->dev_attrib.emulate_write_cache) {
+			pr_err("Got CDB: 0x%02x with FUA bit set, but device"
+			       " does not advertise support for FUA write\n",
+			       cdb[0]);
+			return -EINVAL;
+		}
+		cmd->se_cmd_flags |= SCF_FUA;
+	}
+	return 0;
+}
+
 sense_reason_t
 sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 {
@@ -713,6 +736,9 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		sectors = transport_get_sectors_10(cdb);
 		cmd->t_task_lba = transport_lba_32(cdb);
 
+		if (sbc_check_dpofua(dev, cmd, cdb))
+			return TCM_INVALID_CDB_FIELD;
+
 		ret = sbc_check_prot(dev, cmd, cdb, sectors, false);
 		if (ret)
 			return ret;
@@ -725,6 +751,9 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		sectors = transport_get_sectors_12(cdb);
 		cmd->t_task_lba = transport_lba_32(cdb);
 
+		if (sbc_check_dpofua(dev, cmd, cdb))
+			return TCM_INVALID_CDB_FIELD;
+
 		ret = sbc_check_prot(dev, cmd, cdb, sectors, false);
 		if (ret)
 			return ret;
@@ -737,6 +766,9 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		sectors = transport_get_sectors_16(cdb);
 		cmd->t_task_lba = transport_lba_64(cdb);
 
+		if (sbc_check_dpofua(dev, cmd, cdb))
+			return TCM_INVALID_CDB_FIELD;
+
 		ret = sbc_check_prot(dev, cmd, cdb, sectors, false);
 		if (ret)
 			return ret;
@@ -757,12 +789,13 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		sectors = transport_get_sectors_10(cdb);
 		cmd->t_task_lba = transport_lba_32(cdb);
 
+		if (sbc_check_dpofua(dev, cmd, cdb))
+			return TCM_INVALID_CDB_FIELD;
+
 		ret = sbc_check_prot(dev, cmd, cdb, sectors, true);
 		if (ret)
 			return ret;
 
-		if (cdb[1] & 0x8)
-			cmd->se_cmd_flags |= SCF_FUA;
 		cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
 		cmd->execute_rw = ops->execute_rw;
 		cmd->execute_cmd = sbc_execute_rw;
@@ -771,12 +804,13 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		sectors = transport_get_sectors_12(cdb);
 		cmd->t_task_lba = transport_lba_32(cdb);
 
+		if (sbc_check_dpofua(dev, cmd, cdb))
+			return TCM_INVALID_CDB_FIELD;
+
 		ret = sbc_check_prot(dev, cmd, cdb, sectors, true);
 		if (ret)
 			return ret;
 
-		if (cdb[1] & 0x8)
-			cmd->se_cmd_flags |= SCF_FUA;
 		cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
 		cmd->execute_rw = ops->execute_rw;
 		cmd->execute_cmd = sbc_execute_rw;
@@ -785,12 +819,13 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		sectors = transport_get_sectors_16(cdb);
 		cmd->t_task_lba = transport_lba_64(cdb);
 
+		if (sbc_check_dpofua(dev, cmd, cdb))
+			return TCM_INVALID_CDB_FIELD;
+
 		ret = sbc_check_prot(dev, cmd, cdb, sectors, true);
 		if (ret)
 			return ret;
 
-		if (cdb[1] & 0x8)
-			cmd->se_cmd_flags |= SCF_FUA;
 		cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
 		cmd->execute_rw = ops->execute_rw;
 		cmd->execute_cmd = sbc_execute_rw;
@@ -801,6 +836,9 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 			return TCM_INVALID_CDB_FIELD;
 		sectors = transport_get_sectors_10(cdb);
 
+		if (sbc_check_dpofua(dev, cmd, cdb))
+			return TCM_INVALID_CDB_FIELD;
+
 		cmd->t_task_lba = transport_lba_32(cdb);
 		cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
 
@@ -810,8 +848,6 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		cmd->execute_rw = ops->execute_rw;
 		cmd->execute_cmd = sbc_execute_rw;
 		cmd->transport_complete_callback = &xdreadwrite_callback;
-		if (cdb[1] & 0x8)
-			cmd->se_cmd_flags |= SCF_FUA;
 		break;
 	case VARIABLE_LENGTH_CMD:
 	{
@@ -820,6 +856,8 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		case XDWRITEREAD_32:
 			sectors = transport_get_sectors_32(cdb);
 
+			if (sbc_check_dpofua(dev, cmd, cdb))
+				return TCM_INVALID_CDB_FIELD;
 			/*
 			 * Use WRITE_32 and READ_32 opcodes for the emulated
 			 * XDWRITE_READ_32 logic.
@@ -834,8 +872,6 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 			cmd->execute_rw = ops->execute_rw;
 			cmd->execute_cmd = sbc_execute_rw;
 			cmd->transport_complete_callback = &xdreadwrite_callback;
-			if (cdb[1] & 0x8)
-				cmd->se_cmd_flags |= SCF_FUA;
 			break;
 		case WRITE_SAME_32:
 			sectors = transport_get_sectors_32(cdb);
-- 
1.9.1

Re: [PATCH 5/8] target: Add sanity checks for DPO/FUA bit usage

[Sagi Grimberg]

On 2/14/2015 5:27 AM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
>
> This patch adds a sbc_check_dpofua() function that performs sanity
> checks for DPO/FUA command bits.
>
> It introduces checks to fail when either bit is set, but the backend
> device is not advertising support for them.
>
> It also moves the existing cmd->se_cmd_flags |= SCF_FUA assignement
> into the new helper function.
>
> Cc: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
>   drivers/target/target_core_sbc.c | 56 +++++++++++++++++++++++++++++++++-------
>   1 file changed, 46 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
> index 87cbbe2..856e800 100644
> --- a/drivers/target/target_core_sbc.c
> +++ b/drivers/target/target_core_sbc.c
> @@ -692,6 +692,29 @@ sbc_check_prot(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb,
>   	return TCM_NO_SENSE;
>   }
>
> +static int
> +sbc_check_dpofua(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb)
> +{
> +	if (cdb[1] & 0x10) {
> +		if (!dev->dev_attrib.emulate_dpo) {

General comment, can you please use unlikely on all these branches in
the critical path?

> +			pr_err("Got CDB: 0x%02x with DPO bit set, but device"
> +			       " does not advertise support for DPO\n", cdb[0]);
> +			return -EINVAL;
> +		}
> +	}
> +	if (cdb[1] & 0x8) {
> +		if (!dev->dev_attrib.emulate_fua_write ||
> +		    !dev->dev_attrib.emulate_write_cache) {
> +			pr_err("Got CDB: 0x%02x with FUA bit set, but device"
> +			       " does not advertise support for FUA write\n",
> +			       cdb[0]);
> +			return -EINVAL;
> +		}
> +		cmd->se_cmd_flags |= SCF_FUA;
> +	}
> +	return 0;
> +}
> +
>   sense_reason_t
>   sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
>   {
> @@ -713,6 +736,9 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
>   		sectors = transport_get_sectors_10(cdb);
>   		cmd->t_task_lba = transport_lba_32(cdb);
>
> +		if (sbc_check_dpofua(dev, cmd, cdb))
> +			return TCM_INVALID_CDB_FIELD;
> +
>   		ret = sbc_check_prot(dev, cmd, cdb, sectors, false);
>   		if (ret)
>   			return ret;
> @@ -725,6 +751,9 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
>   		sectors = transport_get_sectors_12(cdb);
>   		cmd->t_task_lba = transport_lba_32(cdb);
>
> +		if (sbc_check_dpofua(dev, cmd, cdb))
> +			return TCM_INVALID_CDB_FIELD;
> +
>   		ret = sbc_check_prot(dev, cmd, cdb, sectors, false);
>   		if (ret)
>   			return ret;
> @@ -737,6 +766,9 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
>   		sectors = transport_get_sectors_16(cdb);
>   		cmd->t_task_lba = transport_lba_64(cdb);
>
> +		if (sbc_check_dpofua(dev, cmd, cdb))
> +			return TCM_INVALID_CDB_FIELD;
> +
>   		ret = sbc_check_prot(dev, cmd, cdb, sectors, false);
>   		if (ret)
>   			return ret;
> @@ -757,12 +789,13 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
>   		sectors = transport_get_sectors_10(cdb);
>   		cmd->t_task_lba = transport_lba_32(cdb);
>
> +		if (sbc_check_dpofua(dev, cmd, cdb))
> +			return TCM_INVALID_CDB_FIELD;
> +
>   		ret = sbc_check_prot(dev, cmd, cdb, sectors, true);
>   		if (ret)
>   			return ret;
>
> -		if (cdb[1] & 0x8)
> -			cmd->se_cmd_flags |= SCF_FUA;
>   		cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
>   		cmd->execute_rw = ops->execute_rw;
>   		cmd->execute_cmd = sbc_execute_rw;
> @@ -771,12 +804,13 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
>   		sectors = transport_get_sectors_12(cdb);
>   		cmd->t_task_lba = transport_lba_32(cdb);
>
> +		if (sbc_check_dpofua(dev, cmd, cdb))
> +			return TCM_INVALID_CDB_FIELD;
> +
>   		ret = sbc_check_prot(dev, cmd, cdb, sectors, true);
>   		if (ret)
>   			return ret;
>
> -		if (cdb[1] & 0x8)
> -			cmd->se_cmd_flags |= SCF_FUA;
>   		cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
>   		cmd->execute_rw = ops->execute_rw;
>   		cmd->execute_cmd = sbc_execute_rw;
> @@ -785,12 +819,13 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
>   		sectors = transport_get_sectors_16(cdb);
>   		cmd->t_task_lba = transport_lba_64(cdb);
>
> +		if (sbc_check_dpofua(dev, cmd, cdb))
> +			return TCM_INVALID_CDB_FIELD;
> +
>   		ret = sbc_check_prot(dev, cmd, cdb, sectors, true);
>   		if (ret)
>   			return ret;
>
> -		if (cdb[1] & 0x8)
> -			cmd->se_cmd_flags |= SCF_FUA;
>   		cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
>   		cmd->execute_rw = ops->execute_rw;
>   		cmd->execute_cmd = sbc_execute_rw;
> @@ -801,6 +836,9 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
>   			return TCM_INVALID_CDB_FIELD;
>   		sectors = transport_get_sectors_10(cdb);
>
> +		if (sbc_check_dpofua(dev, cmd, cdb))
> +			return TCM_INVALID_CDB_FIELD;
> +
>   		cmd->t_task_lba = transport_lba_32(cdb);
>   		cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
>
> @@ -810,8 +848,6 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
>   		cmd->execute_rw = ops->execute_rw;
>   		cmd->execute_cmd = sbc_execute_rw;
>   		cmd->transport_complete_callback = &xdreadwrite_callback;
> -		if (cdb[1] & 0x8)
> -			cmd->se_cmd_flags |= SCF_FUA;
>   		break;
>   	case VARIABLE_LENGTH_CMD:
>   	{
> @@ -820,6 +856,8 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
>   		case XDWRITEREAD_32:
>   			sectors = transport_get_sectors_32(cdb);
>
> +			if (sbc_check_dpofua(dev, cmd, cdb))
> +				return TCM_INVALID_CDB_FIELD;
>   			/*
>   			 * Use WRITE_32 and READ_32 opcodes for the emulated
>   			 * XDWRITE_READ_32 logic.
> @@ -834,8 +872,6 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
>   			cmd->execute_rw = ops->execute_rw;
>   			cmd->execute_cmd = sbc_execute_rw;
>   			cmd->transport_complete_callback = &xdreadwrite_callback;
> -			if (cdb[1] & 0x8)
> -				cmd->se_cmd_flags |= SCF_FUA;
>   			break;
>   		case WRITE_SAME_32:
>   			sectors = transport_get_sectors_32(cdb);
>

Other than that, Looks OK.

Reviewed-by: Sagi Grimberg <sagig@mellanox.com>

Re: [PATCH 5/8] target: Add sanity checks for DPO/FUA bit usage

[Christoph Hellwig]

On Sat, Feb 14, 2015 at 03:27:40AM +0000, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> This patch adds a sbc_check_dpofua() function that performs sanity
> checks for DPO/FUA command bits.
> 
> It introduces checks to fail when either bit is set, but the backend
> device is not advertising support for them.
> 
> It also moves the existing cmd->se_cmd_flags |= SCF_FUA assignement
> into the new helper function.

This causes I/O errors with ext4 on tcm_loop on what seems to be the first
journal commit:

[   41.818126] EXT4-fs (sdc): mounted filesystem with ordered data mode. Opts: (null)
[   48.919107] Got CDB: 0x2a with FUA bit set, but device does not advertise support for FUA write
[   48.920245] sd 3:0:0:0: [sdc] tag#113 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
[   48.921219] sd 3:0:0:0: [sdc] tag#113 Sense Key : Illegal Request [current] 
[   48.921980] sd 3:0:0:0: [sdc] tag#113 Add. Sense: Invalid field in cdb
[   48.922696] sd 3:0:0:0: [sdc] tag#113 CDB: Write(10) 2a 08 00 c4 22 c8 00 00 08 00
[   48.923619] blk_update_request: critical target error, dev sdc, sector 12853960
[   48.924501] blk_update_request: critical target error, dev sdc, sector 12853960
[   48.925598] Aborting journal on device sdc-8.

Re: [PATCH 5/8] target: Add sanity checks for DPO/FUA bit usage

[Douglas Gilbert]

On 15-02-22 11:41 AM, Christoph Hellwig wrote:
> On Sat, Feb 14, 2015 at 03:27:40AM +0000, Nicholas A. Bellinger wrote:
>> From: Nicholas Bellinger <nab@linux-iscsi.org>
>>
>> This patch adds a sbc_check_dpofua() function that performs sanity
>> checks for DPO/FUA command bits.
>>
>> It introduces checks to fail when either bit is set, but the backend
>> device is not advertising support for them.
>>
>> It also moves the existing cmd->se_cmd_flags |= SCF_FUA assignement
>> into the new helper function.
>
> This causes I/O errors with ext4 on tcm_loop on what seems to be the first
> journal commit:
>
> [   41.818126] EXT4-fs (sdc): mounted filesystem with ordered data mode. Opts: (null)
> [   48.919107] Got CDB: 0x2a with FUA bit set, but device does not advertise support for FUA write
> [   48.920245] sd 3:0:0:0: [sdc] tag#113 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
> [   48.921219] sd 3:0:0:0: [sdc] tag#113 Sense Key : Illegal Request [current]
> [   48.921980] sd 3:0:0:0: [sdc] tag#113 Add. Sense: Invalid field in cdb

Setting (or clearing) the FUA bit on READ or WRITE commands
does not cause an error according to sbc4r05 irrespective of
the LU's support for volatile and non-volatile caches. I'm
pretty sure that hasn't changed recently (say 15 years).

There is the curious DPOFUA bit in the device specific parameter
field for block devices. It can be fetched with MODE SENSE and serves
to inform the app client (Linux) that the DPO and FUA bit are
not supported. Hence when those bits are used in commands associated
with reading and writing the medium, they should acted upon if
supported or else ignored silently.


Please consider the code in scsi_debug (i.e. mk_sense_invalid_fld())
to set the sense key specific sense data on Illegal Requests. This
is to set the Field pointer to indicate which byte (and optionally
bit) in the cdb (or parameter data) that the target objects to.

Doug Gilbert

Re: [PATCH 5/8] target: Add sanity checks for DPO/FUA bit usage

[James Bottomley]

On Sun, 2015-02-22 at 15:19 -0500, Douglas Gilbert wrote:
> On 15-02-22 11:41 AM, Christoph Hellwig wrote:
> > On Sat, Feb 14, 2015 at 03:27:40AM +0000, Nicholas A. Bellinger wrote:
> >> From: Nicholas Bellinger <nab@linux-iscsi.org>
> >>
> >> This patch adds a sbc_check_dpofua() function that performs sanity
> >> checks for DPO/FUA command bits.
> >>
> >> It introduces checks to fail when either bit is set, but the backend
> >> device is not advertising support for them.
> >>
> >> It also moves the existing cmd->se_cmd_flags |= SCF_FUA assignement
> >> into the new helper function.
> >
> > This causes I/O errors with ext4 on tcm_loop on what seems to be the first
> > journal commit:
> >
> > [   41.818126] EXT4-fs (sdc): mounted filesystem with ordered data mode. Opts: (null)
> > [   48.919107] Got CDB: 0x2a with FUA bit set, but device does not advertise support for FUA write

Where is this coming from?  It's not a message I can find in the current
kernel.

> > [   48.920245] sd 3:0:0:0: [sdc] tag#113 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
> > [   48.921219] sd 3:0:0:0: [sdc] tag#113 Sense Key : Illegal Request [current]
> > [   48.921980] sd 3:0:0:0: [sdc] tag#113 Add. Sense: Invalid field in cdb
> 
> Setting (or clearing) the FUA bit on READ or WRITE commands
> does not cause an error according to sbc4r05 irrespective of
> the LU's support for volatile and non-volatile caches. I'm
> pretty sure that hasn't changed recently (say 15 years).

Well, firstly, that doesn't change the fact that sending FUA commands to
a non-FUA supporting device is a bug because the stable storage
guarantee the filesystem is relying on is broken.

However, your characterisation of the standards isn't quite correct:
you've forgotten SAT.  SAT allows a non FUA supporting ATA device to
return illegal request ... and I bet this is what's happening.

James

Re: [PATCH 5/8] target: Add sanity checks for DPO/FUA bit usage

[Douglas Gilbert]

On 15-02-22 03:44 PM, James Bottomley wrote:
> On Sun, 2015-02-22 at 15:19 -0500, Douglas Gilbert wrote:
>> On 15-02-22 11:41 AM, Christoph Hellwig wrote:
>>> On Sat, Feb 14, 2015 at 03:27:40AM +0000, Nicholas A. Bellinger wrote:
>>>> From: Nicholas Bellinger <nab@linux-iscsi.org>
>>>>
>>>> This patch adds a sbc_check_dpofua() function that performs sanity
>>>> checks for DPO/FUA command bits.
>>>>
>>>> It introduces checks to fail when either bit is set, but the backend
>>>> device is not advertising support for them.
>>>>
>>>> It also moves the existing cmd->se_cmd_flags |= SCF_FUA assignement
>>>> into the new helper function.
>>>
>>> This causes I/O errors with ext4 on tcm_loop on what seems to be the first
>>> journal commit:
>>>
>>> [   41.818126] EXT4-fs (sdc): mounted filesystem with ordered data mode. Opts: (null)
>>> [   48.919107] Got CDB: 0x2a with FUA bit set, but device does not advertise support for FUA write
>
> Where is this coming from?  It's not a message I can find in the current
> kernel.
>
>>> [   48.920245] sd 3:0:0:0: [sdc] tag#113 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
>>> [   48.921219] sd 3:0:0:0: [sdc] tag#113 Sense Key : Illegal Request [current]
>>> [   48.921980] sd 3:0:0:0: [sdc] tag#113 Add. Sense: Invalid field in cdb
>>
>> Setting (or clearing) the FUA bit on READ or WRITE commands
>> does not cause an error according to sbc4r05 irrespective of
>> the LU's support for volatile and non-volatile caches. I'm
>> pretty sure that hasn't changed recently (say 15 years).
>
> Well, firstly, that doesn't change the fact that sending FUA commands to
> a non-FUA supporting device is a bug because the stable storage
> guarantee the filesystem is relying on is broken.

> However, your characterisation of the standards isn't quite correct:
> you've forgotten SAT.  SAT allows a non FUA supporting ATA device to
> return illegal request ... and I bet this is what's happening.

That is indeed surprising. And I can see words to
support that (i.e. failing the command with illegal
request) for the translation of various SCSI READ
commands in sat4r00a.

However sat4r00a does not say that for WRITE commands.
It looks like the SATL given SCSI WRITE(FUA) in the
absence of ATA FUA support should translate that to
ATA WRITE followed by ATA VERIFY.

Since this case involves SCSI WRITE (FUA) being rejected
with Illegal Request then as far as I can see that is
wrong. The target subsystem might follow the same
pattern suggested by SAT and add a SCSI VERIFY when the
preceding WRITE has FUA set and it is not supported.

Doug Gilbert

Re: [PATCH 5/8] target: Add sanity checks for DPO/FUA bit usage

[Nicholas A. Bellinger]

On Sun, 2015-02-22 at 17:41 +0100, Christoph Hellwig wrote:
> On Sat, Feb 14, 2015 at 03:27:40AM +0000, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > 
> > This patch adds a sbc_check_dpofua() function that performs sanity
> > checks for DPO/FUA command bits.
> > 
> > It introduces checks to fail when either bit is set, but the backend
> > device is not advertising support for them.
> > 
> > It also moves the existing cmd->se_cmd_flags |= SCF_FUA assignement
> > into the new helper function.
> 
> This causes I/O errors with ext4 on tcm_loop on what seems to be the first
> journal commit:
> 
> [   41.818126] EXT4-fs (sdc): mounted filesystem with ordered data mode. Opts: (null)
> [   48.919107] Got CDB: 0x2a with FUA bit set, but device does not advertise support for FUA write
> [   48.920245] sd 3:0:0:0: [sdc] tag#113 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
> [   48.921219] sd 3:0:0:0: [sdc] tag#113 Sense Key : Illegal Request [current] 
> [   48.921980] sd 3:0:0:0: [sdc] tag#113 Add. Sense: Invalid field in cdb
> [   48.922696] sd 3:0:0:0: [sdc] tag#113 CDB: Write(10) 2a 08 00 c4 22 c8 00 00 08 00
> [   48.923619] blk_update_request: critical target error, dev sdc, sector 12853960
> [   48.924501] blk_update_request: critical target error, dev sdc, sector 12853960
> [   48.925598] Aborting journal on device sdc-8.
> 

Thanks for the heads up.

I'm able to reproduce this one, but only when emulate_write_cache has
been explicitly disabled via configfs before an FS unmount.  It happens
when WCE is disabled underneath the existing host side LUN registration,
yes..?

If that's the only case, then one option is to just keep the new
sbc_check_dpofua() check around and -EINVAL attribute modification of
emulate_[write_cache,fua_write] if any active fabric exports exist.

A quick patch to that end:

diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
index 58f49ff..37449bd 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -767,6 +767,16 @@ int se_dev_set_emulate_fua_write(struct se_device *dev, int flag)
                pr_err("Illegal value %d\n", flag);
                return -EINVAL;
        }
+       if (flag &&
+           dev->transport->get_write_cache) {
+               pr_err("emulate_fua_write not supported for this device\n");
+               return -EINVAL;
+       }
+       if (dev->export_count) {
+               pr_err("emulate_fua_write cannot be changed with active"
+                      " exports: %d\n", dev->export_count);
+               return -EINVAL;
+       }
        dev->dev_attrib.emulate_fua_write = flag;
        pr_debug("dev[%p]: SE Device Forced Unit Access WRITEs: %d\n",
                        dev, dev->dev_attrib.emulate_fua_write);
@@ -801,7 +811,11 @@ int se_dev_set_emulate_write_cache(struct se_device *dev, int flag)
                pr_err("emulate_write_cache not supported for this device\n");
                return -EINVAL;
        }
-
+       if (dev->export_count) {
+               pr_err("emulate_write_cache cannot be changed with active"
+                      " exports: %d\n", dev->export_count);
+               return -EINVAL;
+       }
        dev->dev_attrib.emulate_write_cache = flag;
        pr_debug("dev[%p]: SE Device WRITE_CACHE_EMULATION flag: %d\n",
                        dev, dev->dev_attrib.emulate_write_cache)

Are you seeing any other sbc_check_dpofua() related failures beyond this
particular case..?

--nab

[PATCH 6/8] target: Fail WRITE_SAME w/ UNMAP=1 when emulate_tpws=0

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch adds a check within sbc_setup_write_same() to fail a
WRITE_SAME w/ UNMAP=1 op, if the backend device has emulate_tpws
disabled.

Cc: Martin Petersen <martin.petersen@oracle.com>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_sbc.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 856e800..0d307c3 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -293,6 +293,11 @@ sbc_setup_write_same(struct se_cmd *cmd, unsigned char *flags, struct sbc_ops *o
 		if (!ops->execute_write_same_unmap)
 			return TCM_UNSUPPORTED_SCSI_OPCODE;
 
+		if (!dev->dev_attrib.emulate_tpws) {
+			pr_err("Got WRITE_SAME w/ UNMAP=1, but backend device"
+			       " has emulate_tpws disabled\n");
+			return TCM_UNSUPPORTED_SCSI_OPCODE;
+		}
 		cmd->execute_cmd = ops->execute_write_same_unmap;
 		return 0;
 	}
-- 
1.9.1

[PATCH 7/8] target: Fail UNMAP when emulate_tpu=0

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch adds a check within sbc_parse_cdb() to fail a UNMAP op,
if the backend device has emulate_tpu disabled.

Cc: Martin Petersen <martin.petersen@oracle.com>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_sbc.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 0d307c3..9661a66 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -962,6 +962,11 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		if (!ops->execute_unmap)
 			return TCM_UNSUPPORTED_SCSI_OPCODE;
 
+		if (!dev->dev_attrib.emulate_tpu) {
+			pr_err("Got UNMAP, but backend device has"
+			       " emulate_tpu disabled\n");
+			return TCM_UNSUPPORTED_SCSI_OPCODE;
+		}
 		size = get_unaligned_be16(&cdb[7]);
 		cmd->execute_cmd = ops->execute_unmap;
 		break;
-- 
1.9.1

[PATCH 8/8] target: Set LBPWS10 bit in Logical Block Provisioning EVPD

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch sets the missing LBPWS10 bit within spc_emulate_evpd_b2()
in order to signal WRITE_SAME (10) w/ UNMAP support, following the
existing LBPWS bit to signal WRITE_SAME (16) w/ UNMAP support.

Cc: Martin Petersen <martin.petersen@oracle.com>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_spc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
index 1307600..f041f93 100644
--- a/drivers/target/target_core_spc.c
+++ b/drivers/target/target_core_spc.c
@@ -650,7 +650,7 @@ spc_emulate_evpd_b2(struct se_cmd *cmd, unsigned char *buf)
 	 * support the use of the WRITE SAME (16) command to unmap LBAs.
 	 */
 	if (dev->dev_attrib.emulate_tpws != 0)
-		buf[5] |= 0x40;
+		buf[5] |= 0x40 | 0x20;
 
 	return 0;
 }
-- 
1.9.1