Re: [PATCH] md: avoid modifying spares while the array is not suspended

[sashiko-bot@kernel.org]

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

New issues:
- [High] The patch prevents the recovery thread from starting or resuming for existing rebuilding spares, silently abandoning recovery and leaving the array permanently degraded.

Pre-existing issues:
- [High] The patch addresses a TOCTOU race for `state_store` but misses other vulnerable paths like `slot_store` and cluster updates, which still call `remove_and_add_spares()` without array suspension.
--

commit 16cf64f20d25bc671dc52422ec0a44672674e524
Author: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>

md: avoid modifying spares while the array is not suspended

This commit ensures that remove_spares() and remove_and_add_spares() are
only called after the array has been suspended. This prevents a race
condition where normal I/O could trigger a NULL pointer dereference when
rdevs are removed or replaced concurrently.

Closes: https://sashiko.dev/#/patchset/20260628142420.1051027-1-abd.masalkhi@gmail.com?part=3

> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index d1465bcd86c81..f555eb5aca5d8 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -10151,7 +10154,11 @@ static bool md_choose_sync_action(struct mddev *mddev, int *spares)
>  	 * also removed and re-added, to allow the personality to fail the
>  	 * re-add.
>  	 */
> -	*spares = remove_and_add_spares(mddev, NULL);
> +	if (array_suspended)
> +		*spares = remove_and_add_spares(mddev, NULL);
> +	else
> +		*spares = 0;
> +

[Severity: High]
Does this else branch prevent the recovery thread from starting or resuming
for existing rebuilding spares?

In md_start_sync(), suspend is determined by checking
md_spares_need_change(mddev). For an existing recovering spare receiving
normal I/O (where nr_pending > 0), both rdev_removeable() and
rdev_addable() can evaluate to false. This causes md_spares_need_change()
to return false, leaving suspend as false.

When md_choose_sync_action() is called with array_suspended == false, this
new else branch forces *spares = 0. By completely bypassing the counting of
existing recovering spares, md_choose_sync_action() will return false and
skip starting the sync thread.

Since nr_pending dropping to zero on a non-faulty spare doesn't trigger a
new MD_RECOVERY_NEEDED event, could this silently abandon recovery and
leave the array permanently degraded?

[Severity: High]
This is a pre-existing issue, but does restricting remove_and_add_spares()
calls to suspended arrays in md_choose_sync_action() leave other
vulnerable paths exposed?

The commit addresses a TOCTOU race by ensuring remove_and_add_spares() is
only called when the array is suspended, preventing concurrent I/O from
encountering a cleared replacement pointer.

However, slot_store() can still call remove_and_add_spares() without an
array suspension when a user writes "none" (slot == -1):

drivers/md/md.c:slot_store() {
    ...
    clear_bit(Blocked, &rdev->flags);
    remove_and_add_spares(rdev->mddev, rdev);
    if (rdev->raid_disk >= 0)
        return -EBUSY;
    ...
}

Similarly, the MD cluster component updates the array configuration in
check_sb_changes() by calling remove_and_add_spares() while the array is
not suspended.

Could normal I/O still increment nr_pending on a replacement drive just
before remove_spares() removes the faulty drive and clears the replacement
pointer in these paths, reproducing the exact TOCTOU crash this patch
aims to fix?

> @@ -10196,11 +10203,12 @@ static void md_start_sync(struct work_struct *ws)
>  		 * As we only add devices that are already in-sync, we can
>  		 * activate the spares immediately.
>  		 */
> -		remove_and_add_spares(mddev, NULL);
> +		if (suspend)
> +			remove_and_add_spares(mddev, NULL);
>  		goto not_running;
>  	}
>  
> -	if (!md_choose_sync_action(mddev, &spares))
> +	if (!md_choose_sync_action(mddev, &spares, suspend))
>  		goto not_running;

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