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

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

New issues:
- [Medium] The patch misses clearing `hw->hw_setup_called` on the first error path, contradicting the commit message claims.

Pre-existing issues:
- [High] Failing to allocate `wqe_buffs` causes a memory leak of all `hw->io[i]` objects.
- [High] The `sli_setup()` function leaks DMA memory and bitmaps on its internal error paths.
- [High] Probe failure paths in `efct_pci_probe()` fail to invoke `efct_hw_teardown()`, leaking hardware setup allocations.
- [Medium] `efct_hw_parse_filter()` leaks a dynamically allocated string when provided an empty string.
--

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

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

This commit fixes a memory leak in the error paths of efct_hw_setup().
It ensures mempools are properly destroyed and hw_setup_called is cleared
if initialization fails.

> diff --git a/drivers/scsi/elx/efct/efct_hw.c b/drivers/scsi/elx/efct/efct_hw.c
> index d645ce256b8aa..efbf29d3386e9 100644
> --- a/drivers/scsi/elx/efct/efct_hw.c
> +++ b/drivers/scsi/elx/efct/efct_hw.c

[ ... ]

>  	/* Create mailbox command ctx pool */
>  	hw->cmd_ctx_pool = mempool_create_kmalloc_pool(EFCT_CMD_CTX_POOL_SZ,
>  					sizeof(struct efct_command_ctx));
>  	if (!hw->cmd_ctx_pool) {
>  		efc_log_err(hw->os, "failed to allocate mailbox buffer pool\n");
>  		return -EIO;

[Severity: Medium]
The commit message states that hw_setup_called is cleared on the way out
so that a later call does not take the early return. However, if this first
allocation fails, we return directly without clearing it.

Should this path also jump to a label that clears hw->hw_setup_called to
prevent handing the caller a half-configured hw structure on subsequent
calls?

>  	}
> 
>  	/* Create mailbox request ctx pool for library callback */
>  	hw->mbox_rqst_pool = mempool_create_kmalloc_pool(EFCT_CMD_CTX_POOL_SZ,
>  					sizeof(struct efct_mbox_rqst_ctx));
>  	if (!hw->mbox_rqst_pool) {
>  		efc_log_err(hw->os, "failed to allocate mbox request pool\n");
> -		return -EIO;
> +		goto free_cmd_ctx_pool;
>  	}

[ ... ]

> +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;
> +	hw->hw_setup_called = false;
> +
> +	return -EIO;
>  }

[Severity: High]
This is a pre-existing issue, but does efct_hw_setup_io() leak memory
if the wqe_buffs allocation fails?

drivers/scsi/elx/efct/efct_hw.c:efct_hw_setup_io() {
        ...
        if (!hw->wqe_buffs) {
                kfree(hw->io);
                return -ENOMEM;
        }
        ...
}

The error path frees the hw->io array but appears to fail to free the
individual objects pointed to by the array, leaking them permanently.
Could this be updated to clean up the inner objects?

[Severity: High]
This is a pre-existing issue, but does sli_setup() leak DMA memory on
its internal error paths?

drivers/scsi/elx/libefc_sli/sli4.c:sli_setup() {
        ...
        sli4->bmbx.virt = dma_alloc_coherent(&pdev->dev, sli4->bmbx.size, ...);
        ...
        if (sli_request_features(sli4, &sli4->features, true))
                return -EIO;
        ...
}

When sli_setup() fails during subsequent initializations like
sli_request_features(), it returns directly without freeing the DMA memory
allocated earlier for sli4->bmbx.virt and sli4->vpd_data.virt. Since the caller
efct_hw_setup() has no way to clean up partial allocations, does this permanently
leak DMA memory?

[Severity: High]
This is a pre-existing issue, but do the probe failure paths in
efct_pci_probe() leak the hardware setup allocations?

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);
}

The probe routine invokes efct_hw_setup() which allocates mempools and DMA
memory. If a subsequent step fails, the probe routine branches to attach_out
or dma_mask_out, which calls efct_device_free() but omits calling
efct_hw_teardown(). Does this result in all hardware allocations and
mempools being permanently leaked?

[Severity: Medium]
This is a pre-existing issue, but does efct_hw_parse_filter() leak a
dynamically allocated string when provided an empty string?

drivers/scsi/elx/efct/efct_hw.c:efct_hw_parse_filter() {
        ...
        p = kstrdup(value, GFP_KERNEL);
        if (!p || !*p) {
                efc_log_err(hw->os, "p is NULL\n");
                return -ENOMEM;
        }
        ...
}

If value is an empty string, kstrdup() allocates a 1-byte buffer containing
just a null terminator. The subsequent check !*p evaluates to true, and the
function returns -ENOMEM without freeing p. Should this call kfree(p) before
returning?

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