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.