[PATCH] scsi: snic: Fix scsi host leak on workqueue allocation failure

[PATCH] scsi: snic: Fix scsi host leak on workqueue allocation failure

[Chen Changcheng]

In snic_add_host(), if scsi_add_host() succeeds but
alloc_ordered_workqueue() fails, the function returns -ENOMEM
with shost->work_q left as NULL. The caller's error path then
calls snic_del_host(), which returns early when !shost->work_q
without calling scsi_remove_host(). The Scsi_Host remains
registered in sysfs as a zombie device even after the probe
has failed. This causes:

 - The leaked host remains visible in /sys/class/scsi_host/
   after probe failure, with state "running".
 - Subsequent SCSI host numbering is permanently shifted
   (the leaked host ID from ida_alloc() is never reclaimed).
 - Memory leak: the Scsi_Host allocation can never be freed
   because device_add() took a reference that can only be
   released by device_del() inside scsi_remove_host().

Fix by adding scsi_remove_host() in the workqueue allocation
failure path inside snic_add_host(), undoing the successful
scsi_add_host() before returning the error. This is cleaner
than modifying snic_del_host() because snic_del_host() is
called from a shared error label that also serves paths where
snic_add_host() was never invoked.

Reproducer (requires no real SNIC hardware):
 - Build CONFIG_SCSI_SNIC=y (built-in)
 - Add snic.test_mode=1 snic.inject_wq_fail=1 to kernel cmdline
 - Boot with a PCI device matching the snic driver (e.g. QEMU
   edu device, PCI ID 0x1234:0x11e8, temporarily added to the
   driver's PCI ID table)

Before the fix:
  # /sys/class/scsi_host/ contains a zombie host0:
  $ cat /sys/class/scsi_host/host0/proc_name
  snic_scsi
  $ cat /sys/class/scsi_host/host0/state
  running
  # ata_piix gets host1, host2 (host0 stuck):
  scsi host1: ata_piix
  scsi host2: ata_piix

After the fix:
  # host0 is properly freed and reused by ata_piix:
  scsi host0: ata_piix
  scsi host1: ata_piix
  # No zombie host in /sys/class/scsi_host/

Signed-off-by: Chen Changcheng <chenchangcheng@kylinos.cn>
---
 drivers/scsi/snic/snic_main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/snic/snic_main.c b/drivers/scsi/snic/snic_main.c
index 82953e6a0915..cd638b4a4d7b 100644
--- a/drivers/scsi/snic/snic_main.c
+++ b/drivers/scsi/snic/snic_main.c
@@ -305,6 +305,7 @@ snic_add_host(struct Scsi_Host *shost, struct pci_dev *pdev)
 	if (!shost->work_q) {
 		SNIC_HOST_ERR(shost, "Failed to Create ScsiHost wq.\n");
 
+		scsi_remove_host(shost);
 		ret = -ENOMEM;
 	}
 
-- 
2.25.1

Re: [PATCH] scsi: snic: Fix scsi host leak on workqueue allocation failure

[sashiko-bot]

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

Pre-existing issues:
- [High] Hardware resources are torn down before removing the SCSI host on the `snic_probe()` error path, leading to race condition and use-after-free.
- [High] Missing `list_del(&snic->list)` on the `snic_probe()` error path leaves a dangling pointer in the global `snic_glob->snic_list`, causing memory corruption.
- [High] Missing `cancel_work_sync(&snic->link_work)` on the `snic_probe()` error path allows asynchronously queued work to dereference a freed `snic` object.
--

--- Patch [1]: [PATCH] scsi: snic: Fix scsi host leak on workqueue allocation failure ---
commit 862e6c3dc0ab1bdd6e8457040994057a804388df
Author: Chen Changcheng <chenchangcheng@kylinos.cn>

scsi: snic: Fix scsi host leak on workqueue allocation failure

This commit fixes a memory leak where a failed workqueue allocation in
snic_add_host() leaves the Scsi_Host registered in sysfs as a zombie
device. It undoes the successful scsi_add_host() by calling
scsi_remove_host() on the error path before returning.

> --- a/drivers/scsi/snic/snic_main.c
> +++ b/drivers/scsi/snic/snic_main.c
> @@ -305,6 +305,7 @@ snic_add_host(struct Scsi_Host *shost, struct pci_dev *pdev)
>  	if (!shost->work_q) {
>  		SNIC_HOST_ERR(shost, "Failed to Create ScsiHost wq.\n");
>  
> +		scsi_remove_host(shost);
>  		ret = -ENOMEM;
>  	}

[Severity: High]
This is a pre-existing issue, but does this error path in snic_probe()
create a race condition?

If snic_disc_start() fails, the driver jumps to err_get_conf:

drivers/scsi/snic/snic_main.c:snic_probe() {
    ...
err_get_conf:
        snic_free_all_untagged_reqs(snic);
        for (i = 0; i < snic->intr_count; i++)
                svnic_intr_mask(&snic->intr[i]);
        snic_free_intr(snic);
err_req_intr:
        svnic_dev_disable(snic->vdev);
    ...
        snic_del_host(snic->shost);
}

The host is already fully registered via snic_add_host() and the state
is SNIC_ONLINE. The hardware and interrupts are torn down, but
snic_del_host() is delayed until the end. Could a concurrent sysfs scan
or SCSI error handler queue commands to the disabled hardware during
this window?

[Severity: High]
This isn't a bug introduced by this patch, but does the err_free_snic
error path in snic_probe() leave a dangling pointer?

The snic structure is added to the global list during probe:

drivers/scsi/snic/snic_main.c:snic_probe() {
    ...
        spin_lock_irqsave(&snic_glob->snic_list_lock, flags);
        list_add_tail(&snic->list, &snic_glob->snic_list);
        spin_unlock_irqrestore(&snic_glob->snic_list_lock, flags);
    ...
}

If a subsequent step like snic_add_host() fails, the snic structure is
freed via scsi_host_put():

drivers/scsi/snic/snic_main.c:snic_probe() {
    ...
err_free_snic:
        snic_stats_debugfs_remove(snic);
        scsi_host_put(shost);
}

Will the lack of list_del(&snic->list) before freeing cause memory
corruption on subsequent device probes when list_add_tail() accesses
the freed structure?

[Severity: High]
This is a pre-existing issue, but can the snic_probe() error path
result in a use-after-free from asynchronously queued work?

Interrupts are requested and unmasked before configuring IO:

drivers/scsi/snic/snic_main.c:snic_probe() {
    ...
        for (i = 0; i < snic->intr_count; i++)
                svnic_intr_unmask(&snic->intr[i]);
    ...
}

A link event could trigger snic_handle_link_event(), which queues
snic->link_work. If a subsequent probe step fails, the err_free_snic
path masks interrupts and frees the snic structure:

drivers/scsi/snic/snic_main.c:snic_probe() {
    ...
err_free_snic:
        snic_stats_debugfs_remove(snic);
        scsi_host_put(shost);
}

Because the error path does not call cancel_work_sync(&snic->link_work)
or flush the workqueue, could the pending work item execute later and
dereference the freed snic object?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727073438.209673-1-chenchangcheng@kylinos.cn?part=1