Re: [PATCH v3 11/21] md/md-llbitmap: add reshape range mapping helpers

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Missing post-map sync-size clamp in llbitmap_map_layout() allows mapped physical sectors to exceed the bitmap capacity, causing OOB array accesses.
- [Critical] The `bitmap_array_sectors` callback is not implemented for any personality (e.g. RAID5), causing the pre-map clamp to incorrectly use component coordinates against array-sector IOs.
- [Critical] Coordinate mapping is completely skipped for the old geometry during reshape, leading to array coordinates being treated as component coordinates.
- [Critical] Bios crossing the reshape boundary are not split before bitmap accounting, causing `llbitmap` to encode the entire range with a single geometry.
--

--- Patch [11]: [PATCH v3 11/21] md/md-llbitmap: add reshape range mapping helpers ---
commit b480f867a40652c2963e56247233a10fff0a56b8
Author: Yu Kuai <[email protected]>

md/md-llbitmap: add reshape range mapping helpers

This commit adds reshape range mapping helpers to llbitmap to differentiate
between old and new geometry during a reshape. It also introduces
discard-specific range encoding and clamps pre-map ranges in the same
coordinate space as incoming IO.

Link: https://lore.kernel.org/all/[email protected]/

> steady-state RAID5 must use bitmap_array_sectors() before mapping and keep
> the existing sync-size clamp after mapping.

Does the implementation match this commit message description? Looking at
llbitmap_map_layout(), it appears the post-map sync-size clamp was omitted.

> diff --git a/drivers/md/md-llbitmap.c b/drivers/md/md-llbitmap.c
> --- a/drivers/md/md-llbitmap.c
> +++ b/drivers/md/md-llbitmap.c
[ ... ]
> @@ -449,6 +450,16 @@ static sector_t llbitmap_personality_sync_size(struct llbitmap *llbitmap,
>  	return mddev->pers->bitmap_sync_size(mddev, previous);
>  }
>  
> +static sector_t llbitmap_logical_size(struct llbitmap *llbitmap, bool previous)
> +{
> +	struct mddev *mddev = llbitmap->mddev;
> +
> +	if (!mddev->private || !mddev->pers ||
> +	    !mddev->pers->bitmap_array_sectors)
> +		return llbitmap_personality_sync_size(llbitmap, previous);

[Severity: Critical]
Is bitmap_array_sectors implemented for the personalities that need it?

If personalities like RAID5 do not wire up bitmap_array_sectors, this falls
back to returning llbitmap_personality_sync_size(), which uses the much
smaller component coordinates.

Because RAID5 receives IOs in array coordinates, and array coordinates are
significantly larger than component coordinates, wouldn't any write targeting
the upper portion of a RAID5 array evaluate to start >= limit inside
llbitmap_map_layout() and be silently dropped? This could lead to silent data
corruption on an unclean shutdown.

> +	return mddev->pers->bitmap_array_sectors(mddev, previous);
> +}
> +
>  static void llbitmap_refresh_reshape(struct llbitmap *llbitmap)
[ ... ]
> +static void llbitmap_map_layout(struct llbitmap *llbitmap, sector_t *offset,
> +				unsigned long *sectors, bool previous)
> +{
> +	sector_t limit = llbitmap_logical_size(llbitmap, previous);
> +	sector_t start = *offset;
> +	sector_t end = start + *sectors;
> +
> +	if (start >= limit) {
> +		*sectors = 0;
> +		return;
> +	}
> +	if (end > limit)
> +		end = limit;
> +
> +	*offset = start;
> +	*sectors = end - start;
> +	if (!*sectors)
> +		return;
> +
> +	if (llbitmap->mddev->pers->bitmap_sector_map)
> +		llbitmap->mddev->pers->bitmap_sector_map(llbitmap->mddev, offset,
> +							 sectors, previous);
> +	else if (!previous && llbitmap->mddev->pers->bitmap_sector)
> +		llbitmap->mddev->pers->bitmap_sector(llbitmap->mddev, offset,
> +							 sectors);

[Severity: Critical]
Does this logic incorrectly skip coordinate mapping for the old geometry?

During an active reshape, IOs targeting the old geometry evaluate previous to
true. Since the !previous condition prevents bitmap_sector() from being called,
array-sector offsets are passed directly to llbitmap_encode_range().

Because array offsets are much larger than component sectors, this produces
chunk indices that wildly exceed llbitmap->chunks. This can lead to
out-of-bounds memory accesses on the pctl array or corrupt the bitmap by
dirtying the wrong chunks.

[Severity: Critical]
Is a post-map sync-size clamp missing at the end of this function?

When a RAID array's size is not perfectly aligned to a multiple of chunk sizes,
bitmap_sector() rounds the end offset up to the next chunk boundary. Without
clamping the result against llbitmap_personality_sync_size(), the mapped
component sector range can exceed the physical capacity.

This un-clamped offset can yield an out-of-bounds page_end when passed into
llbitmap_start_write(), causing an out-of-bounds dereference on the
dynamically allocated llbitmap->pctl array via llbitmap_raise_barrier().

> +}
> +
> +static void llbitmap_encode_range(struct llbitmap *llbitmap, sector_t *offset,
[ ... ]
> @@ -1385,11 +1470,35 @@ static void llbitmap_destroy(struct mddev *mddev)
>  	mutex_unlock(&mddev->bitmap_info.mutex);
>  }
>  
> +static bool llbitmap_map_previous(struct llbitmap *llbitmap, sector_t offset,
> +				  unsigned long sectors)
> +{
> +	struct mddev *mddev = llbitmap->mddev;
> +	sector_t boundary = mddev->reshape_position;
> +
> +	if (!llbitmap_reshaping(llbitmap))
> +		return false;
> +
> +	WARN_ON_ONCE(sectors && offset < boundary && offset + sectors > boundary);

[Severity: Critical]
Will this WARN_ON_ONCE trigger if bios crossing the reshape boundary are not
split beforehand?

The block layer currently accounts the entire contiguous bio as a single range
in md_account_bio(). While the helper mddev_bio_split_at_reshape_offset is
exported in this patch series, it appears it is never actually invoked by the
personalities (such as in raid5_make_request) prior to accounting.

Because this function returns a single geometry decision for the entire IO,
llbitmap_encode_range() will map the entire crossing bio using a single
chunksize. This miscalculates chunk indices for the portion of the bio sitting
in the other geometry, dirtying the wrong chunks and risking data corruption if
resync skips the correctly-associated chunks after a crash.

> +
> +	return mddev->reshape_backwards ? offset < boundary : offset >= boundary;
> +}

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