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

[Jinpu Wang]

Hi Kuai,

My apologies, missed that. I'll follow up on your previous comments.

Thx!

On Sat, Oct 24, 2026 at 8:05 AM yu kuai <yukuai@xxxxxxx> wrote:
>
> Hi,
>
> 在 2026/10/14 19:53, Jack Wang 写道:
> > From: Jack Wang <jinpu.wang@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxx/>
>
>
> >
> > 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