[PATCH v6 0/8] lpfc: Update lpfc to revision 15.0.0.1

[PATCH v6 0/8] lpfc: Update lpfc to revision 15.0.0.1

[Nigel Kirkland]

This patch set contains important bug fixes related to cleanup
handling in both normal and error paths, and refactoring of
duplicate code.

The patches were cut against Martin's 7.4/scsi-queue tree.

Nigel Kirkland (8):
  lpfc: Fix use-after-free in lpfc_cmpl_ct_cmd_vmid
  lpfc: Early return out of lpfc_els_abort when HBA_SETUP flag is not
    set
  lpfc: Fix kernel oops when unmapping scsi dma buffers for an aborted
    cmd
  lpfc: Check fc4_xpt_flags before decrementing ndlp kref on FDISC error
  lpfc: Rework I/O flush ordering when unloading driver
  lpfc: Refactor calls on fc_disctmo to lpfc_set_disctmo in RSCN handler
  lpfc: Update correct ndlp refcnt when rejecting an unsolicited PLOGI
  lpfc: Update lpfc version to 15.0.0.1

Changes since v5:
  - Two patches have been dropped from the set pending analysis of
    sashiko feedback. The patches will be resubmitted in a future
    patch set after additional testing.
  - Updated patch 4 to address sashiko comment.

Changes since v4:
  - 4 patches have been dropped from the set for review of sashiko
    comment and soak testing of any necessary changes. The patches
    will be resubmitted in a future patch set.
  - Updated patch 6 to address sashiko comments.


 drivers/scsi/lpfc/lpfc_ct.c        |  2 -
 drivers/scsi/lpfc/lpfc_els.c       | 80 ++++++++++++++++++++----------
 drivers/scsi/lpfc/lpfc_hbadisc.c   |  7 +++
 drivers/scsi/lpfc/lpfc_init.c      | 16 +++++-
 drivers/scsi/lpfc/lpfc_nportdisc.c | 30 ++++++++++-
 drivers/scsi/lpfc/lpfc_sli.c       |  8 ++-
 drivers/scsi/lpfc/lpfc_version.h   |  2 +-
 7 files changed, 111 insertions(+), 34 deletions(-)

-- 
2.38.0

[PATCH v6 1/8] lpfc: Fix use-after-free in lpfc_cmpl_ct_cmd_vmid

[Nigel Kirkland]

In lpfc_cmpl_ct_cmd_vmid, there is an early call to lpfc_ct_free_iocb
when cmd is SLI_CTAS_DALLAPP_ID.  Within lpfc_ct_free_iocb the
cmdiocb->rsp_dmabuf will be freed.  This means any ctrsp ptr
dereference for SLI_CT_RESPONSE_FS_RJT or even ctrsp->ReasonCode and
ctrsp->Explanation when handling a CT LS_RJT response is a
use-after-free.

Remove the early lpfc_ct_free_iocb call for SLI_CTAS_DALLAPP_ID.
There already is a free_res label that calls lpfc_ct_free_iocb so
there doesn't need to be an early lpfc_ct_free_iocb at the start of
lpfc_cmpl_ct_cmd_vmid.

Signed-off-by: Nigel Kirkland <nkirkland2304@gmail.com>

---
 drivers/scsi/lpfc/lpfc_ct.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/scsi/lpfc/lpfc_ct.c b/drivers/scsi/lpfc/lpfc_ct.c
index 0734ab3be3e3..f709e5087577 100644
--- a/drivers/scsi/lpfc/lpfc_ct.c
+++ b/drivers/scsi/lpfc/lpfc_ct.c
@@ -3576,8 +3576,6 @@ lpfc_cmpl_ct_cmd_vmid(struct lpfc_hba *phba, struct lpfc_iocbq *cmdiocb,
 	int i;
 
 	cmd = be16_to_cpu(ctcmd->CommandResponse.bits.CmdRsp);
-	if (cmd == SLI_CTAS_DALLAPP_ID)
-		lpfc_ct_free_iocb(phba, cmdiocb);
 
 	if (lpfc_els_chk_latt(vport) || get_job_ulpstatus(phba, rspiocb)) {
 		if (cmd != SLI_CTAS_DALLAPP_ID)
-- 
2.38.0

Re: [PATCH v6 1/8] lpfc: Fix use-after-free in lpfc_cmpl_ct_cmd_vmid

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] scsi: lpfc: evaluation of uninitialized response payload on transport error
--

commit c46419e38a4d473126adc74a9b8003eb31bdd1e9
Author: Nigel Kirkland <nkirkland2304@gmail.com>

lpfc: Fix use-after-free in lpfc_cmpl_ct_cmd_vmid

This commit resolves a use-after-free issue in lpfc_cmpl_ct_cmd_vmid by
removing an early call to lpfc_ct_free_iocb for SLI_CTAS_DALLAPP_ID commands.
The previous code freed the IOCB and then continued to access the response
buffer.

> diff --git a/drivers/scsi/lpfc/lpfc_ct.c b/drivers/scsi/lpfc/lpfc_ct.c
> index 0734ab3be3e30..f709e50875770 100644
> --- a/drivers/scsi/lpfc/lpfc_ct.c
> +++ b/drivers/scsi/lpfc/lpfc_ct.c
> @@ -3576,8 +3576,6 @@ lpfc_cmpl_ct_cmd_vmid(struct lpfc_hba *phba, struct lpfc_iocbq *cmdiocb,
>  	int i;
>  
>  	cmd = be16_to_cpu(ctcmd->CommandResponse.bits.CmdRsp);
> -	if (cmd == SLI_CTAS_DALLAPP_ID)
> -		lpfc_ct_free_iocb(phba, cmdiocb);
>  
>  	if (lpfc_els_chk_latt(vport) || get_job_ulpstatus(phba, rspiocb)) {
>  		if (cmd != SLI_CTAS_DALLAPP_ID)

[Severity: Medium]
Does this code evaluate uninitialized memory if there is a transport error?

If get_job_ulpstatus() indicates a transport error, the device did not populate
the response buffer. Since the error handler is intentionally bypassed for
SLI_CTAS_DALLAPP_ID:

drivers/scsi/lpfc/lpfc_ct.c:lpfc_cmpl_ct_cmd_vmid() {
	if (lpfc_els_chk_latt(vport) || get_job_ulpstatus(phba, rspiocb)) {
		if (cmd != SLI_CTAS_DALLAPP_ID)
			goto free_res;
	}
	...

The code will then fall through to the next check:

	if (be16_to_cpu(rsp) == SLI_CT_RESPONSE_FS_RJT) {
	...

And evaluate rsp, ctrsp->ReasonCode, and ctrsp->Explanation from the outp->virt
buffer. Since lpfc_alloc_ct_rsp() allocates this buffer via lpfc_mbuf_alloc()
without zero-initialization, this reads uninitialized memory.

Could this lead to non-deterministic behavior, such as spuriously retrying the
command or incorrectly assuming success and deregistering all VMID app IDs?

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

[PATCH v6 2/8] lpfc: Early return out of lpfc_els_abort when HBA_SETUP flag is not set

[Nigel Kirkland]

It is possible that a dev_loss_tmo callback fires during an hba reset.
The ELS pring structure is cleared by the hba reset path and the
dev_loss_tmo callback executing lpfc_els_abort could be using a stale ELS
pring pointer. To significantly reduce exposure, check if HBA_SETUP flag is set
before proceeding to use the ELS pring pointer in lpfc_els_abort. There is
no point to issue aborts when the sli port is not setup anyways.

Signed-off-by: Nigel Kirkland <nkirkland2304@gmail.com>

---
 drivers/scsi/lpfc/lpfc_nportdisc.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/scsi/lpfc/lpfc_nportdisc.c b/drivers/scsi/lpfc/lpfc_nportdisc.c
index 9c449055a55e..2c8d995a45bf 100644
--- a/drivers/scsi/lpfc/lpfc_nportdisc.c
+++ b/drivers/scsi/lpfc/lpfc_nportdisc.c
@@ -227,6 +227,11 @@ lpfc_els_abort(struct lpfc_hba *phba, struct lpfc_nodelist *ndlp)
 	struct lpfc_iocbq *iocb, *next_iocb;
 	int retval = 0;
 
+	/* Exit early to prevent race with queue teardown. */
+	if (unlikely(phba->sli_rev == LPFC_SLI_REV4 &&
+		     !test_bit(HBA_SETUP, &phba->hba_flag)))
+		return;
+
 	pring = lpfc_phba_elsring(phba);
 
 	/* In case of error recovery path, we might have a NULL pring here */
-- 
2.38.0

Re: [PATCH v6 2/8] lpfc: Early return out of lpfc_els_abort when HBA_SETUP flag is not set

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: lpfc: lockless HBA_SETUP check leaves TOCTOU Use-After-Free race on pring
--

commit b6474390ac6cffa345174f9e11e10a15b8a091a7
Author: Nigel Kirkland <nkirkland2304@gmail.com>

lpfc: Early return out of lpfc_els_abort when HBA_SETUP flag is not set

This commit adds an early check on the HBA_SETUP flag in lpfc_els_abort()
to avoid using a stale ELS pring pointer during an HBA reset.

> diff --git a/drivers/scsi/lpfc/lpfc_nportdisc.c b/drivers/scsi/lpfc/lpfc_nportdisc.c
> index 9c449055a55e6..2c8d995a45bf8 100644
> --- a/drivers/scsi/lpfc/lpfc_nportdisc.c
> +++ b/drivers/scsi/lpfc/lpfc_nportdisc.c
> @@ -227,6 +227,11 @@ lpfc_els_abort(struct lpfc_hba *phba, struct lpfc_nodelist *ndlp)
>  	struct lpfc_iocbq *iocb, *next_iocb;
>  	int retval = 0;
>  
> +	/* Exit early to prevent race with queue teardown. */
> +	if (unlikely(phba->sli_rev == LPFC_SLI_REV4 &&
> +		     !test_bit(HBA_SETUP, &phba->hba_flag)))
> +		return;
> +

[Severity: High]
Does this lockless check leave a time-of-check to time-of-use race window
that can still result in a use-after-free on the pring pointer?

If a dev_loss_tmo callback executing lpfc_els_abort() is running
concurrently with an HBA reset, could this sequence occur:

CPU1 (dev_loss_tmo)
lpfc_els_abort()
  test_bit(HBA_SETUP, &phba->hba_flag) // returns true
  pring = lpfc_phba_elsring(phba);

CPU2 (HBA reset)
lpfc_sli_brdrestart_s4()
  clear_bit(HBA_SETUP, &phba->hba_flag);
  lpfc_sli4_queue_unset(phba);
    lpfc_wq_destroy(wq);
      kfree(wq->pring);

CPU1
  spin_lock_irq(&phba->hbalock);
  spin_lock(&pring->ring_lock); // use-after-free on stale pring pointer

Should the check and the subsequent usage of pring be synchronized against
the queue teardown path?

>  	pring = lpfc_phba_elsring(phba);
>  
>  	/* In case of error recovery path, we might have a NULL pring here */

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

[PATCH v6 3/8] lpfc: Fix kernel oops when unmapping scsi dma buffers for an aborted cmd

[Nigel Kirkland]

A kernel oops at dma_unmap_sg_attrs may occur due to a race between an
aborted scsi I/O completion and a scsi error handler issued TUR using the
same repurposed scsi_cmnd structure.

The LPFC_DRIVER_ABORTED cmd_flag is set when inflight I/Os are aborted
via lpfc_sli_abort_taskmgmt, and this flag is not cleared until after
scsi_done is called.  The inflight I/O test is changed to check scsi I/O
for either LFPC_IO_ON_TXCMPLQ or LPFC_DRIVER_ABORTED.  If either cmd_flag
is set, then the I/O should still be counted.

Signed-off-by: Nigel Kirkland <nkirkland2304@gmail.com>

---
 drivers/scsi/lpfc/lpfc_sli.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/lpfc/lpfc_sli.c b/drivers/scsi/lpfc/lpfc_sli.c
index cd285e87c278..b370a64e6b67 100644
--- a/drivers/scsi/lpfc/lpfc_sli.c
+++ b/drivers/scsi/lpfc/lpfc_sli.c
@@ -12730,8 +12730,12 @@ lpfc_sli_sum_iocb(struct lpfc_vport *vport, uint16_t tgt_id, uint64_t lun_id,
 
 		if (!iocbq || iocbq->vport != vport)
 			continue;
-		if (!(iocbq->cmd_flag & LPFC_IO_FCP) ||
-		    !(iocbq->cmd_flag & LPFC_IO_ON_TXCMPLQ))
+		/* Only count FCP i/o */
+		if (!(iocbq->cmd_flag & LPFC_IO_FCP))
+			continue;
+		/* Count i/o whilst LLDD retains an interest in the scsi_cmnd */
+		if (!(iocbq->cmd_flag &
+				(LPFC_IO_ON_TXCMPLQ | LPFC_DRIVER_ABORTED)))
 			continue;
 
 		/* Include counting outstanding aborts */
-- 
2.38.0

Re: [PATCH v6 3/8] lpfc: Fix kernel oops when unmapping scsi dma buffers for an aborted cmd

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: lpfc: incomplete fix leaves DMA unmap oops window open during LUN reset
- [High] scsi: lpfc: data race on cmd_flag can lose LPFC_DRIVER_ABORTED
--

commit 675425a729cb7c6ac7d9c7e57d4ee3019d93591f
Author: Nigel Kirkland <nkirkland2304@gmail.com>

lpfc: Fix kernel oops when unmapping scsi dma buffers for an aborted cmd

This commit attempts to fix a kernel oops at dma_unmap_sg_attrs by ensuring
inflight scsi I/Os are counted if either LPFC_IO_ON_TXCMPLQ or
LPFC_DRIVER_ABORTED is set. This is to keep the LLDD retaining an interest
in the scsi_cmnd and avoid premature unmapping.

> diff --git a/drivers/scsi/lpfc/lpfc_sli.c b/drivers/scsi/lpfc/lpfc_sli.c
> index a167ad96d2a62..7a187d0efaa92 100644
> --- a/drivers/scsi/lpfc/lpfc_sli.c
> +++ b/drivers/scsi/lpfc/lpfc_sli.c
> @@ -12739,8 +12739,12 @@ lpfc_sli_sum_iocb(struct lpfc_vport *vport, uint16_t tgt_id, uint64_t lun_id,
[ ... ]
> -		if (!(iocbq->cmd_flag & LPFC_IO_FCP) ||
> -		    !(iocbq->cmd_flag & LPFC_IO_ON_TXCMPLQ))
> +		/* Only count FCP i/o */
> +		if (!(iocbq->cmd_flag & LPFC_IO_FCP))
> +			continue;
> +		/* Count i/o whilst LLDD retains an interest in the scsi_cmnd */
> +		if (!(iocbq->cmd_flag &
> +				(LPFC_IO_ON_TXCMPLQ | LPFC_DRIVER_ABORTED)))
>  			continue;

[Severity: High]
Does this still leave a window open for the DMA unmap oops during LUN reset?

If a hardware completion arrives just before the SCSI error handler calls
lpfc_abort_handler(), the fast-path IRQ handler lpfc_sli_iocbq_lookup_by_tag()
removes the IOCB from TXCMPLQ and clears LPFC_IO_ON_TXCMPLQ.

Because the command is no longer on the TX queue, lpfc_abort_handler()
skips it and returns FAILED without ever setting LPFC_DRIVER_ABORTED:

drivers/scsi/lpfc/lpfc_scsi.c:lpfc_abort_handler() {
    ...
	/* the command is in process of being cancelled */
	if (!(iocb->cmd_flag & LPFC_IO_ON_TXCMPLQ)) {
		lpfc_printf_vlog(vport, KERN_WARNING, LOG_FCP,
			"3169 SCSI Layer abort requested I/O has been "
			"cancelled by LLD.\n");
		ret = FAILED;
		goto out_unlock_ring;
	}
    ...
}

If the SCSI error handler escalates to lpfc_device_reset_handler(), it
flushes I/Os by calling lpfc_sli_sum_iocb(). Since both LPFC_IO_ON_TXCMPLQ
and LPFC_DRIVER_ABORTED are 0, wouldn't lpfc_sli_sum_iocb() skip the
command and return 0?

This could allow the LUN reset handler to assume no commands are inflight
and finish, allowing the SCSI midlayer to free or repurpose the scsi_cmnd.
Finally, the delayed softirq completion handler lpfc_scsi_cmd_iocb_cmpl()
could execute and call lpfc_scsi_unprep_dma_buf() on the freed scsi_cmnd.


[Severity: High]
Can a data race on cmd_flag cause LPFC_DRIVER_ABORTED to be lost?

The cmd_flag is a 32-bit field updated using non-atomic bitwise operators.
The lpfc_sli_iocbq_lookup_by_tag() function clears LPFC_IO_ON_TXCMPLQ in
the completion handler holding only pring->ring_lock:

drivers/scsi/lpfc/lpfc_sli.c:lpfc_sli_iocbq_lookup_by_tag() {
    ...
			cmd_iocb->cmd_flag &= ~LPFC_IO_ON_TXCMPLQ;
    ...
}

Concurrently, abort handlers like lpfc_sli_issue_abort_iotag() and
lpfc_sli4_issue_abort_iotag() set LPFC_DRIVER_ABORTED holding buf_lock
and hbalock, but explicitly without pring->ring_lock:

drivers/scsi/lpfc/lpfc_sli.c:lpfc_sli4_issue_abort_iotag() {
    ...
	cmdiocb->cmd_flag |= LPFC_DRIVER_ABORTED;
    ...
}

If LPFC_DRIVER_ABORTED is lost due to this race, lpfc_sli_sum_iocb()
could miss the command. This would bypass the logic introduced in this
patch and lead to early unload or unmap while hardware DMA is still active.

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

[PATCH v6 4/8] lpfc: Check fc4_xpt_flags before decrementing ndlp kref on FDISC error

[Nigel Kirkland]

The current initial kref count drop logic for an ndlp that fails FDISC
assumes that the ndlp has never registered with transport layer and thus
the lpfc_dev_loss_tmo_callbk never called.  However, a failed FDISC can
occur after a successful transport layer registration too.  So,
lpfc_dev_loss_tmo_callbk can occur and there is a potential use-after-free
on the ndlp.

Check ndlp->fc4_xpt_flags if previously registered with an upper layer
transport and check ndlp->nlp_flags if there is a LPFC_EVT_DEV_LOSS work
pending.  If not previously registered nor LPFC_EVT_DEV_LOSS work pending,
then set the NLP_DROPPED flag as before and decrement the initial kref on
FDISC error.  However, if ndlp has been previously registered, then let the
pre-existing logic for each transport's respective dev_loss_tmo_callbk
perform the initial kref decrement.

Signed-off-by: Nigel Kirkland <nkirkland2304@gmail.com>

---
 drivers/scsi/lpfc/lpfc_els.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/lpfc/lpfc_els.c b/drivers/scsi/lpfc/lpfc_els.c
index 6f6394a0047c..4918d71ebfe4 100644
--- a/drivers/scsi/lpfc/lpfc_els.c
+++ b/drivers/scsi/lpfc/lpfc_els.c
@@ -11395,6 +11395,7 @@ lpfc_cmpl_els_fdisc(struct lpfc_hba *phba, struct lpfc_iocbq *cmdiocb,
 	struct serv_parm *sp;
 	uint8_t fabric_param_changed;
 	u32 ulp_status, ulp_word4;
+	unsigned long flags;
 
 	ulp_status = get_job_ulpstatus(phba, rspiocb);
 	ulp_word4 = get_job_word4(phba, rspiocb);
@@ -11416,7 +11417,6 @@ lpfc_cmpl_els_fdisc(struct lpfc_hba *phba, struct lpfc_iocbq *cmdiocb,
 		ulp_status, ulp_word4, vport->fc_prevDID);
 
 	if (ulp_status) {
-
 		if (lpfc_fabric_login_reqd(phba, cmdiocb, rspiocb)) {
 			lpfc_retry_pport_discovery(phba);
 			goto out;
@@ -11427,12 +11427,28 @@ lpfc_cmpl_els_fdisc(struct lpfc_hba *phba, struct lpfc_iocbq *cmdiocb,
 			goto out;
 		/* Warn FDISC status */
 		lpfc_vlog_msg(vport, KERN_WARNING, LOG_ELS,
-			      "0126 FDISC cmpl status: x%x/x%x)\n",
-			      ulp_status, ulp_word4);
+			      "0126 FDISC cmpl status: (x%x/x%x) ndlp x%px "
+			      "Data: x%lx x%x x%x x%x x%x x%x x%x x%x x%x\n",
+			      ulp_status, ulp_word4, ndlp, ndlp->nlp_flag,
+			      ndlp->nlp_DID, ndlp->nlp_last_elscmd,
+			      ndlp->nlp_type, ndlp->nlp_rpi, ndlp->nlp_state,
+			      ndlp->nlp_prev_state, ndlp->fc4_xpt_flags,
+			      kref_read(&ndlp->kref));
 
-		/* drop initial reference */
-		if (!test_and_set_bit(NLP_DROPPED, &ndlp->nlp_flag))
+		/* If have not previously registered with transport layer and no
+		 * LPFC_EVT_DEV_LOSS work pending, then drop initial reference.
+		 * Otherwise, let the dev_loss_tmo_callbk drop the initial
+		 * reference.
+		 */
+		spin_lock_irqsave(&ndlp->lock, flags);
+		if (!(ndlp->fc4_xpt_flags & (SCSI_XPT_REGD | NVME_XPT_REGD)) &&
+		    !test_bit(NLP_IN_DEV_LOSS, &ndlp->nlp_flag) &&
+		    !test_and_set_bit(NLP_DROPPED, &ndlp->nlp_flag)) {
+			spin_unlock_irqrestore(&ndlp->lock, flags);
 			lpfc_nlp_put(ndlp);
+		} else {
+			spin_unlock_irqrestore(&ndlp->lock, flags);
+		}
 
 		goto fdisc_failed;
 	}
-- 
2.38.0

[PATCH v6 5/8] lpfc: Rework I/O flush ordering when unloading driver

[Nigel Kirkland]

The lpfc_els_abort routine has a code path that cancels outstanding
I/Os on the ELS ring when attempted aborts fail.  The failed aborts are
queued to a drv_cmpl_list and then cancelled after the ELS pring->txcmplq
is fully traversed.  However if the abort failure returns IOCB_ABORTING,
then the driver should not have cancelled it.  Doing so starts two threads
working on the same iocb and ndlp, leading to unintended race conditions.

Fix by capturing the IOCB_ABORTING return value in lpfc_els_abort and not
adding it to the list of iocbs for cancelling.  We should allow the iocb
scheduled for abort to complete naturally.  This avoids simultaneous
threads acting on the same iocb and ndlp objects.

The lpfc_free_iocb_list is moved to execute after lpfc_sli4_hba_unset
allowing the routine to flush I/O before freeing it.  And, in
lpfc_pci_remove_one_s4 a call to flush the phba->wq is added.  This makes
the unload logic consistent with offline handling logic.

Signed-off-by: Nigel Kirkland <nkirkland2304@gmail.com>

---
 drivers/scsi/lpfc/lpfc_init.c      | 16 ++++++++++++++--
 drivers/scsi/lpfc/lpfc_nportdisc.c | 11 +++++++++--
 2 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/lpfc/lpfc_init.c b/drivers/scsi/lpfc/lpfc_init.c
index 6460127bcc7b..7352cb6e584b 100644
--- a/drivers/scsi/lpfc/lpfc_init.c
+++ b/drivers/scsi/lpfc/lpfc_init.c
@@ -13514,6 +13514,9 @@ lpfc_sli4_hba_unset(struct lpfc_hba *phba)
 	/* Stop the SLI4 device port */
 	if (phba->pport)
 		phba->pport->work_port_events = 0;
+
+	/* All IO completed and queues released. Free the IOCBs. */
+	lpfc_free_iocb_list(phba);
 }
 
 /*
@@ -14948,11 +14951,20 @@ lpfc_pci_remove_one_s4(struct pci_dev *pdev)
 
 	/* Perform scsi free before driver resource_unset since scsi
 	 * buffers are released to their corresponding pools here.
+	 * lpfc_sli4_hba_unset() issues aborts via lpfc_sli_hba_iocb_abort(),
+	 * which allocates abort IOCBs from phba->lpfc_iocb_list; the pool
+	 * must still exist, so lpfc_free_iocb_list() runs only after unset.
 	 */
 	lpfc_io_free(phba);
-	lpfc_free_iocb_list(phba);
-	lpfc_sli4_hba_unset(phba);
 
+	/* Flush the PHBA WQ - there could be a race with ELS IOs while lpfc
+	 * is unloading.  This stops a race between completions, aborts and
+	 * resource recovery.
+	 */
+	if (phba->wq)
+		flush_workqueue(phba->wq);
+
+	lpfc_sli4_hba_unset(phba);
 	lpfc_unset_driver_resource_phase2(phba);
 	lpfc_sli4_driver_resource_unset(phba);
 
diff --git a/drivers/scsi/lpfc/lpfc_nportdisc.c b/drivers/scsi/lpfc/lpfc_nportdisc.c
index 2c8d995a45bf..f917a5bcfd02 100644
--- a/drivers/scsi/lpfc/lpfc_nportdisc.c
+++ b/drivers/scsi/lpfc/lpfc_nportdisc.c
@@ -255,8 +255,9 @@ lpfc_els_abort(struct lpfc_hba *phba, struct lpfc_nodelist *ndlp)
 	spin_lock_irq(&phba->hbalock);
 	if (phba->sli_rev == LPFC_SLI_REV4)
 		spin_lock(&pring->ring_lock);
+
 	list_for_each_entry_safe(iocb, next_iocb, &pring->txcmplq, list) {
-	/* Add to abort_list on on NDLP match. */
+		/* Add to abort_list on NDLP match. */
 		if (lpfc_check_sli_ndlp(phba, pring, iocb, ndlp))
 			list_add_tail(&iocb->dlist, &abort_list);
 	}
@@ -271,7 +272,13 @@ lpfc_els_abort(struct lpfc_hba *phba, struct lpfc_nodelist *ndlp)
 		retval = lpfc_sli_issue_abort_iotag(phba, pring, iocb, NULL);
 		spin_unlock_irq(&phba->hbalock);
 
-		if (retval && test_bit(FC_UNLOADING, &phba->pport->load_flag)) {
+		/* An abort that fails here is just cancelled when the driver is
+		 * going offline.  However, if the abort failure is because the
+		 * IOCB is already getting aborted, don't cancel.  Just let it
+		 * complete.
+		 */
+		if (test_bit(FC_UNLOADING, &phba->pport->load_flag) &&
+		    retval && retval != IOCB_ABORTING) {
 			list_del_init(&iocb->list);
 			list_add_tail(&iocb->list, &drv_cmpl_list);
 		}
-- 
2.38.0

Re: [PATCH v6 5/8] lpfc: Rework I/O flush ordering when unloading driver

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] lpfc: lockless mutation of txcmplq in lpfc_els_abort causes list corruption
- [High] lpfc: Use-After-Free of lpfc_queue in destroy_workqueue during unload
- [High] scsi: lpfc: memory leak / infinite wait for IOCBs on txcmplq during unload
--

commit c519125dd5fca5410ebb69899d1a9f5ce0a92a82
Author: Nigel Kirkland <nkirkland2304@gmail.com>

lpfc: Rework I/O flush ordering when unloading driver

This commit modifies the handling of failed aborts during driver unload
to avoid concurrent threads acting on the same IOCB. It also delays
freeing the IOCB list until after the HBA is unset and flushes the
workqueue during PCI removal.

> diff --git a/drivers/scsi/lpfc/lpfc_init.c b/drivers/scsi/lpfc/lpfc_init.c
> --- a/drivers/scsi/lpfc/lpfc_init.c
> +++ b/drivers/scsi/lpfc/lpfc_init.c
[ ... ]
> @@ -14948,11 +14951,20 @@ lpfc_pci_remove_one_s4(struct pci_dev *pdev)
>  
>  	/* Perform scsi free before driver resource_unset since scsi
>  	 * buffers are released to their corresponding pools here.
> +	 * lpfc_sli4_hba_unset() issues aborts via lpfc_sli_hba_iocb_abort(),
> +	 * which allocates abort IOCBs from phba->lpfc_iocb_list; the pool
> +	 * must still exist, so lpfc_free_iocb_list() runs only after unset.
>  	 */
>  	lpfc_io_free(phba);
> -	lpfc_free_iocb_list(phba);
> -	lpfc_sli4_hba_unset(phba);
>  
> +	/* Flush the PHBA WQ - there could be a race with ELS IOs while lpfc
> +	 * is unloading.  This stops a race between completions, aborts and
> +	 * resource recovery.
> +	 */
> +	if (phba->wq)
> +		flush_workqueue(phba->wq);
> +

[Severity: High]
Could flushing the workqueue here lead to a use-after-free later during
unload?

If lpfc_sli4_hba_unset() issues hardware aborts, completions can still
trigger interrupts that queue new work to phba->wq. Later in the same unset
function, lpfc_sli4_queue_destroy() frees the lpfc_queue structures (which
contain the work_structs) without explicitly canceling the enqueued works.

When lpfc_unset_driver_resource_phase2() eventually calls
destroy_workqueue(phba->wq), it drains the queue and executes those newly
enqueued items on the freed lpfc_queue memory.

> +	lpfc_sli4_hba_unset(phba);
>  	lpfc_unset_driver_resource_phase2(phba);
>  	lpfc_sli4_driver_resource_unset(phba);

[ ... ]
> diff --git a/drivers/scsi/lpfc/lpfc_nportdisc.c b/drivers/scsi/lpfc/lpfc_nportdisc.c
> --- a/drivers/scsi/lpfc/lpfc_nportdisc.c
> +++ b/drivers/scsi/lpfc/lpfc_nportdisc.c
[ ... ]
> @@ -271,7 +272,13 @@ lpfc_els_abort(struct lpfc_hba *phba, struct lpfc_nodelist *ndlp)
>  		retval = lpfc_sli_issue_abort_iotag(phba, pring, iocb, NULL);
>  		spin_unlock_irq(&phba->hbalock);
>  
> -		if (retval && test_bit(FC_UNLOADING, &phba->pport->load_flag)) {
> +		/* An abort that fails here is just cancelled when the driver is
> +		 * going offline.  However, if the abort failure is because the
> +		 * IOCB is already getting aborted, don't cancel.  Just let it
> +		 * complete.
> +		 */
> +		if (test_bit(FC_UNLOADING, &phba->pport->load_flag) &&
> +		    retval && retval != IOCB_ABORTING) {
>  			list_del_init(&iocb->list);

[Severity: High]
Does modifying the IOCB list here happen without the required locks?

The target iocb was found by traversing pring->txcmplq, but the lock
(phba->hbalock) was dropped immediately before this block. Calling
list_del_init(&iocb->list) alters the txcmplq list locklessly, which could
race with concurrent completion handlers or other abort routines traversing
the same list, resulting in list corruption.

[Severity: High]
Also, by intentionally bypassing cancellation for IOCB_ABORTING and leaving
the IOCB on txcmplq, is there a risk of leaking these IOCB structures?

Later in the unload path, lpfc_free_iocb_list() only frees items currently
on the free list (phba->lpfc_iocb_list). Any IOCB stranded on txcmplq would
not be returned to the free list and could be permanently leaked.
Additionally, if NVMe is enabled, could this cause functions like
lpfc_nvme_wait_for_io_drain() to loop infinitely while waiting for txcmplq
to be fully empty?

>  			list_add_tail(&iocb->list, &drv_cmpl_list);
>  		}

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

[PATCH v6 6/8] lpfc: Refactor calls on fc_disctmo to lpfc_set_disctmo in RSCN handler

[Nigel Kirkland]

The lpfc_set_disctmo routine is not used for all cases when the driver
needs to restart discovery on the fc_disctmo timer.  Not doing so, makes
discovery timer actions invisible in some cases as they do not get logged.
This patch substitutes calls on fc_disctmo to use lpfc_set_disctmo in
lpfc_els_rcv_rscn.

Signed-off-by: Nigel Kirkland <nkirkland2304@gmail.com>

---
 drivers/scsi/lpfc/lpfc_els.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/lpfc/lpfc_els.c b/drivers/scsi/lpfc/lpfc_els.c
index 4918d71ebfe4..5691af515b43 100644
--- a/drivers/scsi/lpfc/lpfc_els.c
+++ b/drivers/scsi/lpfc/lpfc_els.c
@@ -8146,7 +8146,7 @@ lpfc_els_rcv_rscn(struct lpfc_vport *vport, struct lpfc_iocbq *cmdiocb,
 	uint32_t payload_len, length, nportid, *cmd;
 	int rscn_cnt;
 	int rscn_id = 0, hba_id = 0;
-	int i, tmo;
+	int i;
 
 	pcmd = cmdiocb->cmd_dmabuf;
 	lp = (uint32_t *) pcmd->virt;
@@ -8225,11 +8225,8 @@ lpfc_els_rcv_rscn(struct lpfc_vport *vport, struct lpfc_iocbq *cmdiocb,
 			lpfc_els_rsp_acc(vport, ELS_CMD_ACC, cmdiocb,
 				ndlp, NULL);
 			/* Restart disctmo if its already running */
-			if (test_bit(FC_DISC_TMO, &vport->fc_flag)) {
-				tmo = ((phba->fc_ratov * 3) + 3);
-				mod_timer(&vport->fc_disctmo,
-					  jiffies + secs_to_jiffies(tmo));
-			}
+			if (test_bit(FC_DISC_TMO, &vport->fc_flag))
+				lpfc_set_disctmo(vport);
 			return 0;
 		}
 	}
@@ -8260,11 +8257,9 @@ lpfc_els_rcv_rscn(struct lpfc_vport *vport, struct lpfc_iocbq *cmdiocb,
 		set_bit(FC_RSCN_DEFERRED, &vport->fc_flag);
 
 		/* Restart disctmo if its already running */
-		if (test_bit(FC_DISC_TMO, &vport->fc_flag)) {
-			tmo = ((phba->fc_ratov * 3) + 3);
-			mod_timer(&vport->fc_disctmo,
-				  jiffies + secs_to_jiffies(tmo));
-		}
+		if (test_bit(FC_DISC_TMO, &vport->fc_flag))
+			lpfc_set_disctmo(vport);
+
 		if ((rscn_cnt < FC_MAX_HOLD_RSCN) &&
 		    !test_bit(FC_RSCN_DISCOVERY, &vport->fc_flag)) {
 			set_bit(FC_RSCN_MODE, &vport->fc_flag);
-- 
2.38.0

[PATCH v6 7/8] lpfc: Update correct ndlp refcnt when rejecting an unsolicited PLOGI

[Nigel Kirkland]

The domain controller FC_ID can change whenever a fabric switch with
an ISL link is replaced or the admin reconfigures the fabric switch
with active links. In such situations, reference count updates should
be applied to the correct nodelist structure.

Signed-off-by: Nigel Kirkland <nkirkland2304@gmail.com>

---
 drivers/scsi/lpfc/lpfc_els.c       | 37 ++++++++++++++++++++++--------
 drivers/scsi/lpfc/lpfc_hbadisc.c   |  7 ++++++
 drivers/scsi/lpfc/lpfc_nportdisc.c | 14 +++++++++++
 3 files changed, 49 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/lpfc/lpfc_els.c b/drivers/scsi/lpfc/lpfc_els.c
index 5691af515b43..0e5038feec9a 100644
--- a/drivers/scsi/lpfc/lpfc_els.c
+++ b/drivers/scsi/lpfc/lpfc_els.c
@@ -5952,11 +5952,12 @@ lpfc_els_rsp_reject(struct lpfc_vport *vport, uint32_t rejectError,
 	/* Xmit ELS RJT <err> response tag <ulpIoTag> */
 	lpfc_printf_vlog(vport, KERN_INFO, LOG_ELS,
 			 "0129 Xmit ELS RJT x%x response tag x%x "
-			 "xri x%x, did x%x, nlp_flag x%lx, nlp_state x%x, "
-			 "rpi x%x\n",
+			 "xri x%x, ndlp x%px, did x%x, nlp_flag x%lx, "
+			 "nlp_state x%x, rpi x%x ref_cnt %d\n",
 			 rejectError, elsiocb->iotag,
-			 get_job_ulpcontext(phba, elsiocb), ndlp->nlp_DID,
-			 ndlp->nlp_flag, ndlp->nlp_state, ndlp->nlp_rpi);
+			 get_job_ulpcontext(phba, elsiocb), ndlp,
+			 ndlp->nlp_DID, ndlp->nlp_flag, ndlp->nlp_state,
+			 ndlp->nlp_rpi, kref_read(&ndlp->kref));
 	lpfc_debugfs_disc_trc(vport, LPFC_DISC_TRC_ELS_RSP,
 		"Issue LS_RJT:    did:x%x flg:x%lx err:x%x",
 		ndlp->nlp_DID, ndlp->nlp_flag, rejectError);
@@ -10457,6 +10458,7 @@ lpfc_els_unsol_buffer(struct lpfc_hba *phba, struct lpfc_sli_ring *pring,
 		      struct lpfc_vport *vport, struct lpfc_iocbq *elsiocb)
 {
 	struct lpfc_nodelist *ndlp;
+	struct lpfc_nodelist *alloc_ndlp = NULL;
 	struct ls_rjt stat;
 	u32 *payload, payload_len;
 	u32 cmd = 0, did = 0, newnode, status = 0;
@@ -10555,7 +10557,9 @@ lpfc_els_unsol_buffer(struct lpfc_hba *phba, struct lpfc_sli_ring *pring,
 			did, vport->port_state, ndlp->nlp_flag);
 
 		phba->fc_stat.elsRcvPLOGI++;
+		alloc_ndlp = ndlp;
 		ndlp = lpfc_plogi_confirm_nport(phba, payload, ndlp);
+
 		if (phba->sli_rev == LPFC_SLI_REV4 &&
 		    test_bit(FC_PT2PT, &phba->pport->fc_flag)) {
 			vport->fc_prevDID = vport->fc_myDID;
@@ -10871,13 +10875,28 @@ lpfc_els_unsol_buffer(struct lpfc_hba *phba, struct lpfc_sli_ring *pring,
 		stat.un.b.lsRjtRsnCodeExp = rjt_exp;
 		lpfc_els_rsp_reject(vport, stat.un.lsRjtError, elsiocb, ndlp,
 				    NULL);
-		/* Remove the reference from above for new nodes. */
-		if (newnode)
-			lpfc_disc_state_machine(vport, ndlp, NULL,
-					NLP_EVT_DEVICE_RM);
+
+		/* Remove the safety reference from routine start. */
+		if (newnode) {
+			/* alloc_ndlp is assigned for an unsolicited PLOGI from
+			 * the ndlp allocated by this routine. The routine
+			 * lpfc_plogi_confirm_nport may have swapped the initial
+			 * ndlp to an existing ndlp. In that case, alloc_ndlp
+			 * is the pre-swap ndlp that needs a DEVICE_RM because
+			 * it is the newnode. For all other paths to lsrjt
+			 * (non-PLOGI ELS) that were rejected by the early
+			 * port_state check, alloc_ndlp is NULL and ndlp is
+			 * still the originally allocated node.
+			 */
+			lpfc_disc_state_machine(vport,
+						alloc_ndlp ? alloc_ndlp : ndlp,
+						NULL, NLP_EVT_DEVICE_RM);
+		}
 	}
 
-	/* Release the reference on this elsiocb, not the ndlp. */
+	/* This elsiocb is not the IOCB issued for a response. Remove
+	 * the safety reference allocated earlier.
+	 */
 	lpfc_nlp_put(elsiocb->ndlp);
 	elsiocb->ndlp = NULL;
 
diff --git a/drivers/scsi/lpfc/lpfc_hbadisc.c b/drivers/scsi/lpfc/lpfc_hbadisc.c
index 4c673dffa671..57cdd669928c 100644
--- a/drivers/scsi/lpfc/lpfc_hbadisc.c
+++ b/drivers/scsi/lpfc/lpfc_hbadisc.c
@@ -4951,6 +4951,13 @@ lpfc_drop_node(struct lpfc_vport *vport, struct lpfc_nodelist *ndlp)
 	 */
 	if (ndlp->nlp_state == NLP_STE_UNUSED_NODE)
 		return;
+
+	lpfc_printf_vlog(vport, KERN_INFO, LOG_NODE | LOG_ELS | LOG_DISCOVERY,
+			"3421 Mark ndlp x%px Dropped. DID x%06x, nflags x%lx "
+			"xflags x%x ref_cnt %d\n",
+			 ndlp, ndlp->nlp_DID, ndlp->nlp_flag,
+			 ndlp->fc4_xpt_flags, kref_read(&ndlp->kref));
+
 	lpfc_nlp_set_state(vport, ndlp, NLP_STE_UNUSED_NODE);
 	if (vport->phba->sli_rev == LPFC_SLI_REV4) {
 		lpfc_cleanup_vports_rrqs(vport, ndlp);
diff --git a/drivers/scsi/lpfc/lpfc_nportdisc.c b/drivers/scsi/lpfc/lpfc_nportdisc.c
index f917a5bcfd02..ffec352c8ab2 100644
--- a/drivers/scsi/lpfc/lpfc_nportdisc.c
+++ b/drivers/scsi/lpfc/lpfc_nportdisc.c
@@ -2878,6 +2878,20 @@ lpfc_device_rm_npr_node(struct lpfc_vport *vport, struct lpfc_nodelist *ndlp,
 		set_bit(NLP_NODEV_REMOVE, &ndlp->nlp_flag);
 		return ndlp->nlp_state;
 	}
+
+	/* It is an error to drop a node while either of its transport
+	 * registrations have not completed.
+	 */
+	if (ndlp->fc4_xpt_flags & (SCSI_XPT_REGD | NVME_XPT_REGD)) {
+		lpfc_printf_vlog(vport, KERN_WARNING,
+				 LOG_ELS | LOG_NODE | LOG_DISCOVERY,
+				 "3423 Not dropping ndlp x%px, DID x%x xflags x%x "
+				 "with dev_loss pending. ref_cnt %d\n",
+				 ndlp, ndlp->nlp_DID, ndlp->fc4_xpt_flags,
+				 kref_read(&ndlp->kref));
+		return ndlp->nlp_state;
+	}
+
 	lpfc_drop_node(vport, ndlp);
 	return NLP_STE_FREED_NODE;
 }
-- 
2.38.0

Re: [PATCH v6 7/8] lpfc: Update correct ndlp refcnt when rejecting an unsolicited PLOGI

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] lpfc: Data race and list corruption in lpfc_findnode_did() due to lock mismatch
- [High] scsi: lpfc: NVMe remote port leak in PLOGI confirm swap
- [Medium] scsi: lpfc: memory leak of alloc_ndlp on PLOGI with existing non-NPR node
--

commit 65d29181555761c7bca4d5992df4a57bb2bdb0d1
Author: Nigel Kirkland <nkirkland2304@gmail.com>

    lpfc: Update correct ndlp refcnt when rejecting an unsolicited PLOGI
    
    The domain controller FC_ID can change whenever a fabric switch with
    an ISL link is replaced or the admin reconfigures the fabric switch
    with active links. In such situations, reference count updates should
    be applied to the correct nodelist structure.

> diff --git a/drivers/scsi/lpfc/lpfc_els.c b/drivers/scsi/lpfc/lpfc_els.c
> index 5691af515b432..0e5038feec9a7 100644
> --- a/drivers/scsi/lpfc/lpfc_els.c
> +++ b/drivers/scsi/lpfc/lpfc_els.c

[ ... ]

> @@ -10457,6 +10458,7 @@ lpfc_els_unsol_buffer(struct lpfc_hba *phba, struct lpfc_sli_ring *pring,
>  		      struct lpfc_vport *vport, struct lpfc_iocbq *elsiocb)
>  {
>  	struct lpfc_nodelist *ndlp;

[Severity: High]
This problem wasn't introduced by this patch, but can this lead to a data race
and list corruption when finding the remote node in lpfc_findnode_did()?

lpfc_findnode_did() protects the traversal of vport->fc_nodes using
shost->host_lock:

drivers/scsi/lpfc/lpfc_hbadisc.c:lpfc_findnode_did() {
	spin_lock_irqsave(&shost->host_lock, iflags);
	ndlp = __lpfc_findnode_did(vport, did);
	spin_unlock_irqrestore(&shost->host_lock, iflags);
}

However, the list is modified in lpfc_enqueue_node() and lpfc_dequeue_node()
using vport->fc_nodes_list_lock instead:

drivers/scsi/lpfc/lpfc_hbadisc.c:lpfc_enqueue_node() {
	spin_lock_irqsave(&vport->fc_nodes_list_lock, iflags);
	list_add_tail(&ndlp->nlp_listp, &vport->fc_nodes);
	spin_unlock_irqrestore(&vport->fc_nodes_list_lock, iflags);
}

Since callers of lpfc_enqueue_node() and lpfc_dequeue_node() do not hold
shost->host_lock, modifying the list concurrently with lpfc_findnode_did()
can corrupt list pointers and trigger a kernel panic during external ELS
command processing.

> +	struct lpfc_nodelist *alloc_ndlp = NULL;
>  	struct ls_rjt stat;
>  	u32 *payload, payload_len;
>  	u32 cmd = 0, did = 0, newnode, status = 0;

[ ... ]

> @@ -10555,7 +10557,9 @@ lpfc_els_unsol_buffer(struct lpfc_hba *phba, struct lpfc_sli_ring *pring,
>  			did, vport->port_state, ndlp->nlp_flag);
>  
>  		phba->fc_stat.elsRcvPLOGI++;
> +		alloc_ndlp = ndlp;
>  		ndlp = lpfc_plogi_confirm_nport(phba, payload, ndlp);

[Severity: High]
Does calling lpfc_plogi_confirm_nport() here leak the NVMe remote port tracking
structure?

When an unsolicited PLOGI arrives from an existing NVMe target or initiator,
the driver creates a new ndlp with a zeroed WWPN, so memcmp against zero
evaluates to true. In lpfc_plogi_confirm_nport(), it finds the existing
new_ndlp and swaps their nrport fields, storing the original in keep_nrport:

	keep_nrport = new_ndlp->nrport;
	new_ndlp->nrport = ndlp->nrport;

	/* Move this back to NPR state */
	if (memcmp(&ndlp->nlp_portname, name, sizeof(struct lpfc_name)) == 0) {
		/* The ndlp doesn't have a portname yet... */

In this branch, keep_nrport is bypassed. It is never restored to new_ndlp or
ndlp, nor is it freed, permanently leaking the pointer and leaving the NVMe
transport with orphaned state that can hang recovery or shutdown.

> +
>  		if (phba->sli_rev == LPFC_SLI_REV4 &&
>  		    test_bit(FC_PT2PT, &phba->pport->fc_flag)) {
>  			vport->fc_prevDID = vport->fc_myDID;

[ ... ]

> @@ -10871,13 +10875,28 @@ lpfc_els_unsol_buffer(struct lpfc_hba *phba, struct lpfc_sli_ring *pring,
>  		stat.un.b.lsRjtRsnCodeExp = rjt_exp;
>  		lpfc_els_rsp_reject(vport, stat.un.lsRjtError, elsiocb, ndlp,
>  				    NULL);
> -		/* Remove the reference from above for new nodes. */
> -		if (newnode)
> -			lpfc_disc_state_machine(vport, ndlp, NULL,
> -					NLP_EVT_DEVICE_RM);
> +
> +		/* Remove the safety reference from routine start. */
> +		if (newnode) {
> +			/* alloc_ndlp is assigned for an unsolicited PLOGI from
> +			 * the ndlp allocated by this routine. The routine
> +			 * lpfc_plogi_confirm_nport may have swapped the initial
> +			 * ndlp to an existing ndlp. In that case, alloc_ndlp
> +			 * is the pre-swap ndlp that needs a DEVICE_RM because
> +			 * it is the newnode. For all other paths to lsrjt
> +			 * (non-PLOGI ELS) that were rejected by the early
> +			 * port_state check, alloc_ndlp is NULL and ndlp is
> +			 * still the originally allocated node.
> +			 */
> +			lpfc_disc_state_machine(vport,
> +						alloc_ndlp ? alloc_ndlp : ndlp,
> +						NULL, NLP_EVT_DEVICE_RM);
> +		}
>  	}
>  
> -	/* Release the reference on this elsiocb, not the ndlp. */
> +	/* This elsiocb is not the IOCB issued for a response. Remove
> +	 * the safety reference allocated earlier.
> +	 */
>  	lpfc_nlp_put(elsiocb->ndlp);
>  	elsiocb->ndlp = NULL;

[Severity: Medium]
Does this bypass the node drop and leak alloc_ndlp when the existing node is
not in NLP_STE_NPR_NODE?

When lpfc_plogi_confirm_nport() finds an existing node, alloc_ndlp takes the
pre-swap state of the existing node (e.g., NLP_STE_MAPPED_NODE). The inline
cleanup check at the end of lpfc_plogi_confirm_nport() evaluates to false
because the state is no longer NPR_NODE, bypassing the NLP_EVT_DEVICE_RM
event.

On the success path (rjt_err == 0), the routine exits after dropping the safety
reference here, leaving the un-dropped initial reference of alloc_ndlp leaked,
accumulating as a garbage node on the vport->fc_nodes list. Furthermore, on
the reject path, this patch calls NLP_EVT_DEVICE_RM on alloc_ndlp, but since
its state is no longer NPR_NODE, the event maps to lpfc_disc_illegal (a no-op)
and the node still leaks.

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

[PATCH v6 8/8] lpfc: Update lpfc version to 15.0.0.1

[Nigel Kirkland]

Update lpfc version to 15.0.0.1

Signed-off-by: Nigel Kirkland <nkirkland2304@gmail.com>
---
 drivers/scsi/lpfc/lpfc_version.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/lpfc/lpfc_version.h b/drivers/scsi/lpfc/lpfc_version.h
index d6e6e436fbfc..7df63d118234 100644
--- a/drivers/scsi/lpfc/lpfc_version.h
+++ b/drivers/scsi/lpfc/lpfc_version.h
@@ -20,7 +20,7 @@
  * included with this package.                                     *
  *******************************************************************/
 
-#define LPFC_DRIVER_VERSION "15.0.0.0"
+#define LPFC_DRIVER_VERSION "15.0.0.1"
 #define LPFC_DRIVER_NAME		"lpfc"
 
 /* Used for SLI 2/3 */
-- 
2.38.0