Re: [PATCH v5 23/29] md/raid10: reject llbitmap reshape when md chunk shrinks

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] The patch intends to reject shrinking reshapes when `ID_LLBITMAP` is active by returning `-EOPNOTSUPP` from `raid10_check_reshape()`. However, a severe pre-existing logic flaw in `mddev_start_reshape()` causes it to evaluate any non-zero return (including negative error codes) as a truthy condition. This bypasses the patch's protection entirely and triggers a destructive array corruption if the reshape is interrupted.
--

--- Patch [23]: [PATCH v5 23/29] md/raid10: reject llbitmap reshape when md chunk shrinks ---
commit f65520f6e55cf9e2cea49909a73ee06aeb4f57b1
Author: Yu Kuai <yukuai@fygo.io>

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. Shrinking mddev->chunk_sectors
can shrink the effective data range covered by each bit for the RAID10
reshape geometry.

> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index ed3c6fbe65f78..1c3393467667b 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -4245,6 +4245,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: Critical]
This is a pre-existing issue, but returning an error code here doesn't seem
to prevent the reshape from starting if it was already interrupted.

Looking at mddev_start_reshape() in drivers/md/md.c, it evaluates the negative
error code as a truthy condition:

	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 -EOPNOTSUPP evaluates to true, will this erroneously call
mddev->pers->start_reshape() instead of aborting the operation?

If so, does this cause raid10_start_reshape() to destructively reset the
reshape progress back to 0 for an interrupted reshape?

	} else
		conf->reshape_progress = 0;
	conf->reshape_safe = conf->reshape_progress;

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260802195038.164272-1-yukuai@kernel.org?part=23