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_sector_map` in RAID personalities breaks range mapping and drops valid array writes.
- [Critical] Omitted post-mapping clamp allows out-of-bounds offsets to trigger 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 [off-list ref]

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/ (local)

[ ... ]

+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);
+	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 *offset,
+				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 *offset,
+				  unsigned long *sectors, bool previous)
+{
+	unsigned long chunksize = previous ? llbitmap->chunksize :
+				      llbitmap->reshape_chunksize;

[ ... ]

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

[Severity: Critical]
Does this decoupling of reshape geometry logic cause issues for the sync path
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 = 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 offset,
+				  unsigned long sectors)
+{
+	struct mddev *mddev = llbitmap->mddev;
+	sector_t boundary = READ_ONCE(mddev->reshape_position);
+
+	if (boundary == MaxSector)
+		return false;
+
+	WARN_ON_ONCE(sectors && offset < boundary && offset + sectors > boundary);
+
+	return mddev->reshape_backwards ? offset < boundary : offset >= boundary;
+}

[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.

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