Re: [PATCH v5 24/27] scsi: sd: support multipath disk

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

New issues:
- [High] Concurrent path removal and addition causes a TOCTOU race that overwrites and frees the shared `mpath_head->disk`.
- [High] Uncancelled `remove_work` triggers a Use-After-Free if a re-attached multipath disk fails probing.
- [Medium] Missing `put_device()` on the `sd_mpath_add_disk()` error path permanently leaks `struct scsi_disk`.

Pre-existing issues:
- [High] Direct `kfree(sdkp)` in the `sd_large_pool_create()` error path causes a use-after-free because the device is already registered in sysfs.
--

--- Patch [24]: [PATCH v5 24/27] scsi: sd: support multipath disk ---
commit 6b75fe218921318a27f170eca853c70058afe60d
Author: John Garry <[email protected]>
Subject: scsi: sd: support multipath disk

Add support to attach a multipath disk.

We still allocate the gendisk per path, and this is required for the
per-path submission. However, those gendisks are marked as hidden. Those
disks are named sdX:Y, where X is the multipath disk index and Y is the
per-path index.

> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 36c745cbb2619..f044b2dfaa9fd 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c

[ ... ]

> @@ -4035,13 +4138,259 @@ static const struct attribute_group sd_mpath_disk_attr_group = {
> +static int sd_mpath_probe(struct scsi_disk *sdkp)
> +{
[ ... ]
> +	mpath_head->parent = &sd_mpath_disk->dev;
> +	mpath_head->drv_module = THIS_MODULE;
> +	mpath_head->disk_groups = sd_mpath_disk_attr_groups;
> +	error = mpath_alloc_head_disk(mpath_head, &lim,
> +				dev_to_node(dma_dev));
> +	if (error)
> +		goto out_free_disk;
> +	disk = mpath_head->disk;

[ ... ]

> +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);

[Severity: High]
Does this create a TOCTOU race that overwrites and frees the shared
mpath_head->disk?

If sd_mpath_remove() and sd_mpath_probe() run concurrently for the same
scsi_mpath_head (e.g., during hotplug/hot-unplug events):

The removal path drops the sd_mpath_disks_lock before tearing down
mpath_head->disk here. A concurrent probe can then allocate and assign a new
disk to mpath_head->disk in sd_mpath_probe().

When the removal thread resumes, it deletes and frees the new disk while the
probe thread proceeds to use it, causing a use-after-free.

> +	}
> +	sd_mpath_put_disk(sd_mpath_disk);
> +}

[ ... ]

> +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]
Can an uncancelled remove_work trigger a use-after-free here if a re-attached
multipath disk fails probing?

A delayed removal is scheduled when the last path is removed. A new path is
then rapidly probed, incrementing the disk count, but fails midway through
sd_probe().

This cleanup function decrements the count back to 0 and frees sd_mpath_disk
and scsi_mpath_head, but it does NOT cancel the pending remove_work (which
would normally be cancelled later in sd_mpath_add_disk()). The scheduled work
will subsequently execute on freed memory.

> +}

[ ... ]

> @@ -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 isn't a bug introduced by this patch, but does the direct kfree(sdkp) in
the sd_large_pool_create() error path cause a use-after-free because the
device is already registered in sysfs?

After device_add(&sdkp->disk_dev) succeeds earlier in sd_probe(), the device
is exposed to sysfs. If sd_large_pool_create() fails, the code branches to
out_free_index which directly calls kfree(sdkp) without unregistering the
sysfs device, leaving dangling pointers in the sysfs tree.

>  		}
>  	}

[ ... ]

> @@ -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]
Will this error path leak the struct scsi_disk?

A failure in sd_mpath_add_disk() triggers the error handling path which calls
device_del() but misses the corresponding put_device().

Because the kobject reference is never dropped, scsi_disk_release() is never
invoked, permanently leaking the memory.

> +		}
> +	}

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