Re: [PATCH v3 14/17] scsi: sd: add multipath disk attr groups

[sashiko-bot@kernel.org]

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

New issues:
- [Low] The patch references several undeclared identifiers and undefined functions, which will cause fatal compilation and linker errors.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but error paths in `sd_mpath_probe()` incorrectly use `kfree()` instead of `put_device()` after `device_initialize()`, leaking kobject resources. Furthermore, it cannot trivially use `put_device()` without causing a use-after-free and IDA corruption.
--

commit c032527f31f85b7a1e95cfcc2527fe7e368f3474
Author: John Garry <john.g.garry@oracle.com>

scsi: sd: add multipath disk attr groups

Set multipath disk attr groups, which includes delayed disk removal and
everything from mpath_attr_group.

> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 3df70b24b688e..a9a29e50f5eec 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c

[ ... ]

> @@ -4106,6 +4106,41 @@ static void sd_mpath_add_disk(struct scsi_disk *sdkp)
>  	mpath_device_set_live(mpath_device);
>  }
>  
> +static ssize_t sd_mpath_device_delayed_removal_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	struct mpath_head *mpath_head = mpath_bd_device_to_head(dev);
> +
> +	return mpath_delayed_removal_secs_store(mpath_head, buf, count);
> +}
> +
> +static ssize_t sd_mpath_device_delayed_removal_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct mpath_head *mpath_head = mpath_bd_device_to_head(dev);
> +
> +	return mpath_delayed_removal_secs_show(mpath_head, buf);
> +}

[Severity: Low]
Are the functions mpath_bd_device_to_head(), mpath_delayed_removal_secs_store(),
and mpath_delayed_removal_secs_show() declared anywhere? It looks like they
might be missing, which would cause a build error.

> +
> +static DEVICE_ATTR(delayed_removal_secs, S_IRUGO | S_IWUSR,
> +		sd_mpath_device_delayed_removal_show,
> +		sd_mpath_device_delayed_removal_store);
> +
> +static struct attribute *sd_mpath_disk_attrs[] = {
> +	&dev_attr_delayed_removal_secs.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group sd_mpath_disk_attr_group = {
> +	.attrs		= sd_mpath_disk_attrs,
> +};
> +
> +static const struct attribute_group *sd_mpath_disk_attr_groups[] = {
> +	&sd_mpath_disk_attr_group,
> +	&mpath_attr_group,

[Severity: Low]
Is the external variable mpath_attr_group declared and defined? Building with
this patch applied results in an undeclared identifier error.

> +	NULL
> +};
> +
>  static int sd_mpath_probe(struct scsi_disk *sdkp)
>  {
>  	struct scsi_device *sdp = sdkp->device;
> @@ -4161,6 +4196,7 @@ static int sd_mpath_probe(struct scsi_disk *sdkp)
>  
>  	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)

[Severity: High]
This isn't a bug introduced by this patch, but since we are modifying
sd_mpath_probe(), is it safe to use kfree() on error paths after
device_initialize() has been called on sd_mpath_disk->dev?

If mpath_alloc_head_disk() fails, the code jumps to out_free_disk:

drivers/scsi/sd.c:sd_mpath_probe() {
    ...
    device_initialize(&sd_mpath_disk->dev);
    ...
    error = mpath_alloc_head_disk(mpath_head, &lim, dev_to_node(dma_dev));
    if (error)
        goto out_free_disk;
    ...
out_free_disk:
    kfree(sd_mpath_disk);
    ...
}

Should this use put_device() instead to ensure the kobject internals are
properly cleaned up?

However, calling put_device() would invoke sd_mpath_disk_release(), which
unconditionally calls ida_free() and scsi_mpath_put_head(). Would
sd_mpath_disk_release() need to be updated to handle partial initialization if
put_device() were used here?

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