Re: [PATCH v3 07/14] lpfc: Rework I/O flush ordering when unloading driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Unsafe lockless modification of `txcmplq` via `list_del_init(&iocb->list)` leads to list corruption.
- [High] Use-After-Free on `lpfc_queue` structures caused by failing to disable hardware interrupts before destroying the queues.
--

--- Patch [7]: [PATCH v3 07/14] lpfc: Rework I/O flush ordering when unloading driver ---
commit c00c06170572ea9d93301e5572a551c25be7b877
Author: Justin Tee <justintee8345@gmail.com>

lpfc: Rework I/O flush ordering when unloading driver

This patch captures the IOCB_ABORTING return value in lpfc_els_abort to
prevent adding it to the list of iocbs for cancelling, allowing it to
complete naturally. It also reorders lpfc_free_iocb_list and adds a call
to flush_workqueue in lpfc_pci_remove_one_s4 to flush I/O before freeing.

> diff --git a/drivers/scsi/lpfc/lpfc_init.c b/drivers/scsi/lpfc/lpfc_init.c
> index 23355f12fbffa..8682f5a445d29 100644
> --- 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);
> +
> +	lpfc_sli4_hba_unset(phba);

[Severity: High]
This is a pre-existing issue, but does flushing the workqueue here before
calling lpfc_sli4_hba_unset() leave a window where new work items could be
queued?

In lpfc_pci_remove_one_s4(), the call to lpfc_sli4_hba_unset() will issue
aborts via lpfc_sli_hba_iocb_abort(). Because interrupts are still enabled at
this point, hardware completions from these aborts could trigger interrupts
that add new work items to phba->wq.

Later in lpfc_sli4_hba_unset(), interrupts are disabled and
lpfc_sli4_queue_destroy() frees the queue memory. However, since
lpfc_unset_driver_resource_phase2() eventually calls destroy_workqueue(), it
will drain the workqueue and execute those pending work items on the
already freed queue objects. Could this result in a use-after-free when the
driver is unloaded while IO is in flight?

>  	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 2c8d995a45bf8..f917a5bcfd025 100644
> --- 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);
>  			list_add_tail(&iocb->list, &drv_cmpl_list);
>  		}

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to modify iocb->list
here without holding the ring lock?

In lpfc_els_abort(), the loop operates on abort_list using iocb->dlist. The
original iocb->list nodes remain on the active pring->txcmplq. After releasing
phba->hbalock, if lpfc_sli_issue_abort_iotag() fails while unloading,
list_del_init(&iocb->list) is called on txcmplq without acquiring any locks.

Could concurrent hardware completion handlers, such as
lpfc_sli_process_cmp_ring() traversing txcmplq under ring_lock or hbalock,
encounter corrupted list pointers if this list modification races with them?

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