Thank you for your contribution! Sashiko AI review found 6 potential issues to consider: - [High] libmultipath: race between mpath_add_device() and mpath_can_remove_head() - [High] libmultipath: UAF from unsynchronized remove_work on head teardown - [Medium] libmultipath: drv_module ref leak in mpath_can_remove_head() - [Medium] libmultipath: missing requeue kick in mpath_delayed_removal_secs_store() - [Medium] libmultipath: remove_work initialized in mpath_alloc_head_disk() instead of mpath_head_init() - [Low] libmultipath: unchecked HZ overflow in mpath_delayed_removal_secs_store() --
commit ac55fad5ec89b5f2112b89a2a217ff2d12ec50d8 Author: John Garry <[email protected]> libmultipath: Add delayed removal support This commit adds delayed head removal support to libmultipath, keeping the multipath disk present and requeuing I/O for a configurable duration when all paths are lost. > diff --git a/lib/multipath.c b/lib/multipath.c > index fd78218244472..cdf8fbc5105b3 100644 > --- a/lib/multipath.c > +++ b/lib/multipath.c > @@ -67,6 +67,9 @@ int mpath_add_device(struct mpath_device *mpath_device, > struct gendisk *disk, > list_add_tail_rcu(&mpath_device->siblings, &mpath_head->dev_list); > mutex_unlock(&mpath_head->lock); > > + if (cancel_delayed_work(&mpath_head->remove_work)) > + module_put(mpath_head->drv_module); > + > return 0; > } > EXPORT_SYMBOL_GPL(mpath_add_device); [ ... ] > @@ -482,6 +494,39 @@ static void mpath_requeue_work(struct work_struct *work) > } > } > > +bool mpath_can_remove_head(struct mpath_head *mpath_head) > +{ > + unsigned long delay; > + bool remove = false; > + > + mutex_lock(&mpath_head->lock); > + /* > + * Ensure that no one could remove this module while the head > + * remove work is pending. > + */ > + if (mpath_head_queue_if_no_path(mpath_head) && > + !check_mul_overflow(mpath_head->delayed_removal_secs, HZ, &delay) && > + try_module_get(mpath_head->drv_module)) { > + mod_delayed_work(mpath_wq, &mpath_head->remove_work, delay); [Severity: Medium] Does this leak a reference on mpath_head->drv_module if mpath_head->remove_work is already pending? try_module_get() increments the module refcount before mod_delayed_work() is called. When remove_work is already pending, mod_delayed_work() updates the existing timer in place and returns true, but neither mpath_remove_head_work() nor cancel_delayed_work() in mpath_add_device() will run more than once to drop the extra reference, preventing the driver module from unloading. > + } else { > + remove = true; > + } > + > + mutex_unlock(&mpath_head->lock); > + return remove; > +} > +EXPORT_SYMBOL_GPL(mpath_can_remove_head); [Severity: High] Can a race between mpath_add_device() and mpath_can_remove_head() remove an active mpath_head while paths are present on dev_list, or cancel remove_work after dev_list becomes empty? mpath_delete_device() drops mpath_head->lock before calling mpath_synchronize() and before the caller invokes mpath_can_remove_head(), and neither mpath_can_remove_head() nor mpath_remove_head_work() rechecks list_empty(&mpath_head->dev_list) under mpath_head->lock: CPU0 CPU1 mpath_delete_device() mutex_lock(&mpath_head->lock) list_del_rcu(&dev1->siblings) last_path = true mutex_unlock(&mpath_head->lock) mpath_add_device() mutex_lock(&mpath_head->lock) list_add_tail_rcu(&dev2->siblings, &mpath_head->dev_list) mutex_unlock(&mpath_head->lock) cancel_delayed_work(&remove_work) mpath_synchronize() mpath_can_remove_head() mutex_lock(&mpath_head->lock) remove = true (or queues remove_work) mutex_unlock(&mpath_head->lock) --> removes head with dev2 on dev_list Conversely, because mpath_add_device() calls cancel_delayed_work() after releasing mpath_head->lock, if dev2 is deleted on another CPU and mpath_can_remove_head() queues remove_work before CPU1 runs cancel_delayed_work(), CPU1 cancels the newly armed remove_work even though dev_list is now empty. > + > +static void mpath_remove_head_work(struct work_struct *work) > +{ > + struct mpath_head *mpath_head = container_of(to_delayed_work(work), > + struct mpath_head, remove_work); > + struct module *drv_module = mpath_head->drv_module; > + > + mpath_head->mpdt->remove_head(mpath_head); > + module_put(drv_module); > +} [Severity: High] Can mpath_remove_head_work() cause a use-after-free on mpath_head during head teardown or path flapping? Unlike requeue_work and partition_scan_work, neither mpath_put_disk() nor mpath_head_uninit() cancels or flushes mpath_head->remove_work, and mpath_add_device() uses non-synchronous cancel_delayed_work(): CPU0 (mpath_wq) CPU1 mpath_remove_head_work() [work dequeued; PENDING cleared] mpath_add_device() cancel_delayed_work(&remove_work) --> returns false mpath_delete_device() mpath_can_remove_head() --> returns true (or re-arms timer) remove_head() frees mpath_head mpath_head->mpdt->remove_head() <-- use-after-free Similarly, if remove_work is armed and a subsequent path probe fails and tears down the head before reaching mpath_add_device(), mpath_head can be freed while remove_work is still pending in the timer wheel. > void mpath_remove_disk(struct mpath_head *mpath_head) > { > if (test_and_clear_bit(MPATH_HEAD_DISK_LIVE, &mpath_head->flags)) { > @@ -529,6 +574,9 @@ int mpath_alloc_head_disk(struct mpath_head *mpath_head, > mpath_head->disk->private_data = mpath_head; > mpath_head->disk->fops = &mpath_ops; > > + INIT_DELAYED_WORK(&mpath_head->remove_work, mpath_remove_head_work); > + mpath_head->delayed_removal_secs = 0; > + > set_bit(GD_SUPPRESS_PART_SCAN, &mpath_head->disk->state); > > return 0; [Severity: Medium] Should remove_work and delayed_removal_secs be initialized in mpath_head_init() instead of mpath_alloc_head_disk()? If mpath_alloc_head_disk() is called again on an existing mpath_head after mpath_put_disk(), resetting delayed_removal_secs to 0 here without clearing MPATH_HEAD_QUEUE_IF_NO_PATH leaves the flag out of sync with delayed_removal_secs. mpath_available_path() then continues returning true when no paths exist, causing mpath_bdev_submit_bio() to requeue I/O indefinitely even though delayed_removal_secs is 0. Initializing remove_work in mpath_head_init() alongside requeue_work and partition_scan_work also ensures remove_work is initialized before mpath_add_device() can touch it. [ ... ] > +ssize_t mpath_delayed_removal_secs_store(struct mpath_head *mpath_head, > + const char *buf, size_t count) > +{ > + unsigned int sec; > + ssize_t ret; > + > + ret = kstrtouint(buf, 0, &sec); > + if (ret < 0) > + return ret; > + > + mutex_lock(&mpath_head->lock); > + mpath_head->delayed_removal_secs = sec; > + if (sec) > + set_bit(MPATH_HEAD_QUEUE_IF_NO_PATH, &mpath_head->flags); > + else > + clear_bit(MPATH_HEAD_QUEUE_IF_NO_PATH, &mpath_head->flags); > + mutex_unlock(&mpath_head->lock); [Severity: Low] On 32-bit architectures, can storing a large sec value set MPATH_HEAD_QUEUE_IF_NO_PATH while mpath_can_remove_head() immediately removes the head? mpath_delayed_removal_secs_store() accepts any unsigned int and sets MPATH_HEAD_QUEUE_IF_NO_PATH without checking whether sec * HZ overflows unsigned long. When sec > ULONG_MAX / HZ on 32-bit systems, I/O is requeued on path loss, but check_mul_overflow() in mpath_can_remove_head() fails and falls back to immediate head removal with zero delay. > + > + /* > + * Ensure that update to MPATH_HEAD_QUEUE_IF_NO_PATH is seen > + * by its reader. > + */ > + mpath_synchronize(mpath_head); > + > + return count; > +} [Severity: Medium] When delayed_removal_secs is set to 0, does this leave previously queued BIOs stalled on mpath_head->requeue_list? Clearing MPATH_HEAD_QUEUE_IF_NO_PATH stops new BIOs from being queued when no paths are available, but without calling mpath_schedule_requeue_work(mpath_head) after mpath_synchronize(), BIOs that were already added to requeue_list while MPATH_HEAD_QUEUE_IF_NO_PATH was set are not kicked to fail. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
