ivampiresp/Spdk
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

lib/iscsi: Fix double free of primary task when write I/O is split

Browse Source 
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 <shuhei.matsumoto.xt@hitachi.com>
Change-Id: Ia6511bd7adaa8fcb9a07bc40d498e8ee0b7a7ccf
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/475044
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Changpeng Liu <changpeng.liu@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
This commit is contained in:
Shuhei Matsumoto 2019-11-27 02:03:32 -05:00 committed by Tomasz Zawadzki
parent 8238265548
commit 9ccf32d64e
4 changed files with 39 additions and 6 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

22
lib/iscsi/conn.c
View File

@ -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);
}

2
lib/iscsi/iscsi.c
View File

@ -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);

1
lib/iscsi/task.h
View File

@ -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;

20
test/unit/lib/iscsi/conn.c/conn_ut.c
View File

@ -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