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

Pre-existing issues:
- [High] Missing list_del() in qla_nvme_xmt_ls_rsp() error path leads to Use-After-Free list corruption
- [High] Concurrent lockless modifications of fcport->unsol_ctx_head list cause list corruption
- [High] Concurrent formatting of shared HBA-wide LS Reject payload buffer causes data corruption
--

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

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

This commit takes the hardware_lock (via qp_lock_ptr) in the unlocked callers
of qla_nvme_ls_reject_iocb(), preventing concurrent execution with normal I/O
submission on the base ring. It ensures the ring producer state is protected 
from corruption leading to duplicated or dropped commands.

> diff --git a/drivers/scsi/qla2xxx/qla_nvme.c b/drivers/scsi/qla2xxx/qla_nvme.c
> --- 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: High]
This is a pre-existing issue, but missing a list_del() in the
qla_nvme_xmt_ls_rsp() error path can lead to a use-after-free list corruption.

When the transport layer attempts to send the response, if sending fails,
the code jumps to the out: error path. It frees the uctx object via kfree()
without unlinking uctx->elem from the fcport->unsol_ctx_head list.

A subsequent unsolicited LS request will perform list_add_tail() on this list,
writing to the freed memory. Should a list_del() be added here?

[ ... ]

> @@ -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,
>  			struct qla_nvme_lsrjt_pt_arg *a, bool is_xchg_terminate)

[Severity: High]
This isn't a bug introduced by this patch, but doesn't
qla_nvme_ls_reject_iocb() format the shared HBA-wide LS Reject payload buffer
without HBA-level synchronization?

When an invalid unsolicited NVMe-FC LS request is received,
qla2xxx_process_purls_iocb() invokes qla_nvme_ls_reject_iocb(..., false).
This formats the reject payload directly into vha->hw->lsrjt.c, a single
HBA-wide buffer allocated at probe time.

Because qla2xxx_process_purls_iocb() is called holding only the per-queue-pair
lock, if multiple NVMe-FC requests arrive on different queue pairs
simultaneously, multiple CPUs will concurrently overwrite the same lsrjt.c
buffer without any HBA-level synchronization.

Could this result in the hardware doing a DMA from concurrently overwritten
memory, transmitting corrupted or interleaved protocol data?

[ ... ]

> @@ -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 does removing elements from the
fcport->unsol_ctx_head list without locks cause list corruption?

Elements are added to fcport->unsol_ctx_head via list_add_tail() in
qla2xxx_process_purls_iocb() from the response queue processing context while
holding the queue pair lock. 

However, they are removed here via list_del() in qla2xxx_process_purls_pkt()
entirely unlocked, which is now clearly outside the newly added qp_lock_ptr
region.

The same unlocked list_del() also occurs in qla_nvme_release_lsrsp_cmd_kref().
Could these concurrent non-atomic operations corrupt the next and prev pointers?

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