Re: [PATCH v9 18/29] scsi-multipath: revalidate paths upon device unblock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] scsi: multipath: race in scsi_internal_device_unblock() on
  scsi_mpath_head
- [High] scsi: multipath: use-after-free in
  scsi_mpath_revalidate_paths() requeue_work
- [Medium] scsi: multipath: race in scsi_device_block() accessing
  scsi_mpath_head
- [Medium] scsi: multipath: premature scsi_mpath_dev publish in
  scsi_mpath_dev_alloc()
--

commit ed012aa632cd8957f54296618fde6fa312555b38
Author: John Garry <john.garry@linux.dev>

scsi-multipath: revalidate paths upon device unblock

This commit adds scsi_mpath_revalidate_paths() and calls it from
scsi_internal_device_unblock() to clear paths and requeue bios when a
multipath SCSI device is unblocked.

> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 1950034f580f4..4dbd01484b4a5 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -3121,6 +3121,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 scsi_internal_device_unblock() race with __scsi_remove_device() and
dereference a NULL or freed scsi_mpath_head?

Callers such as scsi_target_unblock() and scsi_host_unblock() iterate
devices with shost_for_each_device(), which pins sdev->sdev_gendev (deferring
scsi_device_dev_release() where sdev->scsi_mpath_dev is freed) without
holding shost->scan_mutex.  Because sdev->state_mutex is dropped before
calling scsi_mpath_revalidate_paths() (and
scsi_internal_device_unblock_nowait() also returns 0 in SDEV_CANCEL),
__scsi_remove_device() can tear down scsi_mpath_dev->scsi_mpath_head
concurrently:

CPU0                                  CPU1
scsi_target_unblock()
  scsi_internal_device_unblock()
    mutex_lock(&sdev->state_mutex)
    scsi_internal_device_unblock_nowait()
    mutex_unlock(&sdev->state_mutex)
                                      __scsi_remove_device()
                                        ...
                                        scsi_mpath_remove_device()
                                          scsi_mpath_remove_head()
                                            scsi_mpath_put_head()
                                            scsi_mpath_dev->scsi_mpath_head = NULL
    scsi_mpath_revalidate_paths()
      mpath_revalidate_paths(mpath_head)  <-- NULL deref or use-after-free

In scsi_mpath_revalidate_paths(), &scsi_mpath_head->mpath_head is at offset
0, so mpath_clear_paths() and mpath_schedule_requeue_work() dereference a
NULL or freed mpath_head.

[Severity: Medium]
Can scsi_internal_device_unblock() also observe a partially initialized or
freed sdev->scsi_mpath_dev during LUN scanning?

In scsi_add_lun(), sdev is already on shost->__devices and starget->devices
and set to SDEV_RUNNING or SDEV_BLOCK before scsi_mpath_dev_alloc() runs:

CPU0                                  CPU1
scsi_add_lun()
  scsi_mpath_dev_alloc()
    scsi_multipath_sdev_init()
      sdev->scsi_mpath_dev = scsi_mpath_dev
                                      scsi_target_unblock()
                                        scsi_internal_device_unblock()
                                          if (ret == 0 && sdev->scsi_mpath_dev)
                                            scsi_mpath_revalidate_paths()
                                              mpath_revalidate_paths(NULL)
                                              <-- NULL pointer dereference
    scsi_mpath_unique_lun_id()
    ...
    sdev->scsi_mpath_dev->scsi_mpath_head = ...

If scsi_mpath_dev_alloc() fails later at out_uninit,
scsi_multipath_sdev_uninit() also frees sdev->scsi_mpath_dev before clearing
the pointer, allowing a concurrent unblock to hit a use-after-free.

> diff --git a/drivers/scsi/scsi_multipath.c b/drivers/scsi/scsi_multipath.c
> index 67e9010d54782..008bcafe73ad2 100644
> --- a/drivers/scsi/scsi_multipath.c
> +++ b/drivers/scsi/scsi_multipath.c
> @@ -240,6 +240,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]
Can mpath_head->requeue_work queued by scsi_mpath_revalidate_paths() run
after scsi_mpath_head is freed?

Transport target teardown paths such as fc_starget_delete(),
iscsi_remove_session(), and rport_dev_loss_timedout() call
scsi_target_unblock(..., SDEV_TRANSPORT_OFFLINE) immediately followed by
scsi_remove_target():

CPU0                                  kworker
fc_starget_delete()
  scsi_target_unblock()
    scsi_internal_device_unblock()
      scsi_mpath_revalidate_paths()
        mpath_revalidate_paths()
          mpath_schedule_requeue_work()
  scsi_remove_target()
    ...
      scsi_mpath_remove_device()
        scsi_mpath_put_head()
          scsi_mpath_head_release()
            kfree(scsi_mpath_head)
                                      mpath_requeue_work()
                                        spin_lock_irq(&mpath_head->requeue_lock)
                                        <-- use-after-free

Unlike scsi_mpath_device_iopolicy_store(), which calls
flush_work(&mpath_head->requeue_work) with a comment noting that
requeue_work could otherwise run after mpath_head is gone, neither
scsi_mpath_revalidate_paths() nor scsi_mpath_head_release() /
mpath_head_uninit() flushes or cancels mpath_head->requeue_work.

> +
>  void scsi_mpath_dev_clear_path(struct scsi_mpath_device *scsi_mpath_dev)
>  {
>         struct mpath_device *mpath_device = &scsi_mpath_dev->mpath_device;

[Severity: Medium]
Can scsi_device_block() hit the same race against __scsi_remove_device() or
scsi_mpath_dev_alloc() when calling scsi_mpath_dev_clear_path()?

drivers/scsi/scsi_lib.c:scsi_device_block() {
    ...
	mutex_unlock(&sdev->state_mutex);
	if ((err == 0) && sdev->scsi_mpath_dev)
		scsi_mpath_dev_clear_path(sdev->scsi_mpath_dev);
    ...
}

Once scsi_device_block() drops sdev->state_mutex, __scsi_remove_device() can
transition sdev from SDEV_BLOCK directly to SDEV_DEL and run
scsi_mpath_remove_device(), freeing scsi_mpath_head and clearing
scsi_mpath_dev->scsi_mpath_head before scsi_mpath_dev_clear_path()
dereferences it in mpath_synchronize().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261108204733.557995-1-john.garry@linux.dev?part=18