scsi: lpfc: Fix mailbox timeout ownership races

Hi Justin and Paul,

While preparing a 6.6.y backport of
commit ede596b1434b ("scsi: lpfc: Handle mailbox timeouts in lpfc_get_sfp_info"), I may have found two ownership problems in the synchronous mailbox timeout handoff. I am attaching patches just in case; I do not have the correct hardware (nor can I emulate it in QEMU) to validate the issue is actually reachable.

These patches were developed with AI assistance, and each carries the required attribution trailer.

Patch 1 fixes lpfc_get_sfp_info_wait(). After MBX_TIMEOUT it no longer reads the mailbox, which the late completion may already have freed, and it cleans up after any other failed issue instead of leaking the mailbox or reporting a zeroed A2 page as success. It is correct on its own with the current wait/wake code.

Patch 2 fixes the generic wait/wake handoff. A completion that loaded the wake callback before the waiter timed out finds no waiter and leaks the mailbox. Because the wake flag is set before hbalock is taken, a waiter that times out in that window can also return success and free the mailbox before the callback reads it. The wake callback now decides ownership under hbalock and runs the default completion itself when the waiter is gone.

Patch 3 adds a hardware-independent KUnit suite for the wait/wake paths and for lpfc_get_sfp_info_wait() itself. The issue failure cases fail without patch 1 and the stale-callback cases fail without patch 2. The tests check mailbox ownership only, not an SFP transaction or a timeout on an adapter.

I built lpfc from x86_64 allmodconfig with CONFIG_SCSI_LPFC=m and CONFIG_WERROR=y after each patch, and ran the KUnit suite in x86_64 QEMU with and without KASAN. KASAN only covers the paths the suite runs. I do not have LPFC hardware, so I have not exercised the SFP transaction on an adapter.

Patch 2 changes the completion path for every lpfc_sli_issue_mbox_wait() caller. I am not sure if this is the correct approach or if there should be a different fix.

Thanks,
Artem Dinaburg

Artem Dinaburg (3):
scsi: lpfc: Do not touch the SFP mailbox after a wait timeout
scsi: lpfc: Resolve synchronous mailbox wait ownership under hbalock
scsi: lpfc: Add KUnit tests for mailbox wait ownership

 drivers/scsi/Kconfig                 |  16 ++
 drivers/scsi/lpfc/.kunitconfig       |   9 +
 drivers/scsi/lpfc/Makefile           |   2 +
 drivers/scsi/lpfc/lpfc_els.c         |  12 +-
 drivers/scsi/lpfc/lpfc_sli.c         |  29 +--
 drivers/scsi/lpfc/tests/mbox_kunit.c | 362 +++++++++++++++++++++++++++
 6 files changed, 410 insertions(+), 20 deletions(-)
 create mode 100644 drivers/scsi/lpfc/.kunitconfig
 create mode 100644 drivers/scsi/lpfc/tests/mbox_kunit.c

base-commit: 5e0f8396d4805a3e7f753fa58c8c55f1f3cc2160

2.43.0

Commit ede596b1434b ("scsi: lpfc: Handle mailbox timeouts in lpfc_get_sfp_info") made the mailbox cleanup in lpfc_get_sfp_info_wait() conditional on LPFC_MBX_WAKE, so that a timed-out mailbox is left for its late completion.

Once lpfc_sli_issue_mbox_wait() returns MBX_TIMEOUT, the mailbox belongs to the late completion, whose default handler frees it together with its DMA buffer. Reading mbox_flag at the error label after a timeout is therefore a use-after-free if the completion has already run.

The LPFC_MBX_WAKE test is also wrong when issuing fails immediately. lpfc_sli_issue_mbox_wait() clears the flag before issuing, so the caller still owns the mailbox but skips the cleanup, leaking the mailbox and its DMA buffer. The A2 page dump does not check for an issue failure at all, so it can report success with a zeroed page.

Never touch the mailbox after MBX_TIMEOUT, and treat every other non-success return from either dump as a failure that the caller cleans up.

---
 drivers/scsi/lpfc/lpfc_els.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

index 52fc50589..ea8835515 100644
@@ -7410,12 +7410,12 @@ int lpfc_get_sfp_info_wait(struct lpfc_hba *phba,
 	}
 	mbox->vport = phba->pport;
 	rc = lpfc_sli_issue_mbox_wait(phba, mbox, LPFC_MBOX_SLI4_CONFIG_TMO);
-	if (rc == MBX_NOT_FINISHED) {
+	if (rc == MBX_TIMEOUT)
+		goto error;
+	if (rc != MBX_SUCCESS) {
 		rc = 1;
 		goto error;
 	}
-	if (rc == MBX_TIMEOUT)
-		goto error;
 	if (phba->sli_rev == LPFC_SLI_REV4)
 		mp = mbox->ctx_buf;
 	else

@@ -7472,6 +7472,10 @@ int lpfc_get_sfp_info_wait(struct lpfc_hba *phba,
 
 	if (rc == MBX_TIMEOUT)
 		goto error;
+	if (rc != MBX_SUCCESS) {
+		rc = 1;
+		goto error;
+	}
 	if (bf_get(lpfc_mqe_status, &mbox->u.mqe)) {
 		rc = 1;
 		goto error;

@@ -7482,7 +7486,7 @@ int lpfc_get_sfp_info_wait(struct lpfc_hba *phba,
 			     DMP_SFF_PAGE_A2_SIZE);
 
 error:
-	if (mbox->mbox_flag & LPFC_MBX_WAKE) {
+	if (rc != MBX_TIMEOUT) {
 		mbox->ctx_buf = mpsave;
 		lpfc_mbox_rsrc_cleanup(phba, mbox, MBOX_THD_UNLOCKED);
 	}

2.43.0

lpfc_sli_issue_mbox_wait() detaches the waiter under hbalock and, on timeout, switches mbox_cmpl to lpfc_sli_def_mbox_cmpl() so that a late completion frees the mailbox. The completion side does not make its decision under the same lock. lpfc_sli_wake_mbox_wait() sets LPFC_MBX_WAKE before taking hbalock, and the mailbox worker and the flush path load mbox_cmpl without hbalock (lpfc_sli_handle_mb_event(), lpfc_sli_mbox_sys_flush()). A completion that loaded the wake callback before the waiter timed out then finds no waiter, wakes nobody and leaks the mailbox. Because LPFC_MBX_WAKE is set before hbalock is taken, a waiter that times out in that window also returns MBX_SUCCESS, and its caller can free the mailbox before the wake handler reads it. This affects every caller of lpfc_sli_issue_mbox_wait().

Decide ownership in lpfc_sli_wake_mbox_wait() under hbalock. If a waiter is still attached, set LPFC_MBX_WAKE and complete it. Otherwise run lpfc_sli_def_mbox_cmpl() after dropping the lock.

The wake callback now handles the detached case itself, so the timeout path no longer switches mbox_cmpl, and the waiter no longer writes it while the completion side may be reading it without the lock.

---
 drivers/scsi/lpfc/lpfc_sli.c | 29 +++++++++++++----------------
 1 file changed, 13 insertions(+), 16 deletions(-)

index cfa437116..c2086f331 100644
@@ -2814,9 +2814,9 @@ lpfc_sli_chk_mbx_command(uint8_t mbxCommand)
  *
  * This is completion handler function for mailbox commands issued from
  * lpfc_sli_issue_mbox_wait function. This function is called by the
- * mailbox event handler function with no lock held. This function
- * will wake up thread waiting on the wait queue pointed by context1
- * of the mailbox.
+ * mailbox event handler function with no lock held. If the waiter is
+ * still attached, wake it; otherwise the caller timed out and the
+ * callback owns the mailbox resources.
  **/
 void
 lpfc_sli_wake_mbox_wait(struct lpfc_hba *phba, LPFC_MBOXQ_t *pmboxq)

@@ -2824,17 +2824,18 @@ lpfc_sli_wake_mbox_wait(struct lpfc_hba *phba, LPFC_MBOXQ_t *pmboxq)
 	unsigned long drvr_flag;
 	struct completion *pmbox_done;
 
-	/*
-	 * If pmbox_done is empty, the driver thread gave up waiting and
-	 * continued running.
-	 */
-	pmboxq->mbox_flag |= LPFC_MBX_WAKE;
+	/* Decide who owns the mailbox under the same lock as waiter detach. */
 	spin_lock_irqsave(&phba->hbalock, drvr_flag);
 	pmbox_done = pmboxq->ctx_u.mbox_wait;
-	if (pmbox_done)
-		complete(pmbox_done);
+	if (!pmbox_done) {
+		spin_unlock_irqrestore(&phba->hbalock, drvr_flag);
+		lpfc_sli_def_mbox_cmpl(phba, pmboxq);
+		return;
+	}
+
+	pmboxq->mbox_flag |= LPFC_MBX_WAKE;
+	complete(pmbox_done);
 	spin_unlock_irqrestore(&phba->hbalock, drvr_flag);
-	return;
 }
 
 /**

@@ -13313,15 +13314,11 @@ lpfc_sli_issue_mbox_wait(struct lpfc_hba *phba, LPFC_MBOXQ_t *pmboxq,
 
 		spin_lock_irqsave(&phba->hbalock, flag);
 		pmboxq->ctx_u.mbox_wait = NULL;
-		/*
-		 * if LPFC_MBX_WAKE flag is set the mailbox is completed
-		 * else do not free the resources.
-		 */
+		/* The callback owns cleanup if it has not woken the waiter. */
 		if (pmboxq->mbox_flag & LPFC_MBX_WAKE) {
 			retval = MBX_SUCCESS;
 		} else {
 			retval = MBX_TIMEOUT;
-			pmboxq->mbox_cmpl = lpfc_sli_def_mbox_cmpl;
 		}
 		spin_unlock_irqrestore(&phba->hbalock, flag);
 	}

2.43.0

Exercise synchronous mailbox ownership without LPFC hardware by replacing the mailbox issue callback and tracking one mailbox through a one-element mempool.

Cover completion before timeout with both accepted issue returns (MBX_SUCCESS and MBX_BUSY), normal late completion, immediate submission failure, and the stale-callback interleaving where the completion dispatcher saved the wake callback before the waiter timed out. The stale-callback test fails without the wake handler change because the mailbox is not returned to the pool.

Also run lpfc_get_sfp_info_wait() on SLI-4 against a KUnit device with a real DMA pool, counting released mbufs through the mbuf safety pool. Cover a successful A0/A2 dump, an issue failure on either page, and a timeout on the A0 page followed by the current or a stale late completion. The issue failure cases fail without the SFP fix: the mailbox and its mbuf are leaked, and an A2 issue failure is reported as success.

The fake issue callback never starts a command, so these tests check mailbox ownership, not an SFP transaction or a timeout on an adapter.

---
 drivers/scsi/Kconfig                 |  16 ++
 drivers/scsi/lpfc/.kunitconfig       |   9 +
 drivers/scsi/lpfc/Makefile           |   2 +
 drivers/scsi/lpfc/tests/mbox_kunit.c | 362 +++++++++++++++++++++++++++
 4 files changed, 389 insertions(+)
 create mode 100644 drivers/scsi/lpfc/.kunitconfig
 create mode 100644 drivers/scsi/lpfc/tests/mbox_kunit.c

index 1eec66195..72f2a9da3 100644
@@ -1163,6 +1163,22 @@ config SCSI_LPFC_DEBUG_FS
 	  This makes debugging information from the lpfc driver
 	  available via the debugfs filesystem.
 
+config LPFC_MBOX_KUNIT_TEST
+	bool "KUnit tests for LPFC mailbox ownership" if !KUNIT_ALL_TESTS
+	depends on KUNIT=y && SCSI_LPFC=y
+	default KUNIT_ALL_TESTS
+	help
+	  Build KUnit tests for synchronous LPFC mailbox wait and completion
+	  ownership, including the SFP page dump. The tests use a fake issue
+	  callback and a one-element memory pool, so they do not require an
+	  LPFC adapter. They are built into LPFC to exercise its non-exported
+	  mailbox callbacks.
+
+	  For more information on KUnit and unit tests in general, please refer
+	  to the KUnit documentation in Documentation/dev-tools/kunit/.
+
+	  If unsure, say N.
+
 source "drivers/scsi/elx/Kconfig"
 
 config SCSI_SIM710

new file mode 100644
index 000000000..3fa4ad272
@@ -0,0 +1,9 @@
+CONFIG_KUNIT=y
+CONFIG_NET=y
+CONFIG_PCI=y
+CONFIG_SCSI=y
+CONFIG_SCSI_LOWLEVEL=y
+CONFIG_SCSI_FC_ATTRS=y
+CONFIG_CPU_FREQ=y
+CONFIG_SCSI_LPFC=y
+CONFIG_LPFC_MBOX_KUNIT_TEST=y

index bbd1faf41..d2b1f0990 100644
@@ -34,3 +34,5 @@ lpfc-objs := lpfc_mem.o lpfc_sli.o lpfc_ct.o lpfc_els.o \
 	lpfc_hbadisc.o	lpfc_init.o lpfc_mbox.o lpfc_nportdisc.o   \
 	lpfc_scsi.o lpfc_attr.o lpfc_vport.o lpfc_debugfs.o lpfc_bsg.o \
 	lpfc_nvme.o lpfc_nvmet.o lpfc_vmid.o
+
+lpfc-$(CONFIG_LPFC_MBOX_KUNIT_TEST) += tests/mbox_kunit.o

2.43.0