Re: [PATCH v3 15/21] md/raid10: reject llbitmap reshape when md chunk shrinks

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

New issues:
- [High] Missing Validation in Array Assembly Path

Pre-existing issues:
- [High] Logic Error in Resume Path forces reshape start despite check failure
--

--- Patch [15]: [PATCH v3 15/21] md/raid10: reject llbitmap reshape when md chunk shrinks ---
commit 0f54995b397bafdc9d0ce8dd025c97e68e5b1284
Author: Yu Kuai <[email protected]>

md/raid10: reject llbitmap reshape when md chunk shrinks

llbitmap reshape keeps one live bitmap and cannot safely make an
existing bitmap bit cover a smaller data range.

The llbitmap chunksize itself will not shrink when mddev->chunk_sectors
stays the same or grows. However, shrinking mddev->chunk_sectors can
shrink the effective data range covered by each bit for the RAID10
reshape geometry. Reject that reshape while llbitmap is active.

> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index 0a3cfdd3f5df8..1242b8d3bb6bd 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -4284,6 +4284,10 @@ static int raid10_check_reshape(struct mddev *mddev)
>  
>  	if (conf->geo.far_copies != 1 && !conf->geo.far_offset)
>  		return -EINVAL;
> +	if (mddev->bitmap_id == ID_LLBITMAP &&
> +	    mddev->new_chunk_sectors &&
> +	    mddev->new_chunk_sectors < mddev->chunk_sectors)
> +		return -EOPNOTSUPP;

[Severity: High]
This is a pre-existing issue, but does returning an error code here actually
prevent the reshape from starting?

Looking at mddev_start_reshape() in drivers/md/md.c, it seems to treat a
non-zero return value from check_reshape() as a trigger to execute
start_reshape():

    if (mddev->reshape_position == MaxSector ||
        mddev->pers->check_reshape == NULL ||
        mddev->pers->check_reshape(mddev)) {
            clear_bit(MD_RECOVERY_FROZEN, &mddev->recovery);
            ret = mddev->pers->start_reshape(mddev);

Since raid10_check_reshape() returns -EOPNOTSUPP,
mddev->pers->check_reshape(mddev) will evaluate to true. Will this bypass
the rejection and force raid10_start_reshape() to proceed with the invalid
geometry?

[Severity: High]
Does this same llbitmap constraint need to be validated during array
assembly?

If an array with an active llbitmap and a suspended reshape with a shrunk
chunk size is assembled, md_run() invokes raid10_run(), which assigns the
new geometry via setup_conf() without validating the llbitmap constraint:

drivers/md/raid10.c:setup_conf() {
    ...
    if (mddev->reshape_position == MaxSector) {
        conf->prev = conf->geo;
        conf->reshape_progress = MaxSector;
    } else {
        if (setup_geo(&conf->prev, mddev, geo_old) != conf->copies) {
            err = -EINVAL;
            goto out;
        }
    ...
}

Could this allow the array to mount and serve I/O using a geometry where
the bitmap bits cover a smaller data range than originally intended, causing
data corruption because the bitmap won't properly track the full range
written?

>  
>  	if (setup_geo(&geo, mddev, geo_start) != conf->copies)
>  		/* mustn't change number of copies */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=15