Re: [PATCH v5 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 4 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Missing `list_del()` on the error path in `qla_nvme_xmt_ls_rsp()` leaves a freed object in `fcport->unsol_ctx_head`, causing use-after-free and list corruption.
- [High] Concurrent, unlocked list operations on `fcport->unsol_ctx_head` lead to race conditions and list pointer corruption.
- [High] Unconditional hardware doorbell writes in `qla_nvme_xmt_ls_rsp()` when firmware is stopped.
- [Medium] Concurrent handling of unsolicited NVMe Link Services on different response queues causes a data race on the globally shared reject DMA buffer (`vha->hw->lsrjt.c`).
--

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

This patch adds queue pair lock protection around calls to
qla_nvme_ls_reject_iocb() in unlocked purex work and xmt_ls_rsp contexts.
This prevents concurrent accesses from corrupting the request 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
[ ... ]
> @@ -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 does the error path in
qla_nvme_xmt_ls_rsp() leave a freed object in the list?

If we reach the out label (for instance, when !ha->flags.fw_started is true),
kfree(uctx) is called, but list_del(&uctx->elem) is omitted. Could this lead
to a use-after-free if the fcport->unsol_ctx_head list is traversed later?

[Severity: High]
This is also a pre-existing issue, but does calling qla_nvme_ls_reject_iocb()
here unconditionally ring the hardware doorbell even when the firmware is
stopped?

Earlier in qla_nvme_xmt_ls_rsp(), if !ha->flags.fw_started is true, it jumps
to the out label, which calls qla_nvme_ls_reject_iocb(). That helper allocates
an iocb and calls qla2x00_start_iocbs(), which writes to the hardware doorbell.
Can this cause hardware faults on a stopped or resetting device?

>  	return rval;
>  }
[ ... ]
> @@ -1243,6 +1246,10 @@ static void qla_nvme_lsrjt_pt_iocb(struct scsi_qla_host *vha,
>  	}
>  }
>  
> +/*
> + * Allocates from and advances the request ring, so the caller must hold
> + * qp->qp_lock_ptr (the response-queue caller already holds it).
> + */
>  static int
>  qla_nvme_ls_reject_iocb(struct scsi_qla_host *vha, struct qla_qpair *qp,

[Severity: Medium]
This is a pre-existing issue, but can concurrent calls to
qla_nvme_ls_reject_iocb() corrupt the shared reject buffer?

When called with is_xchg_terminate as false from different response queues,
the helper writes directly to the globally shared vha->hw->lsrjt.c buffer:

qla_nvme_ls_reject_iocb() {
    ...
    if (!is_xchg_terminate) {
        qla_nvme_fc_format_rjt((void *)vha->hw->lsrjt.c, a->opcode,
                               a->reason, a->explanation, 0);
        a->tx_addr = vha->hw->lsrjt.cdma;
    ...
}

Since multiple response queues can process unsolicited link service packets
concurrently while holding different queue-specific locks, can this lead to
torn writes and corrupted payloads being sent to the hardware?

>  			struct qla_nvme_lsrjt_pt_arg *a, bool is_xchg_terminate)
[ ... ]
> @@ -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);
>  		kfree(uctx);

[Severity: High]
This is a pre-existing issue, but is the fcport->unsol_ctx_head list protected
against concurrent modifications?

In qla2xxx_process_purls_pkt(), list_del(&uctx->elem) is called from a workqueue
context without holding a lock:

qla2xxx_process_purls_pkt() {
    ...
    list_del(&uctx->elem);
    kfree(uctx);
    ...
}

Elements are added to this list concurrently using list_add_tail() from the
response queue context (like in qla2xxx_process_purls_iocb), and removed
from other contexts (like qla_nvme_release_lsrsp_cmd_kref). Could this result
in list pointer corruption?

>  	}
>  }

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