Re: [PATCH v3 03/17] scsi-multipath: provide sysfs link from to scsi_device

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Double free and use-after-free of `scsi_mpath_head` on `dev_set_name()` error path.
- [High] NULL pointer dereference from uninitialized `list_head` on `device_add()` error path.
--

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

scsi-multipath: introduce scsi_device head structure

This commit introduces the scsi_mpath_head structure to manage multipathing
for SCSI devices. It adds a list to look up matching multipathed SCSI devices
using their unique IDs and creates a new scsi_mpath_device_class for managing
the head devices.

> diff --git a/drivers/scsi/scsi_multipath.c b/drivers/scsi/scsi_multipath.c
> index ff37cfdf2f9d1..65ee3da5cc7fc 100644
> --- a/drivers/scsi/scsi_multipath.c
> +++ b/drivers/scsi/scsi_multipath.c
> @@ -88,6 +146,71 @@ static int scsi_multipath_sdev_init(struct scsi_device *sdev)
[ ... ]
> +static struct scsi_mpath_head *scsi_mpath_alloc_head(void)
> +{
> +	struct scsi_mpath_head *scsi_mpath_head;
> +	int ret;
> +
> +	scsi_mpath_head = kzalloc(sizeof(*scsi_mpath_head), GFP_KERNEL);
> +	if (!scsi_mpath_head)
> +		return NULL;
[ ... ]
> +	device_initialize(&scsi_mpath_head->dev);
> +	scsi_mpath_head->dev.class = &scsi_mpath_device_class;
> +	ret = dev_set_name(&scsi_mpath_head->dev, "scsi_mpath_device%d",
> +				scsi_mpath_head->index);
> +	if (ret) {
> +		put_device(&scsi_mpath_head->dev);
> +		goto out_free_ida;
> +	}
> +
> +	return scsi_mpath_head;
> +
> +out_free_ida:
> +	ida_free(&scsi_multipath_dev_ida, scsi_mpath_head->index);
> +out_put_head:
> +	mpath_put_head(&scsi_mpath_head->mpath_head);
> +out_free:
> +	kfree(scsi_mpath_head);
> +	return NULL;
> +}

[Severity: High]
Does this error path cause a use-after-free and double free?

If dev_set_name() fails, put_device() drops the kobject refcount to zero,
which triggers scsi_mpath_head_release() to free the IDA index and kfree
the scsi_mpath_head structure.

When the code falls through to out_free_ida, it attempts to free the index
again using the already-freed pointer, and finally calls kfree() a second
time.

> @@ -96,6 +219,7 @@ static void scsi_multipath_sdev_uninit(struct scsi_device *sdev)
[ ... ]
>  int scsi_mpath_dev_alloc(struct scsi_device *sdev)
>  {
> +	struct scsi_mpath_head *scsi_mpath_head;
>  	int ret;
[ ... ]
> +	ret = device_add(&scsi_mpath_head->dev);
> +	if (ret) {
> +		mutex_unlock(&scsi_mpath_heads_lock);
> +		goto out_put_head;
> +	}
> +
> +	list_add_tail(&scsi_mpath_head->entry, &scsi_mpath_heads_list);
> +found:
> +	mutex_unlock(&scsi_mpath_heads_lock);
> +	ret = ida_alloc(&scsi_mpath_head->ida, GFP_KERNEL);
> +	if (ret < 0)
> +		goto out_put_head;
> +	sdev->scsi_mpath_dev->index = ret;
> +
> +	sdev->scsi_mpath_dev->scsi_mpath_head = scsi_mpath_head;
> +	return 0;
> +out_put_head:
> +	scsi_mpath_put_head(scsi_mpath_head);
>  out_uninit:
>  	scsi_multipath_sdev_uninit(sdev);
>  	return ret;
>  }

> @@ -133,13 +302,47 @@ void scsi_mpath_dev_release(struct scsi_device *sdev)
[ ... ]
> +static void scsi_mpath_free_head(struct kref *ref)
> +{
> +	struct scsi_mpath_head *scsi_mpath_head =
> +		container_of(ref, struct scsi_mpath_head, ref);
> +
> +	/*
> +	 * If we race with scsi_mpath_find_head(), then that function may
> +	 * find this scsi_mpath_head in the heads list; however we would fail
> +	 * to take a reference to this scsi_mpath_head and continue the search.
> +	 * As such, it is safe to call device_unregister (and free
> +	 * scsi_mpath_head) after we delete this head from the list.
> +	 */
> +	mutex_lock(&scsi_mpath_heads_lock);
> +	list_del_init(&scsi_mpath_head->entry);
> +	mutex_unlock(&scsi_mpath_heads_lock);
> +
> +	device_unregister(&scsi_mpath_head->dev);
> +}

[Severity: High]
Can this list_del_init() and device_unregister() crash if called from the
device_add() error path in scsi_mpath_dev_alloc()?

If device_add() fails, scsi_mpath_put_head() is called, which eventually
invokes this cleanup function. However, because device_add() failed before
list_add_tail() was called, and the list head was never initialized on the
kzalloc-ed structure, the list pointers are NULL. This would cause a NULL
pointer dereference in list_del_init().

Additionally, device_unregister() expects the device to have been
successfully added, which isn't the case on this error path.

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