Re: [PATCH v2 19/20] md/raid5: wire llbitmap reshape lifecycle

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Checkpointing the translated llbitmap to disk before the MD superblock is updated creates a crash window that leads to silent data corruption on restart.
- [Critical] The llbitmap page control array (`llbitmap->pctl`) can be accessed after being freed during RAID5 reshape initialization because the bitmap resize happens before the array is quiesced.
--

commit 18675e43f792016c6cde2f177af7a007c667120d
Author: Yu Kuai <[email protected]>

md/raid5: wire llbitmap reshape lifecycle

Prepare llbitmap before RAID5 reshape starts, checkpoint the bitmap
before advancing reshape_position, and finish the llbitmap geometry
update when reshape completes.

> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 9de648f75dd04..ac7ea483502fd 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c

[ ... ]

> @@ -6430,6 +6430,13 @@ static sector_t reshape_request(struct mddev *mddev, sector_t sector_nr, int *sk
>  			   || test_bit(MD_RECOVERY_INTR, &mddev->recovery));
>  		if (atomic_read(&conf->reshape_stripes) != 0)
>  			return 0;
> +		if (md_bitmap_enabled(mddev, false) &&
> +		    mddev->bitmap_ops->reshape_mark &&
> +		    conf->reshape_safe != conf->reshape_progress) {
> +			mddev->bitmap_ops->reshape_mark(mddev, conf->reshape_safe,
> +						       conf->reshape_progress);
> +			mddev->bitmap_ops->unplug(mddev, true);
> +		}

[Severity: Critical]
Does checkpointing the translated llbitmap to disk before the MD superblock
is updated create a crash window that leads to silent data corruption on
restart?

During RAID5 reshape, the code translates the bitmap to the new layout via
reshape_mark() and flushes it to disk before asynchronously waking the MD
thread to update the superblock.

If the system crashes after the bitmap flush but before the superblock write
completes, the on-disk metadata will still point to the old reshape position
(reshape_safe), while the on-disk bitmap for the region 
[reshape_safe, reshape_progress] will be stored in the new layout.

On reboot, the array will resume the reshape from reshape_safe and
misinterpret the newly mapped bitmap bits as if they were in the old layout.
If the array is degraded during this restart, it might incorrectly assume the
parity for these sectors is in-sync and reconstruct missing data using
corrupted parity.

>  		mddev->reshape_position = conf->reshape_progress;
>  		mddev->curr_resync_completed = sector_nr;
>  		if (!mddev->reshape_backwards)

[ ... ]

> @@ -8571,6 +8585,12 @@ static int raid5_start_reshape(struct mddev *mddev)
>  			mdname(mddev));
>  		return -EINVAL;
>  	}
> +	if (md_bitmap_enabled(mddev, false) &&
> +	    mddev->bitmap_id == ID_LLBITMAP) {
> +		i = mddev->bitmap_ops->resize(mddev, mddev->dev_sectors, 0);
> +		if (i)
> +			return i;
> +	}

[Severity: Critical]
Can this resize operation trigger a use-after-free regression since it
occurs before raid5_quiesce() stops concurrent I/O?

In raid5_start_reshape(), mddev->bitmap_ops->resize() is called to prepare
the bitmap for the new geometry before raid5_quiesce() is invoked. This means
the array is not suspended, and concurrent I/O is actively being handled.

If the resize operation calls llbitmap_expand_pages() and frees the old
pctl array via kfree() without RCU synchronization, concurrent fast-path I/O
threads executing llbitmap_start_write() could locklessly dereference the
freed pointer array in llbitmap_raise_barrier().

>  
>  	atomic_set(&conf->reshape_stripes, 0);
>  	spin_lock_irq(&conf->device_lock);

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