Re: [PATCH v3 06/17] scsi-multipath: clear path when device is blocked

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [Critical] Unsafe lockless access to `scsi_mpath_head` in `scsi_mpath_dev_clear_path` races with device removal, causing a NULL pointer dereference or Use-After-Free.
- [High] Adding a blocking synchronization call to the `nowait` context of `__scsi_internal_device_block_nowait` violates non-blocking guarantees.
- [High] Publishing `sdev->scsi_mpath_dev` before `scsi_mpath_head` is initialized leads to NULL pointer dereferences during concurrent block requests.

Pre-existing issues:
- [High] The error path in `scsi_mpath_alloc_head()` double-frees the `scsi_mpath_head` structure and its associated resources.
- [High] The error path for `device_add()` failure in `scsi_mpath_dev_alloc()` causes a NULL pointer dereference and an invalid `device_unregister()`.
--

commit 848d562af61faa57ddead6bc3e5e7a75ce63ea4e
Author: John Garry <john.g.garry@oracle.com>
Subject: scsi-multipath: clear path when device is blocked

This patch adds scsi_mpath_dev_clear_path() to clear a device path when it
becomes blocked. It updates __scsi_internal_device_block_nowait() to call
this new function when a path is available.

> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index daeb3693fe559..3c493068adb98 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
[ ... ]
> @@ -2923,6 +2924,8 @@ EXPORT_SYMBOL(scsi_target_resume);
>  
>  static int __scsi_internal_device_block_nowait(struct scsi_device *sdev)
>  {
> +	if (sdev->scsi_mpath_dev)

[Severity: High]
Does this early access race with device probe? 

If scsi_multipath_sdev_init() publishes sdev->scsi_mpath_dev before
allocating scsi_mpath_head, could an asynchronous block request see this as
true and invoke scsi_mpath_dev_clear_path(), leading to a NULL pointer
dereference on the head?

> +		scsi_mpath_dev_clear_path(sdev->scsi_mpath_dev);
>  	if (scsi_device_set_state(sdev, SDEV_BLOCK))
>  		return scsi_device_set_state(sdev, SDEV_CREATED_BLOCK);
>  
> diff --git a/drivers/scsi/scsi_multipath.c b/drivers/scsi/scsi_multipath.c
> index ca4ab720c19af..0f3f3f9fa5fae 100644
> --- a/drivers/scsi/scsi_multipath.c
> +++ b/drivers/scsi/scsi_multipath.c
[ ... ]
> @@ -113,6 +113,17 @@ static ssize_t scsi_mpath_device_vpd_id_show(struct device *dev,
>  }
>  static DEVICE_ATTR(vpd_id, S_IRUGO, scsi_mpath_device_vpd_id_show, NULL);
>  
> +void scsi_mpath_dev_clear_path(struct scsi_mpath_device *scsi_mpath_dev)
> +{
> +	struct mpath_device *mpath_device = &scsi_mpath_dev->mpath_device;
> +	struct scsi_mpath_head *scsi_mpath_head = scsi_mpath_dev->scsi_mpath_head;

[Severity: Critical]
Is this lockless access safe against concurrent device removal?

If scsi_mpath_remove_head() drops the head reference locklessly and sets:
    scsi_mpath_dev->scsi_mpath_head = NULL;

Could a concurrent event hitting __scsi_internal_device_block_nowait()
unconditionally call this and blindly dereference the NULL or freed
scsi_mpath_head?

> +	struct mpath_head *mpath_head = &scsi_mpath_head->mpath_head;
> +
> +	if (mpath_clear_current_path(mpath_device))
> +		mpath_synchronize(mpath_head);

[Severity: High]
Does this introduce a sleeping call into a nowait context?

__scsi_internal_device_block_nowait() is designed not to sleep to preserve
batching optimizations. Since mpath_synchronize() typically sleeps to wait
for grace periods or draining I/O, will calling it here break those
optimizations or cause scheduling while atomic panics?

> +}
> +EXPORT_SYMBOL_GPL(scsi_mpath_dev_clear_path);
> +
>  static ssize_t scsi_mpath_device_iopolicy_store(struct device *dev,


While reviewing this patch, I also noticed a couple of pre-existing issues
in the earlier patches of this series:

[Severity: High]
This is a pre-existing issue, but does the error path in
scsi_mpath_alloc_head() double-free the structure?

If dev_set_name() fails:
    if (dev_set_name(&scsi_mpath_head->dev, ...)) {
        put_device(&scsi_mpath_head->dev);
        goto out_free_ida;
    }

put_device() drops the refcount to 0, which invokes the release function
scsi_mpath_head_release() to free the structure and its resources.
The code then falls through to out_free_ida where it manually executes the
IDA free, bioset exit, and a second kfree() on the already freed struct.


[Severity: High]
This is a pre-existing issue, but does the error path for device_add()
cause a NULL pointer dereference in scsi_mpath_dev_alloc()?

If device_add() fails, it jumps to out_put_head:
    ret = device_add(&scsi_mpath_head->dev);
    if (ret) {
        mutex_unlock(&scsi_mpath_heads_lock);
        goto out_put_head;
    }

This ultimately calls scsi_mpath_free_head(), which unconditionally
executes:
    list_del_init(&scsi_mpath_head->entry);

Since the device was never added to the list on this error path, entry is
still zero-initialized. Will list_del_init() dereference a NULL pointer
here? Also, would the cleanup code call device_unregister() on a device
that failed device_add()?

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