Commit 22adcd14 authored by Shuhei Matsumoto's avatar Shuhei Matsumoto Committed by Tomasz Zawadzki
Browse files

lib/iscsi: Fix orphaned PDUs when freeing PDUs and tasks for LUN hotplug 



In _iscsi_conn_free_tasks(), we had parsed conn->write_pdu_list
and then parsed conn->queued_datain_tasks.  However when we parsed
conn->write_pdu_list, if there was any task in conn->queued_datain_tasks,
some PDUs were inserted conn->write_pdu_list.  Hence after parsing
conn->write_pdu_list, new PDUs were in conn->write_pdu_list as orphan.
Then orphaned PDUs were freed later but LUN was already freed and
critical failure occurred.

This patch swaps the order of conn->queued_datain_tasks and
conn->write_pdu_list, and add comment to explain the change.

Additionally, this patch adds unit test which fails if it runs
without this fix.

Fixes issue #1030.

Signed-off-by: default avatarShuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Change-Id: Icb0ffbbbac70792a62939dc55a69df05d2ab9128
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/475453


Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: default avatarChangpeng Liu <changpeng.liu@intel.com>
Reviewed-by: default avatarJim Harris <james.r.harris@intel.com>
parent 9ccf32d6

lib/iscsi/conn.c

+19 −13
Original line number Diff line number Diff line
@@ -317,19 +317,6 @@ _iscsi_conn_free_tasks(struct spdk_iscsi_conn *conn, struct spdk_scsi_lun *lun) 
	struct spdk_iscsi_pdu *pdu, *tmp_pdu;

	struct spdk_iscsi_task *iscsi_task, *tmp_iscsi_task;



	TAILQ_FOREACH_SAFE(pdu, &conn->write_pdu_list, tailq, tmp_pdu) {

		/* If connection is exited (no LUN is specified) or the PDU's LUN matches

		 * the LUN that was removed, free this PDU immediately.  If the pdu's LUN

		 * is NULL, then we know the datain handling code already detected the hot

		 * removal, so we can free that PDU as well.

		 */

		if ((lun == NULL) ||

		    (pdu->task && (lun == pdu->task->scsi.lun || NULL == pdu->task->scsi.lun))) {

			TAILQ_REMOVE(&conn->write_pdu_list, pdu, tailq);

			spdk_iscsi_conn_free_pdu(conn, pdu);

		}

	}



	TAILQ_FOREACH_SAFE(pdu, &conn->snack_pdu_list, tailq, tmp_pdu) {

		if (lun == NULL || lun == pdu->task->scsi.lun) {

			TAILQ_REMOVE(&conn->snack_pdu_list, pdu, tailq);
@@ -343,6 +330,25 @@ _iscsi_conn_free_tasks(struct spdk_iscsi_conn *conn, struct spdk_scsi_lun *lun) 
			spdk_iscsi_task_put(iscsi_task);

		}

	}



	/* We have to parse conn->write_pdu_list in the end.  In spdk_iscsi_conn_free_pdu(),

	 *  spdk_iscsi_conn_handle_queued_datain_tasks() may be called, and

	 *  spdk_iscsi_conn_handle_queued_datain_tasks() will parse conn->queued_datain_tasks

	 *  and may stack some PDUs to conn->write_pdu_list.  Hence when we come here, we

	 *  have to ensure there is no associated task in conn->queued_datain_tasks.

	 */

	TAILQ_FOREACH_SAFE(pdu, &conn->write_pdu_list, tailq, tmp_pdu) {

		/* If connection is exited (no LUN is specified) or the PDU's LUN matches

		 * the LUN that was removed, free this PDU immediately.  If the pdu's LUN

		 * is NULL, then we know the datain handling code already detected the hot

		 * removal, so we can free that PDU as well.

		 */

		if ((lun == NULL) ||

		    (pdu->task && (lun == pdu->task->scsi.lun || NULL == pdu->task->scsi.lun))) {

			TAILQ_REMOVE(&conn->write_pdu_list, pdu, tailq);

			spdk_iscsi_conn_free_pdu(conn, pdu);

		}

	}

}



static int

test/unit/lib/iscsi/conn.c/conn_ut.c

+56 −1
Original line number Diff line number Diff line
@@ -170,6 +170,15 @@ DEFINE_STUB(spdk_del_transfer_task, bool, 
int

spdk_iscsi_conn_handle_queued_datain_tasks(struct spdk_iscsi_conn *conn)

{

	struct spdk_iscsi_task *task, *tmp;



	TAILQ_FOREACH_SAFE(task, &conn->queued_datain_tasks, link, tmp) {

		TAILQ_REMOVE(&conn->queued_datain_tasks, task, link);

		if (task->pdu) {

			TAILQ_INSERT_TAIL(&conn->write_pdu_list, task->pdu, tailq);

		}

	}



	return 0;

}
@@ -679,6 +688,51 @@ free_tasks_on_connection(void) 
	CU_ASSERT(task3.scsi.ref == 1);

}



static void

free_tasks_with_queued_datain(void)

{

	struct spdk_iscsi_conn conn = {};

	struct spdk_iscsi_pdu pdu1 = {}, pdu2 = {}, pdu3 = {}, pdu4 = {}, pdu5 = {}, pdu6 = {};

	struct spdk_iscsi_task task1 = {}, task2 = {}, task3 = {}, task4 = {}, task5 = {}, task6 = {};



	TAILQ_INIT(&conn.write_pdu_list);

	TAILQ_INIT(&conn.snack_pdu_list);

	TAILQ_INIT(&conn.queued_datain_tasks);



	pdu1.task = &task1;

	pdu2.task = &task2;

	pdu3.task = &task3;



	task1.scsi.ref = 1;

	task2.scsi.ref = 1;

	task3.scsi.ref = 1;



	pdu3.bhs.opcode = ISCSI_OP_SCSI_DATAIN;

	task3.scsi.offset = 1;

	conn.data_in_cnt = 1;



	TAILQ_INSERT_TAIL(&conn.write_pdu_list, &pdu1, tailq);

	TAILQ_INSERT_TAIL(&conn.write_pdu_list, &pdu2, tailq);

	TAILQ_INSERT_TAIL(&conn.write_pdu_list, &pdu3, tailq);



	task4.scsi.ref = 1;

	task5.scsi.ref = 1;

	task6.scsi.ref = 1;



	task4.pdu = &pdu4;

	task5.pdu = &pdu5;

	task6.pdu = &pdu6;



	TAILQ_INSERT_TAIL(&conn.queued_datain_tasks, &task4, link);

	TAILQ_INSERT_TAIL(&conn.queued_datain_tasks, &task5, link);

	TAILQ_INSERT_TAIL(&conn.queued_datain_tasks, &task6, link);



	_iscsi_conn_free_tasks(&conn, NULL);



	CU_ASSERT(TAILQ_EMPTY(&conn.write_pdu_list));

	CU_ASSERT(TAILQ_EMPTY(&conn.queued_datain_tasks));

}



int

main(int argc, char **argv)

{
@@ -704,7 +758,8 @@ main(int argc, char **argv) 
		CU_add_test(suite, "process_non_read_task_completion_test",

			    process_non_read_task_completion_test) == NULL ||

		CU_add_test(suite, "recursive_flush_pdus_calls", recursive_flush_pdus_calls) == NULL ||

		CU_add_test(suite, "free_tasks_on_connection", free_tasks_on_connection) == NULL

		CU_add_test(suite, "free_tasks_on_connection", free_tasks_on_connection) == NULL ||

		CU_add_test(suite, "free_tasks_with_queued_datain", free_tasks_with_queued_datain) == NULL

	) {

		CU_cleanup_registry();

		return CU_get_error();