[PATCH] scsi: elx: efct: Use timer_shutdown_sync() for the stats timer

[PATCH] scsi: elx: efct: Use timer_shutdown_sync() for the stats timer

[Fan Wu]

The statistics timer re-arms itself on every expiry through
efct_xport_config_stats_timer().  timer_delete() only detaches a pending
timer: it does not wait for a callback that is already running and does
not stop that callback from re-arming the timer.  In efct_xport_detach()
the timer is stopped before efct_xport_free() frees the xport,
so a callback that re-armed it can fire after the free and dereference the
freed object.  efct_fw_reset() stops the same timer before a hardware
reset, where a callback left running can race the reset and re-arm the
timer.

timer_shutdown_sync() waits for a running callback to finish and
guarantees that the timer cannot be rearmed once it returns, so use it at
both points.  The now-pointless timer_pending() checks are dropped.

This issue was found by an in-house static analysis tool.

Fixes: ab332fcbcd81 ("scsi: elx: efct: Transport and hardware teardown routines")
Cc: stable@vger.kernel.org # v6.2+
Assisted-by: Codex:gpt-5.6
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---
 drivers/scsi/elx/efct/efct_driver.c | 3 +--
 drivers/scsi/elx/efct/efct_xport.c  | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/elx/efct/efct_driver.c b/drivers/scsi/elx/efct/efct_driver.c
index 07c2f4534..3556c6d8a 100644
--- a/drivers/scsi/elx/efct/efct_driver.c
+++ b/drivers/scsi/elx/efct/efct_driver.c
@@ -309,8 +309,7 @@ efct_fw_reset(struct efct *efct)
 	 * Function 0 will update and load the new firmware
 	 * during attach.
 	 */
-	if (timer_pending(&efct->xport->stats_timer))
-		timer_delete(&efct->xport->stats_timer);
+	timer_shutdown_sync(&efct->xport->stats_timer);
 
 	if (efct_hw_reset(&efct->hw, EFCT_HW_RESET_FIRMWARE)) {
 		efc_log_info(efct, "failed to reset firmware\n");
diff --git a/drivers/scsi/elx/efct/efct_xport.c b/drivers/scsi/elx/efct/efct_xport.c
index 9dcaef6fc..84fa5589e 100644
--- a/drivers/scsi/elx/efct/efct_xport.c
+++ b/drivers/scsi/elx/efct/efct_xport.c
@@ -507,8 +507,7 @@ efct_xport_detach(struct efct_xport *xport)
 	efct_scsi_del_device(efct);
 
 	/*Shutdown FC Statistics timer*/
-	if (timer_pending(&xport->stats_timer))
-		timer_delete(&xport->stats_timer);
+	timer_shutdown_sync(&xport->stats_timer);
 
 	efct_hw_teardown(&efct->hw);
 
-- 
2.34.1

Re: [PATCH] scsi: elx: efct: Use timer_shutdown_sync() for the stats timer

[sashiko-bot]

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

Pre-existing issues:
- [High] The static global workqueue `lio_wq` is re-allocated per-device but never destroyed, leaking memory and leading to a Use-After-Free when multiple devices are present.
- [High] The timer callback indirectly re-initializes the timer on every expiration by calling timer_setup(), which violates timer API constraints, creates a data race, and corrupts the lockdep state.
--

--- Patch [1]: [PATCH] scsi: elx: efct: Use timer_shutdown_sync() for the stats timer ---
commit caf9ba0b14580d400c5a8efd53a50eb0210dd8ce
Author: Fan Wu <fanwu01@zju.edu.cn>

scsi: elx: efct: Use timer_shutdown_sync() for the stats timer

This commit replaces timer_delete() with timer_shutdown_sync() for the
statistics timer in the efct driver. This guarantees that running callbacks
finish before the transport is freed or hardware is reset.

Cc: stable@vger.kernel.org # v6.2+

> The statistics timer re-arms itself on every expiry through
> efct_xport_config_stats_timer().

[Severity: High]
This is a pre-existing issue, but does efct_xport_config_stats_timer()
re-initialize the timer instead of just re-arming it?

If the callback calls timer_setup() on every expiration:

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

Calling timer_setup() on an active timer overwrites timer->flags without
synchronization.

Does this create a data race with concurrent timer API calls and cause lockdep
or debugobjects warnings about initializing an active object?

> diff --git a/drivers/scsi/elx/efct/efct_xport.c b/drivers/scsi/elx/efct/efct_xport.c
> index 9dcaef6fc188e..84fa5589e0213 100644
> --- a/drivers/scsi/elx/efct/efct_xport.c
> +++ b/drivers/scsi/elx/efct/efct_xport.c
> @@ -507,8 +507,7 @@ efct_xport_detach(struct efct_xport *xport)
>  	efct_scsi_del_device(efct);

[Severity: High]
This isn't a bug introduced by this patch, but does the teardown process leak
a global workqueue if multiple devices are present?

When multiple EFCT adapters are initialized, efct_scsi_tgt_new_device()
unconditionally overwrites a static global workqueue without destroying the
previous one:

drivers/scsi/elx/efct/efct_lio.c:efct_scsi_tgt_new_device() {
    ...
    lio_wq = alloc_ordered_workqueue("%s", WQ_MEM_RECLAIM, "efct_lio_worker");
    ...
}

During teardown, efct_scsi_tgt_del_device() flushes lio_wq, which now points
only to the latest allocated workqueue.

Will this fail to flush works queued for earlier devices, potentially leading
to a use-after-free on efct structures if those works execute concurrently
while the older device is torn down?

Is destroy_workqueue() also completely missing, causing a memory leak of the
workqueue structures themselves?

>  
>  	/*Shutdown FC Statistics timer*/
> -	if (timer_pending(&xport->stats_timer))
> -		timer_delete(&xport->stats_timer);
> +	timer_shutdown_sync(&xport->stats_timer);
>  
>  	efct_hw_teardown(&efct->hw);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260810123443.122397-1-fanwu01@zju.edu.cn?part=1