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

On 29/07/2026 18:01, [email protected] wrote:
> 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.

That needs to be fixed in mkp-scsi tree

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

I think that the earlier del_gendisk() -> device_del(disk_to_dev(disk)) 
drops that reference

> 
> [ ... ]
> 
>> @@ -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(),

the count must be non-zero here

later I can add try get variant to make this clear

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

maybe it's a problem, but I did not experience an issue. There is a 
later put on the &sd_mpath_disk->dev in the sd_mpath_put_disk() call, so 
the parent is not gone before disk

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

Same answer as above

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


Yes, mkp-scsi tree needs fixing


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

let's fix mkp-scsi tree first

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