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

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?

> +
> +	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, 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? 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
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=23