[v2,1/5] iscsi-target: split iscsit_check_dataout_hdr()

Commit Message

Nicholas A. Bellinger Feb. 19, 2017, 5:28 a.m. UTC

On Fri, 2017-01-13 at 20:53 +0530, Varun Prakash wrote:
> Split iscsit_check_dataout_hdr() into two functions
> 1. __iscsit_check_dataout_hdr() - This function
>    validates data out hdr.
> 2. iscsit_check_dataout_hdr() - This function finds
>    iSCSI cmd using iscsit_find_cmd_from_itt_or_dump(),
>    then it calls __iscsit_check_dataout_hdr() to
>    validate iSCSI hdr.
> 
> This split is required to support Chelsio T6 iSCSI
> DDP completion feature. T6 adapters reduce number of
> completions to host by generating single completion
> for all directly placed(DDP) iSCSI pdus in a sequence,
> DDP completion contains iSCSI hdr of the last pdu in a
> sequence.
> 
> On receiving DDP completion cxgbit driver will first
> find iSCSI cmd using iscsit_find_cmd_from_itt_or_dump()
> then updates cmd->write_data_done, cmd->next_burst_len,
> cmd->data_sn and calls  __iscsit_check_dataout_hdr()
> to validate iSCSI hdr.
> 
> Signed-off-by: Varun Prakash <[e-mail]>
> ---
>  drivers/target/iscsi/iscsi_target.c      | 38 +++++++++++++++++++++++---------
>  drivers/target/iscsi/iscsi_target_util.c |  1 +
>  include/target/iscsi/iscsi_transport.h   | 11 +++++++--
>  3 files changed, 37 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
> index da2c73a..3120068 100644
> --- a/drivers/target/iscsi/iscsi_target.c
> +++ b/drivers/target/iscsi/iscsi_target.c
> @@ -1431,11 +1431,10 @@ static void iscsit_do_crypto_hash_buf(
>  }
>  
>  int
> -iscsit_check_dataout_hdr(struct iscsi_conn *conn, unsigned char *buf,
> -			  struct iscsi_cmd **out_cmd)
> +__iscsit_check_dataout_hdr(struct iscsi_conn *conn, void *buf,
> +			   struct iscsi_cmd *cmd, bool *success)
>  {
> -	struct iscsi_data *hdr = (struct iscsi_data *)buf;
> -	struct iscsi_cmd *cmd = NULL;
> +	struct iscsi_data *hdr = buf;
>  	struct se_cmd *se_cmd;
>  	u32 payload_length = ntoh24(hdr->dlength);
>  	int rc;
> @@ -1456,11 +1455,6 @@ iscsit_check_dataout_hdr(struct iscsi_conn *conn, unsigned char *buf,
>  					 buf);
>  	}
>  
> -	cmd = iscsit_find_cmd_from_itt_or_dump(conn, hdr->itt,
> -			payload_length);
> -	if (!cmd)
> -		return 0;
> -

Proceeding this ITT lookup in existing iscsit_check_dataout_hdr() code,
there are checks for zero payload_length and MXDSL vs. received
payload_length.

As-is this change moves the ITT lookup and possible dump payload before
these two checks, which leaves the potential for a bogus payload length
to make it into iscsit_find_cmd_from_itt_or_dump(), which for a payload
dump involves a memory allocation.

That said, here is the updated version I've applied to avoid this case.
Note patch #5 has also been updated to use the new
__iscsit_check_dataout_hdr() parameter.

Thank you,

--nab

Patch

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index b4f1d1c..2285988 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -1431,36 +1431,17 @@  static void iscsit_do_crypto_hash_buf(
 }
 
 int
-iscsit_check_dataout_hdr(struct iscsi_conn *conn, unsigned char *buf,
-			  struct iscsi_cmd **out_cmd)
+__iscsit_check_dataout_hdr(struct iscsi_conn *conn, void *buf,
+			   struct iscsi_cmd *cmd, u32 payload_length,
+			   bool *success)
 {
-	struct iscsi_data *hdr = (struct iscsi_data *)buf;
-	struct iscsi_cmd *cmd = NULL;
+	struct iscsi_data *hdr = buf;
 	struct se_cmd *se_cmd;
-	u32 payload_length = ntoh24(hdr->dlength);
 	int rc;
 
-	if (!payload_length) {
-		pr_warn("DataOUT payload is ZERO, ignoring.\n");
-		return 0;
-	}
-
 	/* iSCSI write */
 	atomic_long_add(payload_length, &conn->sess->rx_data_octets);
 
-	if (payload_length > conn->conn_ops->MaxXmitDataSegmentLength) {
-		pr_err("DataSegmentLength: %u is greater than"
-			" MaxXmitDataSegmentLength: %u\n", payload_length,
-			conn->conn_ops->MaxXmitDataSegmentLength);
-		return iscsit_add_reject(conn, ISCSI_REASON_PROTOCOL_ERROR,
-					 buf);
-	}
-
-	cmd = iscsit_find_cmd_from_itt_or_dump(conn, hdr->itt,
-			payload_length);
-	if (!cmd)
-		return 0;
-
 	pr_debug("Got DataOut ITT: 0x%08x, TTT: 0x%08x,"
 		" DataSN: 0x%08x, Offset: %u, Length: %u, CID: %hu\n",
 		hdr->itt, hdr->ttt, hdr->datasn, ntohl(hdr->offset),
@@ -1553,10 +1534,44 @@  static void iscsit_do_crypto_hash_buf(
 		return 0;
 	else if (rc == DATAOUT_CANNOT_RECOVER)
 		return -1;
-
-	*out_cmd = cmd;
+	*success = true;
 	return 0;
 }
+EXPORT_SYMBOL(__iscsit_check_dataout_hdr);
+
+int
+iscsit_check_dataout_hdr(struct iscsi_conn *conn, void *buf,
+			 struct iscsi_cmd **out_cmd)
+{
+	struct iscsi_data *hdr = buf;
+	struct iscsi_cmd *cmd;
+	u32 payload_length = ntoh24(hdr->dlength);
+	int rc;
+	bool success = false;
+
+	if (!payload_length) {
+		pr_warn_ratelimited("DataOUT payload is ZERO, ignoring.\n");
+		return 0;
+	}
+
+	if (payload_length > conn->conn_ops->MaxXmitDataSegmentLength) {
+		pr_err_ratelimited("DataSegmentLength: %u is greater than"
+			" MaxXmitDataSegmentLength: %u\n", payload_length,
+			conn->conn_ops->MaxXmitDataSegmentLength);
+		return iscsit_add_reject(conn, ISCSI_REASON_PROTOCOL_ERROR, buf);
+	}
+
+	cmd = iscsit_find_cmd_from_itt_or_dump(conn, hdr->itt, payload_length);
+	if (!cmd)
+		return 0;
+
+	rc = __iscsit_check_dataout_hdr(conn, buf, cmd, payload_length, &success);
+
+	if (success)
+		*out_cmd = cmd;
+
+	return rc;
+}
 EXPORT_SYMBOL(iscsit_check_dataout_hdr);
 
 static int
diff --git a/drivers/target/iscsi/iscsi_target_util.c b/drivers/target/iscsi/iscsi_target_util.c
index b5a1b4c..f46eadf 100644
--- a/drivers/target/iscsi/iscsi_target_util.c
+++ b/drivers/target/iscsi/iscsi_target_util.c
@@ -417,6 +417,7 @@  struct iscsi_cmd *iscsit_find_cmd_from_itt_or_dump(
 
 	return NULL;
 }
+EXPORT_SYMBOL(iscsit_find_cmd_from_itt_or_dump);
 
 struct iscsi_cmd *iscsit_find_cmd_from_ttt(
 	struct iscsi_conn *conn,
diff --git a/include/target/iscsi/iscsi_transport.h b/include/target/iscsi/iscsi_transport.h
index 1277e9b..ff1a4f4 100644
--- a/include/target/iscsi/iscsi_transport.h
+++ b/include/target/iscsi/iscsi_transport.h
@@ -55,8 +55,12 @@  extern int iscsit_setup_scsi_cmd(struct iscsi_conn *, struct iscsi_cmd *,
 extern void iscsit_set_unsoliticed_dataout(struct iscsi_cmd *);
 extern int iscsit_process_scsi_cmd(struct iscsi_conn *, struct iscsi_cmd *,
 				struct iscsi_scsi_req *);
-extern int iscsit_check_dataout_hdr(struct iscsi_conn *, unsigned char *,
-				struct iscsi_cmd **);
+extern int
+__iscsit_check_dataout_hdr(struct iscsi_conn *, void *,
+			   struct iscsi_cmd *, u32, bool *);
+extern int
+iscsit_check_dataout_hdr(struct iscsi_conn *conn, void *buf,
+			 struct iscsi_cmd **out_cmd);
 extern int iscsit_check_dataout_payload(struct iscsi_cmd *, struct iscsi_data *,
 				bool);
 extern int iscsit_setup_nop_out(struct iscsi_conn *, struct iscsi_cmd *,
@@ -125,6 +129,9 @@  extern int iscsit_sequence_cmd(struct iscsi_conn *, struct iscsi_cmd *,
 extern void iscsit_free_cmd(struct iscsi_cmd *, bool);
 extern void iscsit_add_cmd_to_immediate_queue(struct iscsi_cmd *,
 					      struct iscsi_conn *, u8);
+extern struct iscsi_cmd *
+iscsit_find_cmd_from_itt_or_dump(struct iscsi_conn *conn,
+				 itt_t init_task_tag, u32 length);
 
 /*
  * From iscsi_target_nego.c