[ipxe-devel] [PATCH] iscsi: Send Pading with Data segment of PDU

[Shyam Iyer]

Hi Michael and List,

We realized that sending the padding bytes to align the data segment to 4byte boundaries as a separate segment causes an issue.

When the full featured login phase starts the BHS segment is sent followed by the data segment. Once the data segment is received EQL targets send login success but are still waiting for the padding bytes as ipxe sends the padding in a separate segment.

Note that since the data segment length field does not include padding bytes the target can't make a decision on if the padding bytes will arrive at all.
So, it bases its decision on receiving the next PDU. If the next PDU is received on a non-aligned boundary it will close the connection.
The reason the padding bytes are never received by the target is because the initiator changes login state on receiving login SUCCESS from the target and starts SCSI initiatialization and since it doesn't allow concurrent tx threads the padding segment is never sent.

The target is not expecting the next set of SCSI initialization TCP segments because it has not yet received the padding bytes and hence closes connection.

Older EQL firmware would respond with login SUCCESS a little slower and that allowed the initiator to send the padding bytes in a new TCP segment before the LOGIN SUCCESS response was sent by the target and thus avoid this situation. Newer EQL firmwares respond quickly with login SUCCESS before the padding bytes are received and hence this issue.

Looking further  seems like this could be an issue for all PDUs as padding should be done such that each segment is 4byte aligned and sending them separately when we don't allow concurrent tx threads may become a problem in other situations.

rfc 3720 Section 10.2.  PDU Template, Header, and Opcodes states-
“

   All PDU segments and digests are padded to the closest integer number

   of four byte words.  For example, all PDU segments and digests start

   at a four byte word boundary and the padding ranges from 0 to 3

   bytes.  The padding bytes SHOULD be sent as 0.

"
The following changes were done in the fix.
1) Removed fns  iscsi_tx_data_padding and the corresponding data_padding state machine in fn iscsi_tx_step.
2) Added padding bytes before performing tx of the data segment.
3) Removed ISCSI_TX_DATA_PADDING enum.

Thanks,
Shyam Iyer
---
 src/include/ipxe/iscsi.h |    2 -
 src/net/tcp/iscsi.c      |   55 ++++++++++++++++++++--------------------------
 2 files changed, 24 insertions(+), 33 deletions(-)

diff --git a/src/include/ipxe/iscsi.h b/src/include/ipxe/iscsi.h
index 5d3d73b..b4de793 100644
--- a/src/include/ipxe/iscsi.h
+++ b/src/include/ipxe/iscsi.h
@@ -515,8 +515,6 @@ enum iscsi_tx_state {
 	ISCSI_TX_AHS,
 	/** Sending the data segment */
 	ISCSI_TX_DATA,
-	/** Sending the data segment padding */
-	ISCSI_TX_DATA_PADDING,
 };
 
 /** State of an iSCSI RX engine */
diff --git a/src/net/tcp/iscsi.c b/src/net/tcp/iscsi.c
index fa1bb39..1ae4da7 100644
--- a/src/net/tcp/iscsi.c
+++ b/src/net/tcp/iscsi.c
@@ -568,23 +568,32 @@ static void iscsi_data_out_done ( struct iscsi_session *iscsi ) {
 static int iscsi_tx_data_out ( struct iscsi_session *iscsi ) {
 	struct iscsi_bhs_data_out *data_out = &iscsi->tx_bhs.data_out;
 	struct io_buffer *iobuf;
+	static const char pad[] = { '\0', '\0', '\0' };
 	unsigned long offset;
-	size_t len;
+	size_t len, pad_len;
 
 	offset = ntohl ( data_out->offset );
 	len = ISCSI_DATA_LEN ( data_out->lengths );
+	pad_len = ISCSI_DATA_PAD_LEN ( data_out->lengths );
 
 	assert ( iscsi->command != NULL );
 	assert ( iscsi->command->data_out );
 	assert ( ( offset + len ) <= iscsi->command->data_out_len );
 
-	iobuf = xfer_alloc_iob ( &iscsi->socket, len );
+	iobuf = xfer_alloc_iob ( &iscsi->socket, len + pad_len );
 	if ( ! iobuf )
 		return -ENOMEM;
 	
 	copy_from_user ( iob_put ( iobuf, len ),
 			 iscsi->command->data_out, offset, len );
 
+	/*Send padding bytes along*/
+	if ( pad_len ) {
+		memcpy( iob_put( iobuf, pad_len ), pad, pad_len );
+		DBGC ( iscsi, "iSCSI %p Adding padding %d bytes \n",
+		       iscsi, pad_len );
+	}
+
 	return xfer_deliver_iob ( &iscsi->socket, iobuf );
 }
 
@@ -799,15 +808,25 @@ static void iscsi_login_request_done ( struct iscsi_session *iscsi ) {
  */
 static int iscsi_tx_login_request ( struct iscsi_session *iscsi ) {
 	struct iscsi_bhs_login_request *request = &iscsi->tx_bhs.login_request;
+	static const char pad[] = { '\0', '\0', '\0' };
 	struct io_buffer *iobuf;
-	size_t len;
+	size_t len, pad_len;
 
 	len = ISCSI_DATA_LEN ( request->lengths );
-	iobuf = xfer_alloc_iob ( &iscsi->socket, len );
+	pad_len = ISCSI_DATA_PAD_LEN ( request->lengths );
+	iobuf = xfer_alloc_iob ( &iscsi->socket, len + pad_len );
 	if ( ! iobuf )
 		return -ENOMEM;
 	iob_put ( iobuf, len );
 	iscsi_build_login_request_strings ( iscsi, iobuf->data, len );
+	
+	/*Send padding bytes along*/
+	if ( pad_len ) {
+		memcpy( iob_put( iobuf, pad_len ), pad, pad_len );
+		DBGC ( iscsi, "iSCSI %p Adding padding %d bytes \n",
+		       iscsi, pad_len );
+	}
+
 	return xfer_deliver_iob ( &iscsi->socket, iobuf );
 }
 
@@ -1416,27 +1435,6 @@ static int iscsi_tx_data ( struct iscsi_session *iscsi ) {
 }
 
 /**
- * Transmit data padding of an iSCSI PDU
- *
- * @v iscsi		iSCSI session
- * @ret rc		Return status code
- * 
- * Handle transmission of any data padding in a PDU data segment.
- * iscsi::tx_bhs will be valid when this is called.
- */
-static int iscsi_tx_data_padding ( struct iscsi_session *iscsi ) {
-	static const char pad[] = { '\0', '\0', '\0' };
-	struct iscsi_bhs_common *common = &iscsi->tx_bhs.common;
-	size_t pad_len;
-	
-	pad_len = ISCSI_DATA_PAD_LEN ( common->lengths );
-	if ( ! pad_len )
-		return 0;
-
-	return xfer_deliver_raw ( &iscsi->socket, pad, pad_len );
-}
-
-/**
  * Complete iSCSI PDU transmission
  *
  * @v iscsi		iSCSI session
@@ -1494,12 +1492,7 @@ static void iscsi_tx_step ( struct iscsi_session *iscsi ) {
 		case ISCSI_TX_DATA:
 			tx = iscsi_tx_data;
 			tx_len = ISCSI_DATA_LEN ( common->lengths );
-			next_state = ISCSI_TX_DATA_PADDING;
-			break;
-		case ISCSI_TX_DATA_PADDING:
-			tx = iscsi_tx_data_padding;
-			tx_len = ISCSI_DATA_PAD_LEN ( common->lengths );
-			next_state = ISCSI_TX_IDLE;
+			next_state= ISCSI_TX_IDLE;
 			break;
 		case ISCSI_TX_IDLE:
 			/* Nothing to do; pause processing */
-- 
1.7.6.5