From 9ccf32d64e68566947793428fbcc15d2ae4ca470 Mon Sep 17 00:00:00 2001 From: Shuhei Matsumoto Date: Wed, 27 Nov 2019 02:03:32 -0500 Subject: [PATCH] lib/iscsi: Fix double free of primary task when write I/O is split When a iSCSI write is large and split, if LUN is removed between creating and submitting the last subtask, spdk_clear_all_transfer_task() completes the primary task and then process_non_read_task_completion() tries to complete the primary task. This is the double free case, and the later have to be skipped. We add a flag is_r2t_active to struct spdk_iscsi_task and use it to check the duplication. We may be able to use primary's initiator task tag (ITT) instead but we can not rely on ITT because it is set by the initiator. We clear is_r2t_active even when primary is removed from conn->queued_r2t_tasks but it will be no harm. Fixes the issue #1064. Signed-off-by: Shuhei Matsumoto Change-Id: Ia6511bd7adaa8fcb9a07bc40d498e8ee0b7a7ccf Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/475044 Tested-by: SPDK CI Jenkins Reviewed-by: Changpeng Liu Reviewed-by: Jim Harris --- lib/iscsi/conn.c | 22 ++++++++++++++++------ lib/iscsi/iscsi.c | 2 ++ lib/iscsi/task.h | 1 + test/unit/lib/iscsi/conn.c/conn_ut.c | 20 ++++++++++++++++++++ 4 files changed, 39 insertions(+), 6 deletions(-) diff --git a/lib/iscsi/conn.c b/lib/iscsi/conn.c index 3732537e6..a0833eb0a 100644 --- a/lib/iscsi/conn.c +++ b/lib/iscsi/conn.c @@ -1090,13 +1090,23 @@ process_non_read_task_completion(struct spdk_iscsi_conn *conn, * the overall transfer length. */ if (task != primary || task->scsi.length != task->scsi.transfer_len) { - spdk_del_transfer_task(conn, primary->tag); - if (primary->rsp_scsi_status != SPDK_SCSI_STATUS_GOOD) { - iscsi_task_copy_from_rsp_scsi_status(&primary->scsi, primary); + /* If LUN is removed in the middle of the iSCSI write sequence, + * primary might complete the write to the initiator because it is not + * ensured that the initiator will send all data requested by R2Ts. + * + * We check it and skip the following if primary is completed. (see + * spdk_clear_all_transfer_task() in iscsi.c.) + */ + if (primary->is_r2t_active) { + spdk_del_transfer_task(conn, primary->tag); + if (primary->rsp_scsi_status != SPDK_SCSI_STATUS_GOOD) { + iscsi_task_copy_from_rsp_scsi_status(&primary->scsi, primary); + } + spdk_iscsi_task_response(conn, primary); + TAILQ_REMOVE(&conn->active_r2t_tasks, primary, link); + primary->is_r2t_active = false; + spdk_iscsi_task_put(primary); } - spdk_iscsi_task_response(conn, primary); - TAILQ_REMOVE(&conn->active_r2t_tasks, primary, link); - spdk_iscsi_task_put(primary); } else { spdk_iscsi_task_response(conn, task); } diff --git a/lib/iscsi/iscsi.c b/lib/iscsi/iscsi.c index dcf65dc5c..0a6d7dea4 100644 --- a/lib/iscsi/iscsi.c +++ b/lib/iscsi/iscsi.c @@ -2772,6 +2772,7 @@ add_transfer_task(struct spdk_iscsi_conn *conn, struct spdk_iscsi_task *task) } TAILQ_INSERT_TAIL(&conn->active_r2t_tasks, task, link); + task->is_r2t_active = true; return 0; } @@ -2837,6 +2838,7 @@ del_connection_queued_task(struct spdk_iscsi_conn *conn, void *tailq, if ((lun == NULL || lun == task->scsi.lun) && (pdu == NULL || SN32_LT(pdu_tmp->cmd_sn, pdu->cmd_sn))) { TAILQ_REMOVE(head, task, link); + task->is_r2t_active = false; if (lun != NULL && spdk_scsi_lun_is_removing(lun)) { spdk_scsi_task_process_null_lun(&task->scsi); spdk_iscsi_task_response(conn, task); diff --git a/lib/iscsi/task.h b/lib/iscsi/task.h index d692b9ff8..1dfc5239a 100644 --- a/lib/iscsi/task.h +++ b/lib/iscsi/task.h @@ -88,6 +88,7 @@ struct spdk_iscsi_task { uint32_t datain_datasn; uint32_t acked_data_sn; /* next expected datain datasn */ uint32_t ttt; + bool is_r2t_active; uint32_t tag; diff --git a/test/unit/lib/iscsi/conn.c/conn_ut.c b/test/unit/lib/iscsi/conn.c/conn_ut.c index 774067217..2dc3e68bd 100644 --- a/test/unit/lib/iscsi/conn.c/conn_ut.c +++ b/test/unit/lib/iscsi/conn.c/conn_ut.c @@ -384,6 +384,7 @@ process_non_read_task_completion_test(void) primary.rsp_scsi_status = SPDK_SCSI_STATUS_GOOD; primary.scsi.ref = 1; TAILQ_INSERT_TAIL(&conn.active_r2t_tasks, &primary, link); + primary.is_r2t_active = true; /* First subtask which failed. */ task.scsi.length = 4096; @@ -442,6 +443,7 @@ process_non_read_task_completion_test(void) primary.rsp_scsi_status = SPDK_SCSI_STATUS_GOOD; primary.scsi.ref = 2; TAILQ_INSERT_TAIL(&conn.active_r2t_tasks, &primary, link); + primary.is_r2t_active = true; process_non_read_task_completion(&conn, &primary, &primary); CU_ASSERT(TAILQ_EMPTY(&conn.active_r2t_tasks)); @@ -449,6 +451,24 @@ process_non_read_task_completion_test(void) CU_ASSERT(primary.scsi.data_transferred == 4096 * 2); CU_ASSERT(primary.rsp_scsi_status == SPDK_SCSI_STATUS_GOOD); CU_ASSERT(primary.scsi.ref == 0); + + /* Further tricky case when the last task completed ws the initial task, + * and the R2T was already terminated. + */ + primary.scsi.ref = 1; + primary.scsi.length = 4096; + primary.bytes_completed = 4096 * 2; + primary.scsi.data_transferred = 4096 * 2; + primary.scsi.transfer_len = 4096 * 3; + primary.scsi.status = SPDK_SCSI_STATUS_GOOD; + primary.rsp_scsi_status = SPDK_SCSI_STATUS_GOOD; + primary.is_r2t_active = false; + + process_non_read_task_completion(&conn, &primary, &primary); + CU_ASSERT(primary.bytes_completed == 4096 * 3); + CU_ASSERT(primary.scsi.data_transferred == 4096 * 2); + CU_ASSERT(primary.rsp_scsi_status == SPDK_SCSI_STATUS_GOOD); + CU_ASSERT(primary.scsi.ref == 0); } static void