Re: [PATCH v6 17/27] scsi-multipath: revalidate paths upon device unblock

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

New issues:
- [High] Calling `scsi_mpath_revalidate_paths()` outside `sdev->state_mutex` without a NULL check leads to TOCTOU races with device removal or allocation, causing a NULL pointer dereference.
- [High] Direct callers of `scsi_internal_device_unblock_nowait` bypass path revalidation, which can leave queued bios indefinitely stalled.

Pre-existing issues:
- [High] The patch calls `mpath_revalidate_paths()`, which schedules `requeue_work` on a background workqueue, but the teardown path fails to cancel this work, leading to a Use-After-Free.
--

--- Patch [17]: [PATCH v6 17/27] scsi-multipath: revalidate paths upon device unblock ---
commit 6253cb1fc352f4ec6d0d42ac99c1827b35592ff4
Author: John Garry <[email protected]>

scsi-multipath: revalidate paths upon device unblock

When a scsi_device is unblocked, we need revalidate paths which means that
we clear paths and requeue any queued bios.

> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index a1ec11050090c..6a2d7dbfbe8c3 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -3092,6 +3092,8 @@ static int scsi_internal_device_unblock(struct scsi_device *sdev,
>  	mutex_lock(&sdev->state_mutex);
>  	ret = scsi_internal_device_unblock_nowait(sdev, new_state);
>  	mutex_unlock(&sdev->state_mutex);
> +	if ((ret == 0) && sdev->scsi_mpath_dev)
> +		scsi_mpath_revalidate_paths(sdev->scsi_mpath_dev);

[Severity: High]
Is it safe to access sdev->scsi_mpath_dev and call scsi_mpath_revalidate_paths()
outside of sdev->state_mutex?

It seems that concurrently, __scsi_remove_device() could nullify
scsi_mpath_dev->scsi_mpath_head. A similar race might exist during device
initialization before the head is populated.

When scsi_mpath_revalidate_paths() is then called, it blindly computes the
offset to &scsi_mpath_head->mpath_head, which could lead to a NULL pointer
dereference.

[Severity: High]
Does placing this hook in the wrapper bypass path revalidation for some drivers?

Low-level drivers like mpt3sas and scsi_transport_srp call the core API
scsi_internal_device_unblock_nowait() directly. Since the revalidation is
only added to the scsi_internal_device_unblock() wrapper, won't devices managed
by those drivers miss path revalidation when unblocked, potentially leaving
queued bios permanently stalled?

>  
>  	return ret;
>  }
> diff --git a/drivers/scsi/scsi_multipath.c b/drivers/scsi/scsi_multipath.c
> index b23cd6f12c602..12750621a7fc0 100644
> --- a/drivers/scsi/scsi_multipath.c
> +++ b/drivers/scsi/scsi_multipath.c
[ ... ]
> @@ -238,6 +238,14 @@ static int scsi_multipath_sdev_init(struct scsi_device *sdev)
>  	return 0;
>  }
>  
> +void scsi_mpath_revalidate_paths(struct scsi_mpath_device *scsi_mpath_dev)
> +{
> +       struct scsi_mpath_head *scsi_mpath_head = scsi_mpath_dev->scsi_mpath_head;
> +       struct mpath_head *mpath_head = &scsi_mpath_head->mpath_head;
> +
> +       mpath_revalidate_paths(mpath_head);

[Severity: High]
This is a pre-existing issue, but by introducing this new asynchronous trigger,
could we run into a use-after-free during device removal?

Calling mpath_revalidate_paths() queues mpath_head->requeue_work. If the device
is subsequently removed, the teardown path in scsi_mpath_head_release() frees
scsi_mpath_head via kfree():

drivers/scsi/scsi_multipath.c:scsi_mpath_head_release() {
    ...
    mpath_head_uninit(mpath_head);
    kfree(scsi_mpath_head);
}

Because cancel_work_sync(&mpath_head->requeue_work) is missing from the teardown
logic, could the pending work execute on a freed mpath_head structure?

> +}
> +

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