Re: [PATCH v2 06/13] scsi: qla2xxx: Fix scan state machine race and IRQ-unsafe sp_unmap

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: qla2xxx: incomplete scan.step race fix in qla_async_scan_sp_done()
- [High] scsi: qla2xxx: sp and DMA leak in qla_async_scan_sp_done() on work alloc fail
--

commit e9cd982d25a90416902027b8c35c59ac8e31a64e
Author: Manish Rangankar <mrangankar@marvell.com>

scsi: qla2xxx: Fix scan state machine race and IRQ-unsafe sp_unmap

This commit saves vha->scan.step into a local variable under work_lock in
qla_async_scan_sp_done() and defers qla24xx_sp_unmap() to process context via
QLA_EVT_UNMAP when posting scan work fails.

> diff --git a/drivers/scsi/qla2xxx/qla_gs.c b/drivers/scsi/qla2xxx/qla_gs.c
> index 20b1aef455c4f..efe98609c149d 100644
> --- a/drivers/scsi/qla2xxx/qla_gs.c
> +++ b/drivers/scsi/qla2xxx/qla_gs.c
> @@ -3512,6 +3512,8 @@ static void qla2x00_find_free_fcp_nvme_slot(struct scsi_qla_host *vha,
>  static void qla_async_scan_sp_done(srb_t *sp, int res)
>  {
>  	struct scsi_qla_host *vha = sp->vha;
> +	struct qla_work_evt *e;
> +	enum scan_step step;
>  	unsigned long flags;
>  	int rc;
>  
> @@ -3544,8 +3546,16 @@ static void qla_async_scan_sp_done(srb_t *sp, int res)
>  		 */
>  		rc = qla2x00_post_next_scan_work(vha, sp, QLA_EVT_SCAN_FINISH);
>  		if (rc) {
> -			/* Cleanup here to prevent memory leak */
> -			qla24xx_sp_unmap(vha, sp);
> +			/* Defer sp_unmap to process context -- we are in IRQ */
> +			e = qla2x00_alloc_work(vha, QLA_EVT_UNMAP);
> +			if (e) {
> +				e->u.iosb.sp = sp;
> +				qla2x00_post_work(vha, e);
> +			} else {
> +				ql_log(ql_log_warn, vha, 0x2029,
> +				       "%s: Failed to schedule sp_unmap, sp leaked\n",
> +				       name);
> +			}
>  
>  			spin_lock_irqsave(&vha->work_lock, flags);
>  			vha->scan.scan_flags &= ~SF_SCANNING;
> @@ -3568,10 +3578,11 @@ static void qla_async_scan_sp_done(srb_t *sp, int res)
>  	qla2x00_find_free_fcp_nvme_slot(vha, sp);
>  
>  	spin_lock_irqsave(&vha->work_lock, flags);
> +	step = vha->scan.step;
>  	vha->scan.scan_flags &= ~SF_SCANNING;
>  	spin_unlock_irqrestore(&vha->work_lock, flags);
>  
> -	switch (vha->scan.step) {
> +	switch (step) {

[Severity: High]
Does saving vha->scan.step into a local variable while clearing SF_SCANNING
between scan steps allow two fabric scans to run concurrently and corrupt
vha->scan?

In the race described by the commit message, as soon as SF_SCANNING is
cleared here in qla_async_scan_sp_done(), another CPU calling
qla_fab_scan_start() -> qla_fab_async_scan(vha, NULL) can see SF_SCANNING
clear, set SF_SCANNING, reset vha->scan.step to FAB_SCAN_START, and zero
vha->scan.l:

drivers/scsi/qla2xxx/qla_gs.c:qla_fab_async_scan() {
	spin_lock_irqsave(&vha->work_lock, flags);
	if (vha->scan.scan_flags & SF_SCANNING) {
		spin_unlock_irqrestore(&vha->work_lock, flags);
		ql_dbg(ql_dbg_disc + ql_dbg_verbose, vha, 0x2012,
		    "%s: scan active for sp:%p\n", __func__, sp);
		goto done_free_sp;
	}
	vha->scan.scan_flags |= SF_SCANNING;
	if (!sp)
		vha->scan.step = FAB_SCAN_START;

	spin_unlock_irqrestore(&vha->work_lock, flags);
	...
		memset(vha->scan.l, 0, vha->scan.size);
}

With this change, qla_async_scan_sp_done() switches on the saved step and
posts QLA_EVT_SCAN_CMD or QLA_EVT_SCAN_FINISH for the first scan while the
second scan is also running:

1. If step was FAB_SCAN_GNNFT_NVME (or FAB_SCAN_GNNFT_FCP without NVMe),
   qla_fab_scan_finish() runs on the workqueue concurrently with the new
   scan, reads vha->scan.l while the new scan zeroes or overwrites it (which
   can leave online ports in QLA_FCPORT_SCAN and schedule them for deletion
   via qlt_schedule_sess_for_deletion()), and then clears SF_SCANNING at the
   out label while the second scan's SRB is in flight.

2. If step was an intermediate step, qla_fab_async_scan(vha, sp) runs with
   the first scan's non-NULL sp. If the second scan failed an allocation in
   FAB_SCAN_START (which clears SF_SCANNING while leaving vha->scan.step as
   FAB_SCAN_START), the first scan enters case FAB_SCAN_START and overwrites
   sp = qla2x00_get_sp(vha, NULL, GFP_KERNEL), leaking the first scan's sp
   and its coherent DMA buffers ctarg.req and ctarg.rsp.

3. Conversely, if qla_fab_async_scan() is entered while SF_SCANNING is
   already set, it jumps to done_free_sp, which unconditionally clears
   SF_SCANNING:

drivers/scsi/qla2xxx/qla_gs.c:qla_fab_async_scan() {
done_free_sp:
	...
	spin_lock_irqsave(&vha->work_lock, flags);
	vha->scan.scan_flags &= ~SF_SCANNING;
}

   Once SF_SCANNING is cleared while an SRB is still in flight, another
   qla_fab_scan_start() call can set vha->scan.step = FAB_SCAN_START before
   qla_async_scan_sp_done() runs, so qla2x00_find_free_fcp_nvme_slot() (which
   reads vha->scan.step outside work_lock) and step = vha->scan.step in
   qla_async_scan_sp_done() will still see FAB_SCAN_START and hit WARN_ON(1).

>  	case FAB_SCAN_GPNFT_FCP:
>  	case FAB_SCAN_GPNFT_NVME:
>  		rc = qla2x00_post_next_scan_work(vha, sp, QLA_EVT_SCAN_CMD);
> @@ -3594,7 +3605,16 @@ static void qla_async_scan_sp_done(srb_t *sp, int res)
>  	}
>  
>  	if (rc) {
> -		qla24xx_sp_unmap(vha, sp);
> +		/* Defer sp_unmap to process context -- we are in IRQ */
> +		e = qla2x00_alloc_work(vha, QLA_EVT_UNMAP);
> +		if (e) {
> +			e->u.iosb.sp = sp;
> +			qla2x00_post_work(vha, e);
> +		} else {
> +			ql_log(ql_log_warn, vha, 0x202a,
> +			       "%s: Failed to schedule sp_unmap, sp leaked\n",
> +			       __func__);
> +		}

[Severity: High]
Can qla2x00_alloc_work(vha, QLA_EVT_UNMAP) ever succeed here or in the error
path above when qla2x00_post_next_scan_work() just failed, and does this leak
sp, its DMA buffers, and its vha and qpair references during unload or vport
deletion?

Looking at qla2x00_post_next_scan_work():

drivers/scsi/qla2xxx/qla_gs.c:qla2x00_post_next_scan_work() {
	e = qla2x00_alloc_work(vha, cmd);
	if (!e)
		return QLA_FUNCTION_FAILED;

	e->u.iosb.sp = sp;

	return qla2x00_post_work(vha, e);
}

Since qla2x00_post_work() always returns QLA_SUCCESS,
qla2x00_post_next_scan_work() only fails when qla2x00_alloc_work() returns
NULL:

drivers/scsi/qla2xxx/qla_os.c:qla2x00_alloc_work() {
	if (test_bit(UNLOADING, &vha->dpc_flags))
		return NULL;

	if (qla_vha_mark_busy(vha))
		return NULL;

	e = kzalloc_obj(struct qla_work_evt, GFP_ATOMIC);
	...
}

When an in-flight scan SRB is aborted during driver unload (where UNLOADING
is set before qla2x00_abort_all_cmds() invokes sp->done()) or vport deletion
(where delete_progress causes qla_vha_mark_busy() to fail),
qla2x00_post_next_scan_work() fails because qla2x00_alloc_work() returns
NULL. Calling qla2x00_alloc_work(vha, QLA_EVT_UNMAP) immediately afterwards
checks the exact same conditions and will also return NULL (and under
GFP_ATOMIC allocation failure, retries the identical allocation in the same
hard IRQ context).

When the else branch is taken, sp, sp->u.iocb_cmd.u.ctarg.req, and
sp->u.iocb_cmd.u.ctarg.rsp are leaked, and kref_put(&sp->cmd_kref,
qla2x00_sp_release) is never called, leaving vha->vref_count elevated (which
stalls qla24xx_deallocate_vp_id()) and leaving objects allocated in
srb_cachep when kmem_cache_destroy(srb_cachep) runs in qla2x00_module_exit().

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