Thank you for your contribution! Sashiko AI review found 7 potential issues to consider: - [High] scsi: sd: premature sd_mpath_remove() before del_gendisk() in sd_remove() - [High] scsi: sd: broken revalidation and state sync in sd_mpath_revalidate_head() - [Medium] scsi: sd: clear current_path after mpath_delete_device() in sd_mpath_remove() - [Medium] scsi: sd: premature sd_mpath_disk list unlink races with sd_mpath_probe() - [Medium] scsi: sd: uncancelled remove_work in sd_mpath_fail_probe() causes UAF - [Medium] scsi: sd: parent sd_mpath_disk->dev deleted before child mpath_head->disk - [Low] scsi: sd: unsigned sdkp->index makes >= 0 check always true --
commit a00d9c64612f4369c0c2dd5d2bf64b95ed52be50 Author: John Garry <[email protected]> scsi: sd: support multipath disk This commit adds support for attaching and managing SCSI multipath disks in the sd driver, creating a multipath head gendisk while keeping per-path gendisks hidden. > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c > index fb87b1663ad62..ab4ce6c2a8dd2 100644 > --- a/drivers/scsi/sd.c > +++ b/drivers/scsi/sd.c [ ... ] > @@ -817,7 +835,8 @@ static void scsi_disk_release(struct device *dev) > { > struct scsi_disk *sdkp = to_scsi_disk(dev); > > - ida_free(&sd_index_ida, sdkp->index); > + if (sdkp->index >= 0) > + ida_free(&sd_index_ida, sdkp->index); > put_device(&sdkp->device->sdev_gendev); > free_opal_dev(sdkp->opal_dev); > > @@ -4021,6 +4040,90 @@ static int sd_format_disk_name(char *prefix, int > index, char *buf, int buflen) [Severity: Low] Is sdkp->index >= 0 always true here? struct scsi_disk declares index as u32 in drivers/scsi/sd.h, while sd_mpath_probe() sets sdkp->index = -1. This triggers a -Wtype-limits warning under W=1 and still calls ida_free(&sd_index_ida, 0xffffffffU) on multipath per-path disks. Changing struct scsi_disk::index to int matches sd_mpath_disk::disk_index and local index in sd_probe(). > return 0; > } > > +#ifdef CONFIG_SCSI_MULTIPATH > +static int sd_mpath_revalidate_head(struct scsi_disk *sdkp) > +{ > + struct sd_mpath_disk *sd_mpath_disk = sdkp->sd_mpath_disk; > + struct scsi_mpath_head *scsi_mpath_head = > sd_mpath_disk->scsi_mpath_head; > + struct mpath_head *mpath_head = &scsi_mpath_head->mpath_head; > + struct gendisk *disk = mpath_head->disk; > + struct queue_limits *sdkp_lim = &sdkp->disk->queue->limits; > + struct queue_limits lim; > + unsigned int memflags; > + int ret; > + > + lim = queue_limits_start_update(disk->queue); > + memflags = blk_mq_freeze_queue(disk->queue); > + > + lim.logical_block_size = sdkp_lim->logical_block_size; > + lim.physical_block_size = sdkp_lim->physical_block_size; > + lim.io_min = sdkp_lim->io_min; > + lim.io_opt = sdkp_lim->io_opt; > + > + queue_limits_stack_bdev(&lim, sdkp->disk->part0, 0, > + disk->disk_name); > + > + /* TODO: setup integrity and zoned limits */ > + lim.max_write_streams = sdkp_lim->max_write_streams; > + lim.write_stream_granularity = sdkp_lim->write_stream_granularity; > + ret = queue_limits_commit_update(disk->queue, &lim); > + > + set_capacity_and_notify(disk, get_capacity(sdkp->disk)); > + > + mpath_revalidate_paths(mpath_head); > + > + blk_mq_unfreeze_queue(disk->queue, memflags); > + > + return ret; > +} [Severity: High] Can probing a standby ALUA path overwrite the multipath head disk capacity with 0? On ALUA arrays, standby or unavailable paths succeed INQUIRY VPD page 0x83 and attach to scsi_mpath_head, but fail READ CAPACITY in sd_revalidate_disk(), leaving get_capacity(sdkp->disk) at 0. Calling sd_mpath_revalidate_head() unconditionally from sd_probe() sets the live mpath_head->disk capacity to 0, causing subsequent I/O on the multipath disk to fail bio_check_eod() with -EIO. In addition, sd_mpath_revalidate_head() has several other state sync issues: - It is only called from sd_probe() and never from sd_revalidate_disk() or sd_rescan(), so LUN resizes or standby-to-active transitions do not update mpath_head->disk. - It never calls set_disk_ro(disk, sdkp->write_prot), leaving write-protected multipath disks openable for writes via mpath_ops. - When queue_limits_commit_update() fails, sd_mpath_revalidate_head() still updates capacity and paths, and sd_probe() only logs a warning before calling sd_mpath_add_disk(sdkp) with uncommitted queue limits. - For TYPE_ZBC disks, queue_limits_stack_bdev() inherits BLK_FEAT_ZONED onto mpath_head->disk->queue even though zoned multipath support is not set up. [ ... ] > @@ -4050,13 +4153,260 @@ static const struct attribute_group > sd_mpath_disk_attr_group = { [ ... ] > +static void sd_mpath_remove(struct scsi_disk *sdkp) > +{ > + struct sd_mpath_disk *sd_mpath_disk = sdkp->sd_mpath_disk; > + struct scsi_device *sdp = sdkp->device; > + struct scsi_mpath_device *scsi_mpath_dev = sdp->scsi_mpath_dev; > + struct mpath_device *mpath_device = &scsi_mpath_dev->mpath_device; > + struct scsi_mpath_head *scsi_mpath_head = > sd_mpath_disk->scsi_mpath_head; > + struct mpath_head *mpath_head = &scsi_mpath_head->mpath_head; > + bool remove = false; > + > + mpath_synchronize(mpath_head); > + > + if (mpath_clear_current_path(mpath_device)) > + mpath_synchronize(mpath_head); > + > + mpath_delete_device(mpath_device); [Severity: Medium] Can mpath_device be re-cached into mpath_head->current_path[node] when mpath_clear_current_path() runs before mpath_delete_device()? When sd_remove() is triggered by driver unbind or module unload, sdev->sdev_state remains SDEV_RUNNING, so scsi_mpath_is_disabled() returns false. Because mpath_device is still on mpath_head->dev_list until mpath_delete_device() unlinks it, a concurrent path lookup can repopulate current_path[node]: CPU0 CPU1 sd_mpath_remove() mpath_clear_current_path(mpath_device) mpath_bdev_submit_bio() mpath_find_path() __mpath_find_path() rcu_assign_pointer( current_path[node], mpath_device) mpath_delete_device(mpath_device) ... mpath_device->disk = NULL; mpath_bdev_submit_bio() mpath_find_path() /* returns cached mpath_device */ mpath_device->disk->part0 <-- NULL pointer dereference Calling mpath_delete_device(mpath_device) before mpath_clear_current_path() prevents __mpath_find_path() from finding mpath_device on dev_list after current_path is cleared. > + > + mutex_lock(&sd_mpath_disks_lock); > + sd_mpath_disk->disk_count--; > + if (!sd_mpath_disk->disk_count && mpath_can_remove_head(mpath_head)) { > + list_del_init(&sd_mpath_disk->entry); > + remove = true; > + } > + mutex_unlock(&sd_mpath_disks_lock); [Severity: Medium] Can unlinking sd_mpath_disk and dropping sd_mpath_disks_lock before teardown completes race with a concurrent sd_mpath_probe() for the same scsi_mpath_head? In sd_mpath_remove(), sd_mpath_remove_head(), and sd_mpath_fail_probe(), sd_mpath_disk is removed from sd_mpath_disks_list and sd_mpath_disks_lock is released before device_del(), mpath_remove_disk(), and sd_mpath_put_disk() finish tearing down the shared mpath_head: CPU0 CPU1 sd_mpath_remove() mutex_lock(&sd_mpath_disks_lock) list_del_init(&sd_mpath_disk->entry) mutex_unlock(&sd_mpath_disks_lock) sd_mpath_probe() mutex_lock(&sd_mpath_disks_lock) sd_mpath_find_disk() -> NULL mpath_alloc_head_disk(mpath_head) /* fails with -EINVAL, or races * with old sd_mpath_disk_release() * calling mpath_put_disk() */ device_del(&sd_mpath_disk->dev) mpath_remove_disk(mpath_head) sd_mpath_put_disk(sd_mpath_disk) Depending on timing, mpath_alloc_head_disk() fails with -EINVAL because mpath_head->disk is still non-NULL, device_add() fails with -EEXIST on the duplicate "scsi_mpath_disk%d" name, or a delayed sd_mpath_disk_release() calls mpath_put_disk(mpath_head) after the new probe has allocated a new mpath_head->disk. > + mpath_remove_sysfs_link(mpath_device); > + mpath_device->disk = NULL; > + > + if (remove) { > + device_del(&sd_mpath_disk->dev); > + mpath_remove_disk(mpath_head); > + } > + sd_mpath_put_disk(sd_mpath_disk); > +} [Severity: Medium] Does calling device_del(&sd_mpath_disk->dev) before mpath_remove_disk(mpath_head) delete the parent device before its child gendisk? sd_mpath_probe() sets mpath_head->parent = &sd_mpath_disk->dev, and mpath_device_set_live() registers mpath_head->disk as a child of that device. In sd_mpath_remove(), sd_mpath_remove_head(), and sd_mpath_fail_probe(), calling device_del(&sd_mpath_disk->dev) first clears sd_mpath_disk->dev.kobj.parent and tears down the parent sysfs directory while mpath_head->disk is still registered, causing del_gendisk() inside mpath_remove_disk() to emit child KOBJ_REMOVE uevents with truncated DEVPATHs. Also, in sd_mpath_remove(), mpath_remove_sysfs_link(mpath_device) runs after dropping sd_mpath_disks_lock, so a concurrent last-path removal or sd_mpath_remove_head() can remove mpath_head->disk while mpath_remove_sysfs_link() is still accessing disk_to_dev(mpath_head->disk). [ ... ] > +static void sd_mpath_fail_probe(struct scsi_disk *sdkp) > +{ > + struct sd_mpath_disk *sd_mpath_disk = sdkp->sd_mpath_disk; > + struct scsi_mpath_device *scsi_mpath_dev; > + struct mpath_device *mpath_device; > + struct scsi_device *sdp = sdkp->device; > + struct scsi_mpath_head *scsi_mpath_head; > + struct mpath_head *mpath_head; > + bool remove = false; > + > + if (!sd_mpath_disk) > + return; > + > + scsi_mpath_dev = sdp->scsi_mpath_dev; > + mpath_device = &scsi_mpath_dev->mpath_device; > + scsi_mpath_head = sd_mpath_disk->scsi_mpath_head; > + mpath_head = &scsi_mpath_head->mpath_head; > + > + mutex_lock(&sd_mpath_disks_lock); > + sd_mpath_disk->disk_count--; > + if (!sd_mpath_disk->disk_count) { > + list_del_init(&sd_mpath_disk->entry); > + remove = true; > + } > + mutex_unlock(&sd_mpath_disks_lock); > + mpath_device->disk = NULL; > + > + if (remove) { > + device_del(&sd_mpath_disk->dev); > + mpath_remove_disk(mpath_head); > + } > + sd_mpath_put_disk(sd_mpath_disk); > +} [Severity: Medium] Can sd_mpath_fail_probe() free scsi_mpath_head while mpath_head->remove_work is still pending? When delayed_removal_secs > 0 and the last path is removed in sd_mpath_remove(), mpath_can_remove_head() schedules mpath_head->remove_work and leaves sd_mpath_disk on sd_mpath_disks_list with disk_count == 0. If a new path starts probing before remove_work fires, sd_mpath_probe() increments disk_count to 1, but remove_work is not cancelled until mpath_add_device() in sd_mpath_add_disk(). If sd_probe() fails earlier and calls sd_mpath_fail_probe(), sd_mpath_disk is torn down without cancelling remove_work: CPU0 kworker sd_mpath_remove() (last path) mpath_can_remove_head() mod_delayed_work(..., &mpath_head->remove_work, ...) sd_probe() (new path) sd_mpath_probe() sd_mpath_disk->disk_count++ device_add_disk() -> fails sd_mpath_fail_probe() sd_mpath_put_disk() scsi_mpath_put_head() scsi_mpath_remove_device() scsi_mpath_put_head() kfree(scsi_mpath_head) mpath_remove_head_work() sd_mpath_remove_head() <-- use-after-free [ ... ] > @@ -4348,6 +4728,9 @@ static void sd_remove(struct scsi_device *sdp) > if (scsi_autopm_get_device(sdkp->device) < 0) > blk_mark_disk_dead(sdkp->disk); > > + if (sdp->scsi_mpath_dev) > + sd_mpath_remove(sdkp); > + > device_del(&sdkp->disk_dev); > del_gendisk(sdkp->disk); > if (!sdkp->suspended) [Severity: High] Can calling sd_mpath_remove() before del_gendisk(sdkp->disk) cause a NULL pointer dereference or use-after-free while REQ_MPATH requests are still queued on sdkp->disk->queue? Cloned multipath bios submitted to sdkp->disk->part0 remain queued or in flight on sdkp->disk->queue until del_gendisk(sdkp->disk) freezes and drains that queue. When the last path is removed, sd_mpath_remove() drops the last reference to sd_mpath_disk->dev, and sd_mpath_disk_release() calls mpath_put_disk(mpath_head) (setting mpath_head->disk = NULL) and frees sd_mpath_disk before del_gendisk(sdkp->disk) runs: CPU0 CPU1 sd_remove() sd_mpath_remove(sdkp) mpath_remove_disk(mpath_head) sd_mpath_put_disk(sd_mpath_disk) sd_mpath_disk_release() mpath_put_disk(mpath_head) mpath_head->disk = NULL; kfree(sd_mpath_disk) sd_init_command() scsi_mpath_start_request(req) disk = mpath_head->disk; blk_queue_io_stat(disk->queue) <-- NULL pointer dereference del_gendisk(sdkp->disk) Completing REQ_MPATH requests in scsi_mpath_end_request() or scsi_mpath_clone_end_io() also access the released mpath_head->disk or requeue work after mpath_put_disk() has already flushed it. Furthermore, because __scsi_remove_device() still holds sdev->scsi_mpath_dev's reference on scsi_mpath_head until after sd_remove() returns, scsi_mpath_put_head() in sd_mpath_disk_release() does not wait in mpath_head_uninit() when the multipath disk is still open, freeing sd_mpath_disk while mpath_head->parent still points to &sd_mpath_disk->dev (used by dev_warn_ratelimited() in mpath_bdev_submit_bio()). -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=26
