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

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 <[email protected]>
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/[email protected]?part=4