Skip to contentSkip to conversation

lpfc: Update lpfc to revision 15.0.0.1

PATCH Applied Nigel Kirkland 3 participants 17 messages Oct 12 – Oct 24

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

v6 cover lpfc: Update lpfc to revision 15.0.0.1

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(-)
Signature · 1 line
2.38.0
#1 lore raw

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

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.

---
 drivers/scsi/lpfc/lpfc_ct.c | 2 --
 1 file changed, 2 deletions(-)
drivers/scsi/lpfc/lpfc_ct.c +0 -2
index 0734ab3be3e3..f709e5087577 100644
@@ -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)
Signature · 1 line
2.38.0
#2 lore raw

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

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 --

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 <[email protected]>

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.

drivers/scsi/lpfc/lpfc_ct.c lpfc_cmpl_ct_cmd_vmid(struct lpfc_hba *phba, struct lpfc_iocbq *cmdiocb, · line 3581

Quoted 13 lines
drivers/scsi/lpfc/lpfc_ct.c +0 -2
index 0734ab3be3e30..f709e50875770 100644
@@ -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?

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
#3 lore raw

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

In reply to 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.

---
 drivers/scsi/lpfc/lpfc_nportdisc.c | 5 +++++
 1 file changed, 5 insertions(+)
drivers/scsi/lpfc/lpfc_nportdisc.c +5 -0
index 9c449055a55e..2c8d995a45bf 100644
@@ -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 */
Signature · 1 line
2.38.0
#4 lore raw

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

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 --

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 <[email protected]>

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.

drivers/scsi/lpfc/lpfc_nportdisc.c lpfc_els_abort(struct lpfc_hba *phba, struct lpfc_nodelist *ndlp) · line 234

Quoted 13 lines
drivers/scsi/lpfc/lpfc_nportdisc.c +5 -0
index 9c449055a55e6..2c8d995a45bf8 100644
@@ -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?

Quoted 3 lines
drivers/scsi/lpfc/lpfc_nportdisc.c +0 -0
 	pring = lpfc_phba_elsring(phba);
 
 	/* In case of error recovery path, we might have a NULL pring here */
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
#5 lore raw

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

In reply to 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.

---
 drivers/scsi/lpfc/lpfc_sli.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/scsi/lpfc/lpfc_sli.c +6 -2
index cd285e87c278..b370a64e6b67 100644
@@ -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 */
Signature · 1 line
2.38.0
#6 lore raw

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

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…

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 <[email protected]>

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.

Quoted 5 lines
drivers/scsi/lpfc/lpfc_sli.c +0 -0
index a167ad96d2a62..7a187d0efaa92 100644
@@ -12739,8 +12739,12 @@ lpfc_sli_sum_iocb(struct lpfc_vport *vport, uint16_t tgt_id, uint64_t lun_id,

[ ... ]

drivers/scsi/lpfc/lpfc_sli.c lpfc_sli_sum_iocb(struct lpfc_vport *vport, uint16_t tgt_id, uint64_t lun_id, · line 12739

Quoted 9 lines
drivers/scsi/lpfc/lpfc_sli.c +6 -2
-		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.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
#7 lore raw

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

In reply to 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.

---
 drivers/scsi/lpfc/lpfc_els.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)
drivers/scsi/lpfc/lpfc_els.c +21 -5
index 6f6394a0047c..4918d71ebfe4 100644
@@ -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;
 	}
Signature · 1 line
2.38.0
#8 lore raw

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

In reply to 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.

---
 drivers/scsi/lpfc/lpfc_init.c      | 16 ++++++++++++++--
 drivers/scsi/lpfc/lpfc_nportdisc.c | 11 +++++++++--
 2 files changed, 23 insertions(+), 4 deletions(-)
drivers/scsi/lpfc/lpfc_init.c +14 -2
index 6460127bcc7b..7352cb6e584b 100644
@@ -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);
 
drivers/scsi/lpfc/lpfc_nportdisc.c +9 -2
index 2c8d995a45bf..f917a5bcfd02 100644
@@ -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);
 		}
Signature · 1 line
2.38.0
#9 lore raw

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

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…

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 <[email protected]>

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.

Quoted 3 lines
drivers/scsi/lpfc/lpfc_init.c +0 -0

[ ... ]

Quoted 9 lines
drivers/scsi/lpfc/lpfc_init.c +3 -0
@@ -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);

drivers/scsi/lpfc/lpfc_init.c lpfc_pci_remove_one_s4(struct pci_dev *pdev) · line 14966

Quoted 10 lines
drivers/scsi/lpfc/lpfc_init.c +7 -2
-	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.

Quoted 3 lines
drivers/scsi/lpfc/lpfc_init.c +1 -0
+	lpfc_sli4_hba_unset(phba);
 	lpfc_unset_driver_resource_phase2(phba);
 	lpfc_sli4_driver_resource_unset(phba);

[ ... ]

Quoted 3 lines
drivers/scsi/lpfc/lpfc_nportdisc.c +0 -0

[ ... ]

drivers/scsi/lpfc/lpfc_nportdisc.c lpfc_els_abort(struct lpfc_hba *phba, struct lpfc_nodelist *ndlp) · line 282

Quoted 13 lines
drivers/scsi/lpfc/lpfc_nportdisc.c +7 -1
@@ -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?

Quoted 2 lines
drivers/scsi/lpfc/lpfc_nportdisc.c +0 -0
 			list_add_tail(&iocb->list, &drv_cmpl_list);
 		}
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5
#10 lore raw

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

In reply to 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.

---
 drivers/scsi/lpfc/lpfc_els.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)
drivers/scsi/lpfc/lpfc_els.c +6 -11
index 4918d71ebfe4..5691af515b43 100644
@@ -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);
Signature · 1 line
2.38.0
#11 lore raw

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

In reply to 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.

---
 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(-)
drivers/scsi/lpfc/lpfc_els.c +28 -9
index 5691af515b43..0e5038feec9a 100644
@@ -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;
 
drivers/scsi/lpfc/lpfc_hbadisc.c +7 -0
index 4c673dffa671..57cdd669928c 100644
@@ -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);
drivers/scsi/lpfc/lpfc_nportdisc.c +14 -0
index f917a5bcfd02..ffec352c8ab2 100644
@@ -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;
 }
Signature · 1 line
2.38.0
#12 lore raw

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

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…

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 <[email protected]>

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.

Quoted 4 lines
drivers/scsi/lpfc/lpfc_els.c +0 -0
index 5691af515b432..0e5038feec9a7 100644

[ ... ]

drivers/scsi/lpfc/lpfc_els.c lpfc_els_unsol_buffer(struct lpfc_hba *phba, struct lpfc_sli_ring *pring, · line 10460

Quoted 4 lines
drivers/scsi/lpfc/lpfc_els.c +0 -0
@@ -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.

Quoted 4 lines
drivers/scsi/lpfc/lpfc_els.c +1 -0
+	struct lpfc_nodelist *alloc_ndlp = NULL;
 	struct ls_rjt stat;
 	u32 *payload, payload_len;
 	u32 cmd = 0, did = 0, newnode, status = 0;

[ ... ]

drivers/scsi/lpfc/lpfc_els.c lpfc_els_unsol_buffer(struct lpfc_hba *phba, struct lpfc_sli_ring *pring, · line 10561

Quoted 6 lines
drivers/scsi/lpfc/lpfc_els.c +1 -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.

Quoted 4 lines
drivers/scsi/lpfc/lpfc_els.c +1 -0
+
 		if (phba->sli_rev == LPFC_SLI_REV4 &&
 		    test_bit(FC_PT2PT, &phba->pport->fc_flag)) {
 			vport->fc_prevDID = vport->fc_myDID;

[ ... ]

Quoted 23 lines
drivers/scsi/lpfc/lpfc_els.c +15 -4
@@ -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,

drivers/scsi/lpfc/lpfc_els.c lpfc_els_unsol_buffer(struct lpfc_hba *phba, struct lpfc_sli_ring *pring, · line 10901

Quoted 10 lines
drivers/scsi/lpfc/lpfc_els.c +5 -1
+						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.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7
#13 lore raw

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

In reply to Nigel Kirkland

Update lpfc version to 15.0.0.1

---
 drivers/scsi/lpfc/lpfc_version.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/scsi/lpfc/lpfc_version.h +1 -1
index d6e6e436fbfc..7df63d118234 100644
@@ -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 */
Signature · 1 line
2.38.0
#14 lore raw
In reply to Nigel Kirkland
Quoted 5 lines · Nigel Kirkland

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

Regarding sashiko-bot findings for v6:
- 1/8, The finding appears to be a false positive. There is no path that makes a request with command SLI_CTAS_DALLAPP_ID, so the command will not be seen in the completion path. We plan to remove the dead code in a future patch set.
- Our analysis is that the 2/8, 3/8, 5/8 and 7/8 findings are
actually preexisting
flaws. In fact many were called out as such in the v3 patch set.
We note many
of the findings were not reported against intervening patch sets. Our plan is to address these in a future patch set.

This patch set contains important fixes that address real world crashes. In some cases the window is significantly narrowed, but a theoretical small risk may remain.
The changes have undergone many months of extensive testing without issue.

Quoted 39 lines

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

maintainer Applied
#15 lore raw
In reply to Nigel Kirkland

Nigel,

Quoted 3 lines

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

Applied to 7.4/scsi-staging, thanks!

Signature · 1 line
Martin K. Petersen
maintainer Applied
#16 lore raw
In reply to Nigel Kirkland
Quoted 18 lines · 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

Quoted 1 line

[...]

Applied to 7.4/scsi-queue, thanks!

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

      https://git.kernel.org/mkp/scsi/c/aef36d2e16f4
[2/8] lpfc: Early return out of lpfc_els_abort when HBA_SETUP flag is not set
      https://git.kernel.org/mkp/scsi/c/86866706d0a9
[3/8] lpfc: Fix kernel oops when unmapping scsi dma buffers for an aborted cmd
      https://git.kernel.org/mkp/scsi/c/e7e065ec672a
[4/8] lpfc: Check fc4_xpt_flags before decrementing ndlp kref on FDISC error
      https://git.kernel.org/mkp/scsi/c/57b0a51937ef
[5/8] lpfc: Rework I/O flush ordering when unloading driver
      https://git.kernel.org/mkp/scsi/c/5750b941ac75
[6/8] lpfc: Refactor calls on fc_disctmo to lpfc_set_disctmo in RSCN handler
      https://git.kernel.org/mkp/scsi/c/75ae9d50b843
[7/8] lpfc: Update correct ndlp refcnt when rejecting an unsolicited PLOGI
      https://git.kernel.org/mkp/scsi/c/8dbdaf88f4b9
[8/8] lpfc: Update lpfc version to 15.0.0.1
      https://git.kernel.org/mkp/scsi/c/3aeac6a2c2ef
Signature · 1 line
Martin K. Petersen