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

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

[Jack Wang]

From: Jack Wang <jinpu.wang@cloud.ionos.com>

Writing "clear", "readonly", "inactive" or "read_auto" to array_state
calls mddev_set_closing_and_sync_blockdev(), which sets MD_CLOSING so the
array cannot be reopened while it is being torn down. The flag is only
cleared again at the tail of array_state_store() (for readonly, read_auto,
inactive, or the failed-clear case).

Between setting the flag and reaching that tail there is an early return:

	err = mddev_lock(mddev);
	if (err)
		return err;

mddev_lock() is mutex_lock_interruptible() on reconfig_mutex. If the
writing task is signalled while blocked there - easy to hit when the mutex
is held for a long time by a running resync/recovery or reshape - it
returns -EINTR and array_state_store() returns with MD_CLOSING still set.
do_md_stop()/md_set_readonly() never ran, so the array keeps working
internally, but every subsequent md_open() now returns -ENODEV for all
callers: the device still shows up in /proc/mdstat and sysfs is fully
populated, yet it cannot be opened and cannot be recovered without a
reboot (the remaining clear paths either need the device open or re-enter
test_and_set_bit(MD_CLOSING) and bail with -EBUSY before the clear).

Route the interrupted-lock exit through a common label so the flag is
released. Guard the clear with a set_closing flag that records whether
this write actually set MD_CLOSING: when mddev->pers is already NULL (for
example a concurrent STOP_ARRAY is mid-teardown) this write skips the
mddev_set_closing_and_sync_blockdev() call above, so it must not clear a
MD_CLOSING that the other thread owns - doing so could let the array be
reopened while it is being destroyed. This also tightens the pre-existing
tail clear, which had the same unconditional behaviour.

mddev_unlock() is correctly skipped on the goto path since the lock was
never acquired.

Fixes: 99b902ac1725 ("md: sync blockdev before stopping raid or setting readonly")
Assisted-by: Claude:claude-opus-4-8 [Claude Code]
Signed-off-by: Jack Wang <jinpu.wang@cloud.ionos.com>
---
 drivers/md/md.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index d1465bcd86c8..7b7a4d925907 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -4649,6 +4649,7 @@ array_state_store(struct mddev *mddev, const char *buf, size_t len)
 {
 	int err = 0;
 	enum array_state st = match_word(buf, array_states);
+	bool set_closing = false;
 
 	/* No lock dependent actions */
 	switch (st) {
@@ -4668,6 +4669,7 @@ array_state_store(struct mddev *mddev, const char *buf, size_t len)
 		err = mddev_set_closing_and_sync_blockdev(mddev, 0);
 		if (err)
 			return err;
+		set_closing = true;
 		break;
 	default:
 		break;
@@ -4696,7 +4698,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;
 
 	switch (st) {
 	case inactive:
@@ -4769,8 +4771,17 @@ array_state_store(struct mddev *mddev, const char *buf, size_t len)
 	}
 	mddev_unlock(mddev);
 
-	if (st == readonly || st == read_auto || st == inactive ||
-	    (err && st == clear))
+out_clear_closing:
+	/*
+	 * Only clear MD_CLOSING if this write actually set it. Otherwise a
+	 * concurrent teardown (e.g. STOP_ARRAY) may own the flag - this write
+	 * would have skipped setting it above when mddev->pers was already
+	 * NULL - and clearing it here would let the array be reopened while it
+	 * is being destroyed.
+	 */
+	if (set_closing &&
+	    (st == readonly || st == read_auto || st == inactive ||
+	     (err && st == clear)))
 		clear_bit(MD_CLOSING, &mddev->flags);
 
 	return err ?: len;
-- 
2.43.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Calling `restart_array()` with the `mddev->lock` spinlock held can sleep because it clears the disk read-only flag, which triggers a `GFP_KERNEL` memory allocation for a uevent.
--

--- Patch [1]: [PATCHv2] md: clear MD_CLOSING when array_state_store() bails on interrupted lock ---
commit 3ae9b81fbeee45b412994652c2c3b3ac1b35d177
Author: Jack Wang <jinpu.wang@cloud.ionos.com>

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

This commit ensures that MD_CLOSING is correctly cleared if
mutex_lock_interruptible() fails while writing to array_state. It introduces
a common exit label and a boolean flag to track whether the flag was actually
set by the current thread.

> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index d1465bcd86c81..7b7a4d9259079 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
[ ... ]
> @@ -4696,7 +4698,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]
This is a pre-existing issue, but just above this code in array_state_store(),
there is a fast path that can sleep while holding a spinlock.

If a privileged user sets the md device to read-only using
blockdev --setro /dev/md0 and then writes 'active' to
/sys/block/md0/md/array_state, the code acquires the mddev->lock spinlock
and calls restart_array():

drivers/md/md.c:array_state_store() {
    ...
    spin_lock(&mddev->lock);
    if (st == active) {
        restart_array(mddev);
    ...
}

restart_array() unconditionally calls set_disk_ro() to ensure the disk is
read-write:

drivers/md/md.c:restart_array() {
    ...
    set_disk_ro(disk, 0);
    ...
}

Because the disk was set to read-only, set_disk_ro() clears the flag and
triggers a uevent:

block/genhd.c:set_disk_ro() {
    ...
    set_disk_ro_uevent(disk, read_only);
    ...
}

This calls into kobject_uevent_env(), which performs an unconditional
GFP_KERNEL allocation:

lib/kobject_uevent.c:kobject_uevent_env() {
    ...
    devpath = kobject_get_path(kobj, GFP_KERNEL);
    ...
}

Can this sequence cause a sleep-in-atomic bug and potentially lead to a
deadlock or panic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717051233.383373-1-jinpu.wang@ionos.com?part=1