Re: [PATCH v3 13/21] md/md-llbitmap: remap checkpointed bits as reshape progresses

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] In-place bitmap array merge corrupts states due to forward iteration overlapping with unread source chunks (equivalent to `memcpy` vs `memmove` bug).
- [High] The reshape state merge function drops proactive sync unwritten states (`BitNeedSyncUnwritten`, `BitCleanUnwritten`), reverting them to basic states and losing sync status.
- [High] `llbitmap_reshape_map_range` fails to map `old` device sectors to array sectors when `bitmap_sector_map` is unavailable, corrupting source chunk calculations.
- [High] Lockless read-modify-write in `llbitmap_reshape_merge` races with concurrent I/O (`llbitmap_start_write`), causing lost BitDirty flags and silent data corruption.
- [Medium] Redundant condition in `llbitmap_rmerge_state` forces all RAID levels to treat mixed BitClean/BitUnwritten as BitClean, bypassing the `level_456` restriction.
- [Medium] Truncation of 64-bit sector counts to 32-bit unsigned long causes incomplete reshape mapping on 32-bit kernels.
--

--- Patch [13]: [PATCH v3 13/21] md/md-llbitmap: remap checkpointed bits as reshape progresses ---
commit 745e2cefd189066340bdf6deca751ae219d34b84
Author: Yu Kuai <[email protected]>

md/md-llbitmap: remap checkpointed bits as reshape progresses
    
Merge checkpointed old llbitmap state forward as reshape_position
advances and record the checkpoint remap through reshape_mark().

> 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

[ ... ]

> @@ -934,6 +942,33 @@ static int llbitmap_prepare_resize(struct llbitmap *llbitmap,

[ ... ]

> +	if (level_456 && ((dst == BitClean && src == BitUnwritten) ||
> +			  (src == BitClean && dst == BitUnwritten)))
> +		return BitClean;
> +	if (dst == BitClean || src == BitClean)
> +		return BitClean;

[Severity: Medium]
Does this unconditional check override the preceding level_456 restriction?

It looks like mixed clean and unwritten states will evaluate to clean on all
array types, including non-parity arrays like RAID 1 or RAID 10. This
effectively renders the level_456 restriction above it dead code.

> +	return BitUnwritten;
> +}

[Severity: High]
Are we unintentionally dropping proactive sync unwritten states here?

When reshaping an array with states like BitNeedSyncUnwritten,
BitSyncingUnwritten, or BitCleanUnwritten, they appear to fall through the
basic checks and downgrade to BitUnwritten. This silently loses sync status
for proactive parity builds on RAID 4/5/6.

[ ... ]

> @@ -1839,6 +1874,120 @@ static int llbitmap_reshape_can_start(struct mddev *mddev)

[ ... ]

> +struct llbitmap_reshape_range {
> +	sector_t offset;
> +	unsigned long sectors;

[Severity: Medium]
Will this truncate the 64-bit sector count to 32 bits on 32-bit architectures?

If the reshape process skips a large fully clean region of more than 2TB, it
seems this sector count might silently overflow since sectors is declared as
unsigned long.

> +	sector_t start;
> +	sector_t end;
> +};

[ ... ]

> +static void llbitmap_reshape_map_range(struct llbitmap *llbitmap,
> +				       sector_t lo, sector_t hi,
> +				       bool previous,
> +				       struct llbitmap_reshape_range *range)
> +{
> +	range->offset = lo;
> +	range->sectors = hi - lo;
> +	llbitmap_map_layout(llbitmap, &range->offset, &range->sectors, previous);

[Severity: High]
Does this fail to convert logical array sectors to bitmap device sectors for
the old layout when bitmap_sector_map is NULL?

For example, during a RAID5 reshape where bitmap_sector is implemented but
not bitmap_sector_map, llbitmap_map_layout skips mapping the old layout but
correctly maps the new one.

> +	range->start = range->offset;
> +	range->end = range->offset + range->sectors;
> +}
> +
> +static bool llbitmap_reshape_src_range(const struct llbitmap_reshape_range *old,
> +				       const struct llbitmap_reshape_range *new,
> +				       const struct llbitmap_reshape_range *dst,
> +				       struct llbitmap_reshape_range *src)
> +{
> +	if (!old->sectors)
> +		return false;
> +
> +	src->start = old->offset +
> +		mul_u64_u64_div_u64(dst->start - new->offset,
> +				    old->sectors, new->sectors);

[Severity: High]
Could this math incorrectly mix old array sectors with new device sectors?

If the mapping failed in the step above, combining these mismatched units
could lead to corrupted source chunk calculations.

> +	src->end = old->offset +

[ ... ]

> +static void llbitmap_reshape_merge(struct llbitmap *llbitmap,
> +				   const struct llbitmap_reshape_range *old,
> +				   const struct llbitmap_reshape_range *new)
> +{
> +	unsigned long dst_start;
> +	unsigned long dst_end;
> +	unsigned long dst;
> +
> +	if (!new->sectors)
> +		return;
> +
> +	dst_start = div64_u64(new->offset, llbitmap->reshape_chunksize);
> +	dst_end = div64_u64(new->end - 1, llbitmap->reshape_chunksize);
> +
> +	for (dst = dst_start; dst <= dst_end; dst++) {

[Severity: High]
Could this forward iteration cause in-place state corruption during the merge?

If the old to new layout mapping causes source chunks to be located at higher
indices than destination chunks, writing to dst could overwrite the state that
a subsequent iteration needs to read as src. This propagates the corrupted
state forward.

> +		struct llbitmap_reshape_range dst_range;
> +		struct llbitmap_reshape_range src;
> +		enum llbitmap_state state;
> +
> +		llbitmap_reshape_dst_range(llbitmap, dst, new, &dst_range);
> +		state = llbitmap_reshape_init_dst(llbitmap, dst, new);
> +		if (llbitmap_reshape_src_range(old, new, &dst_range, &src))
> +			state = llbitmap_rmerge_src(llbitmap, state, &src);
> +		else
> +			state = llbitmap_rmerge_state(llbitmap, state, BitUnwritten);
> +		llbitmap_write(llbitmap, state, dst);

[Severity: High]
Does this lockless read-modify-write race with concurrent I/O updates to the
same chunk?

Because reshape checkpoint boundaries might not be chunk-aligned, these
boundary chunks can cover sectors outside the suspended reshape window. A
concurrent llbitmap_start_write to those unsuspended sectors performs a
lockless read-modify-write on the chunk state. 

If llbitmap_reshape_merge performs a lockless write on the exact same chunk
index concurrently, one could overwrite the other, losing BitDirty flags and
risking silent data corruption.

> +	}
> +}
> +

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