Re: [PATCH v2 17/20] md/raid5: add exact old and new llbitmap mapping helpers

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

New issues:
- [High] raid5_bitmap_array_sectors() incorrectly returns the new, smaller array size for the previous layout during a shrink reshape, leading to truncated bitmap tracking.

Pre-existing issues:
- [High] Invalid use of round_down() and round_up() with a non-power-of-2 sectors_per_chunk multiplier, causing corrupt bitmap mappings.
- [High] Bounding box calculation for mapping ranges between old and new layouts can overflow `unsigned long` on 32-bit systems, truncating dirty regions.
- [High] raid5_bitmap_sector_map() accesses reshape geometry parameters without holding the required sequence lock.
--

commit 7f0559d3994c9371245bbc6a23cf3ce1e8d48872
Author: Yu Kuai <[email protected]>

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 0c5c9fb0606ee..1613a42cc25dc 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
[ ... ]
> @@ -5942,25 +5942,43 @@ static enum reshape_loc get_reshape_loc(struct mddev *mddev,
[ ... ]
> -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);
> +	else
> +		sectors_per_chunk = conf->chunk_sectors *
> +			(conf->raid_disks - conf->max_degraded);

[Severity: High]
This is a pre-existing issue, but does raid5_bitmap_sector_map() access
these reshape geometry parameters safely?

During a concurrent reshape operation (e.g., raid5_start_reshape()), these
configuration fields can be updated in-place. Without being protected by
read_seqcount_begin(&conf->gen_lock), could a concurrent I/O observe torn
state or a mismatch between chunk size and raid disks, leading to an
incorrect bitmap mapping?

>  	start = round_down(start, sectors_per_chunk);
>  	end = round_up(end, sectors_per_chunk);

[Severity: High]
This is a pre-existing issue, but do round_down() and round_up() work correctly
here when sectors_per_chunk is not a power of 2?

The round_down() and round_up() macros perform a bitwise masking operation
which strictly requires the multiplier to be a power of 2. Because the
number of data disks (raid_disks - max_degraded) is often not a power of 2
(for example, 3 data disks in a 4-disk RAID5), sectors_per_chunk is frequently
not a power of 2.

Will this bitwise mask silently corrupt the logical offsets instead of rounding
to stripe boundaries, leading to incorrect physical regions being tracked in
the bitmap?

> -	start = raid5_compute_sector(conf, start, 0, &dd_idx, NULL);
> -	end = raid5_compute_sector(conf, end, 0, &dd_idx, NULL);
> +	start = raid5_compute_sector(conf, start, previous, &dd_idx, NULL);
> +	end = raid5_compute_sector(conf, end, previous, &dd_idx, NULL);
> +	*offset = start;
> +	*sectors = end - start;
> +}
> +
> +static void raid5_bitmap_sector(struct mddev *mddev, sector_t *offset,
> +				unsigned long *sectors)
> +{
> +	struct r5conf *conf = mddev->private;
> +	sector_t start = *offset;
> +	sector_t end = start + *sectors;
> +	sector_t prev_start = start;
> +	unsigned long prev_sectors = end - start;
> +	enum reshape_loc loc;
> +
> +	raid5_bitmap_sector_map(mddev, &start, sectors, false);
> +	end = start + *sectors;
[ ... ]
> @@ -5987,7 +5998,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 isn't a bug introduced by this patch, but can this bounding box
calculation overflow on 32-bit systems?

When calculating the bounding box spanning the old and new physical layout
mappings for an I/O ahead of a reshape, the physical sector offsets diverge
linearly due to differing data disk counts.

For large arrays (e.g., >40TB), the gap between the mapped sectors can
exceed 2TB (4,294,967,295 sectors). Because *sectors is a pointer to
unsigned long (which is 32-bit on 32-bit architectures), could this
subtraction silently overflow, truncating the dirty region and potentially
missing necessary bits?

>  }
>  
>  static enum stripe_result make_stripe_request(struct mddev *mddev,
[ ... ]
> @@ -9049,6 +9060,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]
Does passing 0 for raid_disks in raid5_size() incorrectly truncate the array
size for the previous layout?

During an array shrink reshape (e.g., from 5 disks to 4), passing 0 forces
raid5_size() to use the minimum disk count between the old and new layouts.
This would return the new, smaller array size instead of the exact previous
layout's size.

Since reshape-aware bitmap users rely on this limit to clip logical I/O
offsets, could I/Os mapping to the old unreshaped region beyond the new
limit be erroneously discarded, leading to dropped dirty bits?

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

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