Re: [PATCH v2 57/60] scsi: qla2xxx: Hold qpair lock when sending NVMe LS reject

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing `list_del()` before `kfree()` on the error path of `qla_nvme_xmt_ls_rsp()` causes use-after-free list corruption.
- [High] Concurrent and unsynchronized operations on the `fcport->unsol_ctx_head` linked list.
- [High] `qla_nvme_ls_reject_iocb()` unconditionally writes to the hardware doorbell even when the firmware is not started or the device is offline.
--

commit d9117eaeb87a4e4531b8933ba8f285e5c93c354a
Author: Nilesh Javali <njavali@marvell.com>

scsi: qla2xxx: Hold qpair lock when sending NVMe LS reject

This commit addresses an issue where qla_nvme_ls_reject_iocb() is called
without holding the producer lock from two paths, which can corrupt the ring
producer state. It takes the qp_lock_ptr around the two unlocked callers and
documents the helper as caller-locked.

> diff --git a/drivers/scsi/qla2xxx/qla_nvme.c b/drivers/scsi/qla2xxx/qla_nvme.c
> index 3b2f255a5d7d4..8dc6df6c2e1c5 100644
> --- a/drivers/scsi/qla2xxx/qla_nvme.c
> +++ b/drivers/scsi/qla2xxx/qla_nvme.c
> @@ -374,6 +374,7 @@ static int qla_nvme_xmt_ls_rsp(struct nvme_fc_local_port *lport,
>  	srb_t *sp;
>  	int rval = QLA_FUNCTION_FAILED;
>  	uint8_t cnt = 0;
> +	unsigned long flags;
>  
>  	if (!fcport || fcport->deleted)
>  		goto out;

[ ... ]

> @@ -440,7 +441,9 @@ static int qla_nvme_xmt_ls_rsp(struct nvme_fc_local_port *lport,
>  	a.vp_idx = vha->vp_idx;
>  	a.nport_handle = uctx->nport_handle;
>  	a.xchg_address = uctx->exchange_address;
> +	spin_lock_irqsave(ha->base_qpair->qp_lock_ptr, flags);
>  	qla_nvme_ls_reject_iocb(vha, ha->base_qpair, &a, true);
> +	spin_unlock_irqrestore(ha->base_qpair->qp_lock_ptr, flags);

[Severity: High]
This isn't a bug introduced by this patch, but does the call to
qla_nvme_ls_reject_iocb() here write to the hardware doorbell even when the
firmware is not started?

If qla_nvme_xmt_ls_rsp() bails out early because !ha->flags.fw_started, it
jumps to the out label and executes qla_nvme_ls_reject_iocb(). Inside that
function, qla2x00_start_iocbs() unconditionally writes to the doorbell ring.

Could accessing MMIO registers when the firmware is stopped or in reset lead
to PCIe bus errors or adapter hangs?

>  	kfree(uctx);

[Severity: High]
This is a pre-existing issue, but does this error path leave a dangling
pointer in the unsol_ctx_head list?

If uctx was previously linked into fcport->unsol_ctx_head (such as in
qla2xxx_process_purls_iocb()), freeing it here without first calling
list_del(&uctx->elem) leaves the freed memory in the list.

Will the next operation on this list access freed memory and cause list
corruption?

>  	return rval;
>  }

[ ... ]

> @@ -1299,6 +1306,7 @@ qla2xxx_process_purls_pkt(struct scsi_qla_host *vha, struct purex_item *item)
>  {
>  	struct qla_nvme_unsol_ctx *uctx = item->purls_context;
>  	struct qla_nvme_lsrjt_pt_arg a;
> +	unsigned long flags;
>  	int ret = 1;
>  
>  #if (IS_ENABLED(CONFIG_NVME_FC))
> @@ -1311,7 +1319,9 @@ qla2xxx_process_purls_pkt(struct scsi_qla_host *vha, struct purex_item *item)
>  		a.vp_idx = vha->vp_idx;
>  		a.nport_handle = uctx->nport_handle;
>  		a.xchg_address = uctx->exchange_address;
> +		spin_lock_irqsave(vha->hw->base_qpair->qp_lock_ptr, flags);
>  		qla_nvme_ls_reject_iocb(vha, vha->hw->base_qpair, &a, true);
> +		spin_unlock_irqrestore(vha->hw->base_qpair->qp_lock_ptr, flags);
>  		list_del(&uctx->elem);

[Severity: High]
This isn't a bug introduced by this patch, but does this list deletion race
with other contexts?

This list_del(&uctx->elem) is executed in a workqueue context without
acquiring any locks to protect the list. Since elements are added to the list
in qla2xxx_process_purls_iocb() while holding a per-queue hardware response
lock, could concurrent unsynchronized operations corrupt the list pointers?

>  		kfree(uctx);
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612095333.1666592-1-njavali@marvell.com?part=57