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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Race condition in `scsi_internal_device_unblock` and `scsi_device_block` leading to NULL pointer dereference.
- [High] Concurrent execution of `scsi_mpath_dev_alloc` and transport block/unblock leads to Use-After-Free and NULL pointer dereference.
--

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

scsi-multipath: revalidate paths upon device unblock
    
When a scsi_device is unblocked, this patch clears paths and requeues
any queued bios to revalidate the paths.

> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> --- 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);
>  
>  	return ret;
>  }

[Severity: High]
Can this lockless check of sdev->scsi_mpath_dev lead to a use-after-free or 
NULL pointer dereference?

During device discovery, scsi_alloc_sdev() publishes the scsi_device to 
shost->__devices. After this, scsi_mpath_dev_alloc() assigns 
sdev->scsi_mpath_dev but can fail and jump to out_uninit where it frees 
the structure:

drivers/scsi/scsi_multipath.c:scsi_mpath_dev_alloc() {
    ...
    ret = scsi_multipath_sdev_init(sdev);
    if (ret)
        return ret;

    ret = scsi_mpath_unique_lun_id(sdev);
    if (ret < 0)
        goto out_uninit;
    ...
}

If an asynchronous transport event triggers scsi_internal_device_unblock() 
during this window, could it read a freed sdev->scsi_mpath_dev, or one where 
scsi_mpath_head is not yet initialized?

Could a concurrent device removal also trigger a similar race?

If __scsi_remove_device() executes after the mutex_unlock() but before 
sdev->scsi_mpath_dev is evaluated, it could set the device state to 
SDEV_CANCEL and call scsi_mpath_remove_device(). This sets 
scsi_mpath_dev->scsi_mpath_head to NULL.

(Note: An earlier patch in this series appears to introduce an identical 
lockless pattern in scsi_device_block() checking sdev->scsi_mpath_dev.)

> diff --git a/drivers/scsi/scsi_multipath.c b/drivers/scsi/scsi_multipath.c
> --- a/drivers/scsi/scsi_multipath.c
> +++ b/drivers/scsi/scsi_multipath.c
[ ... ]
> @@ -231,6 +231,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]
Does this code safely handle a NULL scsi_mpath_head?

If the race mentioned above occurs and a concurrent teardown clears 
scsi_mpath_head to NULL, would this assignment dereference the NULL pointer 
when attempting to resolve mpath_head?

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