On Tue, 2014-03-11 at 16:35 +0530, Arshad Hussain wrote:
> 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.
<SNIP>
> > 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");
A few comments on this patch.
First, kernel coding style dictates that there are no spaces between the
parentheses in the conditional statement. Also, please use TABs and not
spaces.
Second, it's line wrapped. Please have a look at the following for
getting your email client to not do this.
https://www.kernel.org/doc/Documentation/email-clients.txt
Lastly, the patch does not contain a changelog or your Signed-off-by
line. I'd highly recommend using git-format-patch for generating
patches.
Please see:
https://www.kernel.org/doc/Documentation/SubmittingPatches
Thanks,
--nab
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
