Re: [PATCH 5/8] qla2xxx: Fix double scsi_done for abort path

["Ewan D. Milne" <emilne@redhat.com>]

On Tue, 2019-11-05 at 07:06 -0800, Himanshu Madhani wrote:
> From: Quinn Tran <qutran@marvell.com>
> 
> Current code assume abort will remove the original command from the
> active list where scsi_done will not be call. Instead, the eh_abort
> thread will do the scsi_done. That is not the case.  Instead, we
> have a double scsi_done calls triggering use after free.
> 
> Abort will tell FW to release the command from FW possesion. The
> original command will return to ULP with error in its normal fashion via
> scsi_done.  eh_abort path would wait for the original command
> completion before returning.  eh_abort path will not perform the
> scsi_done call.
> 
> Fixes: 219d27d7147e0 ("scsi: qla2xxx: Fix race conditions in the code for aborting SCSI commands")
> Cc: stable@vger.kernel.org # 5.2
> Signed-off-by: Quinn Tran <qutran@marvell.com>
> Signed-off-by: Arun Easi <aeasi@marvell.com>
> Signed-off-by: Himanshu Madhani <hmadhani@marvell.com>
> ---
>  drivers/scsi/qla2xxx/qla_def.h  |   5 +-
>  drivers/scsi/qla2xxx/qla_isr.c  |   5 ++
>  drivers/scsi/qla2xxx/qla_nvme.c |   4 +-
>  drivers/scsi/qla2xxx/qla_os.c   | 117 +++++++++++++++++++++-------------------
>  4 files changed, 72 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
> index ef9bb3c7ad6f..2a9e6a9a8c9d 100644
> --- a/drivers/scsi/qla2xxx/qla_def.h
> +++ b/drivers/scsi/qla2xxx/qla_def.h
> @@ -591,13 +591,16 @@ typedef struct srb {
>  	 */
>  	uint8_t cmd_type;
>  	uint8_t pad[3];
> -	atomic_t ref_count;
>  	struct kref cmd_kref;	/* need to migrate ref_count over to this */
>  	void *priv;
>  	wait_queue_head_t nvme_ls_waitq;
>  	struct fc_port *fcport;
>  	struct scsi_qla_host *vha;
>  	unsigned int start_timer:1;
> +	unsigned int abort:1;
> +	unsigned int aborted:1;
> +	unsigned int completed:1;
> +
>  	uint32_t handle;
>  	uint16_t flags;
>  	uint16_t type;
> diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
> index acef3d73983c..1b8f297449cf 100644
> --- a/drivers/scsi/qla2xxx/qla_isr.c
> +++ b/drivers/scsi/qla2xxx/qla_isr.c
> @@ -2487,6 +2487,11 @@ qla2x00_status_entry(scsi_qla_host_t *vha, struct rsp_que *rsp, void *pkt)
>  		return;
>  	}
>  
> +	if (sp->abort)
> +		sp->aborted = 1;
> +	else
> +		sp->completed = 1;
> +
>  	if (sp->cmd_type != TYPE_SRB) {
>  		req->outstanding_cmds[handle] = NULL;
>  		ql_dbg(ql_dbg_io, vha, 0x3015,
> diff --git a/drivers/scsi/qla2xxx/qla_nvme.c b/drivers/scsi/qla2xxx/qla_nvme.c
> index 6cc19e060afc..941aa53363f5 100644
> --- a/drivers/scsi/qla2xxx/qla_nvme.c
> +++ b/drivers/scsi/qla2xxx/qla_nvme.c
> @@ -224,8 +224,8 @@ static void qla_nvme_abort_work(struct work_struct *work)
>  
>  	if (ha->flags.host_shutting_down) {
>  		ql_log(ql_log_info, sp->fcport->vha, 0xffff,
> -		    "%s Calling done on sp: %p, type: 0x%x, sp->ref_count: 0x%x\n",
> -		    __func__, sp, sp->type, atomic_read(&sp->ref_count));
> +		    "%s Calling done on sp: %p, type: 0x%x\n",
> +		    __func__, sp, sp->type);
>  		sp->done(sp, 0);
>  		goto out;
>  	}
> diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
> index 2e7a4a2d6c5a..b59d579834ea 100644
> --- a/drivers/scsi/qla2xxx/qla_os.c
> +++ b/drivers/scsi/qla2xxx/qla_os.c
> @@ -698,11 +698,6 @@ void qla2x00_sp_compl(srb_t *sp, int res)
>  	struct scsi_cmnd *cmd = GET_CMD_SP(sp);
>  	struct completion *comp = sp->comp;
>  
> -	if (WARN_ON_ONCE(atomic_read(&sp->ref_count) == 0))
> -		return;
> -
> -	atomic_dec(&sp->ref_count);
> -
>  	sp->free(sp);
>  	cmd->result = res;
>  	CMD_SP(cmd) = NULL;
> @@ -794,11 +789,6 @@ void qla2xxx_qpair_sp_compl(srb_t *sp, int res)
>  	struct scsi_cmnd *cmd = GET_CMD_SP(sp);
>  	struct completion *comp = sp->comp;
>  
> -	if (WARN_ON_ONCE(atomic_read(&sp->ref_count) == 0))
> -		return;
> -
> -	atomic_dec(&sp->ref_count);
> -
>  	sp->free(sp);
>  	cmd->result = res;
>  	CMD_SP(cmd) = NULL;
> @@ -903,7 +893,7 @@ qla2xxx_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
>  
>  	sp->u.scmd.cmd = cmd;
>  	sp->type = SRB_SCSI_CMD;
> -	atomic_set(&sp->ref_count, 1);
> +
>  	CMD_SP(cmd) = (void *)sp;
>  	sp->free = qla2x00_sp_free_dma;
>  	sp->done = qla2x00_sp_compl;
> @@ -985,11 +975,9 @@ qla2xxx_mqueuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd,
>  
>  	sp->u.scmd.cmd = cmd;
>  	sp->type = SRB_SCSI_CMD;
> -	atomic_set(&sp->ref_count, 1);
>  	CMD_SP(cmd) = (void *)sp;
>  	sp->free = qla2xxx_qpair_sp_free_dma;
>  	sp->done = qla2xxx_qpair_sp_compl;
> -	sp->qpair = qpair;
>  
>  	rval = ha->isp_ops->start_scsi_mq(sp);
>  	if (rval != QLA_SUCCESS) {
> @@ -1187,16 +1175,6 @@ qla2x00_wait_for_chip_reset(scsi_qla_host_t *vha)
>  	return return_status;
>  }
>  
> -static int
> -sp_get(struct srb *sp)
> -{
> -	if (!refcount_inc_not_zero((refcount_t *)&sp->ref_count))
> -		/* kref get fail */
> -		return ENXIO;
> -	else
> -		return 0;
> -}
> -
>  #define ISP_REG_DISCONNECT 0xffffffffU
>  /**************************************************************************
>  * qla2x00_isp_reg_stat
> @@ -1252,6 +1230,9 @@ qla2xxx_eh_abort(struct scsi_cmnd *cmd)
>  	uint64_t lun;
>  	int rval;
>  	struct qla_hw_data *ha = vha->hw;
> +	uint32_t ratov_j;
> +	struct qla_qpair *qpair;
> +	unsigned long flags;
>  
>  	if (qla2x00_isp_reg_stat(ha)) {
>  		ql_log(ql_log_info, vha, 0x8042,
> @@ -1264,13 +1245,26 @@ qla2xxx_eh_abort(struct scsi_cmnd *cmd)
>  		return ret;
>  
>  	sp = scsi_cmd_priv(cmd);
> +	qpair = sp->qpair;
>  
> -	if (sp->fcport && sp->fcport->deleted)
> +	if ((sp->fcport && sp->fcport->deleted) || !qpair)
>  		return SUCCESS;
>  
> -	/* Return if the command has already finished. */
> -	if (sp_get(sp))
> +	spin_lock_irqsave(qpair->qp_lock_ptr, flags);
> +	if (sp->completed) {
> +		spin_unlock_irqrestore(qpair->qp_lock_ptr, flags);
>  		return SUCCESS;
> +	}
> +
> +	if (sp->abort || sp->aborted) {
> +		spin_unlock_irqrestore(qpair->qp_lock_ptr, flags);
> +		return FAILED;
> +	}
> +
> +	sp->abort = 1;
> +	sp->comp = &comp;
> +	spin_unlock_irqrestore(qpair->qp_lock_ptr, flags);
> +
>  
>  	id = cmd->device->id;
>  	lun = cmd->device->lun;
> @@ -1279,47 +1273,37 @@ qla2xxx_eh_abort(struct scsi_cmnd *cmd)
>  	    "Aborting from RISC nexus=%ld:%d:%llu sp=%p cmd=%p handle=%x\n",
>  	    vha->host_no, id, lun, sp, cmd, sp->handle);
>  
> +	/*
> +	 * Abort will release the original Command/sp from FW. Let the
> +	 * original command call scsi_done. In return, he will wakeup
> +	 * this sleeping thread.
> +	 */
>  	rval = ha->isp_ops->abort_command(sp);
> +
>  	ql_dbg(ql_dbg_taskm, vha, 0x8003,
>  	       "Abort command mbx cmd=%p, rval=%x.\n", cmd, rval);
>  
> +	/* Wait for the command completion. */
> +	ratov_j = ha->r_a_tov/10 * 4 * 1000;
> +	ratov_j = msecs_to_jiffies(ratov_j);
>  	switch (rval) {
>  	case QLA_SUCCESS:
> -		/*
> -		 * The command has been aborted. That means that the firmware
> -		 * won't report a completion.
> -		 */
> -		sp->done(sp, DID_ABORT << 16);
> -		ret = SUCCESS;
> -		break;
> -	case QLA_FUNCTION_PARAMETER_ERROR: {
> -		/* Wait for the command completion. */
> -		uint32_t ratov = ha->r_a_tov/10;
> -		uint32_t ratov_j = msecs_to_jiffies(4 * ratov * 1000);
> -
> -		WARN_ON_ONCE(sp->comp);
> -		sp->comp = &comp;
>  		if (!wait_for_completion_timeout(&comp, ratov_j)) {
>  			ql_dbg(ql_dbg_taskm, vha, 0xffff,
>  			    "%s: Abort wait timer (4 * R_A_TOV[%d]) expired\n",
> -			    __func__, ha->r_a_tov);
> +			    __func__, ha->r_a_tov/10);
>  			ret = FAILED;
>  		} else {
>  			ret = SUCCESS;
>  		}
>  		break;
> -	}
>  	default:
> -		/*
> -		 * Either abort failed or abort and completion raced. Let
> -		 * the SCSI core retry the abort in the former case.
> -		 */
>  		ret = FAILED;
>  		break;
>  	}
>  
>  	sp->comp = NULL;
> -	atomic_dec(&sp->ref_count);
> +
>  	ql_log(ql_log_info, vha, 0x801c,
>  	    "Abort command issued nexus=%ld:%d:%llu -- %x.\n",
>  	    vha->host_no, id, lun, ret);
> @@ -1711,32 +1695,53 @@ static void qla2x00_abort_srb(struct qla_qpair *qp, srb_t *sp, const int res,
>  	scsi_qla_host_t *vha = qp->vha;
>  	struct qla_hw_data *ha = vha->hw;
>  	int rval;
> +	bool ret_cmd;
> +	uint32_t ratov_j;
>  
> -	if (sp_get(sp))
> +	if (qla2x00_chip_is_down(vha)) {
> +		sp->done(sp, res);
>  		return;
> +	}
>  
>  	if (sp->type == SRB_NVME_CMD || sp->type == SRB_NVME_LS ||
>  	    (sp->type == SRB_SCSI_CMD && !ha->flags.eeh_busy &&
>  	     !test_bit(ABORT_ISP_ACTIVE, &vha->dpc_flags) &&
>  	     !qla2x00_isp_reg_stat(ha))) {
> +		if (sp->comp) {
> +			sp->done(sp, res);
> +			return;
> +		}
> +
>  		sp->comp = &comp;
> +		sp->abort =  1;
>  		spin_unlock_irqrestore(qp->qp_lock_ptr, *flags);
> -		rval = ha->isp_ops->abort_command(sp);
>  
> +		rval = ha->isp_ops->abort_command(sp);
> +		/* Wait for command completion. */
> +		ret_cmd = false;
> +		ratov_j = ha->r_a_tov/10 * 4 * 1000;
> +		ratov_j = msecs_to_jiffies(ratov_j);
>  		switch (rval) {
>  		case QLA_SUCCESS:
> -			sp->done(sp, res);
> +			if (wait_for_completion_timeout(&comp, ratov_j)) {
> +				ql_dbg(ql_dbg_taskm, vha, 0xffff,
> +				    "%s: Abort wait timer (4 * R_A_TOV[%d]) expired\n",
> +				    __func__, ha->r_a_tov/10);
> +				ret_cmd = true;
> +			}
> +			/* else FW return SP to driver */
>  			break;
> -		case QLA_FUNCTION_PARAMETER_ERROR:
> -			wait_for_completion(&comp);
> +		default:
> +			ret_cmd = true;
>  			break;
>  		}
>  
>  		spin_lock_irqsave(qp->qp_lock_ptr, *flags);
> -		sp->comp = NULL;
> +		if (ret_cmd && (!sp->completed || !sp->aborted))
> +			sp->done(sp, res);
> +	} else {
> +		sp->done(sp, res);
>  	}
> -
> -	atomic_dec(&sp->ref_count);
>  }
>  
>  static void
> @@ -1758,7 +1763,6 @@ __qla2x00_abort_all_cmds(struct qla_qpair *qp, int res)
>  	for (cnt = 1; cnt < req->num_outstanding_cmds; cnt++) {
>  		sp = req->outstanding_cmds[cnt];
>  		if (sp) {
> -			req->outstanding_cmds[cnt] = NULL;
>  			switch (sp->cmd_type) {
>  			case TYPE_SRB:
>  				qla2x00_abort_srb(qp, sp, res, &flags);
> @@ -1780,6 +1784,7 @@ __qla2x00_abort_all_cmds(struct qla_qpair *qp, int res)
>  			default:
>  				break;
>  			}
> +			req->outstanding_cmds[cnt] = NULL;
>  		}
>  	}
>  	spin_unlock_irqrestore(qp->qp_lock_ptr, flags);

Reviewed-by: Ewan D. Milne <emilne@redhat.com>