Re: [PATCH 1/2] scsi: target: iscsi: handle SCSI immediate commands

[Lee Duncan]

Apologies on my first reply having HTML. I'm learning a new MUA.

On Wed, Dec 13, 2023 at 12:06 PM Chris Leech <cleech@xxxxxxxxxx> wrote:
>
> 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?

The spec is confusing with respect to this. The "Immediate Bit"
means an immediate command. These commands are done "now",
not queued, and they do not increment the expected sequence number.

Immediate data is different, and unfortunately named IMHO. It's when a
PDU supplies the data for the SCSI command in the current PDU instead
of the next PDU.

>
> >        *
> >        * 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?

The immediate data check was there already, and there haven't been any
bugs I know of, so I assumed that part of the code was ok.

>
> 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.

I will check that but I suspect you are correct.

>
> - Chris Leech
>