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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Unprotected overwrite of mpath_head->parent before validation causes Use-After-Free during concurrent I/O when probe fails.
- [High] Queue limits are explicitly overwritten before stacking, breaking multipath limits aggregation.
- [Medium] Circular reference between sd_mpath_disk and its child multipath gendisk prevents both structures from ever being freed.
- [Medium] Missing put_device() in sd_probe() error path leaks the scsi_disk structure and device references.
- [Medium] TOCTOU race in disk removal paths allows a concurrent probe to fail with a duplicate sysfs name error.
--

commit 7a8d9795d762973e091f3a0357c1112c3afc4c60
Author: John Garry <john.garry@linux.dev>
Subject: scsi: sd: support multipath disk

Add support to attach a multipath disk, allocating gendisks per path but
marking them as hidden, and maintaining a global list of multipath disks
for scsi_device matching.

> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 60e071d23ddd0..28304a497d8c6 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
[ ... ]
> @@ -115,12 +115,30 @@ static mempool_t *sd_large_page_pool;
>  static atomic_t sd_large_page_pool_users = ATOMIC_INIT(0);
>  static struct lock_class_key sd_bio_compl_lkclass;
>  #ifdef CONFIG_SCSI_MULTIPATH
> +static LIST_HEAD(sd_mpath_disks_list);
> +static DEFINE_MUTEX(sd_mpath_disks_lock);
> +
>  struct sd_mpath_disk {
> +	struct device			dev;
> +	int				disk_index;
> +	int				disk_count;
> +	struct list_head		entry;
>  	struct scsi_mpath_head		*scsi_mpath_head;
>  };
>  
>  static void sd_mpath_disk_release(struct device *dev)
>  {
> +	struct sd_mpath_disk *sd_mpath_disk =
> +		container_of(dev, struct sd_mpath_disk, dev);
> +	struct scsi_mpath_head *scsi_mpath_head =
> +		sd_mpath_disk->scsi_mpath_head;
> +	struct mpath_head *mpath_head = &scsi_mpath_head->mpath_head;
> +
> +	mpath_put_disk(mpath_head);

[Severity: Medium]
In sd_mpath_probe(), the child multipath gendisk takes a reference on this
device via mpath_head->parent = &sd_mpath_disk->dev. Since the child holds
a strong reference to the parent, the parent's refcount cannot reach zero
while the child exists.

Does dropping the child reference here in the parent's release function create
an unbreakable circular dependency that leaks both structures during removal?

> +	ida_free(&sd_index_ida, sd_mpath_disk->disk_index);
> +	scsi_mpath_put_head(scsi_mpath_head);
> +
> +	kfree(sd_mpath_disk);
>  }
[ ... ]
> @@ -4019,6 +4038,90 @@ static int sd_format_disk_name(char *prefix, int index, char *buf, int buflen)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_SCSI_MULTIPATH
> +static int sd_mpath_revalidate_head(struct scsi_disk *sdkp)
> +{
> +	struct sd_mpath_disk *sd_mpath_disk = sdkp->sd_mpath_disk;
> +	struct scsi_mpath_head *scsi_mpath_head = sd_mpath_disk->scsi_mpath_head;
> +	struct mpath_head *mpath_head = &scsi_mpath_head->mpath_head;
> +	struct gendisk *disk = mpath_head->disk;
> +	struct queue_limits *sdkp_lim = &sdkp->disk->queue->limits;
> +	struct queue_limits lim;
> +	unsigned int memflags;
> +	int ret;
> +
> +	lim = queue_limits_start_update(disk->queue);
> +	memflags = blk_mq_freeze_queue(disk->queue);
> +
> +	lim.logical_block_size = sdkp_lim->logical_block_size;
> +	lim.physical_block_size = sdkp_lim->physical_block_size;
> +	lim.io_min = sdkp_lim->io_min;
> +	lim.io_opt = sdkp_lim->io_opt;
> +
> +	queue_limits_stack_bdev(&lim, sdkp->disk->part0, 0,
> +					disk->disk_name);

[Severity: High]
Does overwriting the multipath limits with sdkp_lim before calling
queue_limits_stack_bdev() erase the previously stacked limits?

If an older path required stricter limits (e.g. larger block size or
alignment), would this downgrade the multipath limits to match only the newly
probed path, potentially causing I/O errors on the older paths?

> +
> +	/* TODO: setup integrity and zoned limits */
[ ... ]
> +static int sd_mpath_probe(struct scsi_disk *sdkp)
> +{
[ ... ]
> +	sd_mpath_disk = kzalloc(sizeof(*sd_mpath_disk), GFP_KERNEL);
> +	if (!sd_mpath_disk) {
> +		error = -ENOMEM;
> +		goto out_unlock;
> +	}
> +
> +	sd_mpath_disk->scsi_mpath_head = scsi_mpath_head;
> +
> +	blk_set_stacking_limits(&lim);
> +	lim.dma_alignment = 3;
> +	lim.features |= BLK_FEAT_IO_STAT | BLK_FEAT_NOWAIT |
> +		BLK_FEAT_POLL | BLK_FEAT_ATOMIC_WRITES;
> +
> +	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;

[Severity: High]
If mpath_alloc_head_disk() fails here, the error path jumps to out_free_disk
and calls kfree(sd_mpath_disk). However, mpath_head->parent was
unconditionally set to &sd_mpath_disk->dev just before the call.

If the multipath disk is still active from a prior failed removal, will it
dereference the freed parent pointer during subsequent I/O or workqueue
processing?

> +	disk = mpath_head->disk;
[ ... ]
> +static void sd_mpath_remove_head(struct scsi_mpath_head *scsi_mpath_head)
> +{
> +	struct mpath_head *mpath_head = &scsi_mpath_head->mpath_head;
> +	struct sd_mpath_disk *sd_mpath_disk;
> +	struct device *dev = &scsi_mpath_head->dev;
> +
> +	mutex_lock(&sd_mpath_disks_lock);
> +	sd_mpath_disk = sd_mpath_find_disk(scsi_mpath_head);
> +	if (!sd_mpath_disk) {
> +		dev_warn(dev, "could not find mpath disk\n");
> +		mutex_unlock(&sd_mpath_disks_lock);
> +		return;
> +	}
> +
> +	if (sd_mpath_disk->disk_count) {
> +		dev_dbg(dev, "non-zero multipath disk count in removal\n");
> +		sd_mpath_put_disk(sd_mpath_disk);
> +		mutex_unlock(&sd_mpath_disks_lock);
> +		return;
> +	}
> +
> +	list_del_init(&sd_mpath_disk->entry);
> +	mutex_unlock(&sd_mpath_disks_lock);
> +
> +	device_del(&sd_mpath_disk->dev);

[Severity: Medium]
Could a time-of-check to time-of-use race happen here during device removal?
The disk is removed from sd_mpath_disks_list inside the lock, but device_del()
is called after dropping the lock.

Would a concurrent sd_mpath_probe() acquire the lock, fail to find the disk,
allocate a new one with the same index, and call device_add() before this
device_del() completes, causing a sysfs naming collision and -EEXIST failure?

> +	mpath_remove_disk(mpath_head);
> +	sd_mpath_put_disk(sd_mpath_disk);
> +}
[ ... ]
> @@ -4192,6 +4556,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);
>  		if (sdp->sector_size > PAGE_SIZE)
> @@ -4199,6 +4564,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]
Does this error path leak the scsi_disk structure and its device reference?

device_del() removes the device from the system, but since sdkp->disk_dev was
initialized with device_initialize() (giving it a refcount of 1), shouldn't
there be a matching put_device(&sdkp->disk_dev) to drop the base reference and
eventually call scsi_disk_release()?

> +		}
> +	}
>  	if (sdkp->security) {
>  		sdkp->opal_dev = init_opal_dev(sdkp, &sd_sec_submit);
>  		if (sdkp->opal_dev)

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