> -----Original Message-----
> From: Mike Christie <michael.christie@xxxxxxxxxx>
> Sent: Sunday, April 11, 2021 12:10 AM
> To: lduncan@xxxxxxxx; martin.petersen@xxxxxxxxxx; Manish Rangankar
> <mrangankar@xxxxxxxxxxx>; Santosh Vernekar <svernekar@xxxxxxxxxxx>;
> linux-scsi@xxxxxxxxxxxxxxx; jejb@xxxxxxxxxxxxx
> Cc: Mike Christie <michael.christie@xxxxxxxxxx>
> Subject: [EXT] [PATCH 04/13] scsi: qedi: fix abort vs cmd re-use race
>
> External Email
>
> ----------------------------------------------------------------------
> 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.
>
> This patch has us 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.
>
> The next patch will fix the case where the abort timesout while we are running
> qedi_tmf_work.
>
> Signed-off-by: Mike Christie <michael.christie@xxxxxxxxxx>
> ---
> drivers/scsi/qedi/qedi_fw.c | 13 -------------
> drivers/scsi/qedi/qedi_iscsi.c | 20 +++++++++++++++++---
> 2 files changed, 17 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/scsi/qedi/qedi_fw.c b/drivers/scsi/qedi/qedi_fw.c index
> cf57b4e49700..ad4357e4c15d 100644
> --- a/drivers/scsi/qedi/qedi_fw.c
> +++ b/drivers/scsi/qedi/qedi_fw.c
> @@ -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);
> @@ -821,8 +811,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 +844,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); diff --git
> a/drivers/scsi/qedi/qedi_iscsi.c b/drivers/scsi/qedi/qedi_iscsi.c index
> 08c05403cd72..d1da34a938da 100644
> --- a/drivers/scsi/qedi/qedi_iscsi.c
> +++ b/drivers/scsi/qedi/qedi_iscsi.c
> @@ -772,7 +772,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);
> }
>
> @@ -783,6 +782,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 = -1;
> +
> if (test_bit(QEDI_IN_SHUTDOWN, &qedi_conn->qedi->flags))
> return -ENODEV;
>
> @@ -1383,13 +1386,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;
> }
>
> - qedi_iscsi_unmap_sg_list(task->dd_data);
> + if (task->sc)
> + qedi_iscsi_unmap_sg_list(task->dd_data);
> +
> + cmd = task->dd_data;
> + if (cmd->task_id != -1)
> + qedi_clear_task_idx(iscsi_host_priv(task->conn->session->host),
> + cmd->task_id);
> +
> + cmd->task_id = -1;
> + cmd->scsi_cmd = NULL;
> }
>
> struct iscsi_transport qedi_iscsi_transport = {
> --
> 2.25.1
Reviewed-by: Manish Rangankar <mrangankar@xxxxxxxxxxx>
