[PATCH v2] md: recheck spare changes before starting sync

[PATCH v2] md: recheck spare changes before starting sync

[Abd-Alrhman Masalkhi]

remove_spares() and remove_and_add_spares() modify the array's rdev
configuration. These operations are only safe after the array has been
suspended.

md_start_sync() checks whether spare configuration changes are needed
before taking reconfig_mutex. However, the rdev state can change before
the mutex is acquired, so the initial check can become stale. In that
case, md_choose_sync_action() may remove or replace rdevs while normal
I/O is still accessing them.

The race can occur as follows:

raid10d          Worker                      Normal IO
____________     _______________________     ______________________

                                             raid10_write_request()
                                             wait_blocked_dev()
set Blocked
set Faulty
                                             Skip Faulty rdev
                                             rrdev->nr_pending++
                                             .repl_bio = bio
                 removeable_rdev = false     .
                 array not suspended         .
lock mddev                                   goto err_handle
                 lock mddev (wait)
                 .
update sb        .
clear Blocked    .
                 .
unlock mddev     .
                 lock mddev (acquires)
                 remove_spares()
                 removeable_rdev = true

                 raid10_remove_disk()
                 rdev = replacement
                 replacement = NULL
                                             rdev_dec_pending(NULL)
                 unlock mddev                (NULL)->nr_pending--

In this case, rdev_dec_pending() is called with a NULL pointer,
resulting in a NULL pointer dereference when attempting to decrement
nr_pending.

Fix this by suspending the array when spare configuration changes are
needed, including for non-read-write arrays, and checking again after
taking reconfig_mutex. If the array was not already suspended and a
change is now needed, release the mutex, suspend the array, and
reacquire the mutex before continuing.

Fixes: bc08041b32ab ("md: suspend array in md_start_sync() if array need reconfiguration")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260628142420.1051027-1-abd.masalkhi@gmail.com?part=3
Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
---
Changes in v2:
 - Recheck whether spare configuration changes are needed after taking
   reconfig_mutex, and suspend the array before continuing if necessary.
 - Account for read only arrays, where reshape_position may not be
   MaxSector even though md_start_sync() can still modify the spare
   configuration.
 - Link to v1: https://lore.kernel.org/linux-raid/20260630075640.1081634-1-abd.masalkhi@gmail.com/
---
 drivers/md/md.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 66a41d482e59..c3fc47a82415 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -10174,13 +10174,25 @@ static void md_start_sync(struct work_struct *ws)
 	 * If reshape is still in progress, spares won't be added or removed
 	 * from conf until reshape is done.
 	 */
-	if (mddev->reshape_position == MaxSector &&
+	if ((mddev->reshape_position == MaxSector || !md_is_rdwr(mddev)) &&
 	    md_spares_need_change(mddev)) {
 		suspend = true;
 		mddev_suspend(mddev, false);
 	}
 
 	mddev_lock_nointr(mddev);
+
+	/*
+	 * The spare configuration can change before reconfig_mutex is acquired.
+	 * Recheck while holding the lock and suspend if needed.
+	 */
+	if (!suspend && (mddev->reshape_position == MaxSector || !md_is_rdwr(mddev)) &&
+	    md_spares_need_change(mddev)) {
+		mddev_unlock(mddev);
+		mddev_suspend_and_lock_nointr(mddev);
+		suspend = true;
+	}
+
 	if (!md_is_rdwr(mddev)) {
 		/*
 		 * On a read-only array we can:
-- 
2.43.0

Re: [PATCH v2] md: recheck spare changes before starting sync

[sashiko-bot]

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

Pre-existing issues:
- [High] Incomplete fix for a TOCTOU race condition causing a NULL pointer dereference when modifying the spare configuration without suspending the array.
--

--- Patch [1]: [PATCH v2] md: recheck spare changes before starting sync ---
commit 0ddb3cd53c96b4255d51183f6d5da4fcd8b938bf
Author: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>

md: recheck spare changes before starting sync

This patch aims to fix a race condition where spare configurations are
modified without suspending the array. It adds a recheck for spare
configuration changes after acquiring the reconfig_mutex to avoid operating
on stale states.

Link: https://lore.kernel.org/linux-raid/20260630075640.1081634-1-abd.masalkhi@gmail.com/

> diff --git a/drivers/md/md.c b/drivers/md/md.c
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -10181,13 +10181,25 @@ static void md_start_sync(struct work_struct *ws)

[ ... ]

>  	mddev_lock_nointr(mddev);
> +
> +	/*
> +	 * The spare configuration can change before reconfig_mutex is acquired.
> +	 * Recheck while holding the lock and suspend if needed.
> +	 */
> +	if (!suspend && (mddev->reshape_position == MaxSector || !md_is_rdwr(mddev)) &&
> +	    md_spares_need_change(mddev)) {
> +		mddev_unlock(mddev);
> +		mddev_suspend_and_lock_nointr(mddev);
> +		suspend = true;
> +	}
> +

[Severity: High]
This is a pre-existing issue, but does this recheck completely close the race
window?

If a faulty drive has pending I/O, md_spares_need_change() will evaluate
rdev_removeable(), which checks atomic_read(&rdev->nr_pending). If that is
greater than zero, md_spares_need_change() returns false. This causes both
the initial check and this new recheck to bypass suspension.

If an asynchronous I/O completion happens immediately after this recheck,
nr_pending can decrement to 0. When md_start_sync() continues and eventually
calls remove_spares(), it will re-evaluate rdev_removeable(), find
nr_pending == 0, and remove the disk. This nullifies the disk pointer.

Because the array was never suspended, couldn't concurrent I/O re-read the
nullified pointer and crash? For example, in drivers/md/raid10.c:

err_handle() {
    ...
    struct md_rdev *rrdev = conf->mirrors[d].replacement;
    ...
    if (r10_bio->devs[k].repl_bio) {
        rdev_dec_pending(rrdev, mddev);
        r10_bio->devs[k].repl_bio = NULL;
    }
}

If rrdev is fetched as NULL due to the concurrent removal, rdev_dec_pending()
will dereference it.

Could remove_spares() be restricted to only remove drives if the array is
actually suspended, or should suspension be enforced earlier regardless of
nr_pending?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708112003.474537-1-abd.masalkhi@gmail.com?part=1

Re: [PATCH v2] md: recheck spare changes before starting sync

[Abd-Alrhman Masalkhi]

Hi Kuai,

On Wed, Jul 08, 2026 at 11:37 +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] Incomplete fix for a TOCTOU race condition causing a NULL pointer dereference when modifying the spare configuration without suspending the array.
> --
>
> --- Patch [1]: [PATCH v2] md: recheck spare changes before starting sync ---
> commit 0ddb3cd53c96b4255d51183f6d5da4fcd8b938bf
> Author: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
>
> md: recheck spare changes before starting sync
>
> This patch aims to fix a race condition where spare configurations are
> modified without suspending the array. It adds a recheck for spare
> configuration changes after acquiring the reconfig_mutex to avoid operating
> on stale states.
>
> Link: https://lore.kernel.org/linux-raid/20260630075640.1081634-1-abd.masalkhi@gmail.com/
>
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -10181,13 +10181,25 @@ static void md_start_sync(struct work_struct *ws)
>
> [ ... ]
>
>>  	mddev_lock_nointr(mddev);
>> +
>> +	/*
>> +	 * The spare configuration can change before reconfig_mutex is acquired.
>> +	 * Recheck while holding the lock and suspend if needed.
>> +	 */
>> +	if (!suspend && (mddev->reshape_position == MaxSector || !md_is_rdwr(mddev)) &&
>> +	    md_spares_need_change(mddev)) {
>> +		mddev_unlock(mddev);
>> +		mddev_suspend_and_lock_nointr(mddev);
>> +		suspend = true;
>> +	}
>> +
>
> [Severity: High]
> This is a pre-existing issue, but does this recheck completely close the race
> window?
>
> If a faulty drive has pending I/O, md_spares_need_change() will evaluate
> rdev_removeable(), which checks atomic_read(&rdev->nr_pending). If that is
> greater than zero, md_spares_need_change() returns false. This causes both
> the initial check and this new recheck to bypass suspension.
>
> If an asynchronous I/O completion happens immediately after this recheck,
> nr_pending can decrement to 0. When md_start_sync() continues and eventually
> calls remove_spares(), it will re-evaluate rdev_removeable(), find
> nr_pending == 0, and remove the disk. This nullifies the disk pointer.
>

Shouldn't the Blocked flag guard against this case? It is set before the
Faulty bit and before nr_pending is decremented, and it should only be
cleared while holding reconfig_mutex, or am I missing something?

> Because the array was never suspended, couldn't concurrent I/O re-read the
> nullified pointer and crash? For example, in drivers/md/raid10.c:
>
> err_handle() {
>     ...
>     struct md_rdev *rrdev = conf->mirrors[d].replacement;
>     ...
>     if (r10_bio->devs[k].repl_bio) {
>         rdev_dec_pending(rrdev, mddev);
>         r10_bio->devs[k].repl_bio = NULL;
>     }
> }
>
> If rrdev is fetched as NULL due to the concurrent removal, rdev_dec_pending()
> will dereference it.
>
> Could remove_spares() be restricted to only remove drives if the array is
> actually suspended, or should suspension be enforced earlier regardless of
> nr_pending?
>
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260708112003.474537-1-abd.masalkhi@gmail.com?part=1

-- 
Best Regards,
Abd-Alrhman

Re: [PATCH v2] md: recheck spare changes before starting sync

[yu kuai]

在 2026/7/8 19:20, Abd-Alrhman Masalkhi 写道:

> remove_spares() and remove_and_add_spares() modify the array's rdev
> configuration. These operations are only safe after the array has been
> suspended.
>
> md_start_sync() checks whether spare configuration changes are needed
> before taking reconfig_mutex. However, the rdev state can change before
> the mutex is acquired, so the initial check can become stale. In that
> case, md_choose_sync_action() may remove or replace rdevs while normal
> I/O is still accessing them.
>
> The race can occur as follows:
>
> raid10d          Worker                      Normal IO
> ____________     _______________________     ______________________
>
>                                               raid10_write_request()
>                                               wait_blocked_dev()
> set Blocked
> set Faulty
>                                               Skip Faulty rdev
>                                               rrdev->nr_pending++
>                                               .repl_bio = bio
>                   removeable_rdev = false     .
>                   array not suspended         .
> lock mddev                                   goto err_handle
>                   lock mddev (wait)
>                   .
> update sb        .
> clear Blocked    .
>                   .
> unlock mddev     .
>                   lock mddev (acquires)
>                   remove_spares()
>                   removeable_rdev = true
>
>                   raid10_remove_disk()
>                   rdev = replacement
>                   replacement = NULL
>                                               rdev_dec_pending(NULL)
>                   unlock mddev                (NULL)->nr_pending--
>
> In this case, rdev_dec_pending() is called with a NULL pointer,
> resulting in a NULL pointer dereference when attempting to decrement
> nr_pending.
>
> Fix this by suspending the array when spare configuration changes are
> needed, including for non-read-write arrays, and checking again after
> taking reconfig_mutex. If the array was not already suspended and a
> change is now needed, release the mutex, suspend the array, and
> reacquire the mutex before continuing.
>
> Fixes: bc08041b32ab ("md: suspend array in md_start_sync() if array need reconfiguration")
> Reported-by: sashiko-bot<sashiko-bot@kernel.org>
> Closes:https://sashiko.dev/#/patchset/20260628142420.1051027-1-abd.masalkhi@gmail.com?part=3
> Signed-off-by: Abd-Alrhman Masalkhi<abd.masalkhi@gmail.com>
> ---
> Changes in v2:
>   - Recheck whether spare configuration changes are needed after taking
>     reconfig_mutex, and suspend the array before continuing if necessary.
>   - Account for read only arrays, where reshape_position may not be
>     MaxSector even though md_start_sync() can still modify the spare
>     configuration.
>   - Link to v1:https://lore.kernel.org/linux-raid/20260630075640.1081634-1-abd.masalkhi@gmail.com/
> ---
>   drivers/md/md.c | 14 +++++++++++++-
>   1 file changed, 13 insertions(+), 1 deletion(-)

Reviewed-by: Yu Kuai <yukuai@fygo.io>

-- 
Thanks,
Kuai