Re: [PATCH v4 13/28] scsi-multipath: introduce scsi_device head structure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] `scsi_mpath_find_head()` prematurely aborts list search on dying heads, leading to split multipath topologies.
- [High] Double removal in `scsi_mpath_remove_device()` triggers a NULL pointer dereference.
- [High] Premature teardown of `scsi_mpath_head` causes a Use-After-Free on inflight bios.
- [Medium] The sysfs attribute `vpd_id` is exposed to userspace before it is initialized.
--

--- Patch [13]: [PATCH v4 13/28] scsi-multipath: introduce scsi_device head structure ---
commit 11c848a95f46ee71e21d42eee402d581f96c8055
Author: John Garry <john.garry@linux.dev>

scsi-multipath: introduce scsi_device head structure

Introduce a scsi_device head structure - scsi_mpath_head - to manage
multipathing for a scsi_device. This is similar to nvme_ns_head structure.

> diff --git a/drivers/scsi/scsi_multipath.c b/drivers/scsi/scsi_multipath.c
> index b9b58a3641aa2..609f8009e4ac3 100644
> --- a/drivers/scsi/scsi_multipath.c
> +++ b/drivers/scsi/scsi_multipath.c

[ ... ]

> @@ -89,6 +141,71 @@ static int scsi_multipath_sdev_init(struct scsi_device *sdev)
> +static struct scsi_mpath_head *scsi_mpath_alloc_head(void)
> +{

[ ... ]

> +	ret = device_add(&scsi_mpath_head->dev);
> +	if (ret) {
> +		put_device(&scsi_mpath_head->dev);
> +		return NULL;
> +	}
> +

[Severity: Medium]
Will calling device_add() here expose an uninitialized vpd_id string to
userspace when the uevent is fired?

The vpd_id is only populated later in scsi_mpath_dev_alloc() via strscpy().
Could a concurrently executing udev rule read an empty string before the
string has been properly set?

> +	return scsi_mpath_head;
> +
> +out_uninit_head:
> +	mpath_head_uninit(&scsi_mpath_head->mpath_head);
> +out_free:
> +	kfree(scsi_mpath_head);
> +	return NULL;
> +}
> +
> +static struct scsi_mpath_head *scsi_mpath_find_head(
> +			struct scsi_mpath_device *scsi_mpath_dev)
> +{
> +	struct scsi_mpath_head *scsi_mpath_head;
> +
> +	list_for_each_entry(scsi_mpath_head, &scsi_mpath_heads_list, entry) {
> +		if (strncmp(scsi_mpath_head->vpd_id,
> +			scsi_mpath_dev->device_id_str,
> +			SCSI_MPATH_DEVICE_ID_LEN) == 0) {
> +			if (scsi_mpath_get_head(scsi_mpath_head))
> +				return NULL;

[Severity: High]
If scsi_mpath_get_head() fails (for example, if the head is dying), does
returning NULL here prematurely abort the list search? Should this be a
continue statement so we don't skip over a valid head further down the
list?

> +			return scsi_mpath_head;
> +		}
> +	}
> +
> +	return NULL;
> +}

[ ... ]

> @@ -133,13 +290,47 @@ void scsi_mpath_dev_release(struct scsi_device *sdev)
> +void scsi_mpath_remove_device(struct scsi_mpath_device *scsi_mpath_dev)
> +{
> +	struct scsi_mpath_head *scsi_mpath_head = scsi_mpath_dev->scsi_mpath_head;
> +
> +	ida_free(&scsi_mpath_head->ida, scsi_mpath_dev->index);
> +
> +	scsi_mpath_remove_head(scsi_mpath_dev);
> +}
> +
>  void scsi_mpath_dev_release(struct scsi_device *sdev)
>  {
>  	struct scsi_mpath_device *scsi_mpath_dev = sdev->scsi_mpath_dev;
>  
> +	if (scsi_mpath_dev)
> +		scsi_mpath_remove_device(scsi_mpath_dev);

[Severity: High]
Is it possible for this to trigger a NULL pointer dereference if the
multipath device has already been detached?

In __scsi_remove_device(), we call scsi_mpath_remove_device() which sets
scsi_mpath_dev->scsi_mpath_head = NULL:

__scsi_remove_device()
    scsi_mpath_remove_device(sdev->scsi_mpath_dev)
        scsi_mpath_remove_head()
            scsi_mpath_dev->scsi_mpath_head = NULL;

When the device release callback runs later, it will call
scsi_mpath_remove_device() again because scsi_mpath_dev is still non-NULL,
leading to a crash when dereferencing scsi_mpath_head.

>  	scsi_multipath_sdev_uninit(sdev);
>  }

> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index 2f80d703ce640..d6bbaf424bd4a 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -1501,6 +1501,9 @@ void __scsi_remove_device(struct scsi_device *sdev)
>  	} else
>  		put_device(&sdev->sdev_dev);
>  
> +	if (sdev->scsi_mpath_dev)
> +		scsi_mpath_remove_device(sdev->scsi_mpath_dev);
> +

[Severity: High]
Could tearing down the multipath head here trigger a use-after-free for
inflight bios?

If this is the last path, the scsi_mpath_head could be unregistered and
freed before the request queue is drained in blk_mq_destroy_queue() later
in this function. When an inflight bio completes, its end_io callback might
try to access the freed head.

Should the multipath head removal happen after the queue is drained?

>  	/*
>  	 * Stop accepting new requests and wait until all queuecommand() and
>  	 * scsi_run_queue() invocations have finished before tearing down the

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