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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2/14/2015 5:27 AM, Nicholas A. Bellinger wrote:
From: Nicholas Bellinger <nab@xxxxxxxxxxxxxxx>

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@xxxxxx>
Signed-off-by: Nicholas Bellinger <nab@xxxxxxxxxxxxxxx>
---
  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@xxxxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]
  Powered by Linux