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

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

New issues:
- [High] Missing `put_device()` in the `sd_mpath_add_disk()` error path permanently leaks the `sdkp` structure.
- [High] Inverted device teardown order causes double `device_del()` and sysfs warnings.
- [High] Cyclic kobject reference between `sd_mpath_disk` and its child gendisk guarantees both are permanently leaked on removal.
- [Medium] Ignored return value of `scsi_mpath_get_head()` leads to potential kref underflow and use-after-free.

Pre-existing issues:
- [High] `kfree()` is called on an active kobject when `sd_large_pool_create()` fails, leading to UAF / Sysfs Corruption.
--

--- Patch [24]: [PATCH v6 24/27] scsi: sd: support multipath disk ---
commit 1896d58c043437e813525b463ccbb2249c843f33
Author: John Garry <[email protected]>
scsi: sd: support multipath disk

This commit adds support for attaching a multipath disk in the SCSI sd
subsystem. It allocates gendisks per-path but marks them as hidden, and
maintains a global list of sd_mpath_disks to match with scsi_devices.

> --- 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: High]
Does this create an unbreakable cyclic kobject reference? The child disk is
created as a child of sd_mpath_disk->dev, meaning it holds a reference to
the parent. If the child's reference is only dropped via mpath_put_disk()
inside the parent's release function (sd_mpath_disk_release), the parent's
release function will never execute because the child still holds a
reference to it. Will this permanently leak both the sd_mpath_disk and the
multipath gendisk on removal?

[ ... ]

> @@ -4035,13 +4138,259 @@ static const struct attribute_group sd_mpath_disk_attr_group = {
>  	.attrs		= sd_mpath_disk_attrs,
>  };
>  
> +static int sd_mpath_probe(struct scsi_disk *sdkp)
> +{
[ ... ]
> +	device_initialize(&sd_mpath_disk->dev);
> +	sd_mpath_disk->dev.class = &sd_mpath_disk_class;
> +
> +	/* undone in sd_mpath_disk_release() */
> +	scsi_mpath_get_head(scsi_mpath_head);

[Severity: Medium]
Is it safe to ignore the return value here? In sd_mpath_probe(),
scsi_mpath_get_head() is called but its return value is discarded. Since
scsi_mpath_get_head() explicitly uses kref_get_unless_zero() and can return
-ENXIO, could a failure lead to a refcount underflow and a use-after-free
during the release path?

[ ... ]

> +static void sd_mpath_remove(struct scsi_disk *sdkp)
> +{
[ ... ]
> +	if (remove) {
> +		device_del(&sd_mpath_disk->dev);
> +		mpath_remove_disk(mpath_head);
> +	}
> +	sd_mpath_put_disk(sd_mpath_disk);
> +}

[Severity: High]
Is this device teardown order inverted? In sd_mpath_remove() and the related
sd_mpath_remove_head(), the parent device (sd_mpath_disk->dev) is deleted
via device_del() before the child gendisk is removed via mpath_remove_disk().
Does this cause double device_del() operations or sysfs hierarchy corruption,
since the device model requires children to be unregistered before their
parents?

[ ... ]

> +static void sd_mpath_fail_probe(struct scsi_disk *sdkp)
> +{
[ ... ]
> +	if (remove) {
> +		device_del(&sd_mpath_disk->dev);
> +		mpath_remove_disk(mpath_head);
> +	}
> +	sd_mpath_put_disk(sd_mpath_disk);
> +}

[Severity: High]
Does the same inverted teardown order apply here in sd_mpath_fail_probe()?
Deleting the parent device before the child gendisk violates the device
model requirements and can trigger sysfs warnings.

[ ... ]

> @@ -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 is a pre-existing issue, but does jumping to out_free_index here leak
the active kobject? In sd_probe(), device_add(&sdkp->disk_dev) has already
succeeded earlier in the function. When sd_large_pool_create() fails, it
jumps to out_free_index and eventually calls kfree(sdkp). Should this error
path use device_del() and put_device() instead to prevent sysfs corruption
and a use-after-free on the active kobject?

[ ... ]

> @@ -4177,6 +4541,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)
> @@ -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: High]
Does this error path leak the sdkp structure? In sd_probe(), if
sd_mpath_add_disk() fails, the code manually calls device_del() and
del_gendisk() but entirely omits put_device(&sdkp->disk_dev). Because the
initial reference count set by device_initialize() is never decremented to
zero, will this permanently leak the scsi_disk structure and its associated
resources?

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