Re: [PATCH v3 09/21] md/md-llbitmap: finish reshape geometry

[sashiko-bot@kernel.org]

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

New issues:
- [High] The newly implemented `.reshape_finish` callback is never called by the md core, leaving the staged geometry uncommitted at runtime.
- [High] Concurrent array resize causes Use-After-Free of `llbitmap->pctl`.

Pre-existing issues:
- [High] Teardown symmetry violation in `llbitmap_destroy` leading to UAF on `llbitmap`.
- [High] Permanent crippling of the bitmap daemon expiration logic after any flush operation.
--

--- Patch [9]: [PATCH v3 09/21] md/md-llbitmap: finish reshape geometry ---
commit 83899f955ca5cb817105e296dabf1a5632ece1df
Author: Yu Kuai <yukuai@fygo.io>

md/md-llbitmap: finish reshape geometry
    
When assembling a stopped reshape, md_run() creates the bitmap before
publishing mddev->pers. This commit refreshes the staged reshape geometry
again from llbitmap_load() after mddev->pers is available, and expands
the in-memory page controls before replaying bitmap state.

Link: https://lore.kernel.org/all/20260726185916.2223460-1-mykola@meshstor.io/

> diff --git a/drivers/md/md-llbitmap.c b/drivers/md/md-llbitmap.c
> index cdced2126e975..55ab2a8a11e5f 100644
> --- a/drivers/md/md-llbitmap.c
> +++ b/drivers/md/md-llbitmap.c
> @@ -1349,11 +1349,20 @@ static int llbitmap_load(struct mddev *mddev)
>  {
>  	enum llbitmap_action action = BitmapActionReload;
>  	struct llbitmap *llbitmap = mddev->bitmap;
> +	int ret;
>  
>  	if (test_and_clear_bit(BITMAP_STALE, &llbitmap->flags))
>  		action = BitmapActionStale;
>  
> +	mutex_lock(&mddev->bitmap_info.mutex);
> +	llbitmap_refresh_reshape(llbitmap);
> +	ret = llbitmap_expand_pages(llbitmap, llbitmap->chunks);

[Severity: High]
Can this concurrent array resize cause a use-after-free of the page control
array?

If llbitmap_expand_pages() frees the active pctl array while replacing it
with a new one:

drivers/md/md-llbitmap.c:llbitmap_expand_pages() {
    ...
    kfree(llbitmap->pctl);
    llbitmap->pctl = pctl;
    ...
}

Normal read/write I/O paths like llbitmap_start_write() call
llbitmap_raise_barrier(), which locklessly dereferences the array:

drivers/md/md-llbitmap.c:llbitmap_raise_barrier() {
    ...
    struct llbitmap_page_ctl *pctl = llbitmap->pctl[page_idx];
    ...
}

Does this require synchronization, such as quiesce or RCU, to safely update
the array without causing a use-after-free during concurrent I/O?

> +	if (ret) {
> +		mutex_unlock(&mddev->bitmap_info.mutex);
> +		return ret;
> +	}
>  	llbitmap_state_machine(llbitmap, 0, llbitmap->chunks - 1, action);
> +	mutex_unlock(&mddev->bitmap_info.mutex);
>  	return 0;
>  }
>  

[ ... ]

> @@ -1680,6 +1689,30 @@ static void llbitmap_dirty_bits(struct mddev *mddev, unsigned long s,
>  	llbitmap_state_machine(mddev->bitmap, s, e, BitmapActionStartwrite);
>  }
>  
> +static void llbitmap_reshape_finish(struct mddev *mddev)
> +{
> +	struct llbitmap *llbitmap = mddev->bitmap;
> +
> +	if (mddev->pers->quiesce)
> +		mddev->pers->quiesce(mddev, 1);
> +
> +	mutex_lock(&mddev->bitmap_info.mutex);
> +	llbitmap_flush(mddev);

[Severity: High]
This is a pre-existing issue, but does calling llbitmap_flush() permanently
cripple the bitmap daemon's expiration logic?

llbitmap_flush() sets the LLPageFlush flag on all page control structures to
force immediate flushing:

drivers/md/md-llbitmap.c:llbitmap_flush() {
    ...
    for (i = 0; i < llbitmap->nr_pages; i++)
        set_bit(LLPageFlush, &llbitmap->pctl[i]->flags);
    ...
}

The daemon skips the expiration check if this flag is set:

drivers/md/md-llbitmap.c:md_llbitmap_daemon_fn() {
    ...
    if (!test_bit(LLPageFlush, &pctl->flags) &&
        time_before(jiffies, pctl->expire)) {
        restart = true;
        continue;
    ...
}

Since there doesn't seem to be a corresponding mechanism to ever clear the
LLPageFlush flag, won't all subsequent I/O bypass write batching and flush
immediately?

> +
> +	llbitmap->chunksize = llbitmap->reshape_chunksize;
> +	llbitmap->chunkshift = ffz(~llbitmap->chunksize);
> +	llbitmap->chunks = llbitmap->reshape_chunks;
> +	llbitmap->sync_size = llbitmap->reshape_sync_size;
> +	llbitmap_refresh_reshape(llbitmap);
> +	mddev->bitmap_info.chunksize = llbitmap->chunksize;
> +	llbitmap_update_sb(llbitmap);
> +	__llbitmap_flush(mddev);
> +	mutex_unlock(&mddev->bitmap_info.mutex);
> +
> +	if (mddev->pers->quiesce)
> +		mddev->pers->quiesce(mddev, 0);
> +}
> +

[ ... ]

> @@ -1977,6 +2010,7 @@ static struct bitmap_operations llbitmap_ops = {
>  	.get_stats		= llbitmap_get_stats,
>  	.dirty_bits		= llbitmap_dirty_bits,
>  	.prepare_range		= llbitmap_prepare_range,
> +	.reshape_finish		= llbitmap_reshape_finish,

[Severity: High]
Is the newly added .reshape_finish callback ever invoked by the md core?

Without a caller in the md core, it appears the staged reshape geometry
remains uncommitted at runtime, which would leave the active geometry stale.
Can this result in incorrect bitmap behavior or data corruption during or
after a reshape?

>  	.write_all		= llbitmap_write_all,
>  
>  	.groups			= md_llbitmap_groups,

As a side note, I noticed another pre-existing issue in the teardown path:

[Severity: High]
This is a pre-existing issue, but does llbitmap_destroy() have a teardown
symmetry violation that could lead to a use-after-free on the llbitmap
structure?

When stopping an array, llbitmap_destroy() deletes the timer and flushes the
workqueue without setting a termination flag:

drivers/md/md-llbitmap.c:llbitmap_destroy() {
    ...
    timer_delete_sync(&llbitmap->pending_timer);
    flush_workqueue(md_llbitmap_io_wq);
    flush_workqueue(md_llbitmap_unplug_wq);

    mddev->bitmap = NULL;
    llbitmap_free_pages(llbitmap);
    kfree(llbitmap);
}

If the daemon work item executes on the workqueue, it can re-arm the timer if
there are unexpired dirty pages:

drivers/md/md-llbitmap.c:md_llbitmap_daemon_fn() {
    ...
    if (restart)
        mod_timer(&llbitmap->pending_timer,
                  jiffies + llbitmap->mddev->bitmap_info.daemon_sleep * HZ);
    ...
}

Because md_llbitmap_daemon_fn() doesn't check for a teardown flag, couldn't
the timer be re-armed, and then fire later on the freed llbitmap memory after
llbitmap_destroy() completes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1785206690.git.yukuai@fygo.io?part=9