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.