| Message ID | 20170504225102.8931-6-bart.vanassche@sandisk.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/05/2017 12:50 AM, 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. > > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> > Cc: Hannes Reinecke <hare@suse.com> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Andy Grover <agrover@redhat.com> > Cc: David Disseldorp <ddiss@suse.de> > Cc: <stable@vger.kernel.org> > --- > drivers/target/target_core_transport.c | 29 ++++++++++------------------- > include/target/target_core_base.h | 4 +++- > 2 files changed, 13 insertions(+), 20 deletions(-) > Reviewed-by: Hannes Reinecke <hare@suse.com> Cheers, Hannes
On Thu, May 04, 2017 at 03:50:48PM -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. Can you add a bit more of an explanation of why this happens? I've just tried to verify the area, but at least while sitting in a conference talk I can't quite make sense of the changes. -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2017-05-05 at 11:06 +0200, Christoph Hellwig wrote: > On Thu, May 04, 2017 at 03:50:48PM -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. > > Can you add a bit more of an explanation of why this happens? I've > just tried to verify the area, but at least while sitting in a conference > talk I can't quite make sense of the changes. Hello Christoph, The aspects of SCSI command processing that are relevant in this context are: * When the iSCSI target driver receives a SCSI command it calls transport_init_se_cmd() to initialize struct se_cmd. The iSCSI target driver passes the number of bytes that will be transferred into the "data_length" argument of transport_init_se_cmd(). That function stores the data length in the .data_length member of struct se_cmd. The value passed by target drivers to transport_init_se_cmd() is what is called the Expected Data Transfer Length (EDTL) in the iSCSI RFC. * After CDB parsing has finished target_cmd_size_check() is called. If EDTL exceeds the data buffer size extracted from the SCSI CDB (CDBL) then .data_length is reduced to CDBL. * Next target_alloc_sgl() allocates an sg-list for .data_length bytes (CDBL). * iscsit_allocate_iovecs() allocates a struct kvec (.iov_data) also for .data_length bytes (CDBL). * iscsit_get_dataout() calls rx_data() and tries to store EDTL bytes in the allocated struct kvec. If EDTL > CDBL then .iov_data overflows and this usually triggers a crash. With the patch that prevents .iov_data overflows the initiator is disconnected. In the case of libiscsi, it keeps retrying forever to resubmit SCSI commands for which EDTL > CDBL. In other words, initially EDTL is stored into .data_length and later on the value of .data_length changes to CDBL. My proposal to avoid the buffer overflows is to store both EDTL and CDBL in struct se_cmd and to allocate an sg-list per command that can store EDTL bytes instead of CDBL bytes. Bart.-- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. > This statement and patch is incorrect. target-core has always rejected SCF_SCSI_DATA_CDB WRITEs with overflow/underflow since day one: From target_cmd_size_check(): } else if (size != cmd->data_length) { pr_warn("TARGET_CORE[%s]: Expected Transfer Length:" " %u does not match SCSI CDB Length: %u for SAM Opcode:" " 0x%02x\n", cmd->se_tfo->get_fabric_name(), cmd->data_length, size, cmd->t_task_cdb[0]); if (cmd->data_direction == DMA_TO_DEVICE && cmd->se_cmd_flags & SCF_SCSI_DATA_CDB) { pr_err("Rejecting underflow/overflow WRITE data\n"); return TCM_INVALID_CDB_FIELD; } ...... } The reason you're suddenly hitting this now is because your patch in target-pending/for-next: commit 0e2eb7d12eaa8e391bf5615d4271bb87a649caaa Author: Bart Van Assche <bart.vanassche@sandisk.com> Date: Thu Mar 30 10:12:39 2017 -0700 incorrectly started allowing WRITE_VERIFY_* w/ bytchk = 0, without actually setting SCF_SCSI_DATA_CDB in sbc_parse_verify(): switch (bytchk) { case 0: *bufflen = 0; break; case 1: *bufflen = sbc_get_size(cmd, *sectors); cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB; break; default: pr_err("Unsupported BYTCHK value %d for SCSI opcode %#x\n", bytchk, cdb[0]); return TCM_INVALID_CDB_FIELD; } Patch #18 is also trying to (incorrectly) address the same problem. I'll post the proper fix for the regression introduced by your earlier patch there. > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> > Cc: Hannes Reinecke <hare@suse.com> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Andy Grover <agrover@redhat.com> > Cc: David Disseldorp <ddiss@suse.de> > Cc: <stable@vger.kernel.org> > --- > drivers/target/target_core_transport.c | 29 ++++++++++------------------- > include/target/target_core_base.h | 4 +++- > 2 files changed, 13 insertions(+), 20 deletions(-) > > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c > index df1ceb2dd110..86b6b0238975 100644 > --- a/drivers/target/target_core_transport.c > +++ b/drivers/target/target_core_transport.c > @@ -1159,11 +1159,11 @@ target_cmd_size_check(struct se_cmd *cmd, unsigned int size) > > if (cmd->unknown_data_length) { > cmd->data_length = size; > - } else if (size != cmd->data_length) { > + } else if (size != cmd->buffer_length) { > pr_warn("TARGET_CORE[%s]: Expected Transfer Length:" > " %u does not match SCSI CDB Length: %u for SAM Opcode:" > " 0x%02x\n", cmd->se_tfo->get_fabric_name(), > - cmd->data_length, size, cmd->t_task_cdb[0]); > + cmd->buffer_length, size, cmd->t_task_cdb[0]); > > if (cmd->data_direction == DMA_TO_DEVICE && > cmd->se_cmd_flags & SCF_SCSI_DATA_CDB) { > @@ -1183,7 +1183,7 @@ target_cmd_size_check(struct se_cmd *cmd, unsigned int size) > } > /* > * For the overflow case keep the existing fabric provided > - * ->data_length. Otherwise for the underflow case, reset > + * ->buffer_length. Otherwise for the underflow case, reset > * ->data_length to the smaller SCSI expected data transfer > * length. > */ > @@ -1227,6 +1227,7 @@ void transport_init_se_cmd( > > cmd->se_tfo = tfo; > cmd->se_sess = se_sess; > + cmd->buffer_length = data_length; > cmd->data_length = data_length; > cmd->data_direction = data_direction; > cmd->sam_task_attr = task_attr; > @@ -2412,41 +2413,31 @@ transport_generic_new_cmd(struct se_cmd *cmd) > * beforehand. > */ > if (!(cmd->se_cmd_flags & SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC) && > - cmd->data_length) { > + cmd->buffer_length) { > > if ((cmd->se_cmd_flags & SCF_BIDI) || > (cmd->se_cmd_flags & SCF_COMPARE_AND_WRITE)) { > - u32 bidi_length; > - > - if (cmd->se_cmd_flags & SCF_COMPARE_AND_WRITE) > - bidi_length = cmd->t_task_nolb * > - cmd->se_dev->dev_attrib.block_size; > - else > - bidi_length = cmd->data_length; > - > ret = target_alloc_sgl(&cmd->t_bidi_data_sg, > &cmd->t_bidi_data_nents, > - bidi_length, zero_flag, false); > + cmd->buffer_length, zero_flag, > + false); > if (ret < 0) > return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; > } > > ret = target_alloc_sgl(&cmd->t_data_sg, &cmd->t_data_nents, > - cmd->data_length, zero_flag, false); > + cmd->buffer_length, zero_flag, false); > if (ret < 0) > return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; > } else if ((cmd->se_cmd_flags & SCF_COMPARE_AND_WRITE) && > - cmd->data_length) { > + cmd->buffer_length) { > /* > * Special case for COMPARE_AND_WRITE with fabrics > * using SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC. > */ > - u32 caw_length = cmd->t_task_nolb * > - cmd->se_dev->dev_attrib.block_size; > - > ret = target_alloc_sgl(&cmd->t_bidi_data_sg, > &cmd->t_bidi_data_nents, > - caw_length, zero_flag, false); > + cmd->buffer_length, zero_flag, false); > if (ret < 0) > return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; > } > diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h > index 49cd03c2d943..0660585cb03d 100644 > --- a/include/target/target_core_base.h > +++ b/include/target/target_core_base.h > @@ -455,7 +455,9 @@ struct se_cmd { > enum transport_state_table t_state; > /* See se_cmd_flags_table */ > u32 se_cmd_flags; > - /* Total size in bytes associated with command */ > + /* Length of Data-Out or Data-In buffer */ > + u32 buffer_length; > + /* Number of bytes affected on storage medium */ > u32 data_length; > u32 residual_count; > u64 orig_fe_lun; -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. > > > > This statement and patch is incorrect. > > target-core has always rejected SCF_SCSI_DATA_CDB WRITEs with > overflow/underflow since day one: > > From target_cmd_size_check(): > > } else if (size != cmd->data_length) { > pr_warn("TARGET_CORE[%s]: Expected Transfer Length:" > " %u does not match SCSI CDB Length: %u for SAM Opcode:" > " 0x%02x\n", cmd->se_tfo->get_fabric_name(), > cmd->data_length, size, cmd->t_task_cdb[0]); > > if (cmd->data_direction == DMA_TO_DEVICE && > cmd->se_cmd_flags & SCF_SCSI_DATA_CDB) { > pr_err("Rejecting underflow/overflow WRITE data\n"); > return TCM_INVALID_CDB_FIELD; > } > > ...... > } 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 INVALID FIELD IN CDB. See e.g. https://github.com/sahlberg/libiscsi/blob/master/test-tool/test_read10_residuals.c. From RFC 3720 (apparently written from the point-of-view of transfers from target to initiator): bit 5 - (O) set for Residual Overflow. In this case, the Residual Count indicates the number of bytes that were not transferred because the initiator's Expected Data Transfer Length was not sufficient. bit 6 - (U) set for Residual Underflow. In this case, the Residual Count indicates the number of bytes that were not transferred out of the number of bytes that were expected to be transferred. 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 described? From source file iscsi_target.c: static int iscsit_handle_scsi_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd, unsigned char *buf) { struct iscsi_scsi_req *hdr = (struct iscsi_scsi_req *)buf; int rc, immed_data; bool dump_payload = false; rc = 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 = cmd->immediate_data; rc = iscsit_process_scsi_cmd(conn, cmd, hdr); if (rc < 0) return rc; else if (rc > 0) dump_payload = 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 (positive value) iscsit_get_immediate_data() will call rx_data() to receive that immediate data if there is immediate data and dump_payload == false. The code that 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 iscsit_get_immediate_data() can call rx_data() to receive more data than what fits in the allocated buffer. Bart.-- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2017-05-08 at 17:46 +0000, Bart Van Assche wrote: > 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. > > > > > > > This statement and patch is incorrect. > > > > target-core has always rejected SCF_SCSI_DATA_CDB WRITEs with > > overflow/underflow since day one: > > > > From target_cmd_size_check(): > > > > } else if (size != cmd->data_length) { > > pr_warn("TARGET_CORE[%s]: Expected Transfer Length:" > > " %u does not match SCSI CDB Length: %u for SAM Opcode:" > > " 0x%02x\n", cmd->se_tfo->get_fabric_name(), > > cmd->data_length, size, cmd->t_task_cdb[0]); > > > > if (cmd->data_direction == DMA_TO_DEVICE && > > cmd->se_cmd_flags & SCF_SCSI_DATA_CDB) { > > pr_err("Rejecting underflow/overflow WRITE data\n"); > > return TCM_INVALID_CDB_FIELD; > > } > > > > ...... > > } > > 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 INVALID > FIELD IN CDB. See e.g. https://github.com/sahlberg/libiscsi/blob/master/test-tool/test_read10_residuals.c. Yes, I understand what the test does. In practice this has never been an issue, and we've not actually encountered a host that sends overflow or underflow for a WRITE CDB of type SCF_SCSI_DATA_CDB. If there is a host environment that does, I'd like to hear about it. In any event, the point is your patch to add sbc_parse_verify() broke existing behavior of WRITE_VERIFY_* by dropping SCF_SCSI_DATA_CDB assignment for all cases. > From RFC 3720 (apparently written from the point-of-view of transfers from > target to initiator): > > bit 5 - (O) set for Residual Overflow. In this case, the Residual > Count indicates the number of bytes that were not transferred > because the initiator's Expected Data Transfer Length was not > sufficient. > > bit 6 - (U) set for Residual Underflow. In this case, the Residual > Count indicates the number of bytes that were not transferred out > of the number of bytes that were expected to be transferred. > > 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 described? > From source file iscsi_target.c: > > static int > iscsit_handle_scsi_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd, > unsigned char *buf) > { > struct iscsi_scsi_req *hdr = (struct iscsi_scsi_req *)buf; > int rc, immed_data; > bool dump_payload = false; > > rc = 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 = cmd->immediate_data; > > rc = iscsit_process_scsi_cmd(conn, cmd, hdr); > if (rc < 0) > return rc; > else if (rc > 0) > dump_payload = 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 (positive > value) iscsit_get_immediate_data() will call rx_data() to receive that immediate > data if there is immediate data and dump_payload == false. The code that 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 iscsit_get_immediate_data() can call > rx_data() to receive more data than what fits in the allocated buffer. > Look again. It's the code right below what you're referencing above, at the bottom of iscsit_process_scsi_cmd(): /* * Call directly into transport_generic_new_cmd() to perform * the backend memory allocation. */ cmd->sense_reason = transport_generic_new_cmd(&cmd->se_cmd); if (cmd->sense_reason) return 1; which propigates '1' up to iscsit_handle_scsi_cmd(), passing 'dump_payload = true' into iscsit_get_immediate_data() and thereby calls iscsit_dump_data_payload() to discard immediate data into a separate throw away buffer. Try it with sg_raw or libiscsi and you'll see. -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2017-05-09 at 21:03 -0700, Nicholas A. Bellinger wrote: > On Mon, 2017-05-08 at 17:46 +0000, Bart Van Assche wrote: > > 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. > > > > > > > > > > This statement and patch is incorrect. > > > > > > target-core has always rejected SCF_SCSI_DATA_CDB WRITEs with > > > overflow/underflow since day one: > > > > > > From target_cmd_size_check(): > > > > > > } else if (size != cmd->data_length) { > > > pr_warn("TARGET_CORE[%s]: Expected Transfer Length:" > > > " %u does not match SCSI CDB Length: %u for SAM Opcode:" > > > " 0x%02x\n", cmd->se_tfo->get_fabric_name(), > > > cmd->data_length, size, cmd->t_task_cdb[0]); > > > > > > if (cmd->data_direction == DMA_TO_DEVICE && > > > cmd->se_cmd_flags & SCF_SCSI_DATA_CDB) { > > > pr_err("Rejecting underflow/overflow WRITE data\n"); > > > return TCM_INVALID_CDB_FIELD; > > > } > > > > > > ...... > > > } > > > > 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 INVALID > > FIELD IN CDB. See e.g. https://github.com/sahlberg/libiscsi/blob/master/test-tool/test_read10_residuals.c. > > Yes, I understand what the test does. In practice this has never been > an issue, and we've not actually encountered a host that sends overflow > or underflow for a WRITE CDB of type SCF_SCSI_DATA_CDB. > > If there is a host environment that does, I'd like to hear about it. > > In any event, the point is your patch to add sbc_parse_verify() broke > existing behavior of WRITE_VERIFY_* by dropping SCF_SCSI_DATA_CDB > assignment for all cases. > > > From RFC 3720 (apparently written from the point-of-view of transfers from > > target to initiator): > > > > bit 5 - (O) set for Residual Overflow. In this case, the Residual > > Count indicates the number of bytes that were not transferred > > because the initiator's Expected Data Transfer Length was not > > sufficient. > > > > bit 6 - (U) set for Residual Underflow. In this case, the Residual > > Count indicates the number of bytes that were not transferred out > > of the number of bytes that were expected to be transferred. > > > > 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 described? > > From source file iscsi_target.c: > > > > static int > > iscsit_handle_scsi_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd, > > unsigned char *buf) > > { > > struct iscsi_scsi_req *hdr = (struct iscsi_scsi_req *)buf; > > int rc, immed_data; > > bool dump_payload = false; > > > > rc = 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 = cmd->immediate_data; > > > > rc = iscsit_process_scsi_cmd(conn, cmd, hdr); > > if (rc < 0) > > return rc; > > else if (rc > 0) > > dump_payload = 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 (positive > > value) iscsit_get_immediate_data() will call rx_data() to receive that immediate > > data if there is immediate data and dump_payload == false. The code that 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 iscsit_get_immediate_data() can call > > rx_data() to receive more data than what fits in the allocated buffer. > > > > Look again. It's the code right below what you're referencing above, at > the bottom of iscsit_process_scsi_cmd(): > > /* > * Call directly into transport_generic_new_cmd() to perform > * the backend memory allocation. > */ > cmd->sense_reason = transport_generic_new_cmd(&cmd->se_cmd); > if (cmd->sense_reason) > return 1; > > which propigates '1' up to iscsit_handle_scsi_cmd(), passing > 'dump_payload = true' into iscsit_get_immediate_data() and thereby calls > iscsit_dump_data_payload() to discard immediate data into a separate > throw away buffer. > > Try it with sg_raw or libiscsi and you'll see. Oh btw, to further clarify your earlier question about the following code in iscsit_process_scsi_cmd() returning zero when cmd->sense_reason && cmd->reject_reason is true: if (cmd->sense_reason) { if (cmd->reject_reason) return 0; return 1; } The earlier callers that invoke iscsi_add_reject*() all return -1 from iscsit_setup_scsi_cmd(), which ensures iscsit_handle_scsi_cmd() returns and this code to return '0' is never reached. -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2017-05-09 at 21:03 -0700, Nicholas A. Bellinger wrote: > In any event, the point is your patch to add sbc_parse_verify() broke > existing behavior of WRITE_VERIFY_* by dropping SCF_SCSI_DATA_CDB > assignment for all cases. As I had already explained in detail I disagree with this statement. BTW, did you know that your patch "target: Fix sbc_parse_verify bytchk = 0 handling" is not sufficient to avoid a buffer overflow in the iSCSI target driver? One way to trigger a buffer overflow is by making the initiator send more immediate data than the Data-Out buffer size derived from the CDB. Bart.-- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2017-05-10 at 20:31 +0000, Bart Van Assche wrote: > On Tue, 2017-05-09 at 21:03 -0700, Nicholas A. Bellinger wrote: > > In any event, the point is your patch to add sbc_parse_verify() broke > > existing behavior of WRITE_VERIFY_* by dropping SCF_SCSI_DATA_CDB > > assignment for all cases. > > As I had already explained in detail I disagree with this statement. BTW, did > you know that your patch "target: Fix sbc_parse_verify bytchk = 0 handling" is > not sufficient to avoid a buffer overflow in the iSCSI target driver? One way > to trigger a buffer overflow is by making the initiator send more immediate > data than the Data-Out buffer size derived from the CDB. > If you think you've found a legitimate bug, then post the test case to trigger it atop what's in target-pending/for-next for -rc1. Regardless, your WRITE_VERIFY changes broke existing behavior, and I'm not going to merge a bunch of new code at the last minute. -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c index df1ceb2dd110..86b6b0238975 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -1159,11 +1159,11 @@ target_cmd_size_check(struct se_cmd *cmd, unsigned int size) if (cmd->unknown_data_length) { cmd->data_length = size; - } else if (size != cmd->data_length) { + } else if (size != cmd->buffer_length) { pr_warn("TARGET_CORE[%s]: Expected Transfer Length:" " %u does not match SCSI CDB Length: %u for SAM Opcode:" " 0x%02x\n", cmd->se_tfo->get_fabric_name(), - cmd->data_length, size, cmd->t_task_cdb[0]); + cmd->buffer_length, size, cmd->t_task_cdb[0]); if (cmd->data_direction == DMA_TO_DEVICE && cmd->se_cmd_flags & SCF_SCSI_DATA_CDB) { @@ -1183,7 +1183,7 @@ target_cmd_size_check(struct se_cmd *cmd, unsigned int size) } /* * For the overflow case keep the existing fabric provided - * ->data_length. Otherwise for the underflow case, reset + * ->buffer_length. Otherwise for the underflow case, reset * ->data_length to the smaller SCSI expected data transfer * length. */ @@ -1227,6 +1227,7 @@ void transport_init_se_cmd( cmd->se_tfo = tfo; cmd->se_sess = se_sess; + cmd->buffer_length = data_length; cmd->data_length = data_length; cmd->data_direction = data_direction; cmd->sam_task_attr = task_attr; @@ -2412,41 +2413,31 @@ transport_generic_new_cmd(struct se_cmd *cmd) * beforehand. */ if (!(cmd->se_cmd_flags & SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC) && - cmd->data_length) { + cmd->buffer_length) { if ((cmd->se_cmd_flags & SCF_BIDI) || (cmd->se_cmd_flags & SCF_COMPARE_AND_WRITE)) { - u32 bidi_length; - - if (cmd->se_cmd_flags & SCF_COMPARE_AND_WRITE) - bidi_length = cmd->t_task_nolb * - cmd->se_dev->dev_attrib.block_size; - else - bidi_length = cmd->data_length; - ret = target_alloc_sgl(&cmd->t_bidi_data_sg, &cmd->t_bidi_data_nents, - bidi_length, zero_flag, false); + cmd->buffer_length, zero_flag, + false); if (ret < 0) return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; } ret = target_alloc_sgl(&cmd->t_data_sg, &cmd->t_data_nents, - cmd->data_length, zero_flag, false); + cmd->buffer_length, zero_flag, false); if (ret < 0) return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; } else if ((cmd->se_cmd_flags & SCF_COMPARE_AND_WRITE) && - cmd->data_length) { + cmd->buffer_length) { /* * Special case for COMPARE_AND_WRITE with fabrics * using SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC. */ - u32 caw_length = cmd->t_task_nolb * - cmd->se_dev->dev_attrib.block_size; - ret = target_alloc_sgl(&cmd->t_bidi_data_sg, &cmd->t_bidi_data_nents, - caw_length, zero_flag, false); + cmd->buffer_length, zero_flag, false); if (ret < 0) return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; } diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h index 49cd03c2d943..0660585cb03d 100644 --- a/include/target/target_core_base.h +++ b/include/target/target_core_base.h @@ -455,7 +455,9 @@ struct se_cmd { enum transport_state_table t_state; /* See se_cmd_flags_table */ u32 se_cmd_flags; - /* Total size in bytes associated with command */ + /* Length of Data-Out or Data-In buffer */ + u32 buffer_length; + /* Number of bytes affected on storage medium */ u32 data_length; u32 residual_count; u64 orig_fe_lun;
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. Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> Cc: Hannes Reinecke <hare@suse.com> Cc: Christoph Hellwig <hch@lst.de> Cc: Andy Grover <agrover@redhat.com> Cc: David Disseldorp <ddiss@suse.de> Cc: <stable@vger.kernel.org> --- drivers/target/target_core_transport.c | 29 ++++++++++------------------- include/target/target_core_base.h | 4 +++- 2 files changed, 13 insertions(+), 20 deletions(-)