Re: PROBLEM: LIO Target Ignores NopOUT->flags field.

Arshad Hussain <arshad.hussain@xxxxxxxxxxxxxx> · Tue, 11 Mar 2014 16:35:48 +0530

Hi Nab,
On 3/11/2014 2:36 AM, Nicholas A. Bellinger wrote:
Hi Arshad,

On Thu, 2014-03-06 at 15:42 +0530, Arshad Hussain wrote:
Hi,

I am looking for help in understanding how LIO processes/handles NopOUT.
The problem which I noticed is that LIO Ignores NopOut->flags field
while processing nop out.
Looking at RFC Section 10.18 the NopOUT->flags LMB (Left most bit)
Must be set which LIO is ignoring.

10.18.  NOP-Out

     Byte/     0       |       1       |       2       |       3 |
        /              |               |               | |
       |0 1 2 3 4 5 6 7|0 1 2 3 4 5 6 7|0 1 2 3 4 5 6 7|0 1 2 3 4 5 6 7|
+---------------+---------------+---------------+---------------+
      0|.|I| 0x00      |1| Reserved |
+---------------+---------------+---------------+---------------+

I understand it is the onus of the Initiator to set set proper flags for
NopOUT,
However if the initiator fails to set it correctly. This flag is
ignored, by LIO
and I understand is a RFC error.

I made changes to the LIO source to confirm my findings. Hope you will find
my experiments useful.

Thanks for the feedback.  I'd be happy to take a patch to add the
explicit check for the F_BIT here..

Comments below..

Thanks
Arshad

--- ./iscsi_target.c    2014-03-06 02:26:26.830460677 -0500
+++ ./drivers/target/iscsi/iscsi_target.c       2014-03-06
04:04:07.074341573 -0500
@@ -1568,10 +1568,22 @@
   {
          struct iscsi_cmd *cmd_p = NULL;
          int cmdsn_ret = 0;
+       pr_err("#D1 - Testing if hitting the correct function.\n");
+       pr_err("#D2 - opcode = [%x]\n",hdr->opcode);
+       pr_err("#D3 - flags  = [%x]\n",hdr->flags);
+       pr_err("#D3 - TTT  = [%x]\n",hdr->ttt);
          /*
           * Initiator is expecting a NopIN ping reply..
           */

+       /*
+        * RFC Section 10.18 Mandates NopOUT Flag's LMB to be set
+        */
+       if ( !(hdr->flags & 0x80) ) {
+               pr_err("#D4 - NopOUT Flag LMB not set.\n");
+               return -1;
+       }
+
So this check should move into iscsit_setup_nop_out(), and should call
iscsit_reject_cmd() with ISCSI_REASON_PROTOCOL_ERROR before returning.

Also, you'll want to use the ISCSI_FLAG_CMD_FINAL macro instead of
hardcoding '0x80' here.

Care to post a proper patch with these comments addressed..?

--nab

Thanks for the direction. The patch follow below. Let me know if this is 
good to go?
I have also attached the .pcap file and added dmesg output that was 
generated while I was
reproducing the bit check.

--- linux-3.12.6/drivers/target/iscsi/iscsi_target.c    2013-12-20 
10:51:33.000000000 -0500
+++ linux-ali/drivers/target/iscsi/iscsi_target.c       2014-03-11 
06:43:01.241728716 -0400
@@ -1511,6 +1511,13 @@
 {
        u32 payload_length = ntoh24(hdr->dlength);

+       if ( !(hdr->flags & ISCSI_FLAG_CMD_FINAL) ) {
+          pr_err("NopOUT Flag's, Left Most Bit not set, protocol 
error.\n");
+          return iscsit_reject_cmd(cmd, ISCSI_REASON_PROTOCOL_ERROR,
+                                (unsigned char *)hdr);
+       }
+
+
        if (hdr->itt == RESERVED_ITT && !(hdr->opcode & 
ISCSI_OP_IMMEDIATE)) {
           pr_err("NOPOUT ITT is reserved, but Immediate Bit is"
                " not set, protocol error.\n");
[root@wfs linux-source]#

[root@wfs ~]# dmesg
[  287.805258] NopOUT Flag's, Left Most Bit not set, protocol error.
[root@wfs ~]#

Thanks,
Arshad
Attachment:
nopout_flag_test.pcapng

Description: Binary data




