Re: [PATCH v6 21/27] scsi-multipath: add delayed disk removal support

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The unchecked return value of `scsi_mpath_get_head()` in `scsi_mpath_remove_head_work()` can lead to a Use-After-Free (UAF) and double-free.
--

--- Patch [21]: [PATCH v6 21/27] scsi-multipath: add delayed disk removal support ---
commit 10f5e133107a91a4b4df48e21559fe51a6b82a5e
Author: John Garry <[email protected]>

scsi-multipath: add delayed disk removal support

This commit adds core code support for delayed disk removal, introducing a
worker function that iterates over the scsi_bus_type to invoke a newly
added mpath_remove_head callback. It also attempts to manage reference
counting for scsi_mpath_head during this removal process.

> diff --git a/drivers/scsi/scsi_multipath.c b/drivers/scsi/scsi_multipath.c
> index 0adc5adb4b234..a42e20d33e422 100644
> --- a/drivers/scsi/scsi_multipath.c
> +++ b/drivers/scsi/scsi_multipath.c
[ ... ]
> @@ -367,7 +367,34 @@ static bool scsi_mpath_available_path(struct mpath_device *mpath_device)
> +static void scsi_mpath_remove_head_work(struct mpath_head *mpath_head)
> +{
> +	struct scsi_mpath_head *scsi_mpath_head = to_scsi_mpath_head(mpath_head);
> +
> +	/*
> +	 * scsi_mpath_remove_head_drv() could drop the final reference to
> +	 * scsi_mpath_head, meaning that we pass a free'd pointer further
> +	 * drivers - take a reference so that won't happen.
> +	 */
> +	scsi_mpath_get_head(scsi_mpath_head);
> +	bus_for_each_drv(&scsi_bus_type, NULL, scsi_mpath_head,
> +		scsi_mpath_remove_head_drv);
> +	scsi_mpath_put_head(scsi_mpath_head);
> +}

[Severity: High]
Can ignoring the return value of scsi_mpath_get_head() cause a use-after-free
regression?

Looking at scsi_mpath_get_head(), it uses kref_get_unless_zero() which can
fail and return -ENXIO. If the reference count drops to zero concurrently
just before scsi_mpath_get_head() is called, the acquisition will fail.

By proceeding unconditionally when it fails, bus_for_each_drv() could
operate on an already freed scsi_mpath_head pointer.

Furthermore, scsi_mpath_put_head() would then invoke kref_put() on an
object that had a zero reference count, causing a double-free regression.

Should this code check the return value of scsi_mpath_get_head() and exit
early if the reference could not be acquired?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=21