Re: [PATCH] scsi: message: fusion: Use more common error handling code in mpt_attach()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Data race on global `ioc_list` and static `mpt_ids` variables during concurrent device probing.
- [High] DMA memory leak on device initialization failure.
--

commit 4e8cbd0babcc92bd561c86ab647d8b47fc357dc0
Author: Markus Elfring <elfring@users.sourceforge.net>

scsi: message: fusion: Use more common error handling code in mpt_attach()

Use an existing label once more so that a bit of exception handling can be
better reused at the end of this function implementation.

> diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c
> index 3a431ffd3e2eb..c2c5a25cf5aec 100644
> --- a/drivers/message/fusion/mptbase.c
> +++ b/drivers/message/fusion/mptbase.c
> @@ -2014,9 +2014,7 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id)
>  
>  		destroy_workqueue(ioc->reset_work_q);
>  		ioc->reset_work_q = NULL;
> -
> -		kfree(ioc);
> -		return r;
> +		goto out_free_ioc;

[Severity: High]
This isn't a bug introduced by this patch, but does this error path
permanently leak DMA memory?

If mpt_do_ioc_recovery() fails, it bails out to this error path that cleans
up some resources before jumping to out_free_ioc to free the ioc struct.

However, mpt_do_ioc_recovery() internally calls PrimeIocFifos(), which
allocates large DMA coherent buffers (saved in ioc->alloc and
ioc->sense_buf_pool). Because mpt_adapter_disable(ioc) or
mpt_adapter_dispose(ioc) is never called on this error path, these DMA
allocations appear to be leaked.

[Severity: High]
This is a pre-existing issue, but is there a data race when modifying global
variables during concurrent device probing?

The mpt_attach() function is called during PCI device probing. The Linux
driver core does not serialize probe routines across different devices; it
only holds a per-device lock. If multiple MPT controllers are bound
concurrently (e.g., via concurrent sysfs bind writes), multiple threads
could modify the static mpt_ids counter and append to the global ioc_list
simultaneously without synchronization:

mpt_attach() {
    ...
    ioc->id = mpt_ids++;
    ...
    list_add_tail(&ioc->list, &ioc_list);
    ...
}

Could this lead to list corruption and kernel crashes?

>  	}
>  
>  	/* call per device driver probe entry point */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/645f850d-bd48-4c72-97e4-5771e247de8f@web.de?part=1