Re: [PATCH v6 23/27] scsi: sd: add multipath disk attr groups

On 29/07/2026 17:48, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [Medium] Result: Dead Code. The newly defined sysfs attribute groups for multipath disks are never assigned or registered, rendering the patch's functionality completely dead.
> - [Medium] Result: Latent Type Confusion. The `sd_mpath_disk_attr_group` lacks an `is_visible` callback, exposing it to a fatal type-confusion or NULL dereference crash if mistakenly wired up to standard SCSI disks.
> - [Medium] Result: Speculative Use-After-Free. Uncanceled delayed work caused by the `delayed_removal_secs` sysfs configuration risks a UAF on `mpath_head` teardown.
> --
> 
> --- Patch [23]: [PATCH v6 23/27] scsi: sd: add multipath disk attr groups ---
> commit 681fea1e5bfb430f8ec567b5c630bf2528c332b3
> Author: John Garry <[email protected]>
> 
> 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 242a15bc2c5bb..36c745cbb2619 100644
>> --- a/drivers/scsi/sd.c
>> +++ b/drivers/scsi/sd.c
>> @@ -4006,6 +4006,42 @@ static int sd_format_disk_name(char *prefix, int index, char *buf, int buflen)
>>   	return 0;
>>   }
>>   
>> +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);
> 
> [Severity: Medium]
> Does the teardown path in mpath_head_uninit() contain an explicit
> cancel_delayed_work_sync(&mpath_head->remove_work)? If a delayed work is
> scheduled via this sysfs store function, could it race with mpath_head
> teardown and trigger a use-after-free if the work isn't canceled?

delayed work is not scheduled via this sysfs store function

> 
>> +
>> +	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);
>> +}
>> +
>> +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 = {
> 
> [Severity: Medium]
> Should this group include an is_visible() callback? Without one, if this
> attribute group is ever mistakenly attached to generic,

it's not used elsewhere, so I won't add a pointless is_visible callback

> non-multipath SCSI
> disks, accessing the attribute would trigger mpath_bd_device_to_head() on
> an invalid device pointer, potentially causing a type confusion or NULL
> dereference crash.
> 
>> +	.attrs		= sd_mpath_disk_attrs,
>> +};
>> +
>> +__maybe_unused
>> +static const struct attribute_group *sd_mpath_disk_attr_groups[] = {
> 
> [Severity: Medium]
> Are these attribute groups actually registered anywhere? 

They are used later

> The array is defined
> as __maybe_unused, but it doesn't appear to be hooked up to any device
> class (e.g., sd_mpath_disk_class.dev_groups) or passed to device_add_disk()
> in this patch. Will these attributes ever be visible to userspace?
> 
>> +	&sd_mpath_disk_attr_group,
>> +	&mpath_attr_group,
>> +	NULL
>> +};
>