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

On 27/07/2026 16:20, [email protected] wrote:
> 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):

If sd_mpath_probe() finds mpath_head->mpath_disk is non-NULL in 
mpath_alloc_head_disk(), it will error. Only after 
sd_mpath_disk_release() -> mpath_put_disk() is mpath_head->disk set to NULL.

> 
> 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.

I'll fix it

> 
>> +}
> 
> [ ... ]
> 
>> @@ -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.

this all needs to be fixed in mkp- scsi treee

> 
>> +		}
>> +	}
>