Commit 2ce00236 authored by Mike Christie's avatar Mike Christie Committed by Martin K. Petersen
Browse files

scsi: qedi: Fix race during abort timeouts 

If the SCSI cmd completes after qedi_tmf_work calls iscsi_itt_to_task then
the qedi qedi_cmd->task_id could be freed and used for another cmd. If we
then call qedi_iscsi_cleanup_task with that task_id we will be cleaning up
the wrong cmd.

Wait to release the task_id until the last put has been done on the
iscsi_task. Because libiscsi grabs a ref to the task when sending the
abort, we know that for the non-abort timeout case that the task_id we are
referencing is for the cmd that was supposed to be aborted.

A latter commit will fix the case where the abort times out while we are
running qedi_tmf_work.

Link: https://lore.kernel.org/r/20210525181821.7617-21-michael.christie@oracle.com


Reviewed-by: default avatarManish Rangankar <mrangankar@marvell.com>
Signed-off-by: default avatarMike Christie <michael.christie@oracle.com>
Signed-off-by: default avatarMartin K. Petersen <martin.petersen@oracle.com>
parent 5777b7f0

drivers/scsi/qedi/qedi_fw.c

+0 −15
Original line number Diff line number Diff line
@@ -73,7 +73,6 @@ static void qedi_process_logout_resp(struct qedi_ctx *qedi, 
	spin_unlock(&qedi_conn->list_lock);



	cmd->state = RESPONSE_RECEIVED;

	qedi_clear_task_idx(qedi, cmd->task_id);

	__iscsi_complete_pdu(conn, (struct iscsi_hdr *)resp_hdr, NULL, 0);



	spin_unlock(&session->back_lock);
@@ -138,7 +137,6 @@ static void qedi_process_text_resp(struct qedi_ctx *qedi, 
	spin_unlock(&qedi_conn->list_lock);



	cmd->state = RESPONSE_RECEIVED;

	qedi_clear_task_idx(qedi, cmd->task_id);



	__iscsi_complete_pdu(conn, (struct iscsi_hdr *)resp_hdr_ptr,

			     qedi_conn->gen_pdu.resp_buf,
@@ -164,13 +162,11 @@ static void qedi_tmf_resp_work(struct work_struct *work) 
	iscsi_block_session(session->cls_session);

	rval = qedi_cleanup_all_io(qedi, qedi_conn, qedi_cmd->task, true);

	if (rval) {

		qedi_clear_task_idx(qedi, qedi_cmd->task_id);

		iscsi_unblock_session(session->cls_session);

		goto exit_tmf_resp;

	}



	iscsi_unblock_session(session->cls_session);

	qedi_clear_task_idx(qedi, qedi_cmd->task_id);



	spin_lock(&session->back_lock);

	__iscsi_complete_pdu(conn, (struct iscsi_hdr *)resp_hdr_ptr, NULL, 0);
@@ -245,8 +241,6 @@ static void qedi_process_tmf_resp(struct qedi_ctx *qedi, 
		goto unblock_sess;

	}



	qedi_clear_task_idx(qedi, qedi_cmd->task_id);



	__iscsi_complete_pdu(conn, (struct iscsi_hdr *)resp_hdr_ptr, NULL, 0);

	kfree(resp_hdr_ptr);
@@ -314,7 +308,6 @@ static void qedi_process_login_resp(struct qedi_ctx *qedi, 
		  "Freeing tid=0x%x for cid=0x%x\n",

		  cmd->task_id, qedi_conn->iscsi_conn_id);

	cmd->state = RESPONSE_RECEIVED;

	qedi_clear_task_idx(qedi, cmd->task_id);

}



static void qedi_get_rq_bdq_buf(struct qedi_ctx *qedi,
@@ -468,7 +461,6 @@ static int qedi_process_nopin_mesg(struct qedi_ctx *qedi, 
		}



		spin_unlock(&qedi_conn->list_lock);

		qedi_clear_task_idx(qedi, cmd->task_id);

	}



done:
@@ -673,7 +665,6 @@ static void qedi_scsi_completion(struct qedi_ctx *qedi, 
	if (qedi_io_tracing)

		qedi_trace_io(qedi, task, cmd->task_id, QEDI_IO_TRACE_RSP);



	qedi_clear_task_idx(qedi, cmd->task_id);

	__iscsi_complete_pdu(conn, (struct iscsi_hdr *)hdr,

			     conn->data, datalen);

error:
@@ -730,7 +721,6 @@ static void qedi_process_nopin_local_cmpl(struct qedi_ctx *qedi, 
		  cqe->itid, cmd->task_id);



	cmd->state = RESPONSE_RECEIVED;

	qedi_clear_task_idx(qedi, cmd->task_id);



	spin_lock_bh(&session->back_lock);

	__iscsi_put_task(task);
@@ -748,7 +738,6 @@ static void qedi_process_cmd_cleanup_resp(struct qedi_ctx *qedi, 
	itt_t protoitt = 0;

	int found = 0;

	struct qedi_cmd *qedi_cmd = NULL;

	u32 rtid = 0;

	u32 iscsi_cid;

	struct qedi_conn *qedi_conn;

	struct qedi_cmd *dbg_cmd;
@@ -779,7 +768,6 @@ static void qedi_process_cmd_cleanup_resp(struct qedi_ctx *qedi, 
			found = 1;

			mtask = qedi_cmd->task;

			tmf_hdr = (struct iscsi_tm *)mtask->hdr;

			rtid = work->rtid;



			list_del_init(&work->list);

			kfree(work);
@@ -821,8 +809,6 @@ static void qedi_process_cmd_cleanup_resp(struct qedi_ctx *qedi, 
			if (qedi_cmd->state == CLEANUP_WAIT_FAILED)

				qedi_cmd->state = CLEANUP_RECV;



			qedi_clear_task_idx(qedi_conn->qedi, rtid);



			spin_lock(&qedi_conn->list_lock);

			if (likely(dbg_cmd->io_cmd_in_list)) {

				dbg_cmd->io_cmd_in_list = false;
@@ -856,7 +842,6 @@ static void qedi_process_cmd_cleanup_resp(struct qedi_ctx *qedi, 
		QEDI_INFO(&qedi->dbg_ctx, QEDI_LOG_TID,

			  "Freeing tid=0x%x for cid=0x%x\n",

			  cqe->itid, qedi_conn->iscsi_conn_id);

		qedi_clear_task_idx(qedi_conn->qedi, cqe->itid);



	} else {

		qedi_get_proto_itt(qedi, cqe->itid, &ptmp_itt);

drivers/scsi/qedi/qedi_iscsi.c

+17 −3
Original line number Diff line number Diff line
@@ -783,7 +783,6 @@ static int qedi_mtask_xmit(struct iscsi_conn *conn, struct iscsi_task *task) 
	}



	cmd->conn = conn->dd_data;

	cmd->scsi_cmd = NULL;

	return qedi_iscsi_send_generic_request(task);

}
@@ -794,6 +793,10 @@ static int qedi_task_xmit(struct iscsi_task *task) 
	struct qedi_cmd *cmd = task->dd_data;

	struct scsi_cmnd *sc = task->sc;



	/* Clear now so in cleanup_task we know it didn't make it */

	cmd->scsi_cmd = NULL;

	cmd->task_id = U16_MAX;



	if (test_bit(QEDI_IN_SHUTDOWN, &qedi_conn->qedi->flags))

		return -ENODEV;
@@ -1391,13 +1394,24 @@ static umode_t qedi_attr_is_visible(int param_type, int param) 


static void qedi_cleanup_task(struct iscsi_task *task)

{

	if (!task->sc || task->state == ISCSI_TASK_PENDING) {

	struct qedi_cmd *cmd;



	if (task->state == ISCSI_TASK_PENDING) {

		QEDI_INFO(NULL, QEDI_LOG_IO, "Returning ref_cnt=%d\n",

			  refcount_read(&task->refcount));

		return;

	}



	if (task->sc)

		qedi_iscsi_unmap_sg_list(task->dd_data);



	cmd = task->dd_data;

	if (cmd->task_id != U16_MAX)

		qedi_clear_task_idx(iscsi_host_priv(task->conn->session->host),

				    cmd->task_id);



	cmd->task_id = U16_MAX;

	cmd->scsi_cmd = NULL;

}



struct iscsi_transport qedi_iscsi_transport = {