Re: [PATCH v6 52/56] scsi: qla2xxx: Hold qpair lock when sending NVMe LS reject

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] This problem wasn't introduced by this patch, but the error path of qla_nvme_xmt_ls_rsp() frees 'uctx' without removing it from the 'fcport->unsol_ctx_head' list, causing a Use-After-Free list corruption.
- [Critical] This problem wasn't introduced by this patch, but concurrent lockless accesses to the doubly-linked list 'fcport->unsol_ctx_head' cause data races and list corruption.
--

--- Patch [52]: [PATCH v6 52/56] scsi: qla2xxx: Hold qpair lock when sending NVMe LS reject ---
commit 26608c7972504e04bce429e36cbbdc786b32af3b
Author: Nilesh Javali <njavali@marvell.com>

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

This commit fixes an issue where qla_nvme_ls_reject_iocb() is called without
holding the hardware lock in error paths and the DPC context. It takes
the qp_lock_ptr lock around the two unlocked callers to prevent concurrent I/O
submission from corrupting the ring producer state.

> 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);
>  	kfree(uctx);

[Severity: Critical]
This isn't a bug introduced by this patch, but on the error path of
qla_nvme_xmt_ls_rsp(), does freeing uctx without removing it from the
fcport->unsol_ctx_head list leave a dangling pointer?

When an unsolicited request arrives, uctx is allocated and added to the
unsol_ctx_head list. If this error path is taken, kfree(uctx) runs but misses
the list_del(&uctx->elem) step, which could corrupt the list and lead to a
use-after-free when the next unsolicited request arrives and calls
list_add_tail().

>  	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: Critical]
This is a pre-existing issue, but is it safe to call list_del(&uctx->elem)
locklessly here?

It looks like list_add_tail() in qla2xxx_process_purls_iocb() modifies this
shared fcport->unsol_ctx_head list, and list_del() is also called locklessly in
qla_nvme_release_lsrsp_cmd_kref().

Could concurrent unsolicited LS requests being processed by the DPC thread
and workqueues race on this doubly-linked list, causing list corruption?

>  		kfree(uctx);
>  	}

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