Re: [PATCH v3 51/88] scsi: qla2xxx: Initialize NVMe abort_work once at submission

[sashiko-bot@kernel.org]

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

New issues:
- [High] Moving INIT_WORK to command submission introduces workqueue list corruption on the error path if an abort was concurrently scheduled.

Pre-existing issues:
- [Critical] The error path in qla_nvme_post_cmd and qla_nvme_ls_req unconditionally frees `sp` and nullifies `priv->sp` without lock protection, bypassing `cmd_kref`.
- [High] Data race and missing memory barrier when initializing priv->abort_work, priv->sp, and priv->cmd_lock.
--

--- Patch [51]: [PATCH v3 51/88] scsi: qla2xxx: Initialize NVMe abort_work once at submission ---
commit b03391e97f40b451e572a2e6302f81908aa69bdd
Author: Nilesh Javali <njavali@marvell.com>
Subject: scsi: qla2xxx: Initialize NVMe abort_work once at submission

This patch moves the initialization of priv->abort_work from the abort
handlers to the command submission paths. This is intended to prevent
corrupting the workqueue list by reinitializing an already pending
work item if an abort is issued more than once for the same command.

> diff --git a/drivers/scsi/qla2xxx/qla_nvme.c b/drivers/scsi/qla2xxx/qla_nvme.c
> index 0038b6274d444..3b2f255a5d7d4 100644
> --- a/drivers/scsi/qla2xxx/qla_nvme.c
> +++ b/drivers/scsi/qla2xxx/qla_nvme.c

[ ... ]

> @@ -501,6 +500,7 @@ static int qla_nvme_ls_req(struct nvme_fc_local_port *lport,
>  	priv->sp = sp;
>  	kref_init(&sp->cmd_kref);
>  	spin_lock_init(&priv->cmd_lock);
> +	INIT_WORK(&priv->abort_work, qla_nvme_abort_work);
>  	nvme = &sp->u.iocb_cmd;

[Severity: High]
This is a pre-existing issue, but is the initialization of priv->abort_work,
priv->sp, and priv->cmd_lock safe from concurrent aborts?

Because the NVMe FC transport can start the block layer timer before
calling command submission, a timeout can trigger a concurrent
qla_nvme_ls_abort() while this code is running.

Without memory barriers, a concurrent abort thread might observe
priv->sp != NULL before the spinlock or work struct are fully initialized
in memory. Could this lead to locking an uninitialized spinlock or
scheduling a partially initialized work struct?

[Severity: High]
If a concurrent abort schedules abort_work while this submission is
executing, and the submission subsequently fails, does this lead to a
use-after-free?

If the submission fails, the transport layer synchronously receives an
error and frees the request buffer. If abort_work was already queued
by a concurrent abort, won't this free the work struct while it is
still pending on the system workqueue?

[ ... ]

> @@ -877,6 +876,7 @@ static int qla_nvme_post_cmd(struct nvme_fc_local_port *lport,
>  
>  	kref_init(&sp->cmd_kref);
>  	spin_lock_init(&priv->cmd_lock);
> +	INIT_WORK(&priv->abort_work, qla_nvme_abort_work);
>  	sp->priv = priv;
>  	priv->sp = sp;

[Severity: High]
If a concurrent abort schedules abort_work while this FCP submission is
executing, and the submission subsequently fails, could this cause
workqueue corruption?

If the transport layer receives an error, the block layer retries the
request using the exact same priv structure. This causes INIT_WORK()
to execute again. Will reinitializing this queued work item reset its
list pointers and corrupt the workqueue linked list?

[Severity: Critical]
This is a pre-existing issue, but does the error path in this function
(and similarly in qla_nvme_ls_req) safely handle a concurrent abort?

If qla2x00_start_nvme_mq() fails, the error path nullifies pointers and
frees the structure:

drivers/scsi/qla2xxx/qla_nvme.c:qla_nvme_post_cmd() {
    ...
    rval = qla2x00_start_nvme_mq(sp);
    if (rval != QLA_SUCCESS) {
        ql_dbg(ql_dbg_io + ql_dbg_verbose, vha, 0x212d,
            "qla2x00_start_nvme_mq failed = %d\n", rval);
        sp->priv = NULL;
        priv->sp = NULL;
        qla2xxx_rel_qpair_sp(sp->qpair, sp);
    }
    ...
}

If a concurrent abort already executed, cmd_kref is elevated and
abort_work is scheduled. This error path locklessly sets priv->sp = NULL
and unconditionally frees sp back to the mempool, bypassing the reference
held by the pending abort_work.

When qla_nvme_abort_work() later executes, won't it either read the
nullified priv->sp or operate on the freed sp structure, causing a
use-after-free?

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