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

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 <[email protected]>

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