Re: [PATCH v6 51/56] scsi: qla2xxx: Initialize NVMe abort_work once at submission

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] The patch moves INIT_WORK to the command submission paths but fails to order it safely against the publication of priv->sp, causing a NULL pointer dereference.
- [High] Calling qla_nvme_fcp_abort or qla_nvme_ls_abort multiple times for the same command leaks its kref and permanently hangs the request.

Pre-existing issues:
- [Critical] The command state (priv->sp) is exposed to concurrent abort handlers before the spinlock (priv->cmd_lock) is initialized, leading to spinlock corruption.
- [Critical] In the error paths of command submission, priv->sp is cleared without holding cmd_lock, and the command structure is freed directly, causing a Use-After-Free.
- [High] A race condition allows qla_nvme_fcp_abort to spuriously abort a completely different, newly submitted command due to tag reuse (ABA problem).
--

--- Patch [51]: [PATCH v6 51/56] scsi: qla2xxx: Initialize NVMe abort_work once at submission ---
commit 9a9f1f14cd9974710e9176cf206ade9b1d04e0e9
Author: Nilesh Javali <njavali@marvell.com>

scsi: qla2xxx: Initialize NVMe abort_work once at submission

This patch moves the initialization of priv->abort_work to the command
submission path to avoid corrupting the workqueue list 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
> @@ -463,7 +463,6 @@ static void qla_nvme_ls_abort(struct nvme_fc_local_port *lport,
>  	}
>  	spin_unlock_irqrestore(&priv->cmd_lock, flags);
>  
> -	INIT_WORK(&priv->abort_work, qla_nvme_abort_work);
>  	schedule_work(&priv->abort_work);
>  }

[Severity: High]
If the transport layer calls the abort functions multiple times for the same
command, is there a kref leak here?

Earlier in this function, we unconditionally increment the reference count via
kref_get_unless_zero(&priv->sp->cmd_kref).

Since the work item is no longer re-initialized, schedule_work() will return
false and do nothing if the work is already pending. However, the return
value is ignored and the extra kref taken is never dropped.

Since the work function qla_nvme_abort_work() only executes once and drops a
single kref, will this permanently hang the NVMe core's I/O queue by
preventing the kref from ever reaching zero?

[ ... ]

> @@ -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;
>  	priv->fd = fd;

[Severity: Critical]
Could this sequence cause a panic due to an uninitialized work item?

We publish the command state with priv->sp = sp explicitly before INIT_WORK()
executes. Because the transport allocates the private data with kzalloc(), the
work struct's function pointer is initially NULL.

If a concurrent error recovery thread calls qla_nvme_ls_abort() while
qla_nvme_ls_req() is executing, it can observe priv->sp != NULL and call
schedule_work(&priv->abort_work).

Will this queue a work item with a NULL function pointer, causing a kernel
panic when the worker thread executes it?

[Severity: Critical]
This is a pre-existing issue, but does initializing the spinlock after
publishing priv->sp lead to lock corruption?

Similar to the work item, priv->sp is exposed to concurrent abort handlers
before spin_lock_init(&priv->cmd_lock) is called. A concurrent abort from
nvme_fc_abort_lsops can see priv->sp != NULL and successfully acquire the
0-initialized lock.

If the submission thread then unconditionally resets the lock internals back
to 0 while the abort handler is inside its critical section, will it cause
memory corruption or lockdep panics when the abort handler tries to unlock it?

[Severity: Critical]
This is a pre-existing issue, but can the error path further down in this
function cause a use-after-free if an abort runs concurrently?

If hardware submission fails, the error path in qla_nvme_ls_req() does this:

    rval = qla2x00_start_sp(sp);
    if (rval != QLA_SUCCESS) {
        ...
        sp->priv = NULL;
        priv->sp = NULL;
        qla2x00_rel_sp(sp);
        return rval;
    }

It sets priv->sp = NULL without acquiring priv->cmd_lock, bypasses kref_put(),
and immediately frees the command structure via qla2x00_rel_sp(). 

If a concurrent abort handler has already checked priv->sp != NULL and
incremented the kref inside qla_nvme_ls_abort(), it is left holding a
reference to freed memory. When the queued abort_work runs, won't it invoke
abort logic on the freed command structure?

[ ... ]

> @@ -545,7 +545,6 @@ static void qla_nvme_fcp_abort(struct nvme_fc_local_port *lport,
>  	}
>  	spin_unlock_irqrestore(&priv->cmd_lock, flags);
>  
> -	INIT_WORK(&priv->abort_work, qla_nvme_abort_work);
>  	schedule_work(&priv->abort_work);
>  }

[Severity: High]
This is a pre-existing issue, but is qla_nvme_fcp_abort() vulnerable to a tag
reuse race condition?

In the NVMe FC core, __nvme_fc_abort_op() initiates an abort without
synchronizing with the I/O completion path. If a hardware completion interrupt
fires and completes the request, the block layer can free and immediately reuse
the same tag (and thus the same fd->private memory) for a completely new
command.

When qla_nvme_fcp_abort() runs and acquires priv->cmd_lock:

    static void qla_nvme_fcp_abort(...)
    {
        ...
        spin_lock_irqsave(&priv->cmd_lock, flags);

it will see the new priv->sp. Does this cause a spurious abort of the newly
submitted, innocent command?

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