Re: [PATCH] md: suspend array while updating raid1 raid_disks via sysfs

[FengWei Shih <dannyshih@synology.com>]

Hi Kual,

Yu Kuai 於 2025/12/25 下午 04:19 寫道:
> Hi,
>
> 在 2025/12/18 17:06, dannyshih 写道:
>> From: FengWei Shih <dannyshih@synology.com>
>>
>> When an I/O error occurs, the corresponding r1bio might be queued during
>> raid1_reshape() and not released. Leads to r1bio release with wrong
>> raid_disks.
> Please be more specific about the problem, mempool_destroy() will warn about
> object still in use, and free r1bio with updated conf->raid_disks will cause
> problem like memory oob.

In raid1_reshape(), freeze_array() is called before modifying the r1bio
memory pool (conf->r1bio_pool) and conf->raid_disks, and
unfreeze_array() is called after the update is completed.

However, freeze_array() only waits until nr_sync_pending and
(nr_pending - nr_queued) of all buckets reaches zero. When an I/O error
occurs, nr_queued is increased and the corresponding r1bio is queued to
either retry_list or bio_end_io_list. As a result, freeze_array() may
unblock before these r1bios are released.

This can lead to a situation where conf->raid_disks and the mempool have
already been updated while queued r1bios, allocated with the old
raid_disks value, are later released. Consequently, free_r1bio() may
access memory out of bounds in put_all_bios() and release r1bios of the
wrong size to the new mempool, potentially causing issues with the
mempool as well.

>> * raid1_reshape() calls freeze_array(), which only waits for r1bios be
>>     queued or released.
>>
>> Since only normal I/O might be queued while an I/O error occurs, suspending
>> the array avoids this issue.
>>
>> Signed-off-by: FengWei Shih <dannyshih@synology.com>
>> ---
>>    drivers/md/md.c | 5 +++--
>>    1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index e5922a682953..6424652bce6e 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -4402,12 +4402,13 @@ raid_disks_store(struct mddev *mddev, const char *buf, size_t len)
>>    {
>>    	unsigned int n;
>>    	int err;
>> +	bool need_suspend = (mddev->pers && mddev->level == 1);
> Perhaps just suspend the array unconditionally, this will make sense.
>
>>    
>>    	err = kstrtouint(buf, 10, &n);
>>    	if (err < 0)
>>    		return err;
>>    
>> -	err = mddev_lock(mddev);
>> +	err = need_suspend ? mddev_suspend_and_lock(mddev) : mddev_lock(mddev);
>>    	if (err)
>>    		return err;
>>    	if (mddev->pers)
>> @@ -4432,7 +4433,7 @@ raid_disks_store(struct mddev *mddev, const char *buf, size_t len)
>>    	} else
>>    		mddev->raid_disks = n;
>>    out_unlock:
>> -	mddev_unlock(mddev);
>> +	need_suspend ? mddev_unlock_and_resume(mddev) : mddev_unlock(mddev);
>>    	return err ? err : len;
>>    }
>>    static struct md_sysfs_entry md_raid_disks =

Thank you for your suggestions. I will update the commit based on your
feedback and prepare the V2 patch for submission.

FengWei Shih

Disclaimer: The contents of this e-mail message and any attachments are confidential and are intended solely for addressee. The information may also be legally privileged. This transmission is sent in trust, for the sole purpose of delivery to the intended recipient. If you have received this transmission in error, any use, reproduction or dissemination of this transmission is strictly prohibited. If you are not the intended recipient, please immediately notify the sender by reply e-mail or phone and delete this message and its attachments, if any.