| Message ID | 1496895685-18464-1-git-send-email-nab@linux-iscsi.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2017-06-08 at 04:21 +0000, Nicholas A. Bellinger wrote: > + /* > + * Check for underflow case where both EDTL and immediate data payload > + * exceeds what is presented by CDB's TRANSFER LENGTH, and what has > + * already been set in target_cmd_size_check() as se_cmd->data_length. > + * > + * For this special case, fail the command and dump the immediate data > + * payload. > + */ > + if (cmd->first_burst_len > cmd->se_cmd.data_length) { > + cmd->sense_reason = TCM_INVALID_CDB_FIELD; > + goto after_immediate_data; > + } A quote from the iSCSI RFC (https://tools.ietf.org/html/rfc5048): If SPDTL < EDTL for a task, iSCSI Underflow MUST be signaled in the SCSI Response PDU as specified in [RFC3720]. The Residual Count MUST be set to the numerical value of (EDTL - SPDTL). Sorry but I don't think that sending TCM_INVALID_CDB_FIELD back to the initiator is compliant with the iSCSI RFC. Please note that a fix that is compliant with the iSCSI RFC is present in the following patch series: [PATCH 00/33] SCSI target driver patches for kernel v4.13, 23 May 2017 (https://www.spinics.net/lists/target-devel/msg15370.html). 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-06-08 at 15:37 +0000, Bart Van Assche wrote: > On Thu, 2017-06-08 at 04:21 +0000, Nicholas A. Bellinger wrote: > > + /* > > + * Check for underflow case where both EDTL and immediate data payload > > + * exceeds what is presented by CDB's TRANSFER LENGTH, and what has > > + * already been set in target_cmd_size_check() as se_cmd->data_length. > > + * > > + * For this special case, fail the command and dump the immediate data > > + * payload. > > + */ > > + if (cmd->first_burst_len > cmd->se_cmd.data_length) { > > + cmd->sense_reason = TCM_INVALID_CDB_FIELD; > > + goto after_immediate_data; > > + } > > A quote from the iSCSI RFC (https://tools.ietf.org/html/rfc5048): > > If SPDTL < EDTL for a task, iSCSI Underflow MUST be signaled in the > SCSI Response PDU as specified in [RFC3720]. The Residual Count MUST > be set to the numerical value of (EDTL - SPDTL). > > Sorry but I don't think that sending TCM_INVALID_CDB_FIELD back to the > initiator is compliant with the iSCSI RFC. Alas, the nuance of what this patch actually does was missed when you cut the context. First, a bit of history. LIO has rejected underflow for all WRITEs for the first ~12.5 years of RFC-3720, and in the context of iscsi-target mode there has never been a single host environment that ever once cared. Since Roland's patch to allow underflow for control CDBs in v4.3+ opened this discussion for control CDBs with a WRITE payload in order to make MSFT/FCP cert for PERSISTENT_RESERVE_OUT happy, the question has become what control CDB WRITE underflow cases should we allow..? The point with this patch is when a host is sending a underflow with a iscsi immediate data payload that exceeds SCSI transfer length, it's a bogus request with a garbage payload. It's a garbage payload because the SCSI CDB itself obviously doesn't want anything to do it. I'm very dubious of any host environment who's trying to do this for any CDB, and expects achieve expected results. Of course, since v4.3+ normal overflow where SCSI transfer length matches the iscsi immediate data payload just works with or without this patch. So to that extent, I'm going to push this patch as a defensive fix for v4.3+, to let those imaginary iscsi host environments know they being very, very naughty. > Please note that a fix that is > compliant with the iSCSI RFC is present in the following patch series: [PATCH > 00/33] SCSI target driver patches for kernel v4.13, 23 May 2017 > (https://www.spinics.net/lists/target-devel/msg15370.html). So I might still consider this as a v4.13-rc item for control CDB underflow, but no way as stable material. Also, there is certainly no way I'm going to allow a patch to randomly enable underflow/overflow for all WRITE non control CDBs tree-wide across all fabric drivers, because 1) no host environments actually care about this, and 2) it's still dangerous to do for all fabrics without some serious auditing. -- 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
Hi Bart, On Thu, 2017-06-08 at 23:55 -0700, Nicholas A. Bellinger wrote: > On Thu, 2017-06-08 at 15:37 +0000, Bart Van Assche wrote: > > On Thu, 2017-06-08 at 04:21 +0000, Nicholas A. Bellinger wrote: > > > + /* > > > + * Check for underflow case where both EDTL and immediate data payload > > > + * exceeds what is presented by CDB's TRANSFER LENGTH, and what has > > > + * already been set in target_cmd_size_check() as se_cmd->data_length. > > > + * > > > + * For this special case, fail the command and dump the immediate data > > > + * payload. > > > + */ > > > + if (cmd->first_burst_len > cmd->se_cmd.data_length) { > > > + cmd->sense_reason = TCM_INVALID_CDB_FIELD; > > > + goto after_immediate_data; > > > + } > > > > A quote from the iSCSI RFC (https://tools.ietf.org/html/rfc5048): > > > > If SPDTL < EDTL for a task, iSCSI Underflow MUST be signaled in the > > SCSI Response PDU as specified in [RFC3720]. The Residual Count MUST > > be set to the numerical value of (EDTL - SPDTL). > > > > Sorry but I don't think that sending TCM_INVALID_CDB_FIELD back to the > > initiator is compliant with the iSCSI RFC. > > Alas, the nuance of what this patch actually does was missed when you > cut the context. > > First, a bit of history. LIO has rejected underflow for all WRITEs for > the first ~12.5 years of RFC-3720, and in the context of iscsi-target > mode there has never been a single host environment that ever once > cared. > > Since Roland's patch to allow underflow for control CDBs in v4.3+ opened > this discussion for control CDBs with a WRITE payload in order to make > MSFT/FCP cert for PERSISTENT_RESERVE_OUT happy, the question has become > what control CDB WRITE underflow cases should we allow..? > > The point with this patch is when a host is sending a underflow with a > iscsi immediate data payload that exceeds SCSI transfer length, it's a > bogus request with a garbage payload. It's a garbage payload because > the SCSI CDB itself obviously doesn't want anything to do it. > > I'm very dubious of any host environment who's trying to do this for any > CDB, and expects achieve expected results. > > Of course, since v4.3+ normal overflow where SCSI transfer length > matches the iscsi immediate data payload just works with or without this > patch. > > So to that extent, I'm going to push this patch as a defensive fix for > v4.3+, to let those imaginary iscsi host environments know they being > very, very naughty. > > > Please note that a fix that is > > compliant with the iSCSI RFC is present in the following patch series: [PATCH > > 00/33] SCSI target driver patches for kernel v4.13, 23 May 2017 > > (https://www.spinics.net/lists/target-devel/msg15370.html). > > So I might still consider this as a v4.13-rc item for control CDB > underflow, but no way as stable material. > > Also, there is certainly no way I'm going to allow a patch to randomly > enable underflow/overflow for all WRITE non control CDBs tree-wide > across all fabric drivers, because 1) no host environments actually care > about this, and 2) it's still dangerous to do for all fabrics without > some serious auditing. After further consideration, I've decided against allowing iscsi-target underflow with a immediate data payload larger than SCSI transfer length. Any host environment that attempts to send an underflow with a immediate data payload larger than SCSI transfer length, expects the target to automatically truncate immediate data, and still return GOOD status is completely bogus. Any host that attempts this is buggy, and needs to be fixed. This is because for the last ~12 years of RFC-3720: - There has never been a host environment in the wild that exhibits this behavior. - There has never been a conformance suite which expects this behavior. So rejecting this case as already done in commit abb85a9b51 is the correct approach for >= v4.3.y. Of course, the typical underflow scenario which Roland's v4.3.y commit enabled, underflow where immediate data matches the SCSI transfer length is supported for control CDBs. That said, thanks for high-lighting this particular corner case, so it could be fixed in >= v4.3.y. -- 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-07-11 at 00:22 -0700, Nicholas A. Bellinger wrote: > So rejecting this case as already done in commit abb85a9b51 is the > correct approach for >= v4.3.y. Hello Nic, I hope that you agree that the current target_cmd_size_check() implementation is complicated and ugly. Patch 30/33 of the patch series I referred to in my e-mail removes a significant number of lines of code from that function. So my patch series not only makes target_cmd_size_check() easier to maintain and to verify but it makes that function also faster. Hence please reconsider the approach from my patch series. For patch 30/33, see also https://www.spinics.net/lists/target-devel/msg15384.html. 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
diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c index c025451..3fdca2c 100644 --- a/drivers/target/iscsi/iscsi_target.c +++ b/drivers/target/iscsi/iscsi_target.c @@ -1279,6 +1279,18 @@ int iscsit_process_scsi_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd, */ if (dump_payload) goto after_immediate_data; + /* + * Check for underflow case where both EDTL and immediate data payload + * exceeds what is presented by CDB's TRANSFER LENGTH, and what has + * already been set in target_cmd_size_check() as se_cmd->data_length. + * + * For this special case, fail the command and dump the immediate data + * payload. + */ + if (cmd->first_burst_len > cmd->se_cmd.data_length) { + cmd->sense_reason = TCM_INVALID_CDB_FIELD; + goto after_immediate_data; + } immed_ret = iscsit_handle_immediate_data(cmd, hdr, cmd->first_burst_len);