From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from esa5.hgst.iphmx.com ([216.71.153.144]:23899 "EHLO esa5.hgst.iphmx.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750851AbdEHRqN (ORCPT ); Mon, 8 May 2017 13:46:13 -0400 From: Bart Van Assche To: "nab@linux-iscsi.org" CC: "hch@lst.de" , "ddiss@suse.de" , "hare@suse.com" , "target-devel@vger.kernel.org" , "agrover@redhat.com" , "stable@vger.kernel.org" Subject: Re: [PATCH 05/19] target: Allocate sg-list correctly Date: Mon, 8 May 2017 17:46:09 +0000 Message-ID: <1494265568.2591.12.camel@sandisk.com> References: <20170504225102.8931-1-bart.vanassche@sandisk.com> <20170504225102.8931-6-bart.vanassche@sandisk.com> <1494197127.30469.30.camel@haakon3.risingtidesystems.com> In-Reply-To: <1494197127.30469.30.camel@haakon3.risingtidesystems.com> Content-Language: en-US Content-Type: text/plain; charset="iso-8859-1" Content-ID: Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Sender: stable-owner@vger.kernel.org List-ID: On Sun, 2017-05-07 at 15:45 -0700, Nicholas A. Bellinger wrote: > On Thu, 2017-05-04 at 15:50 -0700, Bart Van Assche wrote: > > Avoid that the iSCSI target driver complains about "Initial page entry > > out-of-bounds" and closes the connection if the SCSI Data-Out buffer > > is larger than the buffer size specified through the CDB. This patch > > prevents that running the libiscsi regression tests against LIO trigger > > an infinite loop of libiscsi submitting a command, LIO closing the > > connection and libiscsi resubmitting the same command. > >=20 >=20 > This statement and patch is incorrect. >=20 > target-core has always rejected SCF_SCSI_DATA_CDB WRITEs with > overflow/underflow since day one: >=20 > From target_cmd_size_check(): >=20 > } else if (size !=3D cmd->data_length) { > pr_warn("TARGET_CORE[%s]: Expected Transfer Length:" > " %u does not match SCSI CDB Length: %u for SAM O= pcode:" > " 0x%02x\n", cmd->se_tfo->get_fabric_name(), > cmd->data_length, size, cmd->t_task_cdb[0= ]); >=20 > if (cmd->data_direction =3D=3D DMA_TO_DEVICE && > cmd->se_cmd_flags & SCF_SCSI_DATA_CDB) { > pr_err("Rejecting underflow/overflow WRITE data\n= "); > return TCM_INVALID_CDB_FIELD; > } >=20 > ...... > } Hello Nic, That behavior is as far as I know not compliant with the SCSI specs. In e.g= . the libiscsi test suite there are many tests that verify that a SCSI target implementation reports OVERFLOW or UNDERFLOW where LIO today reports INVALI= D FIELD IN CDB. See e.g.=A0https://github.com/sahlberg/libiscsi/blob/master/t= est-tool/test_read10_residuals.c. >>From RFC 3720 (apparently written from the point-of-view of transfers from target to initiator): =A0=A0=A0=A0=A0bit 5 - (O) set for Residual Overflow.=A0=A0In this case, th= e Residual =A0=A0=A0=A0=A0=A0=A0Count indicates the number of bytes that were not tran= sferred =A0=A0=A0=A0=A0=A0=A0because the initiator's Expected Data Transfer Length = was not =A0=A0=A0=A0=A0=A0=A0sufficient. =A0=A0=A0=A0=A0bit 6 - (U) set for Residual Underflow.=A0=A0In this case, t= he Residual =A0=A0=A0=A0=A0=A0=A0Count indicates the number of bytes that were not tran= sferred out =A0=A0=A0=A0=A0=A0=A0of the number of bytes that were expected to be transf= erred. But even with the current implementation, why do you think that the reject = by target_cmd_size_check() would be sufficient to prevent the behavior I descr= ibed? >>From source file iscsi_target.c: static int iscsit_handle_scsi_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd, =A0=A0=A0unsigned char *buf) { struct iscsi_scsi_req *hdr =3D (struct iscsi_scsi_req *)buf; int rc, immed_data; bool dump_payload =3D false; rc =3D iscsit_setup_scsi_cmd(conn, cmd, buf); if (rc < 0) return 0; /* * Allocation iovecs needed for struct socket operations for * traditional iSCSI block I/O. */ if (iscsit_allocate_iovecs(cmd) < 0) { return iscsit_reject_cmd(cmd, ISCSI_REASON_BOOKMARK_NO_RESOURCES, buf); } immed_data =3D cmd->immediate_data; rc =3D iscsit_process_scsi_cmd(conn, cmd, hdr); if (rc < 0) return rc; else if (rc > 0) dump_payload =3D true; if (!immed_data) return 0; return iscsit_get_immediate_data(cmd, hdr, dump_payload); } In other words, if target_setup_cmd_from_cdb() returns a sense code (positi= ve value) iscsit_get_immediate_data() will call rx_data() to receive that imme= diate data if there is immediate data and dump_payload =3D=3D false. The code tha= t controls the value of dump_payload is as follows (from iscsit_process_scsi_cmd()): if (cmd->sense_reason) { if (cmd->reject_reason) return 0; return 1; } In other words, if both .sense_reason and .reject_reason are set before iscsit_process_scsi_cmd() is called then=A0iscsit_get_immediate_data() can = call rx_data() to receive more data than what fits in the allocated buffer. Bart.=