Re: [PATCH v3 1/2] scsi: elx: efct: free the RQ buffers already allocated when one fails

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

Pre-existing issues:
- [High] Internal memory leak in `efct_hw_rx_allocate` on partial allocation failure.
- [High] Massive resource leaks on hardware setup and probe failure paths due to missing teardown.
- [Medium] Dangling pointer in `efct_hw_rx_post` error path after freeing `hw->seq_pool`.
--

--- Patch [1]: [PATCH v3 1/2] scsi: elx: efct: free the RQ buffers already allocated when one fails ---
commit 4b765f42513cfc306a047983d0027b54fc865497
Author: Ali Ahmet Memis <[email protected]>

scsi: elx: efct: free the RQ buffers already allocated when one fails

This commit fixes a memory leak in efct_hw_rx_buffer_alloc. When the DMA
allocation for an entry fails, the function now properly uses
efct_hw_rx_buffer_free to unmap previously allocated buffers and free the
array before returning NULL.

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

[ ... ]

> @@ -1196,7 +1200,7 @@ efct_hw_rx_buffer_alloc(struct efct_hw *hw, u32 rqindex, u32 count,
>  						   GFP_KERNEL);
>  		if (!prq->dma.virt) {
>  			efc_log_err(hw->os, "DMA allocation failed\n");
> -			kfree(rq_buf);
> +			efct_hw_rx_buffer_free(hw, rq_buf, i);
>  			return NULL;
>  		}
>  	}

[Severity: High]
This is a pre-existing issue, but looking at how efct_hw_rx_buffer_alloc is
called, is there a similar leak in efct_hw_rx_allocate?

If efct_hw_rx_buffer_alloc fails when allocating rq->payload_buf:

drivers/scsi/elx/efct/efct_hw.c:efct_hw_rx_allocate() {
    ...
    if (!rq->payload_buf) {
        efc_log_err(efct, "rx_buffer_alloc fb_buf failed\n");
        rc = -EIO;
        break;
    }
    ...
}

Does this break out of the loop without freeing the rq->hdr_buf that was
successfully allocated in the same iteration, leaving it permanently leaked?

[Severity: High]
This is also a pre-existing issue, but does the hardware setup and probe
failure path leak resources by missing a teardown call?

For example, if efct_hw_init fails during IO allocation:

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

Or if efct_hw_setup fails after allocating the command context pool:

drivers/scsi/elx/efct/efct_hw.c:efct_hw_setup() {
    ...
    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;
    }
    ...
}

And when efct_pci_probe handles failure, it jumps to attach_out:

drivers/scsi/elx/efct/efct_driver.c:efct_pci_probe() {
    ...
attach_out:
    efct_teardown_msix(efct);
dma_mask_out:
    ...
}

Do these error paths bypass unwinding local allocations and skip calling
efct_hw_teardown, resulting in a leak of the initialized hardware structures?

[Severity: Medium]
This is another pre-existing issue, but could this lead to a dangling pointer
risk in efct_hw_rx_post?

If sequence buffer posting fails, the sequence pool is freed:

drivers/scsi/elx/efct/efct_hw.c:efct_hw_rx_post() {
    ...
    if (rc && hw->seq_pool)
        kfree(hw->seq_pool);

    return rc;
}

Should hw->seq_pool be set to NULL here? While an immediate double-free might
not occur if efct_hw_teardown is skipped on error paths, fixing the teardown
behavior could trigger a double-free when efct_hw_teardown unconditionally
calls kfree(hw->seq_pool) later.

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