[PATCH] MaxRecvDataSegmentLength used incorrectly
Tony Battersby
According to the iSCSI spec, both the target and the initiator should
declare their own values for MaxRecvDataSegmentLength, and the two values
should coexist independently rather than being combined with a "result
function". A PDU sender should respect the receiver's
MaxRecvDataSegmentLength value, but the values do not need to be the same
for both directions. For example, if the initiator declares a
MaxRecvDataSegmentLength of 131072 and the target declares a
MaxRecvDataSegmentLength of 65536, the target may send a PDU to the
initiator with a data segment length of 131072 even though it exceeds the
target's declared value of 65536. Currently core-iscsi-v1.6.2.2 will flag
a protocol error in this case because it checks incoming PDUs against
CONN_OPS(conn)->MaxRecvDataSegmentLength, which is the value declared by
the target rather than the value declared by the initiator. To fix this
problem, I recommend simply making a #define for the initiator's
declared MaxRecvDataSegmentLength and checking incoming PDUs against
that value rather than CONN_OPS(conn)->MaxRecvDataSegmentLength, but
still using CONN_OPS(conn)->MaxRecvDataSegmentLength for limiting the
length of outgoing PDUs. A patch that does this is attached below.
Anthony J. Battersby
Cybernetics
diff -urNp core-iscsi-v1.6.2.2-orig/iscsi_initiator.c core-iscsi-v1.6.2.2/iscsi_initiator.c
--- core-iscsi-v1.6.2.2-orig/iscsi_initiator.c 2005-12-09 17:50:29.000000000 -0500
+++ core-iscsi-v1.6.2.2/iscsi_initiator.c 2005-12-27 11:23:04.000000000 -0500
@@ -1160,10 +1160,10 @@ static inline int iscsi_handle_data_in (
return(-1);
}
- if (hdr->length > CONN_OPS(conn)->MaxRecvDataSegmentLength) {
+ if (hdr->length > MY_MAX_RECV_DATA_SEGMENT_LENGTH) {
TRACE_ERROR("DataIN payload size %u is greater than"
" MaxRecvDataSegmentLength %u, protocol error.\n",
- hdr->length, CONN_OPS(conn)->MaxRecvDataSegmentLength);
+ hdr->length, MY_MAX_RECV_DATA_SEGMENT_LENGTH);
return(-1);
}
@@ -1537,11 +1537,11 @@ static inline int iscsi_handle_nop_in (
iscsi_stop_nopout_response_timer(conn);
- if (hdr->length > CONN_OPS(conn)->MaxRecvDataSegmentLength) {
+ if (hdr->length > MY_MAX_RECV_DATA_SEGMENT_LENGTH) {
TRACE_ERROR("Ping data payload size %u is greater"
" than MaxRecvDataSegmentLength %u,"
" protocol error.\n", hdr->length,
- CONN_OPS(conn)->MaxRecvDataSegmentLength);
+ MY_MAX_RECV_DATA_SEGMENT_LENGTH);
return(-1);
}
@@ -2261,10 +2261,10 @@ static inline int iscsi_handle_async_msg
hdr->parameter2 = be16_to_cpu(hdr->parameter2);
hdr->parameter3 = be16_to_cpu(hdr->parameter3);
- if (hdr->length > CONN_OPS(conn)->MaxRecvDataSegmentLength) {
+ if (hdr->length > MY_MAX_RECV_DATA_SEGMENT_LENGTH) {
TRACE_ERROR("Reject payload size %u is greater than"
" MaxRecvDataSegmentLength %u, protocol error.\n",
- hdr->length, CONN_OPS(conn)->MaxRecvDataSegmentLength);
+ hdr->length, MY_MAX_RECV_DATA_SEGMENT_LENGTH);
return(-1);
}
@@ -2360,10 +2360,10 @@ static inline int iscsi_handle_rjt (
print_reject_reason(hdr->reason);
- if (hdr->length > CONN_OPS(conn)->MaxRecvDataSegmentLength) {
+ if (hdr->length > MY_MAX_RECV_DATA_SEGMENT_LENGTH) {
TRACE_ERROR("Reject payload size %u is greater than"
" MaxRecvDataSegmentLength %u, protocol error.\n",
- hdr->length, CONN_OPS(conn)->MaxRecvDataSegmentLength);
+ hdr->length, MY_MAX_RECV_DATA_SEGMENT_LENGTH);
return(-1);
}
@@ -2448,10 +2448,10 @@ static inline int iscsi_handle_rsp (
hdr->exp_data_sn = be32_to_cpu(hdr->exp_data_sn);
hdr->bidi_res_count = be32_to_cpu(hdr->bidi_res_count);
- if (hdr->length > CONN_OPS(conn)->MaxRecvDataSegmentLength) {
+ if (hdr->length > MY_MAX_RECV_DATA_SEGMENT_LENGTH) {
TRACE_ERROR("Response payload size %u is greater than"
" MaxRecvDataSegmentLength %u, protocol error.\n",
- hdr->length, CONN_OPS(conn)->MaxRecvDataSegmentLength);
+ hdr->length, MY_MAX_RECV_DATA_SEGMENT_LENGTH);
return(-1);
}
@@ -2578,10 +2578,10 @@ static inline int iscsi_handle_rsp (
hdr->exp_cmd_sn = be32_to_cpu(hdr->exp_cmd_sn);
hdr->max_cmd_sn = be32_to_cpu(hdr->max_cmd_sn);
- if (hdr->length > CONN_OPS(conn)->MaxRecvDataSegmentLength) {
+ if (hdr->length > MY_MAX_RECV_DATA_SEGMENT_LENGTH) {
TRACE_ERROR("Response payload size %u is greater than"
" MaxRecvDataSegmentLength %u, protocol error.\n",
- hdr->length, CONN_OPS(conn)->MaxRecvDataSegmentLength);
+ hdr->length, MY_MAX_RECV_DATA_SEGMENT_LENGTH);
return(-1);
}
diff -urNp core-iscsi-v1.6.2.2-orig/iscsi_initiator_channel.c core-iscsi-v1.6.2.2/iscsi_initiator_channel.c
--- core-iscsi-v1.6.2.2-orig/iscsi_initiator_channel.c 2005-11-29 17:00:18.000000000 -0500
+++ core-iscsi-v1.6.2.2/iscsi_initiator_channel.c 2005-12-27 11:27:19.000000000 -0500
@@ -466,7 +466,7 @@ static unsigned char *full_params_array[
"HeaderDigest=CRC32C,None",
"DataDigest=CRC32C,None",
"MaxConnections=10",
- "MaxRecvDataSegmentLength=131072",
+ "MaxRecvDataSegmentLength=" __stringify(MY_MAX_RECV_DATA_SEGMENT_LENGTH),
"ErrorRecoveryLevel=0",
"IFMarker=Yes",
"OFMarker=Yes",
diff -urNp core-iscsi-v1.6.2.2-orig/iscsi_initiator_parameters.h core-iscsi-v1.6.2.2/iscsi_initiator_parameters.h
--- core-iscsi-v1.6.2.2-orig/iscsi_initiator_parameters.h 2005-11-29 03:15:57.000000000 -0500
+++ core-iscsi-v1.6.2.2/iscsi_initiator_parameters.h 2005-12-27 11:26:46.000000000 -0500
@@ -153,6 +153,11 @@ extern void iscsi_set_session_ops_dump(i
#define NORMAL "Normal"
/*
+ * MaxRecvDataSegmentLength for incoming PDUs.
+ */
+#define MY_MAX_RECV_DATA_SEGMENT_LENGTH 131072
+
+/*
* iscsi_param_t->use
*/
#define USE_LEADING_ONLY 0x01
diff -urNp core-iscsi-v1.6.2.2-orig/iscsi_initiator_seq_and_pdu_list.c core-iscsi-v1.6.2.2/iscsi_initiator_seq_and_pdu_list.c
--- core-iscsi-v1.6.2.2-orig/iscsi_initiator_seq_and_pdu_list.c 2005-11-29 03:05:22.000000000 -0500
+++ core-iscsi-v1.6.2.2/iscsi_initiator_seq_and_pdu_list.c 2005-12-27 11:23:04.000000000 -0500
@@ -21,6 +21,7 @@
#include <iscsi_initiator_debug_opcodes.h>
#include <iscsi_initiator_core.h>
#include <iscsi_initiator_util.h>
+#include <iscsi_initiator_parameters.h>
#undef ISCSI_SEQ_AND_PDU_LIST_C
@@ -254,6 +255,10 @@ static inline void iscsi_determine_count
u32 burstlength = 0, offset = 0;
u32 unsolicited_data_length = 0;
iscsi_conn_t *conn = CONN(cmd);
+ u32 max_recv_data_segment_length =
+ (bl->data_direction & ISCSI_PDU_WRITE) ?
+ CONN_OPS(conn)->MaxRecvDataSegmentLength :
+ MY_MAX_RECV_DATA_SEGMENT_LENGTH;
if ((bl->type == PDULIST_IMMEDIATE) ||
(bl->type == PDULIST_IMMEDIATE_AND_UNSOLICITED))
@@ -277,13 +282,13 @@ static inline void iscsi_determine_count
continue;
}
if (unsolicited_data_length > 0) {
- if ((offset + CONN_OPS(conn)->MaxRecvDataSegmentLength) >=
+ if ((offset + max_recv_data_segment_length) >=
cmd->data_length) {
unsolicited_data_length -= (cmd->data_length - offset);
offset += (cmd->data_length - offset);
continue;
}
- if ((offset + CONN_OPS(conn)->MaxRecvDataSegmentLength) >=
+ if ((offset + max_recv_data_segment_length) >=
SESS_OPS_C(conn)->FirstBurstLength) {
unsolicited_data_length -=
(SESS_OPS_C(conn)->FirstBurstLength - offset);
@@ -293,16 +298,16 @@ static inline void iscsi_determine_count
continue;
}
- offset += CONN_OPS(conn)->MaxRecvDataSegmentLength;
- unsolicited_data_length -= CONN_OPS(conn)->MaxRecvDataSegmentLength;
+ offset += max_recv_data_segment_length;
+ unsolicited_data_length -= max_recv_data_segment_length;
continue;
}
- if ((offset + CONN_OPS(conn)->MaxRecvDataSegmentLength) >=
+ if ((offset + max_recv_data_segment_length) >=
cmd->data_length) {
offset += (cmd->data_length - offset);
continue;
}
- if ((burstlength + CONN_OPS(conn)->MaxRecvDataSegmentLength) >=
+ if ((burstlength + max_recv_data_segment_length) >=
SESS_OPS_C(conn)->MaxBurstLength) {
offset += (SESS_OPS_C(conn)->MaxBurstLength - burstlength);
burstlength = 0;
@@ -310,8 +315,8 @@ static inline void iscsi_determine_count
continue;
}
- burstlength += CONN_OPS(conn)->MaxRecvDataSegmentLength;
- offset += CONN_OPS(conn)->MaxRecvDataSegmentLength;
+ burstlength += max_recv_data_segment_length;
+ offset += max_recv_data_segment_length;
}
return;
@@ -333,6 +338,10 @@ static inline int iscsi_build_pdu_and_se
iscsi_conn_t *conn = CONN(cmd);
iscsi_pdu_t *pdu = cmd->pdu_list;
iscsi_seq_t *seq = cmd->seq_list;
+ u32 max_recv_data_segment_length =
+ (bl->data_direction & ISCSI_PDU_WRITE) ?
+ CONN_OPS(conn)->MaxRecvDataSegmentLength :
+ MY_MAX_RECV_DATA_SEGMENT_LENGTH;
datapduinorder = SESS_OPS_C(conn)->DataPDUInOrder;
datasequenceinorder = SESS_OPS_C(conn)->DataSequenceInOrder;
@@ -379,7 +388,7 @@ static inline int iscsi_build_pdu_and_se
continue;
}
if (unsolicited_data_length > 0) {
- if ((offset + CONN_OPS(conn)->MaxRecvDataSegmentLength) >=
+ if ((offset + max_recv_data_segment_length) >=
cmd->data_length) {
if (!datapduinorder) {
pdu[i].type = PDUTYPE_UNSOLICITED;
@@ -395,7 +404,7 @@ static inline int iscsi_build_pdu_and_se
offset += (cmd->data_length - offset);
continue;
}
- if ((offset + CONN_OPS(conn)->MaxRecvDataSegmentLength) >=
+ if ((offset + max_recv_data_segment_length) >=
SESS_OPS_C(conn)->FirstBurstLength) {
if (!datapduinorder) {
pdu[i].type = PDUTYPE_UNSOLICITED;
@@ -419,14 +428,14 @@ static inline int iscsi_build_pdu_and_se
if (!datapduinorder) {
pdu[i].type = PDUTYPE_UNSOLICITED;
- pdu[i++].length = CONN_OPS(conn)->MaxRecvDataSegmentLength;
+ pdu[i++].length = max_recv_data_segment_length;
}
- burstlength += CONN_OPS(conn)->MaxRecvDataSegmentLength;
- offset += CONN_OPS(conn)->MaxRecvDataSegmentLength;
- unsolicited_data_length -= CONN_OPS(conn)->MaxRecvDataSegmentLength;
+ burstlength += max_recv_data_segment_length;
+ offset += max_recv_data_segment_length;
+ unsolicited_data_length -= max_recv_data_segment_length;
continue;
}
- if ((offset + CONN_OPS(conn)->MaxRecvDataSegmentLength) >=
+ if ((offset + max_recv_data_segment_length) >=
cmd->data_length) {
if (!datapduinorder) {
pdu[i].type = PDUTYPE_NORMAL;
@@ -441,7 +450,7 @@ static inline int iscsi_build_pdu_and_se
offset += (cmd->data_length - offset);
continue;
}
- if ((burstlength + CONN_OPS(conn)->MaxRecvDataSegmentLength) >=
+ if ((burstlength + max_recv_data_segment_length) >=
SESS_OPS_C(conn)->MaxBurstLength) {
if (!datapduinorder) {
pdu[i].type = PDUTYPE_NORMAL;
@@ -463,10 +472,10 @@ static inline int iscsi_build_pdu_and_se
if (!datapduinorder) {
pdu[i].type = PDUTYPE_NORMAL;
- pdu[i++].length = CONN_OPS(conn)->MaxRecvDataSegmentLength;
+ pdu[i++].length = max_recv_data_segment_length;
}
- burstlength += CONN_OPS(conn)->MaxRecvDataSegmentLength;
- offset += CONN_OPS(conn)->MaxRecvDataSegmentLength;
+ burstlength += max_recv_data_segment_length;
+ offset += max_recv_data_segment_length;
}
if (!datasequenceinorder) {
Nicholas A. Bellinger
I have been unable to locate the specific wording within RFC-3720 that
discusses that either side may declare a seperate
MaxRecvDataSegmentLength parameter value. The nearest that I have been
able to determine is the following passage in Section 12.12:
The initiator or target declares the maximum data segment length in
bytes it can receive in an iSCSI PDU.
The transmitter (initiator or target) is required to send PDUs with a
data segment that does not exceed MaxRecvDataSegmentLength of the
receiver.
My initial reasoning against the patch is a result of drafts that
directly apply to RFC-3720 (iSCSI Extentions for RDMA for example) that
define a InitiatorRecvDataSegmentLength and TargetRecvDataSegmentLength
key that explictly define node specific PDU lengths. For the moment I
am going to hold off, I would like to see this discussion posted to the
IPS TWG, as I think the Julian and Co. can provide us with a final
comment on this issue.
Thanks again for your invaluable input,
--
Nicholas A. Bellinger <n...@kernel.org>
Nicholas A. Bellinger
A quick follow up on this, I would like to see a perminent solution for
this (depending upon the outcome of the discussion :-) other than a CPP
define. This would be something along the lines of the Initiator
defined value stored in a per iscsi_conn_t structure member determined
from /etc/sysconfig/initiator, with the target side value returned to
the initiator stored in CONN_OPS(conn)->MaxRecvDataSegmentLength.
Thanks for your input!
--
Nicholas A. Bellinger <n...@kernel.org>
On Tue, 2005-12-27 at 12:16 -0500, Tony Battersby wrote:
Tony
> I have been unable to locate the specific wording within RFC-3720 that
> discusses that either side may declare a seperate
> MaxRecvDataSegmentLength parameter value.
I can't point to specific text in the RFC that says it explicitly.
However, the following two things in section 12.12 imply it:
1) MaxRecvDataSegmentLength is marked "Declarative", which means that
it does not require a response.
2) There is no "result function" specified to combine the two values
into one.
Compare to section 12.13 MaxBurstLength, which is not marked
Declarative and the text says that the "Result function is Minimum."
> My initial reasoning against the patch is a result of drafts that
> directly apply to RFC-3720 (iSCSI Extentions for RDMA for example) that
> define a InitiatorRecvDataSegmentLength and TargetRecvDataSegmentLength
> key that explictly define node specific PDU lengths.
I am not familiar with these, so I cannot comment.
Anthony J. Battersby
Cybernetics