Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] md: use-after-free on md_misc_wq in md_seq_show() during md_exit() - [High] md: use-after-free on mdstat_entry in md_new_event() during md_exit() --
commit d5381c9dd419c5377924fd70f1baf55ca30c4844 Author: Christian Brauner <[email protected]> md: let procfs own the wait queue of /proc/mdstat This commit replaces the module-owned md_event_waiters wait queue with proc_poll_notify() on the /proc/mdstat proc_dir_entry, removing the md_unloading flag and wait loop on module exit. It also moves the removal of /proc/mdstat in md_exit() to after destroy_workqueue(md_misc_wq) so that late md_new_event() calls do not dereference a removed proc entry. > diff --git a/drivers/md/md.c b/drivers/md/md.c > index 64abf1743e41d..7560dcf0f5c87 100644 > --- a/drivers/md/md.c > +++ b/drivers/md/md.c [ ... ] > @@ -11074,25 +11069,12 @@ void md_autostart_arrays(int part) > static __exit void md_exit(void) > { > struct mddev *mddev; > - int delay = 1; > > unregister_blkdev(MD_MAJOR,"md"); > unregister_blkdev(mdp_major, "mdp"); > unregister_reboot_notifier(&md_notifier); > unregister_sysctl_table(raid_table_header); > > - /* We cannot unload the modules while some process is > - * waiting for us in select() or poll() - wake them up > - */ > - md_unloading = 1; > - while (waitqueue_active(&md_event_waiters)) { > - /* not safe to leave yet */ > - wake_up(&md_event_waiters); > - msleep(delay); > - delay += delay; > - } > - remove_proc_entry("mdstat", NULL); [Severity: High] If remove_proc_entry("mdstat", NULL) is moved after the all_mddevs teardown loop and destroy_workqueue(md_misc_wq), can a concurrent reader of /proc/mdstat trigger a use-after-free on md_misc_wq and leak the mddev? Previously, calling remove_proc_entry("mdstat", NULL) before iterating over all_mddevs waited in proc_entry_rundown() for all active seq_file readers in md_seq_show() to complete before mddev teardown started. Reading /proc/mdstat pins the proc_dir_entry rather than holding a module reference on md_mod. In drivers/md/md.c:md_seq_show(), a reader takes a reference on mddev->active and drops all_mddevs_lock while formatting output: drivers/md/md.c:md_seq_show() { ... mddev = list_entry(v, struct mddev, all_mddevs); if (!mddev_get(mddev)) return 0; spin_unlock(&all_mddevs_lock); ... spin_lock(&all_mddevs_lock); mddev_put_locked(mddev); return 0; } If md_exit() runs while md_seq_show() is between mddev_get() and mddev_put_locked() on an inactive array: 1. md_exit() calls mddev_get(mddev) (incrementing mddev->active from 1 to 2), drops all_mddevs_lock, calls export_array(mddev) (which also mutates mddev->disks, mddev->raid_disks, and mddev->major_version without holding mddev->lock while md_seq_show() reads them), clears mddev->ctime = 0 and mddev->hold_active = 0, and calls mddev_put_locked(mddev). 2. Because md_seq_show() still holds its reference, atomic_dec_and_test(&mddev->active) in md_exit() drops mddev->active from 2 to 1 and returns false, so __mddev_put() is not called and mddev->del_work is not queued on md_misc_wq. 3. md_exit() then calls destroy_workqueue(md_misc_wq) and blocks in remove_proc_entry("mdstat", NULL) waiting for md_seq_show() to finish. 4. When md_seq_show() finishes and calls mddev_put_locked(mddev), mddev->active drops from 1 to 0 and __mddev_put() queues mddev->del_work onto the already-destroyed md_misc_wq workqueue: drivers/md/md.c:__mddev_put() { ... set_bit(MD_DELETED, &mddev->flags); queue_work(md_misc_wq, &mddev->del_work); } > @@ -11112,6 +11094,11 @@ static __exit void md_exit(void) > spin_unlock(&all_mddevs_lock); > > destroy_workqueue(md_misc_wq); > + /* > + * Only now can nothing reach md_new_event() anymore, which wakes the > + * pollers of /proc/mdstat through its entry. > + */ > + remove_proc_entry("mdstat", NULL); [Severity: High] Does destroy_workqueue(md_misc_wq) guarantee that an in-flight sysfs store on an inactive array has finished before remove_proc_entry("mdstat", NULL) frees mdstat_entry? In drivers/md/md.c:md_attr_store(), the caller holds a reference on mddev->active across entry->store() (where md_new_event() is called) and only drops it afterward: drivers/md/md.c:md_attr_store() { ... spin_lock(&all_mddevs_lock); if (!mddev_get(mddev)) { spin_unlock(&all_mddevs_lock); if (kn) sysfs_unbreak_active_protection(kn); return -EBUSY; } spin_unlock(&all_mddevs_lock); rv = entry->store(mddev, page, length); if (kn) sysfs_unbreak_active_protection(kn); mddev_put(mddev); return rv; } If md_attr_store() acquires mddev->active before md_exit() reaches mddev_put_locked(mddev) (for example, while md_exit() drops all_mddevs_lock around export_array(mddev)), md_exit()'s call to mddev_put_locked(mddev) only decrements mddev->active from 2 to 1 and does not queue mddev->del_work onto md_misc_wq. Similarly, if the inactive array had bound rdevs, export_array() moves them to mddev->deleting via md_kick_rdev_from_array() without draining mddev->deleting or dropping rdev->kobj, which holds a parent reference on mddev->kobj; if rdev_attr_store() is in flight, kobject_put(&mddev->kobj) in mddev_delayed_delete() returns without waiting for rdev_attr_store() to finish. In either case, destroy_workqueue(md_misc_wq) returns while the sysfs store is still running, remove_proc_entry("mdstat", NULL) frees mdstat_entry, and the in-flight store can then call md_new_event()->proc_poll_notify(mdstat_entry) on the freed mdstat_entry (followed by md_attr_store() calling mddev_put(mddev) and queuing mddev->del_work onto the destroyed md_misc_wq). -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
