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

[sashiko-bot@kernel.org]

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

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

>  		if (sdp->sector_size > PAGE_SIZE)

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