[PATCH 0/2] scsi: target: iscsi: Fix two protocol issues

[PATCH 0/2] scsi: target: iscsi: Fix two protocol issues

[lduncan]

From: Lee Duncan <lduncan@suse.com>

Recent boot testing using the fastlinq qedi iSCSI Converged Network
Adapter and an LIO target uncovered a couple of issues with the
kernel iSCSI target driver. The first patch addresses an issue
with the handling of iSCSI "immediate commands", which are allowed
but not common, and the second patch lowers the noise level of the
driver when initiators send PDUs with the read and/or write bits
set but no data, which is also allowed. (See RFC 3720)

Lee Duncan (2):
  scsi: target: iscsi: handle SCSI immediate commands
  scsi: target: iscsi: don't warn of R/W when no data

 drivers/target/iscsi/iscsi_target.c      | 17 ++++++-----------
 drivers/target/iscsi/iscsi_target_util.c | 10 ++++++++--
 2 files changed, 14 insertions(+), 13 deletions(-)

-- 
2.43.0

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

[lduncan]

From: Lee Duncan <lduncan@suse.com>

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@suse.com>
Reviewed-by: David Bond <dbond@suse.com>
---
 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);
@@ -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.
 	 *
 	 * 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)
diff --git a/drivers/target/iscsi/iscsi_target_util.c b/drivers/target/iscsi/iscsi_target_util.c
index 91a75a4a7cc1..546816b3de86 100644
--- a/drivers/target/iscsi/iscsi_target_util.c
+++ b/drivers/target/iscsi/iscsi_target_util.c
@@ -285,13 +285,19 @@ static inline int iscsit_check_received_cmdsn(struct iscsit_session *sess, u32 c
 int iscsit_sequence_cmd(struct iscsit_conn *conn, struct iscsit_cmd *cmd,
 			unsigned char *buf, __be32 cmdsn)
 {
-	int ret, cmdsn_ret;
+	int ret, cmdsn_ret = CMDSN_NORMAL_OPERATION;
 	bool reject = false;
 	u8 reason = ISCSI_REASON_BOOKMARK_NO_RESOURCES;
 
 	mutex_lock(&conn->sess->cmdsn_mutex);
 
-	cmdsn_ret = iscsit_check_received_cmdsn(conn->sess, be32_to_cpu(cmdsn));
+	/*
+	 * Check the sequence number iff we are not in an immediate command.
+	 * See rfc3730 Section 3.2.2.1. Immediate commands can be outside
+	 * the normal range.
+	 */
+	if (!cmd->immediate_cmd)
+		cmdsn_ret = iscsit_check_received_cmdsn(conn->sess, be32_to_cpu(cmdsn));
 	switch (cmdsn_ret) {
 	case CMDSN_NORMAL_OPERATION:
 		ret = iscsit_execute_cmd(cmd, 0);
-- 
2.43.0

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

[Chris Leech]

On Thu, Dec 07, 2023 at 09:42:34AM -0800, lduncan@suse.com wrote:
> From: Lee Duncan <lduncan@suse.com>
> 
> 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@suse.com>
> Reviewed-by: David Bond <dbond@suse.com>
> ---
>  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

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@redhat.com> wrote:
>
> On Thu, Dec 07, 2023 at 09:42:34AM -0800, lduncan@suse.com wrote:
> > From: Lee Duncan <lduncan@suse.com>
> >
> > 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@suse.com>
> > Reviewed-by: David Bond <dbond@suse.com>
> > ---
> >  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
>

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

[Chris Leech]

On Wed, Dec 13, 2023 at 05:24:54PM -0800, Lee Duncan wrote:
> >
> > > @@ -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.

I understand the protocol, just trying to make sense of the
implementation and what the existing comment meant. And the existing
comment already has two conditions in it, even if the code doesn't.

I think I understand now why this is delaying CmdSN validation when
there is immediate data, until after the DataCRC can be checked.

This comment in iscsit_get_immediate_data, where the delayed processing
occurs, also seems to read that "Immediate Bit" is in reference to an
immediate command.

  * A PDU/CmdSN carrying Immediate Data passed
  * DataCRC, check against ExpCmdSN/MaxCmdSN if
  * Immediate Bit is not set.

but neither of these locations (before these changes) that mention the
"Immediate Bit" in the comments actually check for cmd->immediate_cmd.

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

Is it correct to skip all of iscsit_sequence_cmd for an immediate
command here? You are already skipping iscsit_check_received_cmdsn
inside iscsit_sequence_cmd in this patch. If cmd->immediate_cmd is set,
where does iscsit_execute_cmd now get called from?

- Chris Leech

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

[Lee Duncan]

Apologies for the delay in the reply, but over the
time to address Chris' two questions about this patch set. See below.

On Thu, Dec 14, 2023 at 12:30 PM Chris Leech <cleech@redhat.com> wrote:
>
> On Wed, Dec 13, 2023 at 05:24:54PM -0800, Lee Duncan wrote:
> > >
> > > > @@ -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.
>
> I understand the protocol, just trying to make sense of the
> implementation and what the existing comment meant. And the existing
> comment already has two conditions in it, even if the code doesn't.
>
> I think I understand now why this is delaying CmdSN validation when
> there is immediate data, until after the DataCRC can be checked.
>
> This comment in iscsit_get_immediate_data, where the delayed processing
> occurs, also seems to read that "Immediate Bit" is in reference to an
> immediate command.
>
>   * A PDU/CmdSN carrying Immediate Data passed
>   * DataCRC, check against ExpCmdSN/MaxCmdSN if
>   * Immediate Bit is not set.
>
> but neither of these locations (before these changes) that mention the
> "Immediate Bit" in the comments actually check for cmd->immediate_cmd.
>

I talked to Chris a bit about this offline, for clarification. I believe I
understand his concern, and rather than try to assert the patch is
ok by inspection, I decided to just test it.

Turns out that normal PDU traffic for lots of writes generally
includes "immediate data", and so it was easy to test this.

Testing showed that Immediate Data still works correctly,
in SCSI Write PDUs. Test was:
* connect to an iSCSI target
* Write a bunch of data
* read back the data
* disconnect from target and compare data

In addition, I captured and analyzed the SCSI/iSCSI tcpdump trace,
and immediate data was present, as expected.

One co-worker ran a similar test (just the SCSI/iSCSI trace part),
and found the same results.

> > > >        *
> > > >        * 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.
>
> Is it correct to skip all of iscsit_sequence_cmd for an immediate
> command here? You are already skipping iscsit_check_received_cmdsn
> inside iscsit_sequence_cmd in this patch. If cmd->immediate_cmd is set,
> where does iscsit_execute_cmd now get called from?

I looked at the code and the SPEC in more detail, and I believe the answer
is "yes", it is correct.

That function checks the current PDU's sequence number with
the following tests (and side effects), but not in this order:
* check that seq# is not larger than maximum
* check that seq# is not larger than expected
* check that seq# is not smaller than expected
* else the seq# is correct, so *SIDE* *EFFECT* increment the
                                               expected seq# for next PDU

It turns out that the SPEC allow the sequence number to be
out of range for immediate commands! So none of the checks
in iscsit_sequence_check_received_cmndsn() are valid for
immediate commands, as far as I can see.

>
> - Chris Leech
>

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

[michael.christie]

On 12/7/23 11:42 AM, lduncan@suse.com wrote:
> From: Lee Duncan <lduncan@suse.com>
> 
> 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.
> 

This is taking me a really long time to review because I've never looked 
at some of these code paths.

Have you tested the error cases?

What happens for a scsi command that's marked with the immediate bit and:
1. We get an abort and
	1.A The scsi command has completed?
	1.B The scsi command is being completed?

	For example, if the command we want to abort is not in the window, does 
iscsit_find_cmd_from_itt just not find the command and do we just return 
ISCSI_TMF_RSP_NO_TASK so the initiator will just escalate to lun reset.

2. For lun reset and abort, if the scsi command we want to abort/reset 
is not in the window (let's say it's cmdsn is higher than max_cmd_sn), 
does the iscsi layer complete the scsi command then complete the TMF or 
if the TMF has a lower cmdsn than the scsi command does the iscsi layer 
complete the scsi command then the TMF?

3. What happens for 1 and 2 and ERL 2 is used so we have 
ISCSI_TM_FUNC_TASK_REASSIGN and maybe are hitting the out of order code 
as well? Does it work ok?

[PATCH 2/2] scsi: target: iscsi: don't warn of R/W when no data

[lduncan]

From: Lee Duncan <lduncan@suse.com>

The LIO target code has a warning about setting the
read and/or write header bits with a PDU that has zero
transfer length, even though the code mentions that the
SPEC (RFC 3720) allows this, and that some initiators
set these bits. But in practice such initiators end up
flooding the logs with thousands of warning messages for
IO that is allowed.

So change this to a debug message, and clean up the wording
just a little bit while at it.

Signed-off-by: Lee Duncan <lduncan@suse.com>
Reviewed-by: David Bond <dbond@suse.com>
---
 drivers/target/iscsi/iscsi_target.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index f246e5015868..c82dc2cd08b3 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -1039,9 +1039,10 @@ int iscsit_setup_scsi_cmd(struct iscsit_conn *conn, struct iscsit_cmd *cmd,
 		hdr->flags &= ~ISCSI_FLAG_CMD_READ;
 		hdr->flags &= ~ISCSI_FLAG_CMD_WRITE;
 
-		pr_warn("ISCSI_FLAG_CMD_READ or ISCSI_FLAG_CMD_WRITE"
+		pr_debug("ISCSI_FLAG_CMD_READ or ISCSI_FLAG_CMD_WRITE"
 			" set when Expected Data Transfer Length is 0 for"
-			" CDB: 0x%02x, Fixing up flags\n", hdr->cdb[0]);
+			" CDB: 0x%02x, cleared READ/WRITE flag(s)\n",
+			hdr->cdb[0]);
 	}
 
 	if (!(hdr->flags & ISCSI_FLAG_CMD_READ) &&
-- 
2.43.0

Re: [PATCH 2/2] scsi: target: iscsi: don't warn of R/W when no data

[Lee Duncan]

I didn't see any objections to this patch.

Are there any issues?

On Sat, Dec 2, 2023 at 10:45 AM lduncan@suse.com <lduncan@suse.com> wrote:
>
> From: Lee Duncan <lduncan@suse.com>
>
> The LIO target code has a warning about setting the
> read and/or write header bits with a PDU that has zero
> transfer length, even though the code mentions that the
> SPEC (RFC 3720) allows this, and that some initiators
> set these bits. But in practice such initiators end up
> flooding the logs with thousands of warning messages for
> IO that is allowed.
>
> So change this to a debug message, and clean up the wording
> just a little bit while at it.
>
> Signed-off-by: Lee Duncan <lduncan@suse.com>
> Reviewed-by: David Bond <dbond@suse.com>
> ---
>  drivers/target/iscsi/iscsi_target.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
> index f246e5015868..c82dc2cd08b3 100644
> --- a/drivers/target/iscsi/iscsi_target.c
> +++ b/drivers/target/iscsi/iscsi_target.c
> @@ -1039,9 +1039,10 @@ int iscsit_setup_scsi_cmd(struct iscsit_conn *conn, struct iscsit_cmd *cmd,
>                 hdr->flags &= ~ISCSI_FLAG_CMD_READ;
>                 hdr->flags &= ~ISCSI_FLAG_CMD_WRITE;
>
> -               pr_warn("ISCSI_FLAG_CMD_READ or ISCSI_FLAG_CMD_WRITE"
> +               pr_debug("ISCSI_FLAG_CMD_READ or ISCSI_FLAG_CMD_WRITE"
>                         " set when Expected Data Transfer Length is 0 for"
> -                       " CDB: 0x%02x, Fixing up flags\n", hdr->cdb[0]);
> +                       " CDB: 0x%02x, cleared READ/WRITE flag(s)\n",
> +                       hdr->cdb[0]);
>         }
>
>         if (!(hdr->flags & ISCSI_FLAG_CMD_READ) &&
> --
> 2.43.0
>

Re: [PATCH 2/2] scsi: target: iscsi: don't warn of R/W when no data

[michael.christie]

On 12/7/23 11:42 AM, lduncan@suse.com wrote:
> From: Lee Duncan <lduncan@suse.com>
> 
> The LIO target code has a warning about setting the
> read and/or write header bits with a PDU that has zero
> transfer length, even though the code mentions that the
> SPEC (RFC 3720) allows this, and that some initiators
> set these bits. But in practice such initiators end up
> flooding the logs with thousands of warning messages for
> IO that is allowed.
> 

I've never seen us hit this. What initiator is doing this and what is 
the command they are sending?

Is it also related to the first patch? Is some initiator sending 
something like a TUR with the immediate bit set during some sort of 
stall/timeout?

Re: [PATCH 0/2] scsi: target: iscsi: Fix two protocol issues

[Lee Duncan]

Ping.

I believe I've addressed Chris' issues. Chris?

On Sat, Dec 2, 2023 at 10:45 AM lduncan@suse.com <lduncan@suse.com> wrote:
>
> From: Lee Duncan <lduncan@suse.com>
>
> Recent boot testing using the fastlinq qedi iSCSI Converged Network
> Adapter and an LIO target uncovered a couple of issues with the
> kernel iSCSI target driver. The first patch addresses an issue
> with the handling of iSCSI "immediate commands", which are allowed
> but not common, and the second patch lowers the noise level of the
> driver when initiators send PDUs with the read and/or write bits
> set but no data, which is also allowed. (See RFC 3720)
>
> Lee Duncan (2):
>   scsi: target: iscsi: handle SCSI immediate commands
>   scsi: target: iscsi: don't warn of R/W when no data
>
>  drivers/target/iscsi/iscsi_target.c      | 17 ++++++-----------
>  drivers/target/iscsi/iscsi_target_util.c | 10 ++++++++--
>  2 files changed, 14 insertions(+), 13 deletions(-)
>
> --
> 2.43.0
>