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 <[email protected]> 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/[email protected]?part=18
