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

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 <[email protected]>

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/[email protected]/

> 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/[email protected]?part=1