Re: [PATCH v4 17/25] md/md-llbitmap: remap checkpointed bits as reshape progresses

[sashiko-bot@kernel.org]

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

New issues:
- [High] Data race and potential bitmap state corruption due to missing `reshape_lock` acquisitions.
- [High] Incomplete feature implementation causing unreachable reshape checkpointing (dead code).
- [High] `llbitmap_rmerge_state` incorrectly drops 'Unwritten' sync state extensions, causing array inconsistency.

Pre-existing issues:
- [Critical] Pre-existing Use-After-Free in llbitmap teardown due to daemon_work re-arming pending_timer.
--

--- Patch [17]: [PATCH v4 17/25] md/md-llbitmap: remap checkpointed bits as reshape progresses ---
commit 51b23c29cf446cf378d49a002b1a356940be2ccd
Author: Yu Kuai <yukuai@fygo.io>

md/md-llbitmap: remap checkpointed bits as reshape progresses

This patch introduces logic to remap checkpointed old llbitmap state forward
as reshape_position advances. It records the checkpoint remap through
reshape_mark() and adds a reshape_lock to serialize reshape checkpoint
remapping against normal I/O bitmap updates.

> 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

[ ... ]

> @@ -930,6 +943,33 @@ static int llbitmap_prepare_resize(struct llbitmap *llbitmap,
>  	return 0;
>  }
>  
> +static enum llbitmap_state
> +llbitmap_rmerge_state(struct llbitmap *llbitmap,
> +		      enum llbitmap_state dst,
> +		      enum llbitmap_state src)
> +{
> +	bool level_456 = raid_is_456(llbitmap->mddev);
> +
> +	if (dst == BitNeedSync || dst == BitSyncing ||
> +	    src == BitNeedSync || src == BitSyncing)
> +		return BitNeedSync;
> +
> +	if (dst == BitDirty || src == BitDirty)
> +		return BitDirty;
> +
> +	/*
> +	 * Reshape generates valid target parity/data for both already-written
> +	 * and not-yet-written regions in the checkpointed range, so a mix of
> +	 * clean and unwritten still results in a clean destination bit.
> +	 */
> +	if (level_456 && ((dst == BitClean && src == BitUnwritten) ||
> +			  (src == BitClean && dst == BitUnwritten)))
> +		return BitClean;
> +	if (dst == BitClean || src == BitClean)
> +		return BitClean;
> +	return BitUnwritten;
> +}

[Severity: High]
Does this switch logic drop the proactive sync extensions like
BitNeedSyncUnwritten, BitSyncingUnwritten, and BitCleanUnwritten?

If either dst or src holds one of these states, it fails the explicit equality
checks and falls through to the default BitUnwritten. Could this cause the
array to lose its proactive sync requirement or pre-built parity state during a
reshape checkpoint merge?

[ ... ]

> @@ -1294,6 +1334,7 @@ static void md_llbitmap_daemon_fn(struct work_struct *work)
>  
>  	if (llbitmap->mddev->degraded)
>  		return;
> +
>  retry:
>  	start = 0;
>  	end = min(llbitmap->chunks, PAGE_SIZE - BITMAP_DATA_OFFSET) - 1;

[Severity: Critical]
This is a pre-existing issue, but does the teardown path have a use-after-free
involving pending_timer?

In llbitmap_destroy(), timer_delete_sync() is called followed by
flush_workqueue(md_llbitmap_io_wq). If md_llbitmap_daemon_fn() is running,
flush_workqueue() will wait for it. However, md_llbitmap_daemon_fn() blindly
re-arms the timer at the end if there are unexpired dirty pages:

md_llbitmap_daemon_fn() {
    ...
    if (restart)
        mod_timer(&llbitmap->pending_timer, ...);
}

Could this cause the timer to fire after the llbitmap struct is freed, since
it re-arms after the teardown path has already deleted it?

[ ... ]

> @@ -1520,7 +1562,9 @@ static void llbitmap_start_write(struct mddev *mddev, sector_t offset,
>  		page_start++;
>  	}
>  
> +	read_lock(&llbitmap->reshape_lock);
>  	llbitmap_state_machine(llbitmap, start, end, BitmapActionStartwrite);
> +	read_unlock(&llbitmap->reshape_lock);
>  }

[Severity: High]
Does this lock need to be acquired in other paths that call
llbitmap_state_machine() as well?

The reshape_lock is taken here to serialize against llbitmap_reshape_mark(),
but paths like md_llbitmap_daemon_fn() and llbitmap_start_sync() appear to
modify the state bytes without acquiring reshape_lock.

Since llbitmap_reshape_mark() bypasses page suspension, could this allow
concurrent execution with the daemon or sync threads, leading to
read-modify-write data races on the bitmap state bytes?

[ ... ]

> @@ -1873,6 +2049,33 @@ static void llbitmap_reshape_finish(struct mddev *mddev)
>  		mddev->pers->quiesce(mddev, 0);
>  }
>  
> +static void llbitmap_reshape_mark(struct mddev *mddev, sector_t old_pos,
> +				  sector_t new_pos)
> +{

[Severity: High]
Is llbitmap_reshape_mark() ever invoked by the md subsystem during a reshape
operation?

A search through the tree doesn't show any callers invoking
mddev->bitmap_ops->reshape_mark(). If this callback is never executed, won't
the reshape checkpoint remapping fail to run and leave the bitmap in a stale
state?

> +	struct llbitmap *llbitmap = mddev->bitmap;
> +	sector_t lo;
> +	sector_t hi;

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