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

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

On 03/07/2026 13:44, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [Low] Build failure due to undefined function `mpath_numa_nodes_show`.
> 
> Pre-existing issues:
> - [Critical] Race condition during device removal leads to a Use-After-Free of `sd_mpath_disk`.
> --
> 
> commit 90ab5069ccf099a5f4cd42027fac637f7869d718
> Author: John Garry <john.g.garry@oracle.com>
> 
> scsi: sd: add mpath_numa_nodes dev attribute
> 
> Add an attribute to show multipath NUMA node per-path (scsi_disk).
> 
>> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
>> index 0c77466f8291a..5e0514304d81f 100644
>> --- a/drivers/scsi/sd.c
>> +++ b/drivers/scsi/sd.c
>> @@ -4075,8 +4075,22 @@ static ssize_t sd_mpath_dev_show(struct device *dev,
>>   }
>>   static DEVICE_ATTR(mpath_dev, 0444, sd_mpath_dev_show, NULL);
> 
> [Severity: Critical]
> This is a pre-existing issue, but does device removal race with sysfs reads
> and cause a Use-After-Free?
> 
> Looking at sd_remove(), sd_mpath_remove(sdkp) is called before del_gendisk(),
> which frees sd_mpath_disk but leaves the sysfs attributes active:
> 
> 	if (sdp->scsi_mpath_dev)
> 		sd_mpath_remove(sdkp);
> 
> 	scsi_autopm_get_device(sdkp->device);
> 
> 	device_del(&sdkp->disk_dev);
> 	del_gendisk(sdkp->disk);
> 
> If a local user concurrently reads the mpath_dev sysfs attribute, won't
> sd_mpath_dev_show() blindly dereference the freed sd_mpath_disk? Will the
> newly added attribute suffer from a similar lifecycle problem?

same as 15/17

> 
>>   
>> +static ssize_t sd_mpath_numa_nodes_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 scsi_mpath_device *scsi_mpath_dev = sdev->scsi_mpath_dev;
>> +	struct mpath_device *mpath_device = &scsi_mpath_dev->mpath_device;
>> +
>> +	return mpath_numa_nodes_show(mpath_device, buf);
> 
> [Severity: Low]
> Will this compile correctly? I couldn't find a declaration or definition for
> mpath_numa_nodes_show() anywhere. Could this lead to an implicit function
> declaration and a subsequent linker error?
> 
>> +}
>> +static DEVICE_ATTR(mpath_numa_nodes, 0444, sd_mpath_numa_nodes_show, NULL);
>> +
>>   static struct attribute *sd_mpath_dev_attrs[] = {
>>   	&dev_attr_mpath_dev.attr,
>> +	&dev_attr_mpath_numa_nodes.attr,
>>   	NULL
>>   };
>