[PATCH v2] scsi: dc395x: sync the waiting_timer before freeing the host

[PATCH v2] scsi: dc395x: sync the waiting_timer before freeing the host

[Fan Wu]

The waiting_timer callback re-arms itself and takes the host lock.
adapter_uninit() currently stops it with timer_delete(), which does not
wait for a callback already running on another CPU. The final
scsi_host_put() that frees the host (and the host lock the callback
dereferences) runs right after adapter_uninit(), so a concurrent or just
re-armed callback can fire after the free, leaving a potential
use-after-free window.

Replace timer_delete() with timer_shutdown_sync() at the top of
adapter_uninit(), before the host-lock section: the timer is dequeued,
a running callback is waited for and rearming is blocked before the chip
is halted, so it cannot fire at all once the adapter has ceased to
function. Waiting outside the lock avoids the self-deadlock that would
result from syncing a callback blocked on the lock we still hold.

No shutdown is needed for selto_timer because it has no reachable armer
or callback.

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

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Cc: Jamie Lenehan <lenehan@twibble.org>
Assisted-by: Codex:gpt-5.6
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---
v2:
  - Move timer_shutdown_sync() before adapter_uninit_chip(), as
    suggested by Oliver Neukum.
v1: https://lore.kernel.org/all/20260810055028.119525-1-fanwu01@zju.edu.cn/

 drivers/scsi/dc395x.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/dc395x.c b/drivers/scsi/dc395x.c
index 6183ce05d..fcd4c0029 100644
--- a/drivers/scsi/dc395x.c
+++ b/drivers/scsi/dc395x.c
@@ -3814,13 +3814,11 @@ static void adapter_uninit_chip(struct AdapterCtlBlk *acb)
 static void adapter_uninit(struct AdapterCtlBlk *acb)
 {
 	unsigned long flags;
-	DC395x_LOCK_IO(acb->scsi_host, flags);
 
-	/* remove timers */
-	if (timer_pending(&acb->waiting_timer))
-		timer_delete(&acb->waiting_timer);
-	if (timer_pending(&acb->selto_timer))
-		timer_delete(&acb->selto_timer);
+	/* Drain the self-rearming timer; must not run under host_lock. */
+	timer_shutdown_sync(&acb->waiting_timer);
+
+	DC395x_LOCK_IO(acb->scsi_host, flags);
 
 	adapter_uninit_chip(acb);
 	adapter_remove_and_free_all_devices(acb);
-- 
2.34.1

Re: [PATCH v2] scsi: dc395x: sync the waiting_timer before freeing the host

[sashiko-bot]

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

Pre-existing issues:
- [High] DeviceCtlBlk fields 'init_tcq_flag' and 'dev_type' are left uninitialized in device_alloc() due to kmalloc usage.
- [High] Flawed list_size check in adapter_remove_and_free_device causes either a Use-After-Free or a Memory Leak.
- [High] The reset_scsi_bus() function polls a hardware register without any timeout. If the hardware is unresponsive or returns 0, the driver will hang the CPU by spinning infinitely.
--

commit 9eeaccf553daafad4a08c01c06f5487a6aed8afe
Author: Fan Wu <fanwu01@zju.edu.cn>

scsi: dc395x: sync the waiting_timer before freeing the host
    
The waiting_timer callback re-arms itself and takes the host lock.
adapter_uninit() currently stops it with timer_delete(), which does not
wait for a callback already running on another CPU.

> diff --git a/drivers/scsi/dc395x.c b/drivers/scsi/dc395x.c
> index 10e2078f4a0fd..5a4f2713dcfa0 100644
> --- a/drivers/scsi/dc395x.c
> +++ b/drivers/scsi/dc395x.c
> @@ -3808,13 +3808,11 @@ static void adapter_uninit_chip(struct AdapterCtlBlk *acb)
>  static void adapter_uninit(struct AdapterCtlBlk *acb)
>  {
>  	unsigned long flags;
> -	DC395x_LOCK_IO(acb->scsi_host, flags);
>  
> -	/* remove timers */
> -	if (timer_pending(&acb->waiting_timer))
> -		timer_delete(&acb->waiting_timer);
> -	if (timer_pending(&acb->selto_timer))
> -		timer_delete(&acb->selto_timer);
> +	/* Drain the self-rearming timer; must not run under host_lock. */
> +	timer_shutdown_sync(&acb->waiting_timer);
> +
> +	DC395x_LOCK_IO(acb->scsi_host, flags);
>  
>  	adapter_uninit_chip(acb);

[Severity: High]
This is a pre-existing issue, but does this code spin infinitely if the
hardware is unresponsive?

Looking at reset_scsi_bus() which is called by adapter_uninit_chip(), it
polls without a timeout or cpu_relax():

drivers/scsi/dc395x.c:reset_scsi_bus() {
    ...
	while (!(DC395x_read8(acb, TRM_S1040_SCSI_INTSTATUS) & INT_SCSIRESET))
		/* nothing */;
}

If the hardware fails to assert the reset interrupt, wouldn't this while loop
spin indefinitely and cause a hard CPU lockup since it is called under
host_lock?

>  	adapter_remove_and_free_all_devices(acb);

[Severity: High]
This isn't a bug introduced by this patch, but does the logic here cause
either a use-after-free or a memory leak depending on the list size?

When adapter_remove_and_free_all_devices() calls
adapter_remove_and_free_device():

drivers/scsi/dc395x.c:adapter_remove_and_free_device() {
	if (list_size(&dcb->srb_going_list) > 1) {
		return;
	}
	adapter_remove_device(acb, dcb);
	kfree(dcb);
}

If there is exactly 1 active command in the list, list_size evaluates to 1,
causing the condition to be false and dcb to be freed. When the active
command completes later, wouldn't it attempt to access this freed memory?

Conversely, if list_size > 1, the function returns early without removing or
freeing the dcb. Does this permanently leak the dcb?

>  	DC395x_UNLOCK_IO(acb->scsi_host, flags);

[ ... ]

[Severity: High]
This is a pre-existing issue, but does device_alloc() leave fields
uninitialized due to kmalloc usage?

drivers/scsi/dc395x.c:device_alloc() {
    ...
	dcb = kmalloc_obj(struct DeviceCtlBlk, GFP_ATOMIC);
	if (!dcb)
		return NULL;
	dcb->acb = NULL;
    ...
}

Several fields are explicitly initialized, but init_tcq_flag and dev_type
are omitted and kmalloc does not zero the memory. Later checks like
if (!dcb->init_tcq_flag) could evaluate incorrectly if init_tcq_flag
contains garbage. Should this explicitly initialize these remaining fields?

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