Re: [PATCH 06/19] target: Fix data buffer size for VERIFY and WRITE AND VERIFY commands

[Christoph Hellwig <hch@lst.de>]

Hi Bart,

what do you think about the variant below instead which avoids
overloading the size variable?

---
>From 206696ec37cf4f6efe093964c2bdc96100de1f62 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Fri, 5 May 2017 11:40:38 +0200
Subject: target: fix and cleanup size calculation in sbc_parse_cdb

Calculate the data buffer size individually for each command instead of
trying to generalize it.  This fixes the size calculation for VERIFY
and WRITE_VERIFY, while making the code a lot easier to understand.

Fixes: commit 0e2eb7d12eaa ("target: Fix VERIFY and WRITE VERIFY command parsing")
Reported-by: Bart Van Assche <bart.vanassche@sandisk.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/target/target_core_sbc.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 5807123214e5..0bd879b9ce38 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -899,12 +899,14 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 	switch (cdb[0]) {
 	case READ_6:
 		sectors = transport_get_sectors_6(cdb);
+		size = sbc_get_size(cmd, sectors);
 		cmd->t_task_lba = transport_lba_21(cdb);
 		cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
 		cmd->execute_cmd = sbc_execute_rw;
 		break;
 	case READ_10:
 		sectors = transport_get_sectors_10(cdb);
+		size = sbc_get_size(cmd, sectors);
 		cmd->t_task_lba = transport_lba_32(cdb);
 
 		if (sbc_check_dpofua(dev, cmd, cdb))
@@ -919,6 +921,7 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		break;
 	case READ_12:
 		sectors = transport_get_sectors_12(cdb);
+		size = sbc_get_size(cmd, sectors);
 		cmd->t_task_lba = transport_lba_32(cdb);
 
 		if (sbc_check_dpofua(dev, cmd, cdb))
@@ -933,6 +936,7 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		break;
 	case READ_16:
 		sectors = transport_get_sectors_16(cdb);
+		size = sbc_get_size(cmd, sectors);
 		cmd->t_task_lba = transport_lba_64(cdb);
 
 		if (sbc_check_dpofua(dev, cmd, cdb))
@@ -947,12 +951,14 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		break;
 	case WRITE_6:
 		sectors = transport_get_sectors_6(cdb);
+		size = sbc_get_size(cmd, sectors);
 		cmd->t_task_lba = transport_lba_21(cdb);
 		cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
 		cmd->execute_cmd = sbc_execute_rw;
 		break;
 	case WRITE_10:
 		sectors = transport_get_sectors_10(cdb);
+		size = sbc_get_size(cmd, sectors);
 		cmd->t_task_lba = transport_lba_32(cdb);
 
 		if (sbc_check_dpofua(dev, cmd, cdb))
@@ -974,6 +980,7 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		goto check_lba;
 	case WRITE_12:
 		sectors = transport_get_sectors_12(cdb);
+		size = sbc_get_size(cmd, sectors);
 		cmd->t_task_lba = transport_lba_32(cdb);
 
 		if (sbc_check_dpofua(dev, cmd, cdb))
@@ -988,6 +995,7 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		break;
 	case WRITE_16:
 		sectors = transport_get_sectors_16(cdb);
+		size = sbc_get_size(cmd, sectors);
 		cmd->t_task_lba = transport_lba_64(cdb);
 
 		if (sbc_check_dpofua(dev, cmd, cdb))
@@ -1005,6 +1013,7 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		    !(cmd->se_cmd_flags & SCF_BIDI))
 			return TCM_INVALID_CDB_FIELD;
 		sectors = transport_get_sectors_10(cdb);
+		size = sbc_get_size(cmd, sectors);
 
 		if (sbc_check_dpofua(dev, cmd, cdb))
 			return TCM_INVALID_CDB_FIELD;
@@ -1024,6 +1033,7 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		switch (service_action) {
 		case XDWRITEREAD_32:
 			sectors = transport_get_sectors_32(cdb);
+			size = sbc_get_size(cmd, sectors);
 
 			if (sbc_check_dpofua(dev, cmd, cdb))
 				return TCM_INVALID_CDB_FIELD;
@@ -1116,7 +1126,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);
 		}
+	
+		/*
+		 * XXX: why treat sectors / size check differently for
+		 * the offload  / non-offload cases?
+		 */
 		if (ops->execute_sync_cache) {
+			size = sbc_get_size(cmd, sectors);
 			cmd->execute_cmd = ops->execute_sync_cache;
 			goto check_lba;
 		}
@@ -1205,9 +1221,6 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 				end_lba, cmd->t_task_lba, sectors);
 			return TCM_ADDRESS_OUT_OF_RANGE;
 		}
-
-		if (!(cmd->se_cmd_flags & SCF_COMPARE_AND_WRITE))
-			size = sbc_get_size(cmd, sectors);
 	}
 
 	return target_cmd_size_check(cmd, size);
-- 
2.11.0