Re: [PATCH v5 19/29] 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 implementation of `bitmap_array_sectors` and `bitmap_s=
ector_map` in RAID personalities breaks range mapping and drops valid array=
 writes.
- [Critical] Omitted post-mapping clamp allows out-of-bounds offsets to tri=
gger out-of-bounds array accesses in `llbitmap_start_write`.
- [Critical] Decoupled reshape geometry logic causes `start_sync` to decode=
 modified bits incorrectly during array recovery.
- [High] Skipping reshape boundary splitting when `MD_RECOVERY_RESHAPE` is =
clear corrupts straddling bios.
--

--- Patch [19]: [PATCH v5 19/29] md/md-llbitmap: add reshape range mapping =
helpers ---
commit 633da220a726aec9691f97e3c1481f36882f4315
Author: Yu Kuai <[email protected]>

md/md-llbitmap: add reshape range mapping helpers

Teach llbitmap to choose old versus new geometry during reshape and to
encode exact bitmap ranges for the active geometry. This is the mapping
groundwork for checkpoint remapping.

Link: https://lore.kernel.org/all/20260726185916.2223460-1-mykola@meshstor.=
io/

> diff --git a/drivers/md/md-llbitmap.c b/drivers/md/md-llbitmap.c
> index f8a1b0f79be6a..fa16a4224c45a 100644
> --- a/drivers/md/md-llbitmap.c
> +++ b/drivers/md/md-llbitmap.c

[ ... ]

> +static sector_t llbitmap_logical_size(struct llbitmap *llbitmap, bool pr=
evious)
> +{
> +	struct mddev *mddev =3D llbitmap->mddev;
> +
> +	if (!mddev->private || !mddev->pers ||
> +	    !mddev->pers->bitmap_array_sectors)
> +		return llbitmap_personality_sync_size(llbitmap, previous);
> +	return mddev->pers->bitmap_array_sectors(mddev, previous);
> +}

[Severity: Critical]
Since bitmap_array_sectors is absent from all RAID personalities in this
patch series, won't this always fall back to
llbitmap_personality_sync_size()?

If a write is issued to a RAID5 array at a logical offset greater than
dev_sectors, it appears this will incorrectly use the physical component
size (dev_sectors) as the limit for the logical array offset, causing
llbitmap_map_layout() to drop valid array writes.

[ ... ]

> +static void llbitmap_map_layout(struct llbitmap *llbitmap, sector_t *off=
set,
> +				unsigned long *sectors, bool previous)
> +{

[ ... ]

> +	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]
The commit message states we need to keep the existing sync-size clamp after
mapping, but does this code actually enforce that post-mapping clamp?

Without a clamp against llbitmap_personality_sync_size() here at the end
of the function, could an out-of-bounds logical offset bypass filtering?
If that happens, llbitmap_start_write() could calculate an out-of-bounds
page_start index and trigger memory corruption inside
llbitmap_raise_barrier() before the state machine drops it.

[ ... ]

> +static void llbitmap_encode_range(struct llbitmap *llbitmap, sector_t *o=
ffset,
> +				  unsigned long *sectors, bool previous)
> +{
> +	unsigned long chunksize =3D previous ? llbitmap->chunksize :
> +				      llbitmap->reshape_chunksize;

[ ... ]

> +	start =3D div64_u64(*offset, chunksize);
> +	end =3D div64_u64(*offset + *sectors - 1, chunksize);
> +	*offset =3D (sector_t)start << llbitmap->chunkshift;
> +	*sectors =3D (end - start + 1) << llbitmap->chunkshift;
> +}

[Severity: Critical]
Does this decoupling of reshape geometry logic cause issues for the sync pa=
th
during recovery?

Here, writers calculate the chunk index using the new geometry
(reshape_chunksize) but scale it using the old chunkshift. Later, during
array recovery, llbitmap_start_sync() unconditionally shifts the physical
component offset by the old chunkshift:

unsigned long p =3D offset >> llbitmap->chunkshift;

Will this mismatch cause llbitmap_start_sync() to decode the wrong bit
index and skip recovering modified active data?

[ ... ]

> +static bool llbitmap_map_previous(struct llbitmap *llbitmap, sector_t of=
fset,
> +				  unsigned long sectors)
> +{
> +	struct mddev *mddev =3D llbitmap->mddev;
> +	sector_t boundary =3D READ_ONCE(mddev->reshape_position);
> +
> +	if (boundary =3D=3D MaxSector)
> +		return false;
> +
> +	WARN_ON_ONCE(sectors && offset < boundary && offset + sectors > boundar=
y);
> +
> +	return mddev->reshape_backwards ? offset < boundary : offset >=3D bound=
ary;
> +}

[Severity: High]
Can a straddling bio trigger this WARN_ON_ONCE and corrupt the bitmap if a
reshape is paused?

If a user pauses a reshape or the array enters a degraded state,
MD_RECOVERY_RESHAPE might be cleared. If mddev_bio_split_at_reshape_offset()
checks this flag and skips splitting the bio, a fully unsplit straddling
bio could reach here. This would force the entire bio to be evaluated
against a single geometry based purely on its starting sector, destroying
bitmap integrity for the straddling tail.

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260802195038.1642=
[email protected]?part=3D19