[PATCH] scsi: qla2xxx: Fix fcport handling in session creation race

[PATCH] scsi: qla2xxx: Fix fcport handling in session creation race

[Serhat Kumral]

When a second WWPN lookup finds an existing fcport due to a race, the
driver continues using the newly allocated fcport instead of the existing
one, then frees it at the end of qla24xx_create_new_sess().

Smatch reports a use-after-free of pla in the trailing cleanup.
qlt_plogi_ack_unref() may have already freed pla, making the subsequent
list_del() and kmem_cache_free() invalid.

Fix this by switching to the existing fcport, freeing the
redundant newly allocated one, and removing the trailing cleanup.

Fixes: b5d1531260b9 ("scsi: qla2xxx: Fix slow mem alloc behind lock")
Assisted-by: LLM
Signed-off-by: Serhat Kumral <serhatkumral1@gmail.com>
---
smatch output:
- drivers/scsi/qla2xxx/qla_os.c:5627 qla24xx_create_new_sess() error: dereferencing freed memory 'pla' (line 5573)

 drivers/scsi/qla2xxx/qla_os.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
index 3c412c7fb6fe..1231265365af 100644
--- a/drivers/scsi/qla2xxx/qla_os.c
+++ b/drivers/scsi/qla2xxx/qla_os.c
@@ -5456,9 +5456,9 @@ void qla24xx_create_new_sess(struct scsi_qla_host *vha, struct qla_work_evt *e)
 {
 	unsigned long flags;
 	fc_port_t *fcport =  NULL, *tfcp;
+	fc_port_t *conflict_fcport = NULL;
 	struct qlt_plogi_ack_t *pla =
 	    (struct qlt_plogi_ack_t *)e->u.new_sess.pla;
-	uint8_t free_fcport = 0;
 
 	ql_dbg(ql_dbg_disc, vha, 0xffff,
 	    "%s %d %8phC enter\n",
@@ -5529,7 +5529,9 @@ void qla24xx_create_new_sess(struct scsi_qla_host *vha, struct qla_work_evt *e)
 			    __func__, tfcp->port_name, tfcp->disc_state,
 			    tfcp->fw_login_state);
 
-			free_fcport = 1;
+			conflict_fcport = fcport;
+			fcport = tfcp;
+
 		} else {
 			list_add_tail(&fcport->list, &vha->vp_fcports);
 
@@ -5542,6 +5544,9 @@ void qla24xx_create_new_sess(struct scsi_qla_host *vha, struct qla_work_evt *e)
 	}
 	spin_unlock_irqrestore(&vha->hw->tgt.sess_lock, flags);
 
+	if (conflict_fcport)
+		qla2x00_free_fcport(conflict_fcport);
+
 	if (fcport) {
 		fcport->id_changed = 1;
 		fcport->scan_state = QLA_FCPORT_FOUND;
@@ -5620,14 +5625,6 @@ void qla24xx_create_new_sess(struct scsi_qla_host *vha, struct qla_work_evt *e)
 			}
 		}
 	}
-
-	if (free_fcport) {
-		qla2x00_free_fcport(fcport);
-		if (pla) {
-			list_del(&pla->list);
-			kmem_cache_free(qla_tgt_plogi_cachep, pla);
-		}
-	}
 }
 
 static void qla_sp_retry(struct scsi_qla_host *vha, struct qla_work_evt *e)
-- 
2.53.0

Re: [PATCH] scsi: qla2xxx: Fix fcport handling in session creation race

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] scsi: qla2xxx: Unlocked state modifications to live fcport
- [Medium] scsi: qla2xxx: missing d_id update on session swap risks stale N_Port ID
--

commit 677908d94136f8ead1918acb926dd5f120568a2d
Author: Serhat Kumral <serhatkumral1@gmail.com>

scsi: qla2xxx: Fix fcport handling in session creation race

This commit addresses a session creation race condition by swapping to
the existing fcport when a second WWPN lookup finds one. It then frees
the newly allocated fcport to avoid a use-after-free in the trailing cleanup.

> diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
> index 3c412c7fb6fe7..1231265365afd 100644
> --- a/drivers/scsi/qla2xxx/qla_os.c
> +++ b/drivers/scsi/qla2xxx/qla_os.c
[ ... ]
> @@ -5529,7 +5529,9 @@ void qla24xx_create_new_sess(struct scsi_qla_host *vha, struct qla_work_evt *e)
>  			    __func__, tfcp->port_name, tfcp->disc_state,
>  			    tfcp->fw_login_state);
>  
> -			free_fcport = 1;
> +			conflict_fcport = fcport;
> +			fcport = tfcp;

[Severity: Medium]
When swapping to the existing session, is there a missing update for the
N_Port ID?

Unlike the first lookup block in qla24xx_create_new_sess(), the N_Port ID of
the session is not updated here. If pla is NULL (e.g., from GPN_FT or mailbox
events rather than target PLOGI), the d_id is never updated.

Could this cause the subsequent qla2x00_find_fcport_by_nportid() check to
fail or incorrectly identify a conflict, leading to the driver logging into
the stale N_Port ID?

> +
>  		} else {
>  			list_add_tail(&fcport->list, &vha->vp_fcports);
>  
> @@ -5542,6 +5544,9 @@ void qla24xx_create_new_sess(struct scsi_qla_host *vha, struct qla_work_evt *e)
>  	}
>  	spin_unlock_irqrestore(&vha->hw->tgt.sess_lock, flags);
>  
> +	if (conflict_fcport)
> +		qla2x00_free_fcport(conflict_fcport);
> +
>  	if (fcport) {
>  		fcport->id_changed = 1;
>  		fcport->scan_state = QLA_FCPORT_FOUND;
>  		fcport->chip_reset = vha->hw->base_qpair->chip_reset;
>  		memcpy(fcport->node_name, e->u.new_sess.node_name, WWN_SIZE);

[Severity: Medium]
Does modifying these state fields on the now-live fcport outside of the
sess_lock introduce data races?

Because fcport was swapped to the existing session (tfcp) inside the lock, it
points to a live session object. Setting fields like id_changed and scan_state
after dropping sess_lock may race with other threads accessing this session.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261007102632.11971-1-serhatkumral1@gmail.com?part=1