RE: [EXT] [PATCH 04/13] scsi: qedi: fix abort vs cmd re-use race

[Manish Rangankar <mrangankar@marvell.com>]

> -----Original Message-----
> From: Mike Christie <michael.christie@oracle.com>
> Sent: Sunday, April 11, 2021 12:10 AM
> To: lduncan@suse.com; martin.petersen@oracle.com; Manish Rangankar
> <mrangankar@marvell.com>; Santosh Vernekar <svernekar@marvell.com>;
> linux-scsi@vger.kernel.org; jejb@linux.ibm.com
> Cc: Mike Christie <michael.christie@oracle.com>
> 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@oracle.com>
> ---
>  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@marvell.com>