Menu
▾
▴
Re: [Scst-devel] [PATCH v3 11/16] scsi: qla2xxx: fix TMR failure handling
|
From: Tony B. <to...@cy...> - 2025-10-03 14:38:30
|
On 10/3/25 04:40, Dan Carpenter wrote: > Hi Tony, > > kernel test robot noticed the following build warnings: > > url: https://github.com/intel-lab-lkp/linux/commits/Tony-Battersby/Revert-scsi-qla2xxx-Perform-lockless-command-completion-in-abort-path/20250930-024814 > base: e5f0a698b34ed76002dc5cff3804a61c80233a7a > patch link: https://lore.kernel.org/r/f52cda16-4952-4b28-bbf7-d44f4e054490%40cybernetics.com > patch subject: [PATCH v2 11/16] scsi: qla2xxx: fix TMR failure handling > config: i386-randconfig-141-20251002 (https://download.01.org/0day-ci/archive/20251003/202...@in.../config) > compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261) > > If you fix the issue in a separate patch/commit (i.e. not just a new version of > the same patch/commit), kindly add following tags > | Reported-by: kernel test robot <lk...@in...> > | Reported-by: Dan Carpenter <dan...@li...> > | Closes: https://lore.kernel.org/r/202...@in.../ > > New smatch warnings: > drivers/scsi/qla2xxx/qla_target.c:5735 qlt_handle_abts_completion() error: we previously assumed 'mcmd' could be null (see line 5723) > v3 patch below, which should fix this issue. From: Tony Battersby <to...@cy...> Date: Fri, 3 Oct 2025 10:03:08 -0400 Subject: [PATCH v3 11/16] scsi: qla2xxx: fix TMR failure handling (target mode) If handle_tmr() fails: - The code for QLA_TGT_ABTS results in memory-use-after-free and double-free: qlt_do_tmr_work() qlt_build_abts_resp_iocb() qpair->req->outstanding_cmds[h] = (srb_t *)mcmd; mempool_free(mcmd, qla_tgt_mgmt_cmd_mempool); FIRST FREE qlt_handle_abts_completion() mcmd = qlt_ctio_to_cmd() cmd = req->outstanding_cmds[h]; return cmd; vha = mcmd->vha; USE-AFTER-FREE ha->tgt.tgt_ops->free_mcmd(mcmd); SECOND FREE - qlt_send_busy() makes no sense because it sends a SCSI command response instead of a TMR response. Instead just call qlt_xmit_tm_rsp() to send a TMR failed response, since that code is well-tested and handles a number of corner cases. But it would be incorrect to call ha->tgt.tgt_ops->free_mcmd() after handle_tmr() failed, so add a flag to mcmd indicating the proper way to free the mcmd so that qlt_xmit_tm_rsp() can be used for both cases. Signed-off-by: Tony Battersby <to...@cy...> --- v2 -> v3: - Check for mcmd == NULL in qlt_free_ul_mcmd(). v1 -> v2: - Change FCP_TMF_REJECTED to FCP_TMF_FAILED. - Add QLA24XX_MGMT_LLD_OWNED and qlt_free_ul_mcmd(). - Improve patch description. drivers/scsi/qla2xxx/qla_os.c | 2 +- drivers/scsi/qla2xxx/qla_target.c | 56 +++++++++++++------------------ drivers/scsi/qla2xxx/qla_target.h | 2 ++ 3 files changed, 27 insertions(+), 33 deletions(-) diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c index 2a3eb1dacf86..64387224f28a 100644 --- a/drivers/scsi/qla2xxx/qla_os.c +++ b/drivers/scsi/qla2xxx/qla_os.c @@ -1893,7 +1893,7 @@ __qla2x00_abort_all_cmds(struct qla_qpair *qp, int res) * Currently, only ABTS response gets on the * outstanding_cmds[] */ - ha->tgt.tgt_ops->free_mcmd( + qlt_free_ul_mcmd(ha, (struct qla_tgt_mgmt_cmd *) sp); break; default: diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c index 849ab256807b..009b9ca5c2b9 100644 --- a/drivers/scsi/qla2xxx/qla_target.c +++ b/drivers/scsi/qla2xxx/qla_target.c @@ -2005,7 +2005,6 @@ static void qlt_do_tmr_work(struct work_struct *work) struct qla_hw_data *ha = mcmd->vha->hw; int rc; uint32_t tag; - unsigned long flags; switch (mcmd->tmr_func) { case QLA_TGT_ABTS: @@ -2020,34 +2019,12 @@ static void qlt_do_tmr_work(struct work_struct *work) mcmd->tmr_func, tag); if (rc != 0) { - spin_lock_irqsave(mcmd->qpair->qp_lock_ptr, flags); - switch (mcmd->tmr_func) { - case QLA_TGT_ABTS: - mcmd->fc_tm_rsp = FCP_TMF_REJECTED; - qlt_build_abts_resp_iocb(mcmd); - break; - case QLA_TGT_LUN_RESET: - case QLA_TGT_CLEAR_TS: - case QLA_TGT_ABORT_TS: - case QLA_TGT_CLEAR_ACA: - case QLA_TGT_TARGET_RESET: - qlt_send_busy(mcmd->qpair, &mcmd->orig_iocb.atio, - qla_sam_status); - break; - - case QLA_TGT_ABORT_ALL: - case QLA_TGT_NEXUS_LOSS_SESS: - case QLA_TGT_NEXUS_LOSS: - qlt_send_notify_ack(mcmd->qpair, - &mcmd->orig_iocb.imm_ntfy, 0, 0, 0, 0, 0, 0); - break; - } - spin_unlock_irqrestore(mcmd->qpair->qp_lock_ptr, flags); - ql_dbg(ql_dbg_tgt_mgt, mcmd->vha, 0xf052, "qla_target(%d): tgt_ops->handle_tmr() failed: %d\n", mcmd->vha->vp_idx, rc); - mempool_free(mcmd, qla_tgt_mgmt_cmd_mempool); + mcmd->flags |= QLA24XX_MGMT_LLD_OWNED; + mcmd->fc_tm_rsp = FCP_TMF_FAILED; + qlt_xmit_tm_rsp(mcmd); } } @@ -2234,6 +2211,21 @@ void qlt_free_mcmd(struct qla_tgt_mgmt_cmd *mcmd) } EXPORT_SYMBOL(qlt_free_mcmd); +/* + * If the upper layer knows about this mgmt cmd, then call its ->free_cmd() + * callback, which will eventually call qlt_free_mcmd(). Otherwise, call + * qlt_free_mcmd() directly. + */ +void qlt_free_ul_mcmd(struct qla_hw_data *ha, struct qla_tgt_mgmt_cmd *mcmd) +{ + if (!mcmd) + return; + if (mcmd->flags & QLA24XX_MGMT_LLD_OWNED) + qlt_free_mcmd(mcmd); + else + ha->tgt.tgt_ops->free_mcmd(mcmd); +} + /* * ha->hardware_lock supposed to be held on entry. Might drop it, then * reacquire @@ -2326,12 +2318,12 @@ void qlt_xmit_tm_rsp(struct qla_tgt_mgmt_cmd *mcmd) "RESET-TMR online/active/old-count/new-count = %d/%d/%d/%d.\n", vha->flags.online, qla2x00_reset_active(vha), mcmd->reset_count, qpair->chip_reset); - ha->tgt.tgt_ops->free_mcmd(mcmd); + qlt_free_ul_mcmd(ha, mcmd); spin_unlock_irqrestore(qpair->qp_lock_ptr, flags); return; } - if (mcmd->flags == QLA24XX_MGMT_SEND_NACK) { + if (mcmd->flags & QLA24XX_MGMT_SEND_NACK) { switch (mcmd->orig_iocb.imm_ntfy.u.isp24.status_subcode) { case ELS_LOGO: case ELS_PRLO: @@ -2364,7 +2356,7 @@ void qlt_xmit_tm_rsp(struct qla_tgt_mgmt_cmd *mcmd) * qlt_xmit_tm_rsp() returns here.. */ if (free_mcmd) - ha->tgt.tgt_ops->free_mcmd(mcmd); + qlt_free_ul_mcmd(ha, mcmd); spin_unlock_irqrestore(qpair->qp_lock_ptr, flags); } @@ -5742,7 +5734,7 @@ static void qlt_handle_abts_completion(struct scsi_qla_host *vha, if (le32_to_cpu(entry->error_subcode1) == 0x1E && le32_to_cpu(entry->error_subcode2) == 0) { if (qlt_chk_unresolv_exchg(vha, rsp->qpair, entry)) { - ha->tgt.tgt_ops->free_mcmd(mcmd); + qlt_free_ul_mcmd(ha, mcmd); return; } qlt_24xx_retry_term_exchange(vha, rsp->qpair, @@ -5753,10 +5745,10 @@ static void qlt_handle_abts_completion(struct scsi_qla_host *vha, vha->vp_idx, entry->compl_status, entry->error_subcode1, entry->error_subcode2); - ha->tgt.tgt_ops->free_mcmd(mcmd); + qlt_free_ul_mcmd(ha, mcmd); } } else if (mcmd) { - ha->tgt.tgt_ops->free_mcmd(mcmd); + qlt_free_ul_mcmd(ha, mcmd); } } diff --git a/drivers/scsi/qla2xxx/qla_target.h b/drivers/scsi/qla2xxx/qla_target.h index eb15d8e9f79e..223c40bc9498 100644 --- a/drivers/scsi/qla2xxx/qla_target.h +++ b/drivers/scsi/qla2xxx/qla_target.h @@ -966,6 +966,7 @@ struct qla_tgt_mgmt_cmd { unsigned int flags; #define QLA24XX_MGMT_SEND_NACK BIT_0 #define QLA24XX_MGMT_ABORT_IO_ATTR_VALID BIT_1 +#define QLA24XX_MGMT_LLD_OWNED BIT_2 uint32_t reset_count; struct work_struct work; uint64_t unpacked_lun; @@ -1059,6 +1060,7 @@ extern int qlt_abort_cmd(struct qla_tgt_cmd *); void qlt_send_term_exchange(struct qla_qpair *qpair, struct qla_tgt_cmd *cmd, struct atio_from_isp *atio, int ha_locked); extern void qlt_xmit_tm_rsp(struct qla_tgt_mgmt_cmd *); +void qlt_free_ul_mcmd(struct qla_hw_data *ha, struct qla_tgt_mgmt_cmd *mcmd); extern void qlt_free_mcmd(struct qla_tgt_mgmt_cmd *); extern void qlt_free_cmd(struct qla_tgt_cmd *cmd); extern void qlt_unmap_sg(struct scsi_qla_host *vha, struct qla_tgt_cmd *cmd); -- 2.43.0 |
