From patchwork Sun Feb 19 05:28:23 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Nicholas A. Bellinger" X-Patchwork-Id: 9581475 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 10312604A0 for ; Sun, 19 Feb 2017 05:28:27 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id E5CA528780 for ; Sun, 19 Feb 2017 05:28:26 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id D74D12879B; Sun, 19 Feb 2017 05:28:26 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id DBFDB28780 for ; Sun, 19 Feb 2017 05:28:25 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750750AbdBSF2Z (ORCPT ); Sun, 19 Feb 2017 00:28:25 -0500 Received: from mail.linux-iscsi.org ([67.23.28.174]:52595 "EHLO linux-iscsi.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750726AbdBSF2Y (ORCPT ); Sun, 19 Feb 2017 00:28:24 -0500 Received: from [192.168.1.66] (75-37-194-224.lightspeed.lsatca.sbcglobal.net [75.37.194.224]) (using SSLv3 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) (Authenticated sender: nab) by linux-iscsi.org (Postfix) with ESMTPSA id 521CB40B05; Sun, 19 Feb 2017 05:28:48 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=linux-iscsi.org; s=default.private; t=1487482128; bh=pyN82SpsdGlCOKFaZsxpRWQwBUP856s yDKMmK7uKDXU=; h=Message-ID:Subject:From:To:Cc:Date:In-Reply-To: References:Content-Type:Mime-Version:Content-Transfer-Encoding; b=fxZk7LOTJYY7b3c1SN+n4fwieN0LQ2RP1khFN8XXk5I7eaN3bwaw9Cu4ruYa+gaRH 3DkUtqXzDSgpyZJI6vcqoE8/1BMcqfcMON7a5ebyGG9vZzDQOgXAUUuI6nAPGnWoNQN sMcLITO9hU3re7ExWN6RZJeNtYkvertZk9pM/Qo= Message-ID: <1487482103.2958.36.camel@haakon3.risingtidesystems.com> Subject: Re: [PATCH v2 1/5] iscsi-target: split iscsit_check_dataout_hdr() From: "Nicholas A. Bellinger" To: Varun Prakash Cc: bart.vanassche@sandisk.com, target-devel@vger.kernel.org, indranil@chelsio.com Date: Sat, 18 Feb 2017 21:28:23 -0800 In-Reply-To: <43e9b31543b6f28dd66463c83696d2b48a938d6a.1484320514.git.varun@chelsio.com> References: <43e9b31543b6f28dd66463c83696d2b48a938d6a.1484320514.git.varun@chelsio.com> X-Mailer: Evolution 3.4.4-1 Mime-Version: 1.0 Sender: target-devel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: target-devel@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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 > --- > 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 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