Re: [PATCH v4 51/56] 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] Unbalanced kref_get_unless_zero() leaks command reference when schedule_work() returns false, causing an NVMe command stall.
- [High] Unconditional INIT_WORK() in qla_nvme_post_cmd() corrupts the workqueue if a command is retried while an abort is pending.

Pre-existing issues:
- [Critical] Submission error paths clear priv->sp and directly free the SRB without synchronization, causing NULL pointer dereference or use-after-free in concurrent aborts.
--

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

scsi: qla2xxx: Initialize NVMe abort_work once at submission

This commit moves the initialization of abort_work from the abort paths to
command submission. This prevents work item corruption if multiple aborts
are issued for the same command while a work item is already queued.

> 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]
Does this change introduce a kref leak if schedule_work() returns false?

With INIT_WORK() removed, if a concurrent abort is already pending,
schedule_work() will return false. However, earlier in qla_nvme_ls_abort(),
we unconditionally increment the command reference with
kref_get_unless_zero(). 

If schedule_work() fails because the work is already queued, the worker
only executes once and drops a single reference, meaning the extra kref
is permanently leaked and the NVMe command never completes.

[ ... ]

> @@ -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]
Could this also lead to the same kref leak as in qla_nvme_ls_abort()
if schedule_work() returns false?

[ ... ]

> @@ -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;
>  	sp->type = SRB_NVME_CMD;

[Severity: High]
Does calling INIT_WORK() here corrupt the workqueue if a command is retried?

If qla2x00_start_nvme_mq() fails with -EBUSY later in qla_nvme_post_cmd(),
the blk-mq layer will eventually retry the request, calling this function
again with the same priv structure.

If a concurrent transport teardown triggered an abort during the initial
failed attempt, priv->abort_work could be actively queued on the system
workqueue. Unconditionally calling INIT_WORK() again here on the retried
attempt would overwrite the list_head of an actively queued work_struct.

[Severity: Critical]
This is a pre-existing issue, but could this error path trigger a NULL
pointer dereference or use-after-free during a concurrent abort?

If qla2x00_start_nvme_mq() fails, the error handling does:

	rval = qla2x00_start_nvme_mq(sp);
	if (rval != QLA_SUCCESS) {
		...
		sp->priv = NULL;
		priv->sp = NULL;
		qla2xxx_rel_qpair_sp(sp->qpair, sp);
	}

This runs without holding priv->cmd_lock and without checking cmd_kref.
If a concurrent abort acquires the lock and kref just before priv->sp is
cleared, it schedules qla_nvme_abort_work().

The abort worker will then read priv->sp without locking:

drivers/scsi/qla2xxx/qla_nvme.c:qla_nvme_abort_work() {
	struct nvme_private *priv = container_of(work, ...);
	srb_t *sp = priv->sp;
	fc_port_t *fcport = sp->fcport;
        ...
}

This means the worker might read a NULL priv->sp and dereference it, or
use the SRB after it has been freed to the mempool by qla2xxx_rel_qpair_sp().

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