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

[sashiko-bot@kernel.org]

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 <nkirkland2304@gmail.com>

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.

> diff --git a/drivers/scsi/lpfc/lpfc_init.c b/drivers/scsi/lpfc/lpfc_init.c
> --- 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);
> +

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

> +	lpfc_sli4_hba_unset(phba);
>  	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
> --- 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);

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

>  			list_add_tail(&iocb->list, &drv_cmpl_list);
>  		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013001648.62289-1-nkirkland2304@gmail.com?part=5