Re: [PATCH v3 2/2] scsi: elx: efct: destroy the mailbox pools when setup fails

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

Pre-existing issues:
- [High] sli_setup() leaks DMA memory allocations on initialization error paths.
- [High] Hardware resources successfully allocated in efct_hw_setup() are leaked on subsequent probe and attach error paths.
- [High] efct_hw_init() leaks partially allocated resources on internal failures and its caller bypasses teardown.
- [High] PCI error recovery handlers ignore critical initialization failures.
--

--- Patch [2]: [PATCH v3 2/2] scsi: elx: efct: destroy the mailbox pools when setup fails ---
commit e796d2f4c5c631f17e0be2ece667e66aa7e2a789
Author: Ali Ahmet Memis <[email protected]>

scsi: elx: efct: destroy the mailbox pools when setup fails

This commit adds missing cleanup logic to the error paths of efct_hw_setup().
It ensures that the mailbox request and command context pools are properly
destroyed if initialization steps fail.

> diff --git a/drivers/scsi/elx/efct/efct_hw.c b/drivers/scsi/elx/efct/efct_hw.c
> index cc600220168a6..20f4821078aa1 100644
> --- a/drivers/scsi/elx/efct/efct_hw.c
> +++ b/drivers/scsi/elx/efct/efct_hw.c
[ ... ]
> @@ -277,7 +277,7 @@ efct_hw_setup(struct efct_hw *hw, void *os, struct pci_dev *pdev)
>  	hw->config.speed = SLI4_LINK_SPEED_AUTO_16_8_4;
>  	if (sli_setup(&hw->sli, hw->os, pdev, ((struct efct *)os)->reg)) {
>  		efc_log_err(hw->os, "SLI setup failed\n");
> -		return -EIO;
> +		goto free_mbox_rqst_pool;
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but does sli_setup() still leak DMA
memory allocations on its initialization error paths?

For example, if sli_setup() in drivers/scsi/elx/libefc_sli/sli4.c fails the
alignment check:

	if (sli4->bmbx.phys & SLI4_BMBX_MASK_LO) {
		efc_log_err(sli4, "bad alignment for bootstrap mailbox\n");
		return -EIO;
	}

it returns -EIO without calling dma_free_coherent() on previously allocated
buffers like bmbx.virt and vpd_data.virt.

[ ... ]
> @@ -313,6 +313,17 @@ efct_hw_setup(struct efct_hw *hw, void *os, struct pci_dev *pdev)
>  	(void)efct_hw_read_max_dump_size(hw);
>  
>  	return 0;
> +
> +free_mbox_rqst_pool:
> +	mempool_destroy(hw->mbox_rqst_pool);
> +	hw->mbox_rqst_pool = NULL;
> +free_cmd_ctx_pool:
> +	mempool_destroy(hw->cmd_ctx_pool);
> +	hw->cmd_ctx_pool = NULL;
> +not_setup:
> +	hw->hw_setup_called = false;
> +
> +	return -EIO;
>  }

[Severity: High]
This is a pre-existing issue, but are hardware resources successfully allocated
in efct_hw_setup() leaked on subsequent probe and attach error paths?

If efct_hw_setup() completes successfully during efct_device_interrupts_required(),
but a later step in efct_pci_probe() fails (like efct_setup_msix()), execution
jumps to the dma_mask_out label:

drivers/scsi/elx/efct/efct_driver.c:efct_pci_probe() {
    ...
attach_out:
	efct_teardown_msix(efct);
dma_mask_out:
	pci_set_drvdata(pdev, NULL);

	for (i = 0; i < EFCT_PCI_MAX_REGS; i++) {
		if (efct->reg[i])
			iounmap(efct->reg[i]);
	}
	efct_device_free(efct);
}

This skips efct_hw_teardown() entirely, permanently leaking the mempools and
SLI DMA buffers this patch is aiming to clean up.

Similarly, if efct_xport_attach() fails to create the IO pool:

drivers/scsi/elx/efct/efct_xport.c:efct_xport_attach() {
    ...
	xport->io_pool = efct_io_pool_create(efct, efct->hw.config.n_sgl);
	if (!xport->io_pool) {
		efc_log_err(efct, "Can't allocate IO pool\n");
		return -ENOMEM;
	}
    ...
}

it returns early without triggering teardown of resources allocated earlier in
efct_hw_setup().


[Severity: High]
This is a pre-existing issue, but does efct_hw_init() leak partially allocated
resources on internal failures, and does its caller bypass the teardown?

If efct_hw_init() fails midway, for example:

drivers/scsi/elx/efct/efct_hw.c:efct_hw_init() {
    ...
	rc = efct_hw_rx_allocate(hw);
	if (rc) {
		efc_log_err(hw->os, "rx_allocate failed\n");
		return rc;
	}
    ...
}

it returns the error directly without cleaning up previously allocated resources.

Additionally, its caller, efct_xport_initialize(), handles the failure by
jumping to the out label, bypassing efct_hw_teardown():

drivers/scsi/elx/efct/efct_xport.c:efct_xport_initialize() {
    ...
	rc = efct_hw_init(&efct->hw);
	if (rc) {
		efc_log_err(efct, "efct_hw_init failure\n");
		goto out;
	}
	...
hw_init_out:
	efct_hw_teardown(&efct->hw);
out:
	return rc;
}


[Severity: High]
This is a pre-existing issue, but do the PCI error recovery handlers ignore
critical initialization failures?

In efct_pci_io_slot_reset(), if efct_setup_msix() or efct_device_attach()
returns an error, the code logs the failure but ignores the return value:

drivers/scsi/elx/efct/efct_driver.c:efct_pci_io_slot_reset() {
    ...
	rc = efct_setup_msix(efct, efct->n_msix_vec);
	if (rc)
		efc_log_err(efct, "rc %d returned, IRQ allocation failed\n", rc);

	/* Perform device reset */
	efct_device_detach(efct);
	/* Bring device to online*/
	efct_device_attach(efct);

	return PCI_ERS_RESULT_RECOVERED;
}

This forces an incorrect success status back to the PCI subsystem even when
the device is broken.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2