Re: [PATCH v2 05/13] 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 gnl_entry double-add fix in qla24xx_async_gnl()
--

commit 92c98a5a2f2d104f2b6d47badef1b553bffd531c
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 to prevent adding fcport->gnl_entry to
vha->gnl.fcports twice.

> 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]
Does returning QLA_SUCCESS directly here leak the FCF_ASYNC_ACTIVE flag on
fcport->flags and cause a permanent session relogin hang?

Every caller that schedules qla24xx_async_gnl() posts a QLA_EVT_GNL work
item via qla24xx_post_gnl_work(), which sets FCF_ASYNC_ACTIVE:

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, when FCF_ASYNC_SENT was set on entry to qla24xx_async_gnl(), it
branched to the done label, which cleared FCF_ASYNC_ACTIVE:

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

With this change, qla24xx_async_gnl() returns QLA_SUCCESS without clearing
FCF_ASYNC_ACTIVE and without adding fcport->gnl_entry to vha->gnl.fcports.

FCF_ASYNC_SENT is a shared per-fcport flag that is also set by other session
management and discovery paths whose completion or cleanup routines only
clear FCF_ASYNC_SENT and never clear FCF_ASYNC_ACTIVE. For example, during
session deletion in drivers/scsi/qla2xxx/qla_target.c, qlt_unreg_sess() sets
FCF_ASYNC_SENT to block discovery commands while a QLA_EVT_GNL work item may
already be queued, and qlt_free_session_done() only clears FCF_ASYNC_SENT:

drivers/scsi/qla2xxx/qla_target.c:qlt_unreg_sess() {
	...
	sess->flags |= FCF_ASYNC_SENT;
	...
}

drivers/scsi/qla2xxx/qla_target.c:qlt_free_session_done() {
	...
	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);
	...
}

Similarly, qla24xx_async_notify_ack() / qla2x00_async_nack_sp_done() in
drivers/scsi/qla2xxx/qla_target.c, qla24xx_async_prli() /
qla2x00_async_prli_sp_done() in drivers/scsi/qla2xxx/qla_init.c, and ISP
reset cleanup in qla2x00_abort_isp_cleanup() and qla_pci_error_cleanup()
only clear FCF_ASYNC_SENT.

When qla24xx_async_gnl() hits this early return while FCF_ASYNC_SENT is set
by any of those paths (or if an in-flight GNL is aborted by chip reset
before qla24xx_async_gnl_sp_done() runs), FCF_ASYNC_ACTIVE remains stuck on
fcport->flags. Both qla24xx_fcport_handle_login() in
drivers/scsi/qla2xxx/qla_init.c and qla2x00_relogin() in
drivers/scsi/qla2xxx/qla_os.c check for FCF_ASYNC_SENT | FCF_ASYNC_ACTIVE
and will defer relogin for that fcport indefinitely, reintroducing the
session hang previously fixed by commit 39d22740712c ("scsi: qla2xxx: Fix
session hang in gnl").

Should this path clear FCF_ASYNC_ACTIVE (or jump to done after unlocking
sess_lock) before returning?

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

[Severity: High]
Can fcport->gnl_entry still be added twice to vha->gnl.fcports even with the
FCF_ASYNC_SENT check under sess_lock?

First, regarding the race described in the commit message,
qla24xx_async_gnl() is only called from qla2x00_do_work() via
qla2x00_iocb_work_fn() in drivers/scsi/qla2xxx/qla_os.c, which runs on the
single per-vha work item vha->iocb_work (and is further serialized by
IOCB_WORK_ACTIVE in vha->dpc_flags). Because a single work_struct does not
execute concurrently with itself, two QLA_EVT_GNL work items for the same
vha are processed sequentially rather than racing at the entry of
qla24xx_async_gnl().

Instead, FCF_ASYNC_SENT can be cleared to 0 while fcport->gnl_entry is still
linked on vha->gnl.fcports (or on the local list in
qla24xx_async_gnl_sp_done()):

1. In drivers/scsi/qla2xxx/qla_init.c, qla24xx_async_gnl_sp_done() clears
FCF_ASYNC_SENT locklessly before acquiring sess_lock and before unlinking
fcport->gnl_entry (dropping sess_lock around qla24xx_handle_gnl_done_event()
for each entry):

drivers/scsi/qla2xxx/qla_init.c:qla24xx_async_gnl_sp_done() {
	...
	sp->fcport->flags &= ~(FCF_ASYNC_SENT|FCF_ASYNC_ACTIVE);
	...
	spin_lock_irqsave(&vha->hw->tgt.sess_lock, flags);
	...
	list_splice_init(&vha->gnl.fcports, &h);
	spin_unlock_irqrestore(&vha->hw->tgt.sess_lock, flags);

	while (!list_empty(&h)) {
		spin_lock_irqsave(&vha->hw->tgt.sess_lock, flags);
		fcport = list_first_entry(&h, struct fc_port, gnl_entry);
		list_del_init(&fcport->gnl_entry);
		spin_unlock_irqrestore(&vha->hw->tgt.sess_lock, flags);
		qla24xx_handle_gnl_done_event(vha, fcport, ...);
	}
	...
}

If qla24xx_async_gnl() runs on qla2xxx_wq for sp->fcport during this window,
FCF_ASYNC_SENT is already 0 under sess_lock while sp->fcport->gnl_entry has
not been deleted from the list yet.

2. At the end of qla24xx_async_gnl_sp_done(), when multiple fcports were
queued onto vha->gnl.fcports while vha->gnl.sent was 1, the retrigger loop
only unlinks the first fcport and breaks out, leaving the remaining fcports
linked on vha->gnl.fcports with vha->gnl.sent set to 0:

drivers/scsi/qla2xxx/qla_init.c:qla24xx_async_gnl_sp_done() {
	...
	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;
		}
	}
	spin_unlock_irqrestore(&vha->hw->tgt.sess_lock, flags);
}

3. Several paths clear FCF_ASYNC_SENT without unlinking fcport->gnl_entry
from vha->gnl.fcports, including ISP reset cleanup in
qla2x00_abort_isp_cleanup() and qla_pci_error_cleanup(), command timeout in
qla2x00_async_iocb_timeout(), session deletion completion in
qlt_free_session_done(), and qla2x00_update_fcport():

drivers/scsi/qla2xxx/qla_init.c:qla2x00_abort_isp_cleanup() {
	...
	list_for_each_entry(fcport, &vha->vp_fcports, list) {
		fcport->flags &= ~(FCF_LOGIN_NEEDED | FCF_ASYNC_SENT);
		fcport->scan_state = 0;
	}
	...
}

drivers/scsi/qla2xxx/qla_init.c:qla2x00_async_iocb_timeout() {
	...
	case SRB_MB_IOCB:
		fcport->flags &= ~(FCF_ASYNC_SENT | FCF_ASYNC_ACTIVE);
	...
}

When a subsequent QLA_EVT_GNL work item runs qla24xx_async_gnl() for that
fcport, FCF_ASYNC_SENT is 0 under sess_lock while fcport->gnl_entry is still
linked on vha->gnl.fcports, so list_add_tail() will still trigger the
__list_add_valid_or_report() bug.

Would checking !list_empty(&fcport->gnl_entry) (or using list_move_tail())
under sess_lock before adding fcport->gnl_entry to vha->gnl.fcports avoid
this list corruption?

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