On Thu, Dec 07, 2023 at 09:42:34AM -0800, lduncan@xxxxxxxx wrote:
> From: Lee Duncan <lduncan@xxxxxxxx>
>
> Some iSCSI initiators send SCSI PDUs with the "immediate" bit
> set, and this is allowed according to RFC 3720. Commands with
> the "Immediate" bit set are called "immediate commands". From
> section 3.2.2.1. "Command Numbering and Acknowledging":
>
> The target MUST NOT transmit a MaxCmdSN that is less than
> ExpCmdSN-1. For non-immediate commands, the CmdSN field can take any
> value from ExpCmdSN to MaxCmdSN inclusive. The target MUST silently
> ignore any non-immediate command outside of this range or non-
> immediate duplicates within the range. The CmdSN carried by
> immediate commands may lie outside the ExpCmdSN to MaxCmdSN range.
> For example, if the initiator has previously sent a non-immediate
> command carrying the CmdSN equal to MaxCmdSN, the target window is
> closed. For group task management commands issued as immediate
> commands, CmdSN indicates the scope of the group action (e.g., on
> ABORT TASK SET indicates which commands are aborted).
>
> This fixed an issue with fastlinq qedi Converged Network Adapter
> initiator firmware, trying to use an LIO target for booting. These
> changes made booting possible, with or without ImmediateData enabled.
>
> Signed-off-by: Lee Duncan <lduncan@xxxxxxxx>
> Reviewed-by: David Bond <dbond@xxxxxxxx>
> ---
> drivers/target/iscsi/iscsi_target.c | 12 +++---------
> drivers/target/iscsi/iscsi_target_util.c | 10 ++++++++--
> 2 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
> index 1d25e64b068a..f246e5015868 100644
> --- a/drivers/target/iscsi/iscsi_target.c
> +++ b/drivers/target/iscsi/iscsi_target.c
> @@ -1060,13 +1060,6 @@ int iscsit_setup_scsi_cmd(struct iscsit_conn *conn, struct iscsit_cmd *cmd,
> ISCSI_REASON_BOOKMARK_INVALID, buf);
> }
>
> - if (hdr->opcode & ISCSI_OP_IMMEDIATE) {
> - pr_err("Illegally set Immediate Bit in iSCSI Initiator"
> - " Scsi Command PDU.\n");
> - return iscsit_add_reject_cmd(cmd,
> - ISCSI_REASON_BOOKMARK_INVALID, buf);
> - }
> -
> if (payload_length && !conn->sess->sess_ops->ImmediateData) {
> pr_err("ImmediateData=No but DataSegmentLength=%u,"
> " protocol error.\n", payload_length);
This seems right, as the flag is checked again later in the same
function.
> @@ -1255,14 +1248,15 @@ int iscsit_process_scsi_cmd(struct iscsit_conn *conn, struct iscsit_cmd *cmd,
> /*
> * Check the CmdSN against ExpCmdSN/MaxCmdSN here if
> * the Immediate Bit is not set, and no Immediate
> - * Data is attached.
> + * Data is attached. Also skip the check if this is
> + * an immediate command.
This comment addition seems redundant, isn't that what the "Immediate
Bit is not set" already means?
> *
> * A PDU/CmdSN carrying Immediate Data can only
> * be processed after the DataCRC has passed.
> * If the DataCRC fails, the CmdSN MUST NOT
> * be acknowledged. (See below)
> */
> - if (!cmd->immediate_data) {
> + if (!cmd->immediate_data && !cmd->immediate_cmd) {
> cmdsn_ret = iscsit_sequence_cmd(conn, cmd,
> (unsigned char *)hdr, hdr->cmdsn);
> if (cmdsn_ret == CMDSN_ERROR_CANNOT_RECOVER)
Are you sure this needs to be checking both conditions here? I'm
struggling to understand why CmdSN checking would be bypassed for
immediate data. Is this a longstanding bug where the condition should
have been on immediate_cmd (and only immediate_cmd) instead?
Or is this because of the handling the immediate data with DataCRC case
mentioned? I do see iscsit_sequence_cmd also being called in
iscsit_get_immediate_data.
- Chris Leech
