[PATCH v2] scsi: lpfc: drain all CQ work before releasing queue dependencies

[PATCH v2] scsi: lpfc: drain all CQ work before releasing queue dependencies

[Hongyan Xu]

A delayed CQ handler can remain on a timer after an HBA workqueue flush.
Disabling it only in the final per-queue free also misses peer resources:
release_hdwq() frees EQs before the ELS, NVMe LS and mailbox CQs, while a
slow CQ callback still updates its associated EQ even for an empty CQ.

Disable both delayed works for every published CQ before releasing any
queues. Once all CQ timers are disabled, flush the HBA workqueue so direct
CQ handlers have completed and cannot rearm a delayed handler. Only then
release fast queues, EQs and the remaining slow queues. Keep the existing
poll-list cleanup ahead of the drain and retain the caller's interrupt
and HBA shutdown requirements. In PCI error recovery, join the registered
interrupt handlers before queue destruction rather than afterwards.

Keep synchronous per-queue disabling for allocation-error cleanup and
initialize both delayed work items before queue allocation can fail.
The global traversal uses the same existing CQ inventory that teardown
releases and accepts NULL entries from partial setup. No new global queue
list or lifetime lock is introduced.

Found by the author's in-house static analysis tool.

Fixes: 32517fc0975b ("scsi: lpfc: Rework EQ/CQ processing to address interrupt coalescing")
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/all/20260928134154.1595-1-getshell@seu.edu.cn/
Signed-off-by: Hongyan Xu <getshell@seu.edu.cn>
Signed-off-by: Zehao Gao <gaozehao@iie.ac.cn>
---

Discovery: found by the author's in-house static analysis tool.

 drivers/scsi/lpfc/lpfc_init.c | 36 +++++++++++++++++++++++++++++++++--
 drivers/scsi/lpfc/lpfc_sli.c  |  7 +++++--
 2 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/lpfc/lpfc_init.c b/drivers/scsi/lpfc/lpfc_init.c
index 6df5f4e..0724dd8 100644
--- a/drivers/scsi/lpfc/lpfc_init.c
+++ b/drivers/scsi/lpfc/lpfc_init.c
@@ -10808,6 +10808,37 @@ lpfc_sli4_release_queues(struct lpfc_queue ***qs, int max)
 	*qs = NULL;
 }
 
+static void lpfc_sli4_disable_cq_work(struct lpfc_queue *cq)
+{
+	if (!cq)
+		return;
+
+	disable_delayed_work_sync(&cq->sched_irqwork);
+	disable_delayed_work_sync(&cq->sched_spwork);
+}
+
+static void lpfc_sli4_drain_cq_work(struct lpfc_hba *phba)
+{
+	struct lpfc_sli4_hba *sli4 = &phba->sli4_hba;
+	unsigned int i;
+
+	if (sli4->hdwq)
+		for (i = 0; i < phba->cfg_hdw_queue; i++)
+			lpfc_sli4_disable_cq_work(sli4->hdwq[i].io_cq);
+
+	if (sli4->nvmet_cqset)
+		for (i = 0; i < phba->cfg_nvmet_mrq; i++)
+			lpfc_sli4_disable_cq_work(sli4->nvmet_cqset[i]);
+
+	lpfc_sli4_disable_cq_work(sli4->els_cq);
+	lpfc_sli4_disable_cq_work(sli4->nvmels_cq);
+	lpfc_sli4_disable_cq_work(sli4->mbx_cq);
+
+	/* Direct CQ work cannot rearm the disabled delayed work. */
+	if (phba->wq)
+		flush_workqueue(phba->wq);
+}
+
 static inline void
 lpfc_sli4_release_hdwq(struct lpfc_hba *phba)
 {
@@ -10868,6 +10899,7 @@ lpfc_sli4_queue_destroy(struct lpfc_hba *phba)
 	spin_unlock_irq(&phba->hbalock);
 
 	lpfc_sli4_cleanup_poll_list(phba);
+	lpfc_sli4_drain_cq_work(phba);
 
 	/* Release HBA eqs */
 	if (phba->sli4_hba.hdwq)
@@ -15130,9 +15162,9 @@ lpfc_sli4_prep_dev_for_reset(struct lpfc_hba *phba)
 	/* stop all timers */
 	lpfc_stop_hba_timers(phba);
 
-	lpfc_sli4_queue_destroy(phba);
-	/* Disable interrupt and pci device */
+	/* Join interrupt handlers before releasing their queues. */
 	lpfc_sli4_disable_intr(phba);
+	lpfc_sli4_queue_destroy(phba);
 	pci_disable_device(phba->pcidev);
 }
 
diff --git a/drivers/scsi/lpfc/lpfc_sli.c b/drivers/scsi/lpfc/lpfc_sli.c
index 9173263..1f56978 100644
--- a/drivers/scsi/lpfc/lpfc_sli.c
+++ b/drivers/scsi/lpfc/lpfc_sli.c
@@ -15815,6 +15815,9 @@ lpfc_sli4_queue_free(struct lpfc_queue *queue)
 	if (!queue)
 		return;
 
+	disable_delayed_work_sync(&queue->sched_irqwork);
+	disable_delayed_work_sync(&queue->sched_spwork);
+
 	if (!list_empty(&queue->wq_list))
 		list_del(&queue->wq_list);
 
@@ -15879,6 +15882,8 @@ lpfc_sli4_queue_alloc(struct lpfc_hba *phba, uint32_t page_size,
 	INIT_LIST_HEAD(&queue->page_list);
 	INIT_LIST_HEAD(&queue->child_list);
 	INIT_LIST_HEAD(&queue->cpu_list);
+	INIT_DELAYED_WORK(&queue->sched_irqwork, lpfc_sli4_dly_hba_process_cq);
+	INIT_DELAYED_WORK(&queue->sched_spwork, lpfc_sli4_dly_sp_process_cq);
 
 	/* Set queue parameters now.  If the system cannot provide memory
 	 * resources, the free routine needs to know what was allocated.
@@ -15909,8 +15914,6 @@ lpfc_sli4_queue_alloc(struct lpfc_hba *phba, uint32_t page_size,
 	}
 	INIT_WORK(&queue->irqwork, lpfc_sli4_hba_process_cq);
 	INIT_WORK(&queue->spwork, lpfc_sli4_sp_process_cq);
-	INIT_DELAYED_WORK(&queue->sched_irqwork, lpfc_sli4_dly_hba_process_cq);
-	INIT_DELAYED_WORK(&queue->sched_spwork, lpfc_sli4_dly_sp_process_cq);
 
 	/* notify_interval will be set during q creation */
 
-- 
2.50.1.windows.1

Re: [PATCH v2] scsi: lpfc: drain all CQ work before releasing queue dependencies

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] scsi: lpfc: UAF on cq->pring in lpfc_sli4_queue_unset() before
  CQ work drain
- [High] scsi: lpfc: incomplete IRQ and CQ work drain in
  lpfc_sli4_queue_destroy()
--

commit a82bf087615a1eaf204b24247167186d3c756921
Author: Hongyan Xu <getshell@seu.edu.cn>

scsi: lpfc: drain all CQ work before releasing queue dependencies

This commit disables delayed CQ work items and flushes the HBA workqueue
before freeing queues in lpfc_sli4_queue_destroy(), and moves interrupt
teardown ahead of queue destruction in lpfc_sli4_prep_dev_for_reset().

Link: https://lore.kernel.org/all/20260928134154.1595-1-getshell@seu.edu.cn/

> diff --git a/drivers/scsi/lpfc/lpfc_init.c b/drivers/scsi/lpfc/lpfc_init.c
> index 6df5f4e7c5d65..0724dd8dcc02f 100644
> --- a/drivers/scsi/lpfc/lpfc_init.c
> +++ b/drivers/scsi/lpfc/lpfc_init.c
> @@ -10808,6 +10808,37 @@ lpfc_sli4_release_queues(struct lpfc_queue ***qs, int max)
>  	*qs = NULL;
>  }
>  
> +static void lpfc_sli4_disable_cq_work(struct lpfc_queue *cq)
> +{
> +	if (!cq)
> +		return;
> +
> +	disable_delayed_work_sync(&cq->sched_irqwork);
> +	disable_delayed_work_sync(&cq->sched_spwork);
> +}
> +
> +static void lpfc_sli4_drain_cq_work(struct lpfc_hba *phba)
> +{
> +	struct lpfc_sli4_hba *sli4 = &phba->sli4_hba;
> +	unsigned int i;
> +
> +	if (sli4->hdwq)
> +		for (i = 0; i < phba->cfg_hdw_queue; i++)
> +			lpfc_sli4_disable_cq_work(sli4->hdwq[i].io_cq);
> +
> +	if (sli4->nvmet_cqset)
> +		for (i = 0; i < phba->cfg_nvmet_mrq; i++)
> +			lpfc_sli4_disable_cq_work(sli4->nvmet_cqset[i]);
> +
> +	lpfc_sli4_disable_cq_work(sli4->els_cq);
> +	lpfc_sli4_disable_cq_work(sli4->nvmels_cq);
> +	lpfc_sli4_disable_cq_work(sli4->mbx_cq);
> +
> +	/* Direct CQ work cannot rearm the disabled delayed work. */
> +	if (phba->wq)
> +		flush_workqueue(phba->wq);
> +}

[Severity: High]
Can a threaded IRQ handler still access or queue CQ work after
lpfc_sli4_drain_cq_work() returns?

For MSI-X, lpfc_sli4_hba_intr_handler_th() runs in a dedicated IRQ kthread
outside phba->wq, calls __lpfc_sli4_hba_process_cq() directly for fast-path
CQs, and queues cq->spwork onto phba->wq for slow-path CQs.  Callers such as
lpfc_sli4_hba_setup() (on failure after lpfc_sli4_arm_cqeq_intr()) and
lpfc_sli_brdrestart_s4() invoke lpfc_sli4_queue_destroy() without disabling or
synchronizing interrupts first, and lpfc_sli4_disable_cq_work() does not call
disable_work_sync() on cq->irqwork or cq->spwork:

CPU0                                  CPU1 (irq kthread)
lpfc_sli4_hba_setup()
  lpfc_sli4_arm_cqeq_intr(phba)
  ...
  lpfc_sli4_queue_destroy(phba)
    lpfc_sli4_drain_cq_work(phba)
      flush_workqueue(phba->wq)
                                      lpfc_sli4_hba_intr_handler_th()
                                        lpfc_sli4_process_eq()
                                          __lpfc_sli4_hba_process_cq(cq)
    lpfc_sli4_release_hdwq(phba)
      lpfc_sli4_queue_free(cq)
        kfree(cq)
                                            ...  <-- use-after-free

Synchronizing active interrupt handlers before draining and calling
disable_work_sync() on cq->irqwork and cq->spwork in
lpfc_sli4_disable_cq_work() would prevent this race.

>  
>  static inline void
>  lpfc_sli4_release_hdwq(struct lpfc_hba *phba)
>  {
> @@ -10868,6 +10899,7 @@ lpfc_sli4_queue_destroy(struct lpfc_hba *phba)
>  	spin_unlock_irq(&phba->hbalock);
>  
>  	lpfc_sli4_cleanup_poll_list(phba);
> +	lpfc_sli4_drain_cq_work(phba);
>  
>  	/* Release HBA eqs */
>  	if (phba->sli4_hba.hdwq)

[Severity: High]
Does draining CQ work only inside lpfc_sli4_queue_destroy() leave a
use-after-free window on cq->pring during lpfc_sli4_queue_unset()?

In lpfc_sli4_hba_unset(), lpfc_sli_brdrestart_s4(), and the
lpfc_sli4_hba_setup() error path, lpfc_sli4_queue_unset() runs before
lpfc_sli4_queue_destroy() while delayed CQ timers (cq->sched_irqwork and
cq->sched_spwork) can still be pending.  Inside lpfc_sli4_queue_unset(),
lpfc_wq_destroy() frees wq->pring, which cq->pring aliases:

drivers/scsi/lpfc/lpfc_sli.c:lpfc_wq_destroy() {
    ...
	list_del_init(&wq->list);
	kfree(wq->pring);
	wq->pring = NULL;
    ...
}

CPU0                                  CPU1 (kworker)
lpfc_sli4_hba_unset()
  lpfc_sli4_queue_unset(phba)
    lpfc_wq_destroy(phba, wq)
      kfree(wq->pring)
                                      lpfc_sli4_dly_hba_process_cq()
                                        ...
                                          lpfc_sli4_fp_handle_fcp_wcqe()
                                            pring = cq->pring;
                                            spin_lock_irqsave(&pring->ring_lock)
                                              <-- use-after-free
  lpfc_sli4_queue_destroy(phba)
    lpfc_sli4_drain_cq_work(phba)

Draining CQ work before lpfc_sli4_queue_unset() tears down WQ state and rings
would close this window.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/179474327480.103840.7747335244166055751@seu.edu.cn?part=1