Re: [PATCH v2] md: remove legacy async del_gendisk

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] md: Use-After-Free in md_notify_reboot and md_exit due to unsafe list_for_each_entry_safe
- [High] md: Use-After-Free in md_seq_show when mddev_get fails
- [High] md: sysfs self-deadlock in md_attr_store due to synchronous kobject_del
--

commit 882f6ccb12f02b986de958463cf3afb9ffc46b20
Author: Li Youhong <liyouhong@kylinos.cn>

md: remove legacy async del_gendisk

This commit removes the legacy async del_gendisk path to resolve a lockdep
cycle between major_names_lock, md_misc_wq, and reconfig_mutex. Deletions
are now done synchronously in mddev_unlock() and mddev_put().

Closes: https://syzkaller.appspot.com/bug?extid=68e1f51046d68329940f

> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 680b34a63cb3d..5d9e76f3256be 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
[ ... ]
>  void mddev_put(struct mddev *mddev)
>  {
> +	bool complete_delete;
> +
>  	if (!atomic_dec_and_lock(&mddev->active, &all_mddevs_lock))
>  		return;
>  
> -	__mddev_put(mddev);
> +	complete_delete = __mddev_put(mddev);
>  	spin_unlock(&all_mddevs_lock);
> +	if (complete_delete)
> +		mddev_complete_delete(mddev);
>  }

[Severity: High]
Could synchronous kobject_del() on the active path in drivers/md/md.c:mddev_put()
cause a self-deadlock when triggered from a sysfs handler like md_attr_store()?

Writing 'stop' to /sys/block/mdX/md/array_state executes md_attr_store() under
kernfs active protection. The handler calls do_md_stop(), marking the array as
deleted, and then calls mddev_put().

Since mddev_put() now synchronously invokes mddev_complete_delete() ->
kobject_del(), kobject_del() will call kernfs_remove() and block in
kernfs_drain() waiting for the active protection to drop. The calling thread
is holding the active protection itself, causing a deadlock regression.

[ ... ]
> @@ -9041,8 +9042,13 @@ static int md_seq_show(struct seq_file *seq, void *v)
>  	}
>  
>  	mddev = list_entry(v, struct mddev, all_mddevs);
> -	if (!mddev_get(mddev))
> +	if (!mddev_get(mddev)) {
> +		md_seq_flush_delete(seq);
>  		return 0;
> +	}

[Severity: High]
Can this sequence in drivers/md/md.c:md_seq_show() lead to a use-after-free
regression when mddev_get() fails?

If mddev_get() fails (because MD_DELETED is set), the current iterator v is
unpinned. The code then calls md_seq_flush_delete(seq), which temporarily
drops all_mddevs_lock to perform delayed deletion of a previous node.

During this unlocked window, the unpinned current node v could be fully
deleted and freed by a concurrent thread. When md_seq_flush_delete() reacquires
the lock and md_seq_show() returns 0, seq_read() will call md_seq_next(),
which dereferences the now-freed v pointer.

[ ... ]
> @@ -10714,10 +10721,11 @@ EXPORT_SYMBOL_GPL(rdev_clear_badblocks);
>  static int md_notify_reboot(struct notifier_block *this,
>  			    unsigned long code, void *x)
>  {
> -	struct mddev *mddev;
> +	struct mddev *mddev, *tmp;
> +	bool complete_delete;
>  
>  	spin_lock(&all_mddevs_lock);
> -	list_for_each_entry(mddev, &all_mddevs, all_mddevs) {
> +	list_for_each_entry_safe(mddev, tmp, &all_mddevs, all_mddevs) {
>  		if (!mddev_get(mddev))
>  			continue;
>  		spin_unlock(&all_mddevs_lock);

[Severity: High]
Does dropping all_mddevs_lock inside list_for_each_entry_safe() in
drivers/md/md.c:md_notify_reboot() expose tmp to concurrent deletion?

Since list_for_each_entry_safe() caches the next pointer into tmp before
executing the loop body, tmp is unpinned and unprotected during the unlocked
window. Because mddev_put() now performs synchronous deletion, a concurrent
array teardown could drop the last reference to tmp and free its memory. When
the lock is reacquired, the loop advances by reading tmp->all_mddevs.next,
which dereferences freed memory.

[ ... ]
> @@ -11075,20 +11088,15 @@ static __exit void md_exit(void)
>  	remove_proc_entry("mdstat", NULL);
>  
>  	spin_lock(&all_mddevs_lock);
> -	list_for_each_entry(mddev, &all_mddevs, all_mddevs) {
> +	list_for_each_entry_safe(mddev, tmp, &all_mddevs, all_mddevs) {
>  		if (!mddev_get(mddev))
>  			continue;
>  		spin_unlock(&all_mddevs_lock);

[Severity: High]
Similarly, does dropping the lock here in drivers/md/md.c:md_exit() while
holding an unpinned tmp risk list corruption on module exit?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260922021533.2935172-1-dayou5941@163.com?part=1