Re: [PATCH v4 22/25] md/raid5: add exact old and new llbitmap mapping helpers

[sashiko-bot@kernel.org]

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

New issues:
- [High] The newly introduced `raid5_bitmap_array_sectors` incorrectly calculates the previous array size when shrinking, returning the new smaller size instead of the exact old size.

Pre-existing issues:
- [Critical] Bitmap mapping logic uses the current `conf->max_degraded` to compute the previous layout's data disks, corrupting mapping during RAID level migration.
- [High] Integer overflow on 32-bit architectures in `raid5_bitmap_sector` when computing `*sectors` during reshape of large arrays.
--

--- Patch [22]: [PATCH v4 22/25] md/raid5: add exact old and new llbitmap mapping helpers ---
commit 2b6e9c9ff8dbd00ebcd0c4b00c57f4d4ef3e5964
Author: Yu Kuai <yukuai@fygo.io>

md/raid5: add exact old and new llbitmap mapping helpers
    
Teach RAID5 to export exact old and new llbitmap mappings and the
corresponding sync and array sizes for reshape-aware bitmap users.

> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 8b3483674398d..af7775278af6d 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -5942,28 +5942,46 @@ static enum reshape_loc get_reshape_loc(struct mddev *mddev,
>  	return LOC_BEHIND_RESHAPE;
>  }
>  
> -static void raid5_bitmap_sector(struct mddev *mddev, sector_t *offset,
> -				unsigned long *sectors)
> +static void raid5_bitmap_sector_map(struct mddev *mddev, sector_t *offset,
> +				    unsigned long *sectors,
> +				    bool previous)
>  {
>  	struct r5conf *conf = mddev->private;
>  	sector_t start = *offset;
>  	sector_t end = start + *sectors;
> -	sector_t prev_start = start;
> -	sector_t prev_end = end;
>  	int sectors_per_chunk;
> -	enum reshape_loc loc;
>  	int dd_idx;
>  
> -	sectors_per_chunk = conf->chunk_sectors *
> -		(conf->raid_disks - conf->max_degraded);
> +	if (previous)
> +		sectors_per_chunk = conf->prev_chunk_sectors *
> +			(conf->previous_raid_disks - conf->max_degraded);

[Severity: Critical]
This is a pre-existing issue, but I noticed we are using the current layout's
max_degraded value to compute the previous layout's physical sectors here.

During a RAID5 to RAID6 migration, conf->max_degraded is updated to 2
immediately at the start of the reshape. However, the old RAID5 layout only
had 1 parity disk.

If we subtract 2 instead of 1, does this corrupt the logical-to-physical chunk
translation for the previous layout? It seems write operations ahead of the
reshape might dirty the wrong bits in the bitmap, potentially leading to silent
data corruption or skipped resyncs after a crash.

I also noticed raid5_compute_sector() applies the current max_degraded to the
previous layout's data_disks computation:

drivers/md/raid5.c:raid5_compute_sector() {
        ...
        int raid_disks = previous ? conf->previous_raid_disks
                                  : conf->raid_disks;
        int data_disks = raid_disks - conf->max_degraded;
        ...
}

Should we be tracking the previous layout's parity disk count separately?

> +	else
> +		sectors_per_chunk = conf->chunk_sectors *
> +			(conf->raid_disks - conf->max_degraded);
>  	sector_div(start, sectors_per_chunk);
>  	start *= sectors_per_chunk;

[ ... ]

> @@ -5992,7 +6001,7 @@ static void raid5_bitmap_sector(struct mddev *mddev, sector_t *offset,
>  	 * we set bits for both.
>  	 */
>  	*offset = min(start, prev_start);
> -	*sectors = max(end, prev_end) - *offset;
> +	*sectors = max(end, prev_start + prev_sectors) - *offset;

[Severity: High]
This is a pre-existing issue, but could this subtraction result in an integer
overflow on 32-bit architectures?

For large arrays (e.g., >24TB), the physical sector distance between the old
and new mappings for a single logical address can exceed ULONG_MAX.

Since *sectors is a 32-bit unsigned long pointer on 32-bit architectures, it
seems this subtraction could truncate the 64-bit sector_t value. If that
happens, might the bitmap logic only mark a small lower portion of the span
as dirty due to the integer wrap-around?

>  }
>  
>  static enum stripe_result make_stripe_request(struct mddev *mddev,

[ ... ]

> @@ -9059,6 +9068,20 @@ static void raid5_prepare_suspend(struct mddev *mddev)
>  	wake_up(&conf->wait_for_reshape);
>  }
>  
> +static sector_t raid5_bitmap_sync_size(struct mddev *mddev, bool previous)
> +{
> +	return mddev->dev_sectors;
> +}
> +
> +static sector_t raid5_bitmap_array_sectors(struct mddev *mddev, bool previous)
> +{
> +	struct r5conf *conf = mddev->private;
> +
> +	if (previous)
> +		return raid5_size(mddev, 0, 0);

[Severity: High]
When shrinking an array, the previous array size is larger than the new array
size.

By passing 0 for the raid_disks argument, raid5_size() calculates the size
using min(conf->raid_disks, conf->previous_raid_disks).

Will this evaluate to the new, smaller size instead of the exact previous
capacity? If reshape-aware bitmap users are tracking the tail of the array
during a backwards reshape, this might fail to export the exact original tail
boundaries and lead to missed bitmap synchronizations.

> +	return raid5_size(mddev, mddev->dev_sectors, conf->raid_disks);
> +}
> +

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