From 395a8aea99affe90faa95e868fbced89017df06e Mon Sep 17 00:00:00 2001 From: Shuhei Matsumoto Date: Thu, 21 Oct 2021 15:29:34 +0900 Subject: [PATCH] iscsi: Fix the case that incoming data is split between data segment and data digest When data segment size is 64KB and data digest is enabled, if data segment and data digest are split into different two packets, - pdu->mobj[0] became full first when reading data semgment, - pdu->mobj[1] was allocated but unused and data digest was read. In this case, two SCSI write tasks were submitted by mistake and the second SCSI write task had no data. Fix the bug in this patch. When iscsi_pdu_payload_read() is called and pdu->mobj[0] is full, allocate pdu->mobj[1] only if any of data segment remains to read. Signed-off-by: Shuhei Matsumoto Change-Id: I9a0c36c05f90092c3c2122a7eb91e10976830b40 Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/9965 Community-CI: Broadcom CI Community-CI: Mellanox Build Bot Tested-by: SPDK CI Jenkins Reviewed-by: Jim Harris Reviewed-by: Ben Walker --- lib/iscsi/iscsi.c | 7 ++++--- test/unit/lib/iscsi/common.c | 13 ++++++++++++ test/unit/lib/iscsi/iscsi.c/iscsi_ut.c | 29 ++++++++++++++++++++++++++ 3 files changed, 46 insertions(+), 3 deletions(-) diff --git a/lib/iscsi/iscsi.c b/lib/iscsi/iscsi.c index afecab9e6..a6e8f49ac 100644 --- a/lib/iscsi/iscsi.c +++ b/lib/iscsi/iscsi.c @@ -4622,6 +4622,7 @@ iscsi_pdu_payload_read(struct spdk_iscsi_conn *conn, struct spdk_iscsi_pdu *pdu) int rc; data_len = pdu->data_segment_len; + read_len = data_len - pdu->data_valid_bytes; mobj = pdu->mobj[0]; if (mobj == NULL) { @@ -4643,7 +4644,7 @@ iscsi_pdu_payload_read(struct spdk_iscsi_conn *conn, struct spdk_iscsi_pdu *pdu) pdu->mobj[0] = mobj; pdu->data = mobj->buf; pdu->data_from_mempool = true; - } else if (mobj->data_len == SPDK_ISCSI_MAX_RECV_DATA_SEGMENT_LENGTH) { + } else if (mobj->data_len == SPDK_ISCSI_MAX_RECV_DATA_SEGMENT_LENGTH && read_len > 0) { mobj = pdu->mobj[1]; if (mobj == NULL) { /* The first data buffer just ran out. Allocate the second data buffer and @@ -4667,8 +4668,8 @@ iscsi_pdu_payload_read(struct spdk_iscsi_conn *conn, struct spdk_iscsi_pdu *pdu) } /* copy the actual data into local buffer */ - read_len = spdk_min(data_len - pdu->data_valid_bytes, - SPDK_ISCSI_MAX_RECV_DATA_SEGMENT_LENGTH - mobj->data_len); + read_len = spdk_min(read_len, SPDK_ISCSI_MAX_RECV_DATA_SEGMENT_LENGTH - mobj->data_len); + if (read_len > 0) { rc = iscsi_conn_read_data_segment(conn, pdu, diff --git a/test/unit/lib/iscsi/common.c b/test/unit/lib/iscsi/common.c index 732f50605..e030d1890 100644 --- a/test/unit/lib/iscsi/common.c +++ b/test/unit/lib/iscsi/common.c @@ -18,6 +18,8 @@ TAILQ_HEAD(, spdk_iscsi_pdu) g_write_pdu_list = TAILQ_HEAD_INITIALIZER(g_write_p static bool g_task_pool_is_empty = false; static bool g_pdu_pool_is_empty = false; static uint32_t g_conn_read_len; +static bool g_conn_read_data_digest = false; +static uint32_t g_data_digest; struct spdk_iscsi_task * iscsi_task_get(struct spdk_iscsi_conn *conn, @@ -181,12 +183,23 @@ iscsi_task_cpl(struct spdk_scsi_task *scsi_task) DEFINE_STUB_V(iscsi_task_mgmt_cpl, (struct spdk_scsi_task *scsi_task)); +#define MAKE_DIGEST_WORD(BUF, CRC32C) \ + ( ((*((uint8_t *)(BUF)+0)) = (uint8_t)((uint32_t)(CRC32C) >> 0)), \ + ((*((uint8_t *)(BUF)+1)) = (uint8_t)((uint32_t)(CRC32C) >> 8)), \ + ((*((uint8_t *)(BUF)+2)) = (uint8_t)((uint32_t)(CRC32C) >> 16)), \ + ((*((uint8_t *)(BUF)+3)) = (uint8_t)((uint32_t)(CRC32C) >> 24))) + int iscsi_conn_read_data(struct spdk_iscsi_conn *conn, int bytes, void *buf) { uint32_t *data = buf; int i; + if (g_conn_read_data_digest) { + MAKE_DIGEST_WORD(buf, g_data_digest); + return ISCSI_DIGEST_LEN; + } + /* Limit the length to 4 bytes multiples. */ SPDK_CU_ASSERT_FATAL((bytes % 4) == 0); diff --git a/test/unit/lib/iscsi/iscsi.c/iscsi_ut.c b/test/unit/lib/iscsi/iscsi.c/iscsi_ut.c index 2992a0975..558540720 100644 --- a/test/unit/lib/iscsi/iscsi.c/iscsi_ut.c +++ b/test/unit/lib/iscsi/iscsi.c/iscsi_ut.c @@ -2130,6 +2130,35 @@ pdu_payload_read_test(void) rc = iscsi_pdu_payload_read(&conn, &pdu); check_pdu_payload_read(&pdu, &mobj2, rc, 1, SPDK_ISCSI_MAX_RECV_DATA_SEGMENT_LENGTH); + /* Case 5: data segment size is SPDK_ISCSI_MAX_RECV_DATA_SEGMENT_LENGTH, data digest + * is enabled, and reading PDU data is split between data segment and data digest. + */ + conn.data_digest = true; + memset(&pdu, 0, sizeof(pdu)); + pdu.crc32c = SPDK_CRC32C_INITIAL; + pdu.data = mobj1.buf; + pdu.data_segment_len = SPDK_ISCSI_MAX_RECV_DATA_SEGMENT_LENGTH; + pdu.mobj[0] = &mobj1; + pdu.data_valid_bytes = SPDK_ISCSI_MAX_RECV_DATA_SEGMENT_LENGTH; + mobj1.data_len = SPDK_ISCSI_MAX_RECV_DATA_SEGMENT_LENGTH; + + /* generate data digest. */ + g_data_digest = spdk_crc32c_update(mobj1.buf, SPDK_ISCSI_MAX_RECV_DATA_SEGMENT_LENGTH, + SPDK_CRC32C_INITIAL); + g_data_digest ^= SPDK_CRC32C_XOR; + g_conn_read_data_digest = true; + + rc = iscsi_pdu_payload_read(&conn, &pdu); + CU_ASSERT(rc == 0); + CU_ASSERT(pdu.ddigest_valid_bytes == ISCSI_DIGEST_LEN); + CU_ASSERT(pdu.mobj[1] == NULL); + + g_conn_read_data_digest = false; + g_conn_read_len = 0; + MOCK_SET(spdk_mempool_get, &mobj1); + mobj1.data_len = 0; + + g_conn_read_len = 0; MOCK_CLEAR(spdk_mempool_get); free(mobj1.buf);