Re: [PATCH 05/14] scsi: qla2xxx: Fix double addition of fcport to gnl.fcport list

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: qla2xxx: FCF_ASYNC_ACTIVE leak on early return in qla24xx_async_gnl()
- [High] scsi: qla2xxx: incomplete fix for gnl_entry double add in qla24xx_async_gnl()
--

commit 26e3803dac33bf8507fb3ff3170def3e0261a1f3
Author: Nilesh Javali <njavali@marvell.com>

scsi: qla2xxx: Fix double addition of fcport to gnl.fcport list

This commit moves the FCF_ASYNC_SENT check in qla24xx_async_gnl() under
vha->hw->tgt.sess_lock so that testing and setting FCF_ASYNC_SENT is
serialized with adding fcport->gnl_entry to vha->gnl.fcports.

> diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
> index bb0cc71de37b8..515a7ea4299db 100644
> --- a/drivers/scsi/qla2xxx/qla_init.c
> +++ b/drivers/scsi/qla2xxx/qla_init.c
> @@ -1158,13 +1158,17 @@ int qla24xx_async_gnl(struct scsi_qla_host *vha, fc_port_t *fcport)
>  	unsigned long flags;
>  	u16 *mb;
>  
> -	if (!vha->flags.online || (fcport->flags & FCF_ASYNC_SENT))
> +	if (!vha->flags.online)
>  		goto done;
>  
>  	ql_dbg(ql_dbg_disc, vha, 0x20d9,
>  	    "Async-gnlist WWPN %8phC \n", fcport->port_name);
>  
>  	spin_lock_irqsave(&vha->hw->tgt.sess_lock, flags);
> +	if (fcport->flags & FCF_ASYNC_SENT) {
> +		spin_unlock_irqrestore(&vha->hw->tgt.sess_lock, flags);
> +		return QLA_SUCCESS;
> +	}

[Severity: High]
Could returning QLA_SUCCESS directly here instead of jumping to done leak
FCF_ASYNC_ACTIVE on fcport->flags and cause a session relogin hang?

When qla24xx_post_gnl_work() queues a QLA_EVT_GNL work item, it sets
FCF_ASYNC_ACTIVE on fcport:

drivers/scsi/qla2xxx/qla_init.c:qla24xx_post_gnl_work() {
	...
	e->u.fcport.fcport = fcport;
	fcport->flags |= FCF_ASYNC_ACTIVE;
	return qla2x00_post_work(vha, e);
}

Previously, commit 39d22740712c ("scsi: qla2xxx: Fix session hang in gnl")
made the FCF_ASYNC_SENT check in qla24xx_async_gnl() jump to done so that
FCF_ASYNC_ACTIVE would be cleared when bailing out:

drivers/scsi/qla2xxx/qla_init.c:qla24xx_async_gnl() {
	...
done:
	fcport->flags &= ~(FCF_ASYNC_ACTIVE);
	return rval;
}

Because FCF_ASYNC_SENT is also set by non-GNL paths that do not clear
FCF_ASYNC_ACTIVE upon completion, such as qlt_unreg_sess() ->
qlt_free_session_done() in drivers/scsi/qla2xxx/qla_target.c:

	spin_lock_irqsave(&vha->work_lock, flags);
	sess->flags &= ~FCF_ASYNC_SENT;
	sess->deleted = QLA_SESS_DELETED;
	sess->free_pending = 0;
	spin_unlock_irqrestore(&vha->work_lock, flags);

as well as qla24xx_async_prli() -> qla2x00_async_prli_sp_done(),
qla24xx_async_notify_ack() -> qla2x00_async_nack_sp_done(), and
qla2x00_abort_isp_cleanup(), returning early without clearing
FCF_ASYNC_ACTIVE or adding fcport to vha->gnl.fcports leaves
FCF_ASYNC_ACTIVE permanently set once the other operation finishes.

When FCF_ASYNC_ACTIVE remains set, both qla24xx_fcport_handle_login() and
qla2x00_relogin() defer login indefinitely:

drivers/scsi/qla2xxx/qla_init.c:qla24xx_fcport_handle_login() {
	...
	if (fcport->flags & (FCF_ASYNC_SENT | FCF_ASYNC_ACTIVE)) {
		set_bit(RELOGIN_NEEDED, &vha->dpc_flags);
		return 0;
	}
	...
}

>  	fcport->flags |= FCF_ASYNC_SENT;
>  	qla2x00_set_fcport_disc_state(fcport, DSC_GNL);
>  	fcport->last_rscn_gen = fcport->rscn_gen;

[Severity: High]
Is checking FCF_ASYNC_SENT under sess_lock sufficient to prevent adding an
already-linked fcport->gnl_entry to vha->gnl.fcports?

All QLA_EVT_GNL work items for a vha are dispatched from the single per-vha
work item vha->iocb_work via qla2x00_iocb_work_fn() -> qla2x00_do_work(), so
two calls to qla24xx_async_gnl() on the same vha do not run concurrently.

Instead, fcport->flags can have FCF_ASYNC_SENT cleared while
fcport->gnl_entry is still linked on vha->gnl.fcports:

1. In drivers/scsi/qla2xxx/qla_init.c, qla24xx_async_gnl_sp_done() clears
FCF_ASYNC_SENT on sp->fcport without holding sess_lock and before splicing
and unlinking entries from vha->gnl.fcports:

	sp->fcport->flags &= ~(FCF_ASYNC_SENT|FCF_ASYNC_ACTIVE);
	...
	spin_lock_irqsave(&vha->hw->tgt.sess_lock, flags);

	INIT_LIST_HEAD(&h);
	fcport = tf = NULL;
	if (!list_empty(&vha->gnl.fcports))
		list_splice_init(&vha->gnl.fcports, &h);
	spin_unlock_irqrestore(&vha->hw->tgt.sess_lock, flags);

2. During ISP abort cleanup, qla2x00_abort_isp_cleanup() in
drivers/scsi/qla2xxx/qla_init.c clears FCF_ASYNC_SENT across vp_fcports
without holding sess_lock or unlinking fcport->gnl_entry:

	list_for_each_entry(fcport, &vha->vp_fcports, list) {
		fcport->flags &= ~(FCF_LOGIN_NEEDED | FCF_ASYNC_SENT);
		fcport->scan_state = 0;
	}

The same happens in qla2x00_async_iocb_timeout() (before the asynchronous
abort completes), qlt_free_session_done(), and qla2x00_update_fcport().

3. If qla2x00_get_sp() fails in qla24xx_async_gnl() after
list_add_tail(&fcport->gnl_entry, &vha->gnl.fcports), jumping to done leaves
fcport->gnl_entry linked on vha->gnl.fcports with vha->gnl.sent == 1, and in
qla24xx_async_gnl_sp_done(), the retrigger loop only unlinks the first entry
before breaking with vha->gnl.sent = 0:

	vha->gnl.sent = 0;
	if (!list_empty(&vha->gnl.fcports)) {
		/* retrigger gnl */
		list_for_each_entry_safe(fcport, tf, &vha->gnl.fcports,
		    gnl_entry) {
			list_del_init(&fcport->gnl_entry);
			fcport->flags &= ~(FCF_ASYNC_SENT | FCF_ASYNC_ACTIVE);
			if (qla24xx_post_gnl_work(vha, fcport) == QLA_SUCCESS)
				break;
		}
	}

In any of those cases, when a subsequent QLA_EVT_GNL runs
qla24xx_async_gnl(), FCF_ASYNC_SENT is clear even under sess_lock, so
list_add_tail(&fcport->gnl_entry, &vha->gnl.fcports) will still be called on
an already-linked gnl_entry. Should qla24xx_async_gnl() also guard against
!list_empty(&fcport->gnl_entry) under sess_lock and ensure gnl_entry is
unlinked on failure and reset paths?

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