[RFC PATCH 2/3] scsi: lpfc: Resolve synchronous mailbox wait ownership under hbalock

[Artem Dinaburg <artem@trailofbits.com>]

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.

Fixes: 858c9f6c19c6 ("[SCSI] lpfc: bug fixes")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Artem Dinaburg <artem@trailofbits.com>
---
 drivers/scsi/lpfc/lpfc_sli.c | 29 +++++++++++++----------------
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/drivers/scsi/lpfc/lpfc_sli.c b/drivers/scsi/lpfc/lpfc_sli.c
index cfa437116..c2086f331 100644
--- a/drivers/scsi/lpfc/lpfc_sli.c
+++ b/drivers/scsi/lpfc/lpfc_sli.c
@@ -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