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

[John Garry <john.g.garry@oracle.com>]

On 03/07/2026 13:57, sashiko-bot@kernel.org wrote:
> 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?


same as 15/17

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

yeah, I should sysfs_emit(0) here

> 
> [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[] = {
>