Re: [PATCH 05/19] target: Allocate sg-list correctly

["Nicholas A. Bellinger" <nab@linux-iscsi.org>]

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.