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

[John Garry <john.garry@linux.dev>]

On 9/7/26 13:25, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [High] Pending delayed removal work is not cancelled when `sd_mpath_fail_probe()` tears down a multipath disk, leading to use-after-free of `mpath_head`.
> 
> Pre-existing issues:
> - [Critical] Preexisting double put of `sdkp->disk_dev` when `sd_large_pool_create()` or `device_add_disk()` fails.
> --
> 
> commit 9972be7d4ae335cb741d2858144500af92bf6dc3
> Author: John Garry <john.garry@linux.dev>
> 
> scsi: sd: support multipath disk
> 
> This commit adds support to attach a multipath disk by allocating a hidden
> gendisk per path. A global list of multipath disks is kept for matching
> SCSI devices, and the multipath gendisk mimics a standard SCSI disk.
> 
>> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
>> index 60e071d23ddd0..27230ecc2337e 100644
>> --- a/drivers/scsi/sd.c
>> +++ b/drivers/scsi/sd.c
> [ ... ]
>> @@ -4048,13 +4151,260 @@ static const struct attribute_group sd_mpath_disk_attr_group = {
> [ ... ]
>> +static void sd_mpath_fail_probe(struct scsi_disk *sdkp)
>> +{
>> +	struct sd_mpath_disk *sd_mpath_disk = sdkp->sd_mpath_disk;
>> +	struct scsi_mpath_device *scsi_mpath_dev;
>> +	struct mpath_device *mpath_device;
>> +	struct scsi_device *sdp = sdkp->device;
>> +	struct scsi_mpath_head *scsi_mpath_head;
>> +	struct mpath_head *mpath_head;
>> +	bool remove = false;
>> +
>> +	if (!sd_mpath_disk)
>> +		return;
>> +
>> +	scsi_mpath_dev = sdp->scsi_mpath_dev;
>> +	mpath_device = &scsi_mpath_dev->mpath_device;
>> +	scsi_mpath_head = sd_mpath_disk->scsi_mpath_head;
>> +	mpath_head = &scsi_mpath_head->mpath_head;
>> +
>> +	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 this sequence leave pending delayed removal work uncancelled when tearing
> down a multipath disk?
> 
> If a multipath disk has its last path removed, it can enter delayed removal
> where disk_count drops to 0 and remove_work is scheduled via
> mpath_can_remove_head().
> 
> If a path is then re-probed, disk_count becomes 1.

The re-probe should fail early as mpath_head->disk is still set

> If that probe subsequently
> fails and sd_mpath_fail_probe() is called, the disk_count drops back to 0,
> which triggers the device deletion and frees the head.
> 
> However, since this path bypasses mpath_can_remove_head(), could the pending
> remove_work scheduled earlier go uncancelled, potentially leading to a
> use-after-free of mpath_head when the delayed work timer expires?
> 
>> +}
> [ ... ]
>> @@ -4171,6 +4529,7 @@ 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;
>>   			device_unregister(&sdkp->disk_dev);
>>   			put_disk(gd);
> 
> [Severity: Critical]
> This is a pre-existing issue, but could this error path result in a
> double-free of sdkp->disk_dev?
> 
> When sd_large_pool_create() fails, device_unregister() is called on
> sdkp->disk_dev, which drops the refcount to 0 and frees sdkp.
> 
> Immediately following this, put_disk(gd) is called. Because gd->fops is
> already set to &sd_fops, dropping the gendisk refcount invokes
> scsi_disk_free_disk(), which reads the already freed sdkp and attempts a
> second put_device() on sdkp->disk_dev.

I'll check it, but I doubt that there is a problem

> 
>>   			goto out;
>>   		}
>>   	}
>>   
>> +	if (sdp->scsi_mpath_dev) {
>> +		error = sd_mpath_revalidate_head(sdkp);
>> +		if (error)
>> +			sdev_printk(KERN_WARNING, sdp, "could not revalidate multipath limits\n");
>> +	}
>> +
>>   	if (sdp->removable) {
>>   		gd->flags |= GENHD_FL_REMOVABLE;
>>   		gd->events |= DISK_EVENT_MEDIA_CHANGE;
>> @@ -4192,6 +4557,7 @@ static int sd_probe(struct scsi_device *sdp)
>>   
>>   	error = device_add_disk(dev, gd, NULL);
>>   	if (error) {
>> +		sd_mpath_fail_probe(sdkp);
>>   		device_unregister(&sdkp->disk_dev);
>>   		put_disk(gd);
> 
> [Severity: Critical]
> This is a pre-existing issue, but might this error path also suffer from the
> same double-free of sdkp->disk_dev?
> 
> Just like the memory allocation failure path above, if device_add_disk()
> fails, device_unregister() frees sdkp, and the subsequent put_disk(gd)
> accesses the freed sdkp and performs a second put_device() on
> sdkp->disk_dev.

As with v7, this is fine

> 
>>   		if (sdp->sector_size > PAGE_SIZE)
>