Re: [PATCH] md: clear MD_CLOSING when array_state_store() bails on interrupted lock

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unconditional clearing of MD_CLOSING on the error path can clear a flag owned by a concurrent thread, exposing the array to use-after-free races during teardown.
--

--- Patch [1]: [PATCH] md: clear MD_CLOSING when array_state_store() bails on interrupted lock ---
commit 919b922dd46da90c5c7e1a2b46106db8f5e8ffbf
Author: Jack Wang <[email protected]>

md: clear MD_CLOSING when array_state_store() bails on interrupted lock

This commit addresses a bug where an interrupted lock in array_state_store()
leaves MD_CLOSING set, preventing the array from being reopened. It modifies
the error path to jump to the existing cleanup logic to clear the flag.

> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index d1465bcd86c81..c4de2053593a0 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -4696,7 +4696,7 @@ array_state_store(struct mddev *mddev, const char *buf, size_t len)
>  	}
>  	err = mddev_lock(mddev);
>  	if (err)
> -		return err;
> +		goto out_clear_closing;

[Severity: High]
Does this unconditionally clear MD_CLOSING even if this thread didn't set it?

If Thread A initiates array teardown (e.g., via md_ioctl(STOP_ARRAY)), it sets
MD_CLOSING, takes reconfig_mutex, and sets mddev->pers = NULL.

If Thread B concurrently writes "inactive" to array_state, it skips setting
MD_CLOSING because mddev->pers is NULL:

drivers/md/md.c:array_state_store() {
...
	case inactive:
...
		if (!mddev->pers || !md_is_rdwr(mddev))
			break;
		err = mddev_set_closing_and_sync_blockdev(mddev, 0);
...
}

Thread B then calls mddev_lock(), which blocks because Thread A holds the
reconfig_mutex. If Thread B receives a signal, mddev_lock() returns -EINTR,
and we jump to out_clear_closing:

>  
>  	switch (st) {
>  	case inactive:

[ ... ]

> @@ -4769,6 +4769,7 @@ array_state_store(struct mddev *mddev, const char *buf, size_t len)
>  	}
>  	mddev_unlock(mddev);
>  
> +out_clear_closing:
>  	if (st == readonly || st == read_auto || st == inactive ||
>  	    (err && st == clear))
>  		clear_bit(MD_CLOSING, &mddev->flags);

Because st == inactive, Thread B blindly clears MD_CLOSING, even though it
never set it.

Could this allow a third thread to successfully open the device (since
md_open() only checks MD_CLOSING) while Thread A is actively destroying the
array data structures, leading to a use-after-free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1