Re: [PATCH v2 10/20] md/md-llbitmap: add reshape range mapping helpers

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] TOCTOU race on mddev->reshape_position causes bios to be mapped to the wrong geometry.
- [High] The resync threads use raw array sectors and blindly compute the bitmap bit index using the old geometry's chunkshift, completely ignoring reshape_chunksize for sectors residing in the new geometry.
--

commit a481d24ff8d12695efeabb7f320e930174126333
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.

> diff --git a/drivers/md/md-llbitmap.c b/drivers/md/md-llbitmap.c
> index 11fede1fc79fd..d27993cfb13da 100644
> --- a/drivers/md/md-llbitmap.c
> +++ b/drivers/md/md-llbitmap.c

[ ... ]

> @@ -1376,11 +1433,32 @@ 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;

[Severity: High]
Can a concurrent reshape thread update mddev->reshape_position from MaxSector to
0 between reading it into boundary and the call to llbitmap_reshaping()?

If that happens, boundary holds MaxSector but llbitmap_reshaping() evaluates to
true. This would cause offset < MaxSector to evaluate to true, mapping the IO
to the new geometry before the array has actually reshaped the sector. Could
this race lead to data corruption?

> +
> +	WARN_ON_ONCE(sectors && offset < boundary && offset + sectors > boundary);
> +
> +	return mddev->reshape_backwards ? offset < boundary : offset >= boundary;
> +}

[ ... ]

> @@ -1549,7 +1627,11 @@ static bool llbitmap_blocks_synced(struct mddev *mddev, sector_t offset)
>  {
>  	struct llbitmap *llbitmap = mddev->bitmap;
>  	unsigned long p = offset >> llbitmap->chunkshift;

[Severity: High]
Does this unconditionally use the old chunkshift to compute the bit index from
the raw sync offset?

During an interrupted reshape recovery, active IO computes the correct bit index
using llbitmap_encode_range() for the new geometry. However, functions like
llbitmap_blocks_synced(), llbitmap_skip_sync_blocks(), and llbitmap_start_sync()
receive raw physical sectors from md_do_sync() and completely bypass
llbitmap_prepare_range().

If reshape_chunksize differs from chunksize, will the sync thread calculate the
wrong bit index, read a clean bit instead of the dirty bit marked by active IO,
and silently skip synchronizing the dirty sectors?

> -	enum llbitmap_state c = llbitmap_read(llbitmap, p);
> +	enum llbitmap_state c;
> +
> +	if (p >= llbitmap->chunks)
> +		return false;
> +	c = llbitmap_read(llbitmap, p);
>  
>  	return c == BitClean || c == BitDirty || c == BitCleanUnwritten;
>  }

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