Re: [PATCH RESEND v2 07/26] md: let procfs own the wait queue of /proc/mdstat

[sashiko-bot@kernel.org]

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 <brauner@kernel.org>

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/20261104-work-procfs-drivers-v2-0-245e54e87116@kernel.org?part=7