Re: [PATCH v4 24/25] md/raid5: wire llbitmap reshape lifecycle

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] raid5_start_reshape fails to invoke the reshape_can_start bitmap operation before starting the reshape process.
- [High] raid5_finish_reshape unconditionally sets mddev->resync_offset = 0 when llbitmap is enabled, forcing an immediate full array parity rebuild.

Pre-existing issues:
- [High] The llbitmap daemon work and pending timer can mutually re-arm each other during llbitmap_destroy(), leading to a Use-After-Free.
--

--- Patch [24]: [PATCH v4 24/25] md/raid5: wire llbitmap reshape lifecycle ---
commit 5324dd2fc19c643561ddccf4575506b5ce251538
Author: Yu Kuai <yukuai@fygo.io>

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 5697570120da9..896aa522f446c 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c

[ ... ]

> @@ -8579,6 +8593,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);

[Severity: High]
Does this code need to call the reshape_can_start() bitmap operation before
proceeding with the resize?

If a reshape begins while the llbitmap is actively tracking out-of-sync
chunks (BitNeedSync or BitSyncing), couldn't the reshape incorrectly map the
sync state to the new geometry? 

This might lead to a loss of sync tracking and potential data regressions.

> +		if (i)
> +			return i;
> +	}
>  
>  	atomic_set(&conf->reshape_stripes, 0);

[ ... ]

> @@ -8693,8 +8722,16 @@ static void raid5_finish_reshape(struct mddev *mddev)
>  {
>  	struct r5conf *conf = mddev->private;
>  	struct md_rdev *rdev;
> +	bool llbitmap = mddev->bitmap_id == ID_LLBITMAP &&
> +		md_bitmap_enabled(mddev, false);
>  
>  	if (!test_bit(MD_RECOVERY_INTR, &mddev->recovery)) {
> +		if (llbitmap && mddev->bitmap_ops->reshape_finish)
> +			mddev->bitmap_ops->reshape_finish(mddev);
> +		if (llbitmap) {
> +			mddev->resync_offset = 0;

[Severity: High]
Does setting mddev->resync_offset to 0 here force an immediate full array
parity rebuild?

By unconditionally resetting the resync offset, the MD core might be
instructed that the entire array is out-of-sync upon reshape completion.

This would force a full resync of all sectors, despite the reshape correctly
remapping in-sync sectors, and cause performance regressions.

> +			mddev->resync_max_sectors = mddev->dev_sectors;
> +		}
>  
>  		if (mddev->delta_disks <= 0) {


As a related note regarding llbitmap teardown:

[Severity: High]
This isn't a bug introduced by this patch, but can the llbitmap daemon work
and pending timer mutually re-arm each other during llbitmap_destroy(),
leading to a use-after-free regression?

Looking at drivers/md/md-llbitmap.c:llbitmap_destroy():

    timer_delete_sync(&llbitmap->pending_timer);
    flush_workqueue(md_llbitmap_io_wq);

If the daemon_work is queued, the flush executes md_llbitmap_daemon_fn().

If there are unexpired dirty pages inside the work function, it will
unconditionally re-arm the timer via mod_timer().

Since there is no cancel_work_sync() or subsequent timer deletion, could the
newly armed timer fire after kfree(llbitmap) completes?

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