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

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

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