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

[PATCH v2] 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 to
keep the array from being reopened while it is 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 catches a signal while blocked there, it returns -EINTR and
MD_CLOSING is left set on a still-running array. 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 shows up in /proc/mdstat and sysfs is fully populated, yet cannot
be opened, and cannot be recovered without a reboot (all clear paths
either need the device open or re-enter test_and_set_bit(MD_CLOSING) and
bail with -EBUSY before the clear).

The window is easy to hit on a busy host: reconfig_mutex is held for long
stretches by a running resync/recovery, widening the interruptible wait,
and a userspace helper writing to array_state and then being killed or
timed out is enough to leak the flag.

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>
---
v2: resend, rebased on md-7.4-20261013 (md/md-7.4), no code changes.
 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 03e35386a802..f6dfd5d76199 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -4671,6 +4671,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) {
@@ -4691,6 +4692,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;
@@ -4719,7 +4721,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:
@@ -4792,8 +4794,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: [PATCH v2] md: clear MD_CLOSING when array_state_store() bails on interrupted lock

[yu kuai]

Hi,

在 2026/10/14 19:53, 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 to
> keep the array from being reopened while it is 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 catches a signal while blocked there, it returns -EINTR and
> MD_CLOSING is left set on a still-running array. 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 shows up in /proc/mdstat and sysfs is fully populated, yet cannot
> be opened, and cannot be recovered without a reboot (all clear paths
> either need the device open or re-enter test_and_set_bit(MD_CLOSING) and
> bail with -EBUSY before the clear).
>
> The window is easy to hit on a busy host: reconfig_mutex is held for long
> stretches by a running resync/recovery, widening the interruptible wait,
> and a userspace helper writing to array_state and then being killed or
> timed out is enough to leak the flag.
>
> 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>
> ---
> v2: resend, rebased on md-7.4-20261013 (md/md-7.4), no code changes.
>   drivers/md/md.c | 17 ++++++++++++++---
>   1 file changed, 14 insertions(+), 3 deletions(-)

Looks like this is just a resend, while I already replied to previous v2 
patch:

Re: [PATCHv2] md: clear MD_CLOSING when array_state_store() bails on 
interrupted lock - yu kuai <https://lore.kernel.org/all/eded31de-2e92-4a3b-9384-03dbebc5e9e7@fygo.io/>


>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 03e35386a802..f6dfd5d76199 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -4671,6 +4671,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) {
> @@ -4691,6 +4692,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;
> @@ -4719,7 +4721,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:
> @@ -4792,8 +4794,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;

-- 
Thanks,
Kuai