Re: [PATCH v2 03/20] md: add helper to split bios at reshape offset

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Lockless read of 64-bit `mddev->reshape_position` risks torn reads on 32-bit architectures and TOCTOU races between bio splitting and bitmap accounting, leading to silent data corruption.
--

commit 4912480eb23a69a6634d083e53f12568752eafb0
Author: Yu Kuai <[email protected]>

md: add helper to split bios at reshape offset

This commit adds the mddev_bio_split_at_reshape_offset() helper function.
It allows MD personalities to share the reshape-offset bio splitting logic
instead of open-coding it in multiple places.

> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 0b59c676f7c03..c18a7f3c27e65 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
[ ... ]
> @@ -9361,6 +9361,45 @@ void md_submit_discard_bio(struct mddev *mddev, struct md_rdev *rdev,
>  }
>  EXPORT_SYMBOL_GPL(md_submit_discard_bio);
>  
> +struct bio *mddev_bio_split_at_reshape_offset(struct mddev *mddev,
> +					      struct bio *bio,
> +					      unsigned int *max_sectors,
> +					      struct bio_set *bs)
> +{
> +	sector_t boundary;
> +	sector_t start;
> +	sector_t end;
> +	unsigned int split_sectors;
> +
> +	split_sectors = bio_sectors(bio);
> +	if (max_sectors && *max_sectors && *max_sectors < split_sectors)
> +		split_sectors = *max_sectors;
> +
> +	if (!test_bit(MD_RECOVERY_RESHAPE, &mddev->recovery))
> +		goto split;
> +
> +	boundary = mddev->reshape_position;

[Severity: Critical]
Can this lockless read of mddev->reshape_position cause a torn read on
32-bit architectures since it is a 64-bit value?

Additionally, does reading this without locking introduce a time-of-check to
time-of-use race with bitmap accounting? If mddev->reshape_position
is updated by the reshape thread after this split check, but before bitmap
accounting in llbitmap_map_previous(), an unsplit bio could end up crossing
the new boundary.

This could trigger the WARN_ON_ONCE in llbitmap_map_previous():

    sector_t boundary = mddev->reshape_position;

    if (!llbitmap_reshaping(llbitmap))
        return false;

    WARN_ON_ONCE(sectors && offset < boundary && offset + sectors > boundary);

Could this race result in a bio being mis-accounted to the wrong array
geometry, potentially causing data corruption if a crash occurs during a
reshape?

> +	start = bio->bi_iter.bi_sector;
> +	end = bio_end_sector(bio);
> +	if (start >= boundary || end <= boundary)
> +		goto split;
> +
> +	if (boundary - start < split_sectors)
> +		split_sectors = boundary - start;
> +
> +split:
> +	if (max_sectors)
> +		*max_sectors = split_sectors;
> +	if (split_sectors < bio_sectors(bio)) {
> +		bio = bio_submit_split_bioset(bio, split_sectors, bs);
> +		if (bio)
> +			bio->bi_opf |= REQ_NOMERGE;
> +	}
> +
> +	return bio;
> +}
> +EXPORT_SYMBOL_GPL(mddev_bio_split_at_reshape_offset);

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