[PATCH 0/5] scsi: elx: efct: fix resources stranded on failure paths

[PATCH 0/5] scsi: elx: efct: fix resources stranded on failure paths

[Ali Ahmet Memis]

Five places in efct take a resource and then return an error without
giving it back. Three of them lose an entry from a fixed size pool, which
stops the driver working once the pool is empty rather than growing
memory; the other two are ordinary leaks.

  1  efct_els_hw_srrs_send() checks hw->state after taking an HIO
  2  the WQE builders in efct_els_hw_srrs_send() and efct_hw_bls_send()
  3  the WQE builder in efct_hw_send_frame(), which loses a request tag
  4  efct_hw_rx_buffer_alloc() drops the coherent buffers it mapped
  5  efct_hw_setup() leaves its two mempools behind

Patch 5 is the one I could exercise. 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

Patches 1 to 4 are reasoned from the code. They need a real Emulex SLI-4
adapter, and for 1 to 3 a live FC link as well, which I do not have. Each
patch builds on its own.

I deliberately left the efct_hw_wq_write() failure paths alone.
efct_hw_wq_write() appends to wq->pending_list and drains from the head,
so it can return an error while this request is still linked there.
Releasing the HIO or the request tag at that point would leave a later
completion looking at something that has been handed back, which needs
more than a free on the error path.

Also not addressed here: efct_xport_attach() and efct_xport_initialize()
return without efct_hw_teardown() on some paths, and efcport_init() leaves
its first two pools behind when the third allocation fails. Those cross
two modules and I would rather send them separately once this is settled.

Ali Ahmet Memis (5):
  scsi: elx: efct: check the HW state before allocating an HIO
  scsi: elx: efct: free the HIO when the WQE cannot be built
  scsi: elx: efct: free the request tag when the send frame WQE fails
  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 | 71 +++++++++++++++++++++------------
 1 file changed, 45 insertions(+), 26 deletions(-)


base-commit: 0d839570765118029aa8bf4a95444c6a11aacf85
-- 
2.55.0

[PATCH 1/5] scsi: elx: efct: check the HW state before allocating an HIO

[Ali Ahmet Memis]

efct_els_hw_srrs_send() takes an HIO from the pool and only then looks at
hw->state, returning without giving it back when the HW is not active:

	hio = efct_hw_io_alloc(hw);
	if (!hio) {
		pr_err("HIO alloc failed\n");
		return -EIO;
	}

	if (hw->state != EFCT_HW_STATE_ACTIVE) {
		efc_log_debug(hw->os,
			      "cannot send SRRS, HW state=%d\n", hw->state);
		return -EIO;
	}

_efct_hw_io_alloc() moves the entry from hw->io_free to hw->io_inuse and
initialises its reference, and the only thing that puts it back is the
completion of a submitted WQE. Nothing is submitted here, so the entry
stays on hw->io_inuse for the lifetime of the adapter. The memory is
reclaimed in efct_hw_teardown(), which frees hw->io[] as a whole, but the
pool loses one usable entry for every ELS or CT send that takes this
path. Once it is empty efct_hw_io_alloc() starts failing and no further
ELS or CT traffic can be sent.

Check the state first, the way efct_hw_bls_send() already does.

Fixes: dd53d333aadb ("scsi: elx: efct: Hardware I/O submission routines")
Signed-off-by: Ali Ahmet Memis <ali@iusegentoo.com>
---
 drivers/scsi/elx/efct/efct_hw.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/elx/efct/efct_hw.c b/drivers/scsi/elx/efct/efct_hw.c
index b79c6a7ea791..15c37ff1d52e 100644
--- a/drivers/scsi/elx/efct/efct_hw.c
+++ b/drivers/scsi/elx/efct/efct_hw.c
@@ -2706,18 +2706,18 @@ efct_els_hw_srrs_send(struct efc *efc, struct efc_disc_io *io)
 	u32 sge0_flags;
 	u32 sge1_flags;
 
-	hio = efct_hw_io_alloc(hw);
-	if (!hio) {
-		pr_err("HIO alloc failed\n");
-		return -EIO;
-	}
-
 	if (hw->state != EFCT_HW_STATE_ACTIVE) {
 		efc_log_debug(hw->os,
 			      "cannot send SRRS, HW state=%d\n", hw->state);
 		return -EIO;
 	}
 
+	hio = efct_hw_io_alloc(hw);
+	if (!hio) {
+		pr_err("HIO alloc failed\n");
+		return -EIO;
+	}
+
 	hio->done = efct_els_ssrs_send_cb;
 	hio->arg  = io;
 
-- 
2.55.0

[PATCH 2/5] scsi: elx: efct: free the HIO when the WQE cannot be built

[Ali Ahmet Memis]

efct_els_hw_srrs_send() and efct_hw_bls_send() allocate an HIO, ask sli4
to build a WQE into it, and give up when that fails without putting the
HIO back:

	if (sli_els_request64_wqe(&hw->sli, hio->wqe.wqebuf, hio->sgl,
				  &els_params)) {
		efc_log_err(hw->os, "REQ WQE error\n");
		rc = -EIO;
	}

Nothing has been submitted at that point, so no completion will arrive to
release it, and the entry sits on hw->io_inuse until the adapter is torn
down. Each failure costs the pool one entry, and once it is empty
efct_hw_io_alloc() fails and no further ELS, CT or BLS frame can be sent.

Release the HIO on those paths. The efct_hw_wq_write() failure below is
deliberately left alone: it can return an error while this request is
still queued on wq->pending_list, so the HIO cannot be handed back there
without more care.

Fixes: dd53d333aadb ("scsi: elx: efct: Hardware I/O submission routines")
Signed-off-by: Ali Ahmet Memis <ali@iusegentoo.com>
---
 drivers/scsi/elx/efct/efct_hw.c | 38 ++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/drivers/scsi/elx/efct/efct_hw.c b/drivers/scsi/elx/efct/efct_hw.c
index 15c37ff1d52e..6cc48fa3e656 100644
--- a/drivers/scsi/elx/efct/efct_hw.c
+++ b/drivers/scsi/elx/efct/efct_hw.c
@@ -2609,6 +2609,7 @@ efct_hw_bls_send(struct efct *efct, u32 type, struct sli_bls_params *bls_params,
 	if (sli_xmit_bls_rsp64_wqe(&hw->sli, hio->wqe.wqebuf,
 				   &bls, bls_params)) {
 		efc_log_err(hw->os, "XMIT_BLS_RSP64 WQE error\n");
+		efct_hw_io_free(hw, hio);
 		return -EIO;
 	}
 
@@ -2820,24 +2821,27 @@ efct_els_hw_srrs_send(struct efc *efc, struct efc_disc_io *io)
 		rc = -EIO;
 	}
 
-	if (rc == 0) {
-		hio->xbusy = true;
+	if (rc) {
+		efct_hw_io_free(hw, hio);
+		return rc;
+	}
 
-		/*
-		 * Add IO to active io wqe list before submitting, in case the
-		 * wcqe processing preempts this thread.
-		 */
-		hio->wq->use_count++;
-		rc = efct_hw_wq_write(hio->wq, &hio->wqe);
-		if (rc >= 0) {
-			/* non-negative return is success */
-			rc = 0;
-		} else {
-			/* failed to write wqe, remove from active wqe list */
-			efc_log_err(hw->os,
-				    "sli_queue_write failed: %d\n", rc);
-			hio->xbusy = false;
-		}
+	hio->xbusy = true;
+
+	/*
+	 * Add IO to active io wqe list before submitting, in case the
+	 * wcqe processing preempts this thread.
+	 */
+	hio->wq->use_count++;
+	rc = efct_hw_wq_write(hio->wq, &hio->wqe);
+	if (rc >= 0) {
+		/* non-negative return is success */
+		rc = 0;
+	} else {
+		/* failed to write wqe, remove from active wqe list */
+		efc_log_err(hw->os,
+			    "sli_queue_write failed: %d\n", rc);
+		hio->xbusy = false;
 	}
 
 	return rc;
-- 
2.55.0

[PATCH 3/5] scsi: elx: efct: free the request tag when the send frame WQE fails

[Ali Ahmet Memis]

efct_hw_send_frame() takes a request tag from the pool and then builds the
WQE. When sli_send_frame_wqe() fails it returns without giving the tag
back:

	ctx->wqcb = efct_hw_reqtag_alloc(hw, callback, arg);
	if (!ctx->wqcb) {
		efc_log_err(hw->os, "can't allocate request tag\n");
		return -ENOSPC;
	}
	...
	if (rc) {
		efc_log_err(hw->os, "sli_send_frame_wqe failed: %d\n", rc);
		return -EIO;
	}

Nothing is submitted, so the completion that would call
efct_hw_reqtag_free() never runs and the tag stays out of the pool. The
pool is bounded by the number of request tags allocated at init, so
repeated failures leave send frame without any.

Free the tag on that path. The efct_hw_wq_write() failure below is left
alone: it can return an error while this request is still queued on
wq->pending_list, and the tag is what a later completion would look the
context up by.

Fixes: dd53d333aadb ("scsi: elx: efct: Hardware I/O submission routines")
Signed-off-by: Ali Ahmet Memis <ali@iusegentoo.com>
---
 drivers/scsi/elx/efct/efct_hw.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/elx/efct/efct_hw.c b/drivers/scsi/elx/efct/efct_hw.c
index 6cc48fa3e656..db68516e8075 100644
--- a/drivers/scsi/elx/efct/efct_hw.c
+++ b/drivers/scsi/elx/efct/efct_hw.c
@@ -3009,6 +3009,7 @@ efct_hw_send_frame(struct efct_hw *hw, struct fc_frame_header *hdr,
 				ctx->wqcb->instance_index);
 	if (rc) {
 		efc_log_err(hw->os, "sli_send_frame_wqe failed: %d\n", rc);
+		efct_hw_reqtag_free(hw, ctx->wqcb);
 		return -EIO;
 	}
 
-- 
2.55.0

[PATCH 4/5] 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.

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 db68516e8075..d645ce256b8a 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

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

[Ali Ahmet Memis]

efct_hw_setup() creates two mempools and then calls sli_setup(). Both of
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 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 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 | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/elx/efct/efct_hw.c b/drivers/scsi/elx/efct/efct_hw.c
index d645ce256b8a..efbf29d3386e 100644
--- a/drivers/scsi/elx/efct/efct_hw.c
+++ b/drivers/scsi/elx/efct/efct_hw.c
@@ -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,16 @@ 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;
+	hw->hw_setup_called = false;
+
+	return -EIO;
 }
 
 static void
-- 
2.55.0

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

[sashiko-bot]

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

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

Re: [PATCH 1/5] scsi: elx: efct: check the HW state before allocating an HIO

[sashiko-bot]

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

Pre-existing issues:
- [High] Hardware I/O object (hio) is permanently leaked on WQE submission failure.
- [High] Hardware I/O object (HIO) is permanently leaked on successful ELS and BLS completions.
--

--- Patch [1]: [PATCH 1/5] scsi: elx: efct: check the HW state before allocating an HIO ---
commit 9f7986d6f2e47353fa2d9861b16b7f4d623a7b71
Author: Ali Ahmet Memis <ali@iusegentoo.com>

scsi: elx: efct: check the HW state before allocating an HIO

This commit moves the hardware state check in efct_els_hw_srrs_send to
happen before allocating a hardware I/O (HIO) object. This prevents an
HIO object leak that previously occurred when the hardware state was not
active.

> diff --git a/drivers/scsi/elx/efct/efct_hw.c b/drivers/scsi/elx/efct/efct_hw.c
> index b79c6a7ea7912..15c37ff1d52e4 100644
> --- a/drivers/scsi/elx/efct/efct_hw.c
> +++ b/drivers/scsi/elx/efct/efct_hw.c
> @@ -2706,18 +2706,18 @@ efct_els_hw_srrs_send(struct efc *efc, struct efc_disc_io *io)
>  	u32 sge0_flags;
>  	u32 sge1_flags;
>  
> -	hio = efct_hw_io_alloc(hw);
> -	if (!hio) {
> -		pr_err("HIO alloc failed\n");
> -		return -EIO;
> -	}
> -
>  	if (hw->state != EFCT_HW_STATE_ACTIVE) {
>  		efc_log_debug(hw->os,
>  			      "cannot send SRRS, HW state=%d\n", hw->state);
>  		return -EIO;
>  	}
>  
> +	hio = efct_hw_io_alloc(hw);
> +	if (!hio) {
> +		pr_err("HIO alloc failed\n");
> +		return -EIO;
> +	}
> +

[Severity: High]
This isn't a bug introduced by this patch, but does this function leak the
newly allocated hio object if WQE submission fails further down?

If efct_hw_wq_write() fails at the end of efct_els_hw_srrs_send():

	hio->wq->use_count++;
	rc = efct_hw_wq_write(hio->wq, &hio->wqe);
	if (rc >= 0) {
		/* non-negative return is success */
		rc = 0;
	} else {
		/* failed to write wqe, remove from active wqe list */
		efc_log_err(hw->os,
			    "sli_queue_write failed: %d\n", rc);
		hio->xbusy = false;
	}

	return rc;

The function returns the error but it doesn't appear to call
efct_hw_io_free(hw, hio), leaving the hio object stranded. Could this
eventually exhaust the adapter's hardware IO pool during transient queue
full conditions?

This same pattern appears to exist in efct_hw_bls_send() when either
sli_xmit_bls_rsp64_wqe() or efct_hw_wq_write() fails.

>  	hio->done = efct_els_ssrs_send_cb;

[Severity: High]
This is also a pre-existing issue, but is the hio object leaked on
successful completions as well?

Looking at the callback assigned here:

drivers/scsi/elx/efct/efct_hw.c:efct_els_ssrs_send_cb() {
	struct efc_disc_io *io = arg;

	efc_disc_io_complete(io, length, status, ext_status);
	return 0;
}

The callback executes the completion handler but doesn't drop the
reference to the passed hio object. 

The overarching WQE completion handler efct_hw_wq_process_io() also seems
to drop the WQE and call done(), but leaves the hio pointer allocated.

Are we missing an efct_hw_io_free(hw, hio) call on the success paths
for ELS and BLS completions?

>  	hio->arg  = io;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260806192345.328621-1-ali@iusegentoo.com?part=1

Re: [PATCH 3/5] scsi: elx: efct: free the request tag when the send frame WQE fails

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Use-After-Free and Double-Free of receive sequence buffer during task set full/busy response.
- [Critical] Deterministic NULL pointer dereference passing uninitialized `wqebuf` to `sli_send_frame_wqe()`.
- [High] NULL pointer dereference in `efct_hw_send_frame()` if `send_frame_io` allocation fails.
- [High] Missing `dma_rmb()` memory barrier after checking the valid bit of the EQE and CQE.
- [High] Incorrect byte swapping of the FC frame header on Big-Endian architectures.
--

--- Patch [3]: [PATCH 3/5] scsi: elx: efct: free the request tag when the send frame WQE fails ---
commit 66a0b41398fa81e3a9b1b1edf2e7a32743d6bf86
Author: Ali Ahmet Memis <ali@iusegentoo.com>

scsi: elx: efct: free the request tag when the send frame WQE fails
    
efct_hw_send_frame() takes a request tag from the pool and then builds the
WQE. When sli_send_frame_wqe() fails it returns without giving the tag
back:

[ ... ]

> diff --git a/drivers/scsi/elx/efct/efct_hw.c b/drivers/scsi/elx/efct/efct_hw.c
> index 6cc48fa3e656a..db68516e80750 100644
> --- a/drivers/scsi/elx/efct/efct_hw.c
> +++ b/drivers/scsi/elx/efct/efct_hw.c
> @@ -3009,6 +3009,7 @@ efct_hw_send_frame(struct efct_hw *hw, struct fc_frame_header *hdr,

[Severity: High]
This is a pre-existing issue, but can this lead to a NULL pointer dereference 
if send_frame_io allocation fails?

During driver initialization, if efct_hw_io_alloc() fails, it logs an error 
but completes successfully:

drivers/scsi/elx/efct/efct_hw.c:efct_hw_init() {
    ...
    hw->hw_wq[0]->send_frame_io = efct_hw_io_alloc(hw);
    if (!hw->hw_wq[0]->send_frame_io)
        efc_log_err(hw->os, "alloc for send_frame_io failed\n");

    /* Initialize send frame sequence id */
    atomic_set(&hw->send_frame_seq_id, 0);

    return 0;
}

Later, efct_hw_send_frame() directly dereferences it without checking for NULL 
just before creating the WQE:

drivers/scsi/elx/efct/efct_hw.c:efct_hw_send_frame() {
    ...
    /* Set XRI and RX_ID in the header based on which WQ, and which
     * send_frame_io we are using
     */
    xri = wq->send_frame_io->indicator;
    ...
}

Should initialization fail if this allocation fails?

[Severity: Critical]
This is a pre-existing issue, but does this lead to a NULL pointer dereference 
when attempting to send a task set full or busy response?

In efct_sframe_common_send(), the context is zero-initialized:

drivers/scsi/elx/efct/efct_unsol.c:efct_sframe_common_send() {
    ...
    memset(ctx, 0, sizeof(*ctx));
    /* Save sequence */
    ctx->seq = seq;
    ...
}

This leaves the embedded ctx->wqe.wqebuf pointer as NULL. When 
efct_hw_send_frame() is called, it passes this uninitialized wqebuf to 
sli_send_frame_wqe():

drivers/scsi/elx/efct/efct_hw.c:efct_hw_send_frame() {
    ...
    rc = sli_send_frame_wqe(&hw->sli, wqe->wqebuf,
                            sof, eof, (u32 *)hdr, payload, payload->len,
    ...
}

Then sli_send_frame_wqe() immediately attempts to memset the buffer:

drivers/scsi/elx/libefc_sli/sli4.c:sli_send_frame_wqe() {
    ...
    struct sli4_send_frame_wqe *sf = buf;

    memset(buf, 0, sli->wqe_size);
    ...
}

Will this cause a guaranteed kernel panic on command flooding when IO contexts 
are exhausted?

[Severity: Critical]
This is a pre-existing issue, but can this sequence lead to a use-after-free 
and double-free of the receive sequence buffer?

When efct_scsi_io_alloc() fails, it rejects the command via 
efct_sframe_send_task_set_full_or_busy(). This uses the receive sequence's 
payload buffer to build the transmission payload and context:

drivers/scsi/elx/efct/efct_unsol.c:efct_sframe_common_send() {
    ...
    ctx = (void *)(heap_virt_base + heap_offset);
    ...
    ctx->seq = seq;
    ...
}

After queuing the transmit with efct_hw_send_frame(), control returns to 
efct_dispatch_frame() which unconditionally frees the sequence, returning it to 
the hardware receive ring:

drivers/scsi/elx/efct/efct_unsol.c:efct_dispatch_frame() {
    ...
    kref_put(&node->ref, node->release);
    efct_hw_sequence_free(&efct->hw, seq);
    return 0;
}

This appears to return the buffer to the hardware receive queue while the 
hardware is simultaneously trying to DMA transmit from it. 

Furthermore, when the asynchronous transmit completes, it frees the exact same 
sequence a second time:

drivers/scsi/elx/efct/efct_unsol.c:efct_sframe_common_send_cb() {
    ...
    /* Free sequence */
    efct_hw_sequence_free(hw, ctx->seq);
}

Is it safe to free the sequence in efct_dispatch_frame() when it has been 
repurposed for transmission?

[Severity: High]
This is a pre-existing issue, but does this incorrectly byte-swap the FC frame 
header on Big-Endian architectures?

In sli_send_frame_wqe(), the frame header bytes are cast to u32 and swapped:

drivers/scsi/elx/libefc_sli/sli4.c:sli_send_frame_wqe() {
    ...
    /* Copy FC header */
    sf->fc_header_0_1[0] = cpu_to_le32(hdr[0]);
    sf->fc_header_0_1[1] = cpu_to_le32(hdr[1]);
    sf->fc_header_2_5[0] = cpu_to_le32(hdr[2]);
    sf->fc_header_2_5[1] = cpu_to_le32(hdr[3]);
    sf->fc_header_2_5[2] = cpu_to_le32(hdr[4]);
    sf->fc_header_2_5[3] = cpu_to_le32(hdr[5]);
    ...
}

On Big-Endian architectures, reading a byte array via a 32-bit load reverses 
the 4 bytes, and cpu_to_le32() applies a second swap. When stored to memory, 
the Big-Endian store behavior reverses it again. Will this result in the 
hardware receiving a byte-reversed FC frame header?

[Severity: High]
This is a pre-existing issue, but is a read memory barrier required after 
checking the valid bit of the DMA-coherent queue entry?

In sli_eq_read(), the valid bit is checked and then the entry is copied:

drivers/scsi/elx/libefc_sli/sli4.c:sli_eq_read() {
    ...
    wflags = le16_to_cpu(((struct sli4_eqe *)qe)->dw0w0_flags);

    if ((wflags & SLI4_EQE_VALID) != q->phase) {
        spin_unlock_irqrestore(&q->lock, flags);
        return -EIO;
    }
    ...
    memcpy(entry, qe, q->size);
    ...
}

A similar pattern exists in sli_cq_read():

drivers/scsi/elx/libefc_sli/sli4.c:sli_cq_read() {
    ...
    dwflags = le32_to_cpu(((struct sli4_mcqe *)qe)->dw3_flags);
    valid_bit_set = (dwflags & SLI4_MCQE_VALID) != 0;

    if (valid_bit_set != q->phase) {
        spin_unlock_irqrestore(&q->lock, flags);
        return -EIO;
    }
    ...
    memcpy(entry, qe, q->size);
    ...
}

On weakly ordered architectures, could the CPU speculatively read the entry's 
body before the valid bit check completes over the bus, leading to the 
processing of stale or corrupted DMA descriptor data? Does this require a 
dma_rmb() before the memcpy()?

>  				ctx->wqcb->instance_index);
>  	if (rc) {
>  		efc_log_err(hw->os, "sli_send_frame_wqe failed: %d\n", rc);
> +		efct_hw_reqtag_free(hw, ctx->wqcb);
>  		return -EIO;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260806192345.328621-1-ali@iusegentoo.com?part=3

Re: [PATCH 2/5] scsi: elx: efct: free the HIO when the WQE cannot be built

[sashiko-bot]

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

Pre-existing issues:
- [Critical] `sli_xmit_sequence64_wqe()` treats the provided SGL array as a direct payload buffer, leading to an out-of-bounds DMA read and transmitting kernel memory contents over Fibre Channel.
- [High] If asynchronous task scheduling fails due to memory pressure, the IO request is leaked and never completed to the upper layer.
- [High] Missing `efct_hw_reqtag_free` on error paths leaks WQ callback context, eventually leading to exhaustion of the request tag pool.
--

--- Patch [2]: [PATCH 2/5] scsi: elx: efct: free the HIO when the WQE cannot be built ---
commit c236a09d90fd9d31b4fd7efd9ef66e408be03d17
Author: Ali Ahmet Memis <ali@iusegentoo.com>

scsi: elx: efct: free the HIO when the WQE cannot be built
    
efct_els_hw_srrs_send() and efct_hw_bls_send() allocate an HIO, ask sli4
to build a WQE into it, and give up when that fails without putting the
HIO back:

[ ... ]

> diff --git a/drivers/scsi/elx/efct/efct_hw.c b/drivers/scsi/elx/efct/efct_hw.c
> index 15c37ff1d52e4..6cc48fa3e656a 100644
> --- a/drivers/scsi/elx/efct/efct_hw.c
> +++ b/drivers/scsi/elx/efct/efct_hw.c

[ ... ]

> @@ -2820,24 +2821,27 @@ efct_els_hw_srrs_send(struct efc *efc, struct efc_disc_io *io)
>  		rc = -EIO;
>  	}
>  
> -	if (rc == 0) {
> -		hio->xbusy = true;
> +	if (rc) {
> +		efct_hw_io_free(hw, hio);
> +		return rc;
> +	}

[Severity: Critical]
This is a pre-existing issue, but does sli_xmit_sequence64_wqe() incorrectly
receive hio->sgl instead of the actual data buffer send?

In efct_els_hw_srrs_send() for EFC_DISC_IO_CT_RESP:

	if (sli_xmit_sequence64_wqe(&hw->sli, hio->wqe.wqebuf, hio->sgl,
				    &ct_params)){

If sli_xmit_sequence64_wqe() treats the provided SGL array as a direct
payload buffer:

libefc_sli/sli4.c:sli_xmit_sequence64_wqe() {
	...
	xmit->bde.bde_type_buflen =
		cpu_to_le32((SLI4_BDE_TYPE_VAL(64)) |
			(params->rsp_len & SLI4_BDE_LEN_MASK));
	xmit->bde.u.data.low  =
			cpu_to_le32(lower_32_bits(payload->phys));
	xmit->bde.u.data.high =
			cpu_to_le32(upper_32_bits(payload->phys));
	xmit->sequence_payload_len = cpu_to_le32(params->rsp_len);
	...
}

Could this lead to an out-of-bounds DMA read and transmit kernel memory
contents over Fibre Channel?

[ ... ]

[Severity: High]
This is a pre-existing issue, but does efct_hw_send_frame() leak the WQ
callback context on error paths?

drivers/scsi/elx/efct/efct_hw.c:efct_hw_send_frame() {
	...
	rc = sli_send_frame_wqe(&hw->sli, wqe->wqebuf,
				sof, eof, (u32 *)hdr, payload, payload->len,
				EFCT_HW_SEND_FRAME_TIMEOUT, xri,
				ctx->wqcb->instance_index);
	if (rc) {
		efc_log_err(hw->os, "sli_send_frame_wqe failed: %d\n", rc);
		return -EIO;
	}

	/* Write to WQ */
	rc = efct_hw_wq_write(wq, wqe);
	if (rc) {
		efc_log_err(hw->os, "efct_hw_wq_write failed: %d\n", rc);
		return -EIO;
	}
	...
}

If sli_send_frame_wqe() or efct_hw_wq_write() fail, the function returns -EIO,
but shouldn't it call efct_hw_reqtag_free() to avoid exhausting the request
tag pool?

[Severity: High]
This is a pre-existing issue, but do efct_scsi_dispatch_pending() and
efct_scsi_check_pending() leak the IO request if asynchronous task scheduling
fails?

drivers/scsi/elx/efct/efct_scsi.c:efct_scsi_dispatch_pending() {
	...
		if (efct_hw_async_call(&efct->hw,
				       efct_scsi_check_pending_async_cb, io)) {
			efc_log_debug(efct, "call hw async failed\n");
		}
	...
}

drivers/scsi/elx/efct/efct_scsi.c:efct_scsi_check_pending() {
	...
			if (efct_hw_async_call(&efct->hw,
					efct_scsi_check_pending_async_cb, io)) {
				efc_log_debug(efct, "hw async failed\n");
			}
	...
}

If efct_hw_async_call() fails due to memory pressure, it logs an error but
doesn't call the callback or return the IO to the pool. Will this silently drop
the SCSI IO request and cause a command timeout?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260806192345.328621-1-ali@iusegentoo.com?part=2

Re: [PATCH 4/5] scsi: elx: efct: free the RQ buffers already allocated when one fails

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing resource teardown in initialization error paths leads to massive memory and resource leaks.
- [High] Unconditional call to `dma_free_coherent` on an unallocated memory region causes undefined behavior/kernel panic.
- [High] Unprotected concurrent access to the global `efct_devices` list.
- [High] Use-after-free of `efct` object when PCI device is removed while LIO configfs retains a reference.
- [High] Missing `timer_delete_sync` and dangerous re-initialization of `stats_timer` leads to concurrent UAF.
--

--- Patch [4]: [PATCH 4/5] scsi: elx: efct: free the RQ buffers already allocated when one fails ---
commit cebc7192c16ac8492a77188ae439ad0352527068
Author: Ali Ahmet Memis <ali@iusegentoo.com>
Date:   Thu Aug 6 19:23:44 2026 +0000

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

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. Use efct_hw_rx_buffer_free() for the entries that were filled in.

> diff --git a/drivers/scsi/elx/efct/efct_hw.c b/drivers/scsi/elx/efct/efct_hw.c
> index db68516e80750..d645ce256b8aa 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;
>  		}
>  	}

[Severity: High]
This is a pre-existing issue, but missing resource teardown in initialization
error paths leads to massive memory and resource leaks.

In efct_hw_rx_allocate(), if efct_hw_rx_buffer_alloc() succeeds for
rq->hdr_buf but fails for rq->payload_buf, it breaks the loop and returns
-EIO without freeing the just-allocated hdr_buf or any previous queues:

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

This error propagates up through efct_hw_init() and efct_xport_initialize(),
both of which return the error and explicitly bypass 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;
    }
    ...
}

Since this patch attempts to fix exactly this type of leak in the callee but
leaves the caller completely unprotected, does this need a similar fix to
prevent identical leaked resources?

[Severity: High]
This is a pre-existing issue, but does this unconditional call to
dma_free_coherent() on an unallocated memory region cause undefined behavior
or a kernel panic?

efct_hw_teardown() unconditionally calls dma_free_coherent() on hw->rnode_mem:

drivers/scsi/elx/efct/efct_hw.c:efct_hw_teardown() {
    ...
    dma_free_coherent(&efct->pci->dev,
              hw->rnode_mem.size, hw->rnode_mem.virt,
              hw->rnode_mem.phys);
    ...
}

Since rnode_mem is never allocated anywhere in the driver, it remains
0-initialized, meaning virt is NULL and size is 0. Calling dma_free_coherent()
with a NULL virtual address is an API violation that can crash or WARN on
architectures that do not check for it.

[Severity: High]
This is a pre-existing issue, but is there unprotected concurrent access to
the global efct_devices list?

The driver adds and removes devices from the global efct_devices list in
efct_device_alloc() without using any locks:

drivers/scsi/elx/efct/efct_driver.c:efct_device_alloc() {
    ...
    list_add_tail(&efct->list_entry, &efct_devices);
    ...
}

Concurrently, efct_find_wwpn() (triggered by LIO configfs operations)
iterates over this list using list_for_each_entry() without any locking:

drivers/scsi/elx/efct/efct_lio.c:efct_find_wwpn() {
    ...
    list_for_each_entry(efct, &efct_devices, list_entry) {
    ...
}

Can this lead to linked list corruption, infinite loops, or use-after-free
if a privileged administrator configures LIO targets concurrently with hotplug
or sysfs unbind events?

[Severity: High]
This is a pre-existing issue, but is there a use-after-free of the efct object
when the PCI device is removed while LIO configfs retains a reference?

efct_lio_make_nport() assigns a raw efct pointer to the LIO target without
taking a reference count on the device:

drivers/scsi/elx/efct/efct_lio.c:efct_lio_make_nport() {
    ...
    lio_nport->efct = efct;
    ...
}

If the PCI device is unbound or removed, efct_pci_remove() completely destroys
the device and frees the efct structure:

drivers/scsi/elx/efct/efct_driver.c:efct_pci_remove() {
    ...
    efct_device_free(efct);
    ...
}

Could subsequent LIO teardown or target I/O dereference the freed efct memory
if configurable LIO targets survive the unloading of the underlying hardware
device?

[Severity: High]
This is a pre-existing issue, but does the missing timer_delete_sync() and
dangerous re-initialization of stats_timer lead to a concurrent
use-after-free?

During device detachment, efct_xport_detach() uses the asynchronous
timer_delete():

drivers/scsi/elx/efct/efct_xport.c:efct_xport_detach() {
    ...
    if (timer_pending(&xport->stats_timer))
        timer_delete(&xport->stats_timer);
    ...
}

If the timer callback efct_xport_stats_timer_cb() is running concurrently, the
driver will free the xport memory immediately after. The callback then
executes efct_xport_config_stats_timer(), which calls timer_setup() and
mod_timer() on the just-freed xport->stats_timer:

drivers/scsi/elx/efct/efct_xport.c:efct_xport_config_stats_timer() {
    ...
    timer_setup(&xport->stats_timer, &efct_xport_stats_timer_cb, 0);
    mod_timer(&xport->stats_timer, jiffies + msecs_to_jiffies(timeout));
    ...
}

Can this cause timer list corruption and a kernel panic via use-after-free if
a driver unload races with the periodic 3-second stats timer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260806192345.328621-1-ali@iusegentoo.com?part=4

[PATCH v2 0/5] scsi: elx: efct: fix resources stranded on failure paths

[Ali Ahmet Memis]

Five places in efct take a resource and then return an error without
giving it back. Three of them lose an entry from a fixed size pool, which
stops the driver working once the pool is empty rather than growing
memory; the other two are ordinary leaks.

  1  efct_els_hw_srrs_send() checks hw->state after taking an HIO
  2  the WQE builders in efct_els_hw_srrs_send() and efct_hw_bls_send()
  3  the WQE builder in efct_hw_send_frame(), which loses a request tag
  4  efct_hw_rx_buffer_alloc() drops the coherent buffers it mapped
  5  efct_hw_setup() leaves its two mempools behind

Changes in v2:
- Patch 5: the first error path, the one taken when the cmd_ctx_pool
  allocation fails, returned without clearing hw_setup_called while the
  two paths below it cleared it. The commit message described the
  behaviour the other two paths had, so it claimed more than the patch
  did. All three now clear it. Pointed out by the Sashiko review of v1.
- No other change, patches 1 to 4 are as sent.

Patch 5 is the one I could exercise. 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 same run against the v2 patch reports none as well.

Patches 1 to 4 are reasoned from the code. They need a real Emulex SLI-4
adapter, and for 1 to 3 a live FC link as well, which I do not have. Each
patch builds on its own.

I deliberately left the efct_hw_wq_write() failure paths alone.
efct_hw_wq_write() appends to wq->pending_list and drains from the head,
so it can return an error while this request is still linked there.
Releasing the HIO or the request tag at that point would leave a later
completion looking at something that has been handed back, which needs
more than a free on the error path.

Also not addressed here: efct_xport_attach() and efct_xport_initialize()
return without efct_hw_teardown() on some paths, and efcport_init() leaves
its first two pools behind when the third allocation fails. Those cross
two modules and I would rather send them separately once this is settled.

v1: https://lore.kernel.org/all/20260806192345.328621-1-ali@iusegentoo.com/

Ali Ahmet Memis (5):
  scsi: elx: efct: check the HW state before allocating an HIO
  scsi: elx: efct: free the HIO when the WQE cannot be built
  scsi: elx: efct: free the request tag when the send frame WQE fails
  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 | 74 +++++++++++++++++++++------------
 1 file changed, 47 insertions(+), 27 deletions(-)


base-commit: 0d839570765118029aa8bf4a95444c6a11aacf85
-- 
2.55.0

[PATCH v2 1/5] scsi: elx: efct: check the HW state before allocating an HIO

[Ali Ahmet Memis]

efct_els_hw_srrs_send() takes an HIO from the pool and only then looks at
hw->state, returning without giving it back when the HW is not active:

	hio = efct_hw_io_alloc(hw);
	if (!hio) {
		pr_err("HIO alloc failed\n");
		return -EIO;
	}

	if (hw->state != EFCT_HW_STATE_ACTIVE) {
		efc_log_debug(hw->os,
			      "cannot send SRRS, HW state=%d\n", hw->state);
		return -EIO;
	}

_efct_hw_io_alloc() moves the entry from hw->io_free to hw->io_inuse and
initialises its reference, and the only thing that puts it back is the
completion of a submitted WQE. Nothing is submitted here, so the entry
stays on hw->io_inuse for the lifetime of the adapter. The memory is
reclaimed in efct_hw_teardown(), which frees hw->io[] as a whole, but the
pool loses one usable entry for every ELS or CT send that takes this
path. Once it is empty efct_hw_io_alloc() starts failing and no further
ELS or CT traffic can be sent.

Check the state first, the way efct_hw_bls_send() already does.

Fixes: dd53d333aadb ("scsi: elx: efct: Hardware I/O submission routines")
Signed-off-by: Ali Ahmet Memis <ali@iusegentoo.com>
---
 drivers/scsi/elx/efct/efct_hw.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/elx/efct/efct_hw.c b/drivers/scsi/elx/efct/efct_hw.c
index b79c6a7ea791..15c37ff1d52e 100644
--- a/drivers/scsi/elx/efct/efct_hw.c
+++ b/drivers/scsi/elx/efct/efct_hw.c
@@ -2706,18 +2706,18 @@ efct_els_hw_srrs_send(struct efc *efc, struct efc_disc_io *io)
 	u32 sge0_flags;
 	u32 sge1_flags;
 
-	hio = efct_hw_io_alloc(hw);
-	if (!hio) {
-		pr_err("HIO alloc failed\n");
-		return -EIO;
-	}
-
 	if (hw->state != EFCT_HW_STATE_ACTIVE) {
 		efc_log_debug(hw->os,
 			      "cannot send SRRS, HW state=%d\n", hw->state);
 		return -EIO;
 	}
 
+	hio = efct_hw_io_alloc(hw);
+	if (!hio) {
+		pr_err("HIO alloc failed\n");
+		return -EIO;
+	}
+
 	hio->done = efct_els_ssrs_send_cb;
 	hio->arg  = io;
 
-- 
2.55.0

[PATCH v2 2/5] scsi: elx: efct: free the HIO when the WQE cannot be built

[Ali Ahmet Memis]

efct_els_hw_srrs_send() and efct_hw_bls_send() allocate an HIO, ask sli4
to build a WQE into it, and give up when that fails without putting the
HIO back:

	if (sli_els_request64_wqe(&hw->sli, hio->wqe.wqebuf, hio->sgl,
				  &els_params)) {
		efc_log_err(hw->os, "REQ WQE error\n");
		rc = -EIO;
	}

Nothing has been submitted at that point, so no completion will arrive to
release it, and the entry sits on hw->io_inuse until the adapter is torn
down. Each failure costs the pool one entry, and once it is empty
efct_hw_io_alloc() fails and no further ELS, CT or BLS frame can be sent.

Release the HIO on those paths. The efct_hw_wq_write() failure below is
deliberately left alone: it can return an error while this request is
still queued on wq->pending_list, so the HIO cannot be handed back there
without more care.

Fixes: dd53d333aadb ("scsi: elx: efct: Hardware I/O submission routines")
Signed-off-by: Ali Ahmet Memis <ali@iusegentoo.com>
---
 drivers/scsi/elx/efct/efct_hw.c | 38 ++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/drivers/scsi/elx/efct/efct_hw.c b/drivers/scsi/elx/efct/efct_hw.c
index 15c37ff1d52e..6cc48fa3e656 100644
--- a/drivers/scsi/elx/efct/efct_hw.c
+++ b/drivers/scsi/elx/efct/efct_hw.c
@@ -2609,6 +2609,7 @@ efct_hw_bls_send(struct efct *efct, u32 type, struct sli_bls_params *bls_params,
 	if (sli_xmit_bls_rsp64_wqe(&hw->sli, hio->wqe.wqebuf,
 				   &bls, bls_params)) {
 		efc_log_err(hw->os, "XMIT_BLS_RSP64 WQE error\n");
+		efct_hw_io_free(hw, hio);
 		return -EIO;
 	}
 
@@ -2820,24 +2821,27 @@ efct_els_hw_srrs_send(struct efc *efc, struct efc_disc_io *io)
 		rc = -EIO;
 	}
 
-	if (rc == 0) {
-		hio->xbusy = true;
+	if (rc) {
+		efct_hw_io_free(hw, hio);
+		return rc;
+	}
 
-		/*
-		 * Add IO to active io wqe list before submitting, in case the
-		 * wcqe processing preempts this thread.
-		 */
-		hio->wq->use_count++;
-		rc = efct_hw_wq_write(hio->wq, &hio->wqe);
-		if (rc >= 0) {
-			/* non-negative return is success */
-			rc = 0;
-		} else {
-			/* failed to write wqe, remove from active wqe list */
-			efc_log_err(hw->os,
-				    "sli_queue_write failed: %d\n", rc);
-			hio->xbusy = false;
-		}
+	hio->xbusy = true;
+
+	/*
+	 * Add IO to active io wqe list before submitting, in case the
+	 * wcqe processing preempts this thread.
+	 */
+	hio->wq->use_count++;
+	rc = efct_hw_wq_write(hio->wq, &hio->wqe);
+	if (rc >= 0) {
+		/* non-negative return is success */
+		rc = 0;
+	} else {
+		/* failed to write wqe, remove from active wqe list */
+		efc_log_err(hw->os,
+			    "sli_queue_write failed: %d\n", rc);
+		hio->xbusy = false;
 	}
 
 	return rc;
-- 
2.55.0

[PATCH v2 3/5] scsi: elx: efct: free the request tag when the send frame WQE fails

[Ali Ahmet Memis]

efct_hw_send_frame() takes a request tag from the pool and then builds the
WQE. When sli_send_frame_wqe() fails it returns without giving the tag
back:

	ctx->wqcb = efct_hw_reqtag_alloc(hw, callback, arg);
	if (!ctx->wqcb) {
		efc_log_err(hw->os, "can't allocate request tag\n");
		return -ENOSPC;
	}
	...
	if (rc) {
		efc_log_err(hw->os, "sli_send_frame_wqe failed: %d\n", rc);
		return -EIO;
	}

Nothing is submitted, so the completion that would call
efct_hw_reqtag_free() never runs and the tag stays out of the pool. The
pool is bounded by the number of request tags allocated at init, so
repeated failures leave send frame without any.

Free the tag on that path. The efct_hw_wq_write() failure below is left
alone: it can return an error while this request is still queued on
wq->pending_list, and the tag is what a later completion would look the
context up by.

Fixes: dd53d333aadb ("scsi: elx: efct: Hardware I/O submission routines")
Signed-off-by: Ali Ahmet Memis <ali@iusegentoo.com>
---
 drivers/scsi/elx/efct/efct_hw.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/elx/efct/efct_hw.c b/drivers/scsi/elx/efct/efct_hw.c
index 6cc48fa3e656..db68516e8075 100644
--- a/drivers/scsi/elx/efct/efct_hw.c
+++ b/drivers/scsi/elx/efct/efct_hw.c
@@ -3009,6 +3009,7 @@ efct_hw_send_frame(struct efct_hw *hw, struct fc_frame_header *hdr,
 				ctx->wqcb->instance_index);
 	if (rc) {
 		efc_log_err(hw->os, "sli_send_frame_wqe failed: %d\n", rc);
+		efct_hw_reqtag_free(hw, ctx->wqcb);
 		return -EIO;
 	}
 
-- 
2.55.0

[PATCH v2 4/5] 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.

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 db68516e8075..d645ce256b8a 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

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

[Ali Ahmet Memis]

efct_hw_setup() creates two mempools and then calls sli_setup(). Both of
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 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 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 d645ce256b8a..f764fde665b0 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 v2 4/5] scsi: elx: efct: free the RQ buffers already allocated when one fails

[sashiko-bot]

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

Pre-existing issues:
- [High] Extensive memory and DMA buffer leaks on device probe and initialization error paths.
- [High] Double-free and use-after-free vulnerability on the error path of efct_hw_new_rq_set().
- [High] Unsafe teardown of stats_timer leading to Use-After-Free and timer wheel corruption.
- [High] Invalid use of `timer_setup()` within the timer callback.
- [High] Error path in efct_hw_setup_io() leaks successfully allocated hw->io[i] inner objects.
--

--- Patch [4]: [PATCH v2 4/5] scsi: elx: efct: free the RQ buffers already allocated when one fails ---
commit be8b053b462b939279de3cc82f2caf1d37ac0767
Author: Ali Ahmet Memis <ali@iusegentoo.com>

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

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. This commit uses efct_hw_rx_buffer_free() for the entries that
were filled in to fix the leak.

> diff --git a/drivers/scsi/elx/efct/efct_hw.c b/drivers/scsi/elx/efct/efct_hw.c
> index db68516e80750..d645ce256b8aa 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;
>  		}
>  	}

[Severity: High]
This is a pre-existing issue, but does this error path still leak memory?

In efct_hw_rx_allocate():

	if (!rq->hdr_buf) {
		efc_log_err(efct, "rx_buffer_alloc hdr_buf failed\n");
		rc = -EIO;
		break;
	}

If efct_hw_rx_buffer_alloc() fails for any queue index, it breaks the loop
and returns -EIO, but it appears to leak all hdr_buf and payload_buf DMA
buffer arrays that were successfully allocated in previous iterations.

Furthermore, in efct_hw_init():

	rc = efct_hw_rx_allocate(hw);
	if (rc) {
		efc_log_err(hw->os, "rx_allocate failed\n");
		return rc;
	}

It returns early on nested allocation failures without cleaning up its own
previous local allocations.

Finally, in efct_xport_initialize():

	rc = efct_hw_init(&efct->hw);
	if (rc) {
		efc_log_err(efct, "efct_hw_init failure\n");
		goto out;
	}

It jumps directly to out: on efct_hw_init() failure, skipping the hw_init_out:
label where efct_hw_teardown() is called. This seems to leak hardware
resources initialized during efct_hw_setup() and efct_hw_init() when the probe
fails.


[Severity: High]
This is a pre-existing issue, but can this error path lead to a double-free?

In efct_hw_new_rq_set():

	list_add_tail(&rqs[i]->list_entry, &cqs[i]->q_list);
	size = sizeof(struct efc_hw_sequence *) * rqs[i]->entry_count;
	rqs[i]->rq_tracker = kzalloc(size, GFP_KERNEL);
	if (!rqs[i]->rq_tracker)
		goto error;

If kzalloc() fails, it jumps to the error label:

error:
	for (i = 0; i < num_rq_pairs; i++) {
		if (rqs[i]) {
			kfree(rqs[i]->rq_tracker);
			kfree(rqs[i]);
		}
	}

This frees rqs[i] but does not remove it from the cqs[i]->q_list. The function
then returns -EIO, triggering efct_hw_queue_teardown(hw), which calls
efct_hw_del_rq():

	list_del(&rq->list_entry);
	hw = rq->cq->eq->hw;
	hw->hw_rq[rq->instance] = NULL;
	kfree(rq);

The teardown iterates over the list, dereferences the already freed rq object,
calls list_del() on it, and then calls kfree(rq) again. Does this double-free
the object and corrupt the list?


[Severity: High]
This is a pre-existing issue, but is the teardown of stats_timer safe here?

In efct_xport_detach():

	if (timer_pending(&xport->stats_timer))
		timer_delete(&xport->stats_timer);

If the timer callback efct_xport_stats_timer_cb() is actively running on
another CPU, timer_pending() might return false. Even if timer_delete()
is called, it does not wait for the concurrent callback to finish.

This could allow efct_xport_detach() to proceed and eventually free the xport
object while the timer callback is still executing. The callback would then
access the freed xport memory and call mod_timer(), inserting a freed timer
structure back into the kernel's active timer wheel. Should this
unconditionally use timer_delete_sync() without the timer_pending() check?


[Severity: High]
This is a pre-existing issue, but is it correct to call timer_setup() here?

In efct_xport_config_stats_timer():

	timer_setup(&xport->stats_timer,
		    &efct_xport_stats_timer_cb, 0);
	mod_timer(&xport->stats_timer,
		  jiffies + msecs_to_jiffies(timeout));

This function is called both for initial setup and from within the timer
callback itself (efct_xport_stats_timer_cb()) to re-arm the timer.

Calling timer_setup() on a timer that is actively being processed by the timer
subsystem resets the structure, clearing list pointers and lock-related fields.
Does this destroy the active timer structure and lead to kernel state
corruption?


[Severity: High]
This is a pre-existing issue, but does this error path leak the inner IO
objects?

In efct_hw_setup_io():

	hw->wqe_buffs = kzalloc((hw->config.n_io * hw->sli.wqe_size),
				GFP_KERNEL);
	if (!hw->wqe_buffs) {
		kfree(hw->io);
		return -ENOMEM;
	}

If the allocation for hw->wqe_buffs fails, the code frees the outer hw->io
array and returns -ENOMEM. However, it appears to completely skip the loop in
the error block that frees the individual hw->io[i] objects allocated prior to
this, which would permanently leak them.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260806192345.328621-1-ali@iusegentoo.com?part=4

Re: [PATCH v2 3/5] scsi: elx: efct: free the request tag when the send frame WQE fails

[sashiko-bot]

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

New issues:
- [Medium] The patch adds cleanup code to a mathematically unreachable error path, completely failing to address the actual root cause of the reqtag exhaustion.

Pre-existing issues:
- [Critical] Unconditional freeing of a reused sequence buffer (`seq`) leads to a catastrophic Use-After-Free, Double-Free, and hardware DMA corruption when sending a task set full/busy response.
- [High] Guaranteed NULL pointer dereference when sending a task set full/busy response.
- [High] `efct_hw_reqtag_free()` detects a double-free but incorrectly proceeds to corrupt the freelist.
- [High] `efct_hw_init()` fails to handle `efct_hw_io_alloc()` failure, leading to a NULL pointer dereference.
--

--- Patch [3]: [PATCH v2 3/5] scsi: elx: efct: free the request tag when the send frame WQE fails ---
commit 415b71fb97ba10c30ffb204dfd40acdb37bb24d1
Author: Ali Ahmet Memis <ali@iusegentoo.com>

scsi: elx: efct: free the request tag when the send frame WQE fails

efct_hw_send_frame() takes a request tag from the pool and then builds the
WQE. When sli_send_frame_wqe() fails it returns without giving the tag
back.

> diff --git a/drivers/scsi/elx/efct/efct_hw.c b/drivers/scsi/elx/efct/efct_hw.c
> index 6cc48fa3e656a..db68516e80750 100644
> --- a/drivers/scsi/elx/efct/efct_hw.c
> +++ b/drivers/scsi/elx/efct/efct_hw.c
> @@ -3009,6 +3009,7 @@ efct_hw_send_frame(struct efct_hw *hw, struct fc_frame_header *hdr,
>  				ctx->wqcb->instance_index);
>  	if (rc) {
>  		efc_log_err(hw->os, "sli_send_frame_wqe failed: %d\n", rc);
> +		efct_hw_reqtag_free(hw, ctx->wqcb);
>  		return -EIO;
>  	}

[Severity: Medium]
Does this error path actually ever execute?

Looking at sli_send_frame_wqe(), it appears to unconditionally return 0:

libefc_sli/sli4.c:sli_send_frame_wqe() {
    ...
    return 0;
}

Since sli_send_frame_wqe() always returns 0, this error handling block seems
unreachable. Is there a different root cause for the reqtag exhaustion?


While reviewing this change, I also noticed a few other areas in the driver
that might need attention.

[Severity: Critical]
This isn't a bug introduced by this patch, but does efct_dispatch_frame()
unconditionally free a reused sequence buffer, causing a use-after-free?

When efct_scsi_io_alloc() fails, the driver sends a task set full/busy
response via efct_sframe_common_send(). It allocates the async transmission
context directly inside the incoming sequence's DMA buffer:

efct_unsol.c:efct_sframe_common_send() {
    ctx = (void *)(heap_virt_base + heap_offset);
    ...
    ctx->seq = seq;
}

However, efct_dispatch_frame() ignores the return value of
efct_dispatch_fcp_cmd() and unconditionally frees the sequence:

efct_unsol.c:efct_dispatch_frame() {
    kref_put(&node->ref, node->release);
    efct_hw_sequence_free(&efct->hw, seq);
    return 0;
}

When transmission completes, efct_sframe_common_send_cb() double-frees the
sequence:

efct_unsol.c:efct_sframe_common_send_cb() {
    /* Free sequence */
    efct_hw_sequence_free(hw, ctx->seq);
}

Could this cause memory corruption and hardware DMA issues if the buffer is
returned to the hardware receive ring before the async send completes?


[Severity: High]
This is a pre-existing issue, but does sending a task set full/busy response
result in a NULL pointer dereference?

In efct_sframe_common_send(), the context is zeroed out, leaving wqebuf as
NULL:

efct_unsol.c:efct_sframe_common_send() {
    memset(ctx, 0, sizeof(*ctx));
    ...
}

This uninitialized context is passed to efct_hw_send_frame(), which passes
the NULL wqe->wqebuf to sli_send_frame_wqe():

efct_hw.c:efct_hw_send_frame() {
    rc = sli_send_frame_wqe(&hw->sli, wqe->wqebuf, ...);
}

sli_send_frame_wqe() immediately calls memset() on this NULL buffer:

libefc_sli/sli4.c:sli_send_frame_wqe() {
    memset(buf, 0, sli->wqe_size);
}

Will this guarantee a kernel panic when triggered?


[Severity: High]
This isn't a bug introduced by this patch, but does efct_hw_reqtag_free()
corrupt the freelist if it detects a double-free?

If a tag is already freed, the function logs an error but doesn't return
early:

efct_hw.c:efct_hw_reqtag_free() {
    if (!wqcb->callback)
        efc_log_err(hw->os, "WQCB is already freed\n");

    spin_lock_irqsave(&reqtag_pool->lock, flags);
    wqcb->callback = NULL;
    ...
    INIT_LIST_HEAD(&wqcb->list_entry);
    list_add(&wqcb->list_entry, &hw->wq_reqtag_pool->freelist);
    spin_unlock_irqrestore(&reqtag_pool->lock, flags);
}

If the tag is already in the freelist, doesn't continuing with
INIT_LIST_HEAD() and list_add() completely corrupt the list pointers?


[Severity: High]
This is a pre-existing issue, but does an allocation failure in
efct_hw_init() lead to a NULL pointer dereference later?

If efct_hw_io_alloc() fails, it logs an error but returns success:

efct_hw.c:efct_hw_init() {
    hw->hw_wq[0]->send_frame_io = efct_hw_io_alloc(hw);
    if (!hw->hw_wq[0]->send_frame_io)
        efc_log_err(hw->os, "alloc for send_frame_io failed\n");

    /* Initialize send frame sequence id */
    atomic_set(&hw->send_frame_seq_id, 0);

    return 0;
}

Later, efct_hw_send_frame() unconditionally dereferences it:

efct_hw.c:efct_hw_send_frame() {
    xri = wq->send_frame_io->indicator;
}

Should efct_hw_init() return an error code instead of 0 when this
allocation fails?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260806192345.328621-1-ali@iusegentoo.com?part=3

Re: [PATCH v2 2/5] scsi: elx: efct: free the HIO when the WQE cannot be built

[sashiko-bot]

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

Pre-existing issues:
- [High] The Hardware I/O context (hio) is leaked on every successful completion of ELS, CT, and BLS commands.
--

--- Patch [2]: [PATCH v2 2/5] scsi: elx: efct: free the HIO when the WQE cannot be built ---
commit d739587d3893fac37f827b9d5a082304c53cf476
Author: Ali Ahmet Memis <ali@iusegentoo.com>
Date:   Thu Aug 6 20:22:02 2026 +0000

    scsi: elx: efct: free the HIO when the WQE cannot be built
    
    efct_els_hw_srrs_send() and efct_hw_bls_send() allocate an HIO, ask sli4
    to build a WQE into it, and give up when that fails without putting the
    HIO back:

[ ... ]

> diff --git a/drivers/scsi/elx/efct/efct_hw.c b/drivers/scsi/elx/efct/efct_hw.c
> index 15c37ff1d52e4..6cc48fa3e656a 100644
> --- a/drivers/scsi/elx/efct/efct_hw.c
> +++ b/drivers/scsi/elx/efct/efct_hw.c
> @@ -2609,6 +2609,7 @@ efct_hw_bls_send(struct efct *efct, u32 type, struct sli_bls_params *bls_params,
>  	if (sli_xmit_bls_rsp64_wqe(&hw->sli, hio->wqe.wqebuf,
>  				   &bls, bls_params)) {
>  		efc_log_err(hw->os, "XMIT_BLS_RSP64 WQE error\n");
> +		efct_hw_io_free(hw, hio);
>  		return -EIO;
>  	}
>  

[Severity: High]
This is a pre-existing issue, but does this code also leak the hio on the
success path?

Looking at the completion callback for BLS commands, efct_bls_send_rjt_cb()
receives the hio but completely ignores it:

drivers/scsi/elx/efct/efct_scsi.c:efct_bls_send_rjt_cb() {
    struct efct_io *io = app;

    efct_scsi_io_free(io);
    return 0;
}

Because the generic hardware completion handler efct_hw_wq_process_io() doesn't
free the hio either, doesn't this result in the hio being leaked on every
successful completion?

If the pool of hardware IO contexts exhausts, won't efct_hw_io_alloc() fail for
all subsequent requests, breaking Fibre Channel discovery, ELS, and BLS
operations?

> @@ -2820,24 +2821,27 @@ efct_els_hw_srrs_send(struct efc *efc, struct efc_disc_io *io)
>  		rc = -EIO;
>  	}
>  
> -	if (rc == 0) {
> -		hio->xbusy = true;
> +	if (rc) {
> +		efct_hw_io_free(hw, hio);
> +		return rc;
> +	}

[Severity: High]
This is a pre-existing issue, but does this function suffer from the same leak
on the success path?

The ELS completion callback receives the hio but also ignores it:

drivers/scsi/elx/efct/efct_hw.c:efct_els_ssrs_send_cb() {
    struct efc_disc_io *io = arg;

    efc_disc_io_complete(io, length, status, ext_status);
    return 0;
}

Should the completion callbacks be responsible for calling efct_hw_io_free(), or
should that be handled by the generic caller?

> -		/*
> -		 * Add IO to active io wqe list before submitting, in case the
> -		 * wcqe processing preempts this thread.
> -		 */
> -		hio->wq->use_count++;
> -		rc = efct_hw_wq_write(hio->wq, &hio->wqe);
> -		if (rc >= 0) {
> -			/* non-negative return is success */
> -			rc = 0;
> -		} else {
> -			/* failed to write wqe, remove from active wqe list */
> -			efc_log_err(hw->os,
> -				    "sli_queue_write failed: %d\n", rc);
> -			hio->xbusy = false;
> -		}
> +	hio->xbusy = true;
> +
> +	/*
> +	 * Add IO to active io wqe list before submitting, in case the
> +	 * wcqe processing preempts this thread.
> +	 */
> +	hio->wq->use_count++;
> +	rc = efct_hw_wq_write(hio->wq, &hio->wqe);
> +	if (rc >= 0) {
> +		/* non-negative return is success */
> +		rc = 0;
> +	} else {
> +		/* failed to write wqe, remove from active wqe list */
> +		efc_log_err(hw->os,
> +			    "sli_queue_write failed: %d\n", rc);
> +		hio->xbusy = false;
>  	}
>  
>  	return rc;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260806192345.328621-1-ali@iusegentoo.com?part=2

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

[sashiko-bot]

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

Pre-existing issues:
- [High] The global list `efct_devices` is modified and accessed concurrently without any locking, causing list corruption and use-after-free races.
--

--- Patch [5]: [PATCH v2 5/5] scsi: elx: efct: destroy the mailbox pools when setup fails ---
commit 250c50655b4add3b10d0ee9bad5b35495210fdcb
Author: Ali Ahmet Memis <ali@iusegentoo.com>

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

This commit prevents memory leaks by destroying mailbox mempools in 
the error paths of efct_hw_setup() before returning.

> 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 when it fails efct_pci_probe()
> unwinds through efct_device_free(), freeing the struct efct that held the
> only pointers to the pools.

[Severity: High]
This is a pre-existing issue, but since the probe error path unwinds through 
efct_device_free(), does this code safely handle concurrent accesses to the 
global device list?

Looking at efct_device_alloc() in drivers/scsi/elx/efct/efct_driver.c, the 
device is added to the global efct_devices list without locking:

    list_add_tail(&efct->list_entry, &efct_devices);

When unwinding through efct_device_free(), it is removed and immediately freed:

    list_del(&efct->list_entry);
    kfree(efct);

Meanwhile, userspace configfs operations can trigger efct_find_wwpn() in 
drivers/scsi/elx/efct/efct_lio.c, which traverses this same list locklessly:

    list_for_each_entry(efct, &efct_devices, list_entry) {

Could concurrent probe/remove operations and configfs reads race here, 
causing list_for_each_entry() to hit corrupted list pointers or trigger 
a use-after-free on the released efct object?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260806192345.328621-1-ali@iusegentoo.com?part=5

Re: [PATCH v2 1/5] scsi: elx: efct: check the HW state before allocating an HIO

[sashiko-bot]

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

Pre-existing issues:
- [High] Pre-existing resource leak of `hio` (Hardware I/O object) in `efct_hw_bls_send()` when WQE formatting fails.
--

--- Patch [1]: [PATCH v2 1/5] scsi: elx: efct: check the HW state before allocating an HIO ---
commit dc19c507aa2788a2b5120f2a9e8e09430d14b440
Author: Ali Ahmet Memis <ali@iusegentoo.com>

scsi: elx: efct: check the HW state before allocating an HIO

efct_els_hw_srrs_send() takes an HIO from the pool and only then looks at
hw->state, returning without giving it back when the HW is not active. This
commit fixes the issue by moving the state check before allocating the HIO.

> Check the state first, the way efct_hw_bls_send() already does.

[Severity: High]
This is a pre-existing issue, but while efct_hw_bls_send() correctly checks
the state first, does it still have a resource leak when WQE formatting fails?

In efct_hw_bls_send(), an HIO is allocated via efct_hw_io_alloc(). If the
subsequent call to sli_xmit_bls_rsp64_wqe() fails, it returns -EIO directly
without releasing hio back to the free list:

drivers/scsi/elx/efct/efct_hw.c:efct_hw_bls_send() {
...
	hio = efct_hw_io_alloc(hw);
...
	if (sli_xmit_bls_rsp64_wqe(&hw->sli, hio->wqe.wqebuf,
				   &bls, bls_params)) {
		efc_log_err(hw->os, "XMIT_BLS_RSP64 WQE error\n");
		return -EIO;
	}
...
}

Should this call efct_hw_io_free(hw, hio) before returning -EIO? Once the
pool is exhausted, efct_hw_io_alloc() will permanently fail.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260806192345.328621-1-ali@iusegentoo.com?part=1