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

[Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>]

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