Re: [PATCH v4 25/28] scsi: sd: support multipath disk

[sashiko-bot@kernel.org]

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

New issues:
- [High] Unconditional sd_mpath_put_disk() in sd_mpath_remove() destroys the multipath disk structure while delayed removal is pending, causing a use-after-free.
- [High] Uncanceled delayed work accesses freed mpath_head after sd_mpath_fail_probe unconditionally deletes the device.
- [High] Concurrent execution of sd_mpath_remove_head and sd_mpath_probe creates a race condition that incorrectly deletes a newly allocated gendisk.
- [Medium] Missing put_device() in sd_probe() error path for sd_mpath_add_disk() failure leaks sdkp and its parent's reference.
- [Medium] Error paths in sd_mpath_probe() call kfree() on a device object after device_initialize(), leaking kobject internals.

Pre-existing issues:
- [High] If sd_large_pool_create() fails, sd_probe() jumps to out_free_index bypassing device unregistration and freeing active sysfs device memory (sdkp).
--

--- Patch [25]: [PATCH v4 25/28] scsi: sd: support multipath disk ---
commit 7776d470d97b7568b27daa7f4e53e7184172c0bc
Author: John Garry <john.garry@linux.dev>

scsi: sd: support multipath disk

This commit adds support to attach a multipath disk. It allocates a
gendisk per path and marks them as hidden while keeping a global list of
sd_mpath_disks for matching scsi_devices.

> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 36c745cbb2619..d5a37b1b312c3 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -4035,13 +4139,258 @@ static const struct attribute_group sd_mpath_disk_attr_group = {
[ ... ]
> +static int sd_mpath_probe(struct scsi_disk *sdkp)
> +{
[ ... ]
> +	sd_mpath_disk->scsi_mpath_head = scsi_mpath_head;
> +	device_initialize(&sd_mpath_disk->dev);
> +	sd_mpath_disk->dev.class = &sd_mpath_disk_class;
[ ... ]
> +	error = mpath_alloc_head_disk(mpath_head, &lim,
> +				dev_to_node(dma_dev));
> +	if (error)
> +		goto out_free_disk;
[ ... ]
> +out_free_disk:
> +	kfree(sd_mpath_disk);

[Severity: Medium]
Is it safe to call kfree() directly on the device object here?
Since device_initialize() was already called on sd_mpath_disk->dev,
shouldn't we use put_device() to prevent leaking resources allocated by
the kobject core?

[ ... ]
> +static void sd_mpath_remove(struct scsi_disk *sdkp)
> +{
[ ... ]
> +	mutex_lock(&sd_mpath_disks_lock);
> +	sd_mpath_disk->disk_count--;
> +	if (!sd_mpath_disk->disk_count && mpath_can_remove_head(mpath_head)) {
> +		list_del_init(&sd_mpath_disk->entry);
> +		remove = true;
> +	}
> +	mutex_unlock(&sd_mpath_disks_lock);
> +	mpath_remove_sysfs_link(mpath_device);
> +	mpath_device->disk = NULL;
> +
> +	if (remove) {
> +		device_del(&sd_mpath_disk->dev);
> +		mpath_remove_disk(mpath_head);
> +	}
> +	sd_mpath_put_disk(sd_mpath_disk);
> +}

[Severity: High]
Does this sequence risk a use-after-free if delayed removal is pending?
If mpath_can_remove_head() returns false, the object remains on the
global list and is not fully removed, but we unconditionally drop the
reference via sd_mpath_put_disk() at the end. This could free the object
while it is still globally accessible.

[ ... ]
> +static void sd_mpath_remove_head(struct scsi_mpath_head *scsi_mpath_head)
> +{
[ ... ]
> +	list_del_init(&sd_mpath_disk->entry);
> +	mutex_unlock(&sd_mpath_disks_lock);
> +
> +	device_del(&sd_mpath_disk->dev);
> +	mpath_remove_disk(mpath_head);
> +	sd_mpath_put_disk(sd_mpath_disk);
> +}

[Severity: High]
Could dropping the mutex here race with a concurrent sd_mpath_probe()?
We drop sd_mpath_disks_lock before calling mpath_remove_disk(mpath_head).
If a new probe allocates a new disk and overwrites mpath_head->disk
in that window, mpath_remove_disk() might delete the newly allocated
gendisk instead of the old one being removed.

[ ... ]
> +static void sd_mpath_fail_probe(struct scsi_disk *sdkp)
> +{
[ ... ]
> +	mutex_lock(&sd_mpath_disks_lock);
> +	sd_mpath_disk->disk_count--;
> +	if (!sd_mpath_disk->disk_count) {
> +		list_del_init(&sd_mpath_disk->entry);
> +		remove = true;
> +	}
> +	mutex_unlock(&sd_mpath_disks_lock);
> +	mpath_device->disk = NULL;
> +
> +	if (remove) {
> +		device_del(&sd_mpath_disk->dev);
> +		mpath_remove_disk(mpath_head);
> +	}
> +	sd_mpath_put_disk(sd_mpath_disk);
> +}

[Severity: High]
What happens if there is pending delayed work on mpath_head->remove_work
when we hit this error path? If a previous removal queued the work,
and a subsequent failed probe triggers this code, we delete and free the
head without canceling the delayed work. Will the workqueue then trigger
a use-after-free on the freed mpath_head?

[ ... ]
> @@ -4158,11 +4515,18 @@ static int sd_probe(struct scsi_device *sdp)
>  	sd_revalidate_disk(gd);
>  	if (sdp->sector_size > PAGE_SIZE) {
>  		if (sd_large_pool_create()) {
> +			sd_mpath_fail_probe(sdkp);
>  			error = -ENOMEM;
>  			goto out_free_index;
>  		}
>  	}

[Severity: High]
This is a pre-existing issue, but can this lead to a use-after-free in
sysfs? At this point, device_add() has already succeeded. Bypassing
device_del() or device_unregister() and jumping directly to
out_free_index (which frees sdkp) leaves the device linked in the sysfs
hierarchy.

[ ... ]
> @@ -4184,6 +4549,19 @@ static int sd_probe(struct scsi_device *sdp)
>  		goto out;
>  	}
>  
> +	if (sdp->scsi_mpath_dev) {
> +		error = sd_mpath_add_disk(sdkp);
> +		if (error) {
> +			sd_mpath_fail_probe(sdkp);
> +			device_del(&sdkp->disk_dev);
> +			del_gendisk(sdkp->disk);
> +
> +			put_disk(sdkp->disk);
> +			if (sdp->sector_size > PAGE_SIZE)
> +				sd_large_pool_destroy();
> +			goto out;
> +		}
> +	}

[Severity: Medium]
Are we leaking the scsi_disk structure and its parent reference here?
We call device_del(&sdkp->disk_dev), but we don't call put_device()
to drop the base reference acquired during device_initialize().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723093627.2327456-1-john.g.garry@oracle.com?part=25