Re: [PATCH v3 17/17] scsi: sd: add mpath_queue_depth dev attribute

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] The patch calls an undeclared function `mpath_qd_iopolicy()`, which will cause a build failure.
- [Low] Returning 0 from a sysfs `show` callback yields an empty read (EOF) instead of a proper error code.

Pre-existing issues:
- [High] Concurrent sysfs read of `mpath_queue_depth` during device teardown accesses a freed `sd_mpath_disk`.
--

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

scsi: sd: add mpath_queue_depth dev attribute

Add a queue_depth file so that the multipath dynamic queue depth can be
looked up from per-path gendisk (scsi_disk) directory.

> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 5e0514304d81f..f6feccd557e76 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
[ ... ]
> @@ -4088,9 +4088,27 @@ static ssize_t sd_mpath_numa_nodes_show(struct device *dev,
>  }
>  static DEVICE_ATTR(mpath_numa_nodes, 0444, sd_mpath_numa_nodes_show, NULL);
>  
> +static ssize_t sd_mpath_queue_depth_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct gendisk *gd = dev_to_disk(dev);
> +	struct scsi_disk *sdkp = gd->private_data;
> +	struct scsi_device *sdev = sdkp->device;
> +	struct sd_mpath_disk *sd_mpath_disk = sdkp->sd_mpath_disk;
> +	struct scsi_mpath_head *scsi_mpath_head = sd_mpath_disk->scsi_mpath_head;

[Severity: High]
This is a pre-existing issue, but can this sysfs read race with device
removal and cause a use-after-free?

During device removal in sd_remove(), sd_mpath_remove() is called to drop
the reference to sd_mpath_disk (and free it when the refcount hits zero):

sd_remove() {
        ...
        if (sdp->scsi_mpath_dev)
                sd_mpath_remove(sdkp);
        
        scsi_autopm_get_device(sdkp->device);
        
        device_del(&sdkp->disk_dev);
        del_gendisk(sdkp->disk);
}

Since device_del() and del_gendisk() happen later in the removal process,
local userspace can trigger a use-after-free by reading this attribute in a
loop while the multipath device is being hot-unplugged.

> +	struct Scsi_Host *shost = sdev->host;
> +
> +	if (!mpath_qd_iopolicy(&scsi_mpath_head->iopolicy))

[Severity: Critical]
Is mpath_qd_iopolicy() declared anywhere in the tree?

Compiling the kernel with CONFIG_SCSI_MULTIPATH enabled will attempt to
compile this code, but a search of the source tree indicates this function
is neither defined nor declared, which will cause the build to fail.

> +		return 0;

[Severity: Low]
Does returning 0 here result in an unexpected End-Of-File?

Returning 0 from a sysfs show callback causes the VFS to report 0 bytes
read, which userspace interpreters like cat treat as an immediate EOF,
resulting in empty output.

Should this return a standard error code like -EOPNOTSUPP instead?

> +
> +	return sysfs_emit(buf, "%d\n", atomic_read(&shost->mpath_nr_active));
> +}
> +static DEVICE_ATTR(mpath_queue_depth, 0444, sd_mpath_queue_depth_show, NULL);
> +
>  static struct attribute *sd_mpath_dev_attrs[] = {

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