[PATCH v3 0/2] scsi: elx: efct: fix two leaks on failure paths

[PATCH v3 0/2] scsi: elx: efct: fix two leaks on failure paths

[Ali Ahmet Memis]

Two of the five patches in v2 fix real leaks. The other three cleaned up
error paths that cannot be reached from their callers, so they are
dropped here. Sashiko's review of v2 3/5 pointed at the first of them,
and checking that properly showed the same problem in the other two.
Sorry for the churn.

  v2 1/5, HW state checked after the HIO is taken. hw->state is only
  non-ACTIVE during efct_hw_teardown(), during efct_hw_reset(), and
  before init. efct_hw_reset() has one caller, efct_fw_reset(), which
  follows it with efct_device_detach() and efct_device_attach(), and
  hw->io[] is torn down and rebuilt by that pair. So an entry stranded
  on hw->io_inuse does not outlive the adapter incarnation, and the
  "the pool runs dry and no further ELS or CT traffic can be sent"
  argument in that commit message does not hold.

  v2 2/5, HIO not returned when the WQE cannot be built. None of the
  builders reachable there can fail from these callers.
  sli_els_request64_wqe() and sli_xmit_els_rsp64_wqe() have no error
  return at all. sli_gen_request64_wqe() and sli_xmit_sequence64_wqe()
  fail only on !sgl || !sgl->virt, but efct_els_hw_srrs_send() has
  already dereferenced hio->sgl->virt before it reaches them.
  sli_xmit_bls_rsp64_wqe() fails on an unknown BLS type, which
  efct_hw_bls_send() itself sets to ACC or RJT, and on rpi_registered
  together with an s_id, which none of its three callers passes:
  efct_scsi.c sets s_id to U32_MAX and efc_send_bls_acc() leaves
  rpi_registered false.

  v2 3/5, request tag not returned when the send frame WQE fails.
  sli_send_frame_wqe() has a single "return 0", as Sashiko noted.

What is left is one leak that a failing dma_alloc_coherent() produces,
and one that I could measure. For the second, binding the driver to a
PCI device that is not an SLI-4 adapter makes sli_setup() fail after the
mempools have been created, and efct_pci_probe() then frees the struct
efct that held the only pointers to them. Repeating that probe 61 times
under CONFIG_DEBUG_KMEMLEAK:

  before   1566 unreferenced objects, every one from efct_hw_setup()
  after    none, and the probe still fails the same way

The first patch needs a real Emulex SLI-4 adapter and an allocation
failure, so it is reasoned from the code rather than exercised. Each
patch builds on its own.

Still not addressed here, and not claimed by these patches:
efct_xport_initialize() jumps past the hw_init_out label when
efct_hw_init() fails, so efct_hw_teardown() never runs and everything
efct_hw_init() managed to set up before failing stays behind, and
efcport_init() returns without destroying node_pool and node_dma_pool
when els_io_pool fails. Those cross two modules and I would rather send
them separately once this is settled.

v3: dropped v2 1/5, 2/5 and 3/5 for the reasons above; named the second
    caller of efct_hw_setup() and said what efct_hw_rx_buffer_free()
    actually frees.
v2: the mailbox pool patch claimed it cleared hw_setup_called on the way
    out while one of its three error paths still returned directly;
    fixed with a shared not_setup label.

Ali Ahmet Memis (2):
  scsi: elx: efct: free the RQ buffers already allocated when one fails
  scsi: elx: efct: destroy the mailbox pools when setup fails

 drivers/scsi/elx/efct/efct_hw.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)


base-commit: 0d839570765118029aa8bf4a95444c6a11aacf85
--
2.55.0

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

[Ali Ahmet Memis]

efct_hw_rx_buffer_alloc() allocates an array of descriptors and then a
coherent DMA buffer for each entry. When one of those allocations fails it
frees the array and returns NULL, leaving every buffer allocated before it
mapped:

	if (!prq->dma.virt) {
		efc_log_err(hw->os, "DMA allocation failed\n");
		kfree(rq_buf);
		return NULL;
	}

The caller only sees NULL and the array that held the addresses is gone,
so nothing can free them afterwards. efct_hw_rx_free() cannot help either,
it walks rq->hdr_buf and rq->payload_buf, which are only assigned once this
function succeeds.

Use efct_hw_rx_buffer_free() for the entries that were filled in. It
unmaps those buffers and frees the array, and it is defined below
efct_hw_rx_buffer_alloc(), so add a forward declaration.

Fixes: 580c0255e4ef ("scsi: elx: efct: RQ buffer, memory pool allocation and deallocation APIs")
Signed-off-by: Ali Ahmet Memis <ali@iusegentoo.com>
---
 drivers/scsi/elx/efct/efct_hw.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/elx/efct/efct_hw.c b/drivers/scsi/elx/efct/efct_hw.c
index b79c6a7ea791..cc600220168a 100644
--- a/drivers/scsi/elx/efct/efct_hw.c
+++ b/drivers/scsi/elx/efct/efct_hw.c
@@ -1170,6 +1170,10 @@ efct_get_wwpn(struct efct_hw *hw)
 	return get_unaligned_be64(p);
 }
 
+static void
+efct_hw_rx_buffer_free(struct efct_hw *hw, struct efc_hw_rq_buffer *rq_buf,
+		       u32 count);
+
 static struct efc_hw_rq_buffer *
 efct_hw_rx_buffer_alloc(struct efct_hw *hw, u32 rqindex, u32 count,
 			u32 size)
@@ -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;
 		}
 	}
-- 
2.55.0

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

[sashiko-bot]

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 <ali@iusegentoo.com>

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/20260806210631.357456-1-ali@iusegentoo.com?part=1

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

[Ali Ahmet Memis]

efct_hw_setup() creates two mempools and then calls sli_setup(). Its error
paths return without destroying what it already created:

	hw->cmd_ctx_pool = mempool_create_kmalloc_pool(...);
	if (!hw->cmd_ctx_pool)
		return -EIO;

	hw->mbox_rqst_pool = mempool_create_kmalloc_pool(...);
	if (!hw->mbox_rqst_pool)
		return -EIO;
	...
	if (sli_setup(&hw->sli, hw->os, pdev, ((struct efct *)os)->reg))
		return -EIO;

mempool_destroy() for these two runs only in efct_hw_teardown(), which is
not reached here. efct_hw_setup() is called from
efct_device_interrupts_required() and from efct_xport_attach(). On the
probe path it is the first of those that runs, and when it fails
efct_pci_probe() unwinds through efct_device_free(), freeing the struct
efct that held the only pointers to the pools.

Destroy them on the way out, and clear hw_setup_called, which the function
sets before the first allocation, so that a later call does not take the
early return and hand the caller a half configured hw.

Reproduced by binding the driver to a PCI device that is not an SLI-4
adapter, so sli_setup() fails, and repeating the probe 61 times. Before,
with CONFIG_DEBUG_KMEMLEAK:

  unreferenced object 0xffff888008449680 (size 96):
    comm "init", pid 1
    backtrace:
      __kmalloc_cache_node_noprof+0x3b9/0x430
      mempool_create_node_noprof+0x78/0xe0
      efct_hw_setup+0x1db/0xb50
      efct_pci_probe+0x3cb/0x6dd
      local_pci_probe+0xd4/0x170

1566 objects in total, every one of them from efct_hw_setup(). After the
change the same run reports none, and the probe still fails the same way.

Fixes: 4df84e846624 ("scsi: elx: efct: Driver initialization routines")
Signed-off-by: Ali Ahmet Memis <ali@iusegentoo.com>
---
 drivers/scsi/elx/efct/efct_hw.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/elx/efct/efct_hw.c b/drivers/scsi/elx/efct/efct_hw.c
index cc600220168a..20f4821078aa 100644
--- a/drivers/scsi/elx/efct/efct_hw.c
+++ b/drivers/scsi/elx/efct/efct_hw.c
@@ -256,7 +256,7 @@ efct_hw_setup(struct efct_hw *hw, void *os, struct pci_dev *pdev)
 					sizeof(struct efct_command_ctx));
 	if (!hw->cmd_ctx_pool) {
 		efc_log_err(hw->os, "failed to allocate mailbox buffer pool\n");
-		return -EIO;
+		goto not_setup;
 	}
 
 	/* Create mailbox request ctx pool for library callback */
@@ -264,7 +264,7 @@ efct_hw_setup(struct efct_hw *hw, void *os, struct pci_dev *pdev)
 					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;
 	}
 
 	spin_lock_init(&hw->io_lock);
@@ -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;
 	}
 
 	efct_hw_link_event_init(hw);
@@ -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;
 }
 
 static void
-- 
2.55.0

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

[sashiko-bot]

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 <ali@iusegentoo.com>

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/20260806210631.357456-1-ali@iusegentoo.com?part=2