Re: [PATCH v2 07/20] md/md-llbitmap: track target reshape geometry fields

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Incomplete teardown of the `pending_timer` and `daemon_work` cycle in `llbitmap_destroy` leads to a Use-After-Free.
- [High] Concurrent array resize causes Use-After-Free on the `llbitmap->pctl` array due to lack of I/O suspension.
--

commit 2a89e33f120722b7687d6b24f4bf95273fa7a498
Author: Yu Kuai <yukuai@fygo.io>

md/md-llbitmap: track target reshape geometry fields

Track llbitmap bookkeeping for the target reshape geometry while keeping
a single live bitmap instance.

Add the reshape geometry fields, refresh helper, and update the load and
resize paths to keep the target geometry in sync.

> diff --git a/drivers/md/md-llbitmap.c b/drivers/md/md-llbitmap.c
> index 98e7824c7362e..cdced2126e975 100644
> --- a/drivers/md/md-llbitmap.c
> +++ b/drivers/md/md-llbitmap.c
> @@ -289,6 +289,9 @@ struct llbitmap {
>  	unsigned long chunks;
>  	/* total number of sectors tracked by current bitmap geometry */
>  	sector_t sync_size;
> +	unsigned long reshape_chunksize;
> +	unsigned long reshape_chunks;
> +	sector_t reshape_sync_size;
>  	unsigned long last_end_sync;
>  	/*
>  	 * time in seconds that dirty bits will be cleared if the page is not

[Severity: High]
This isn't a bug introduced by this patch, but is there a use-after-free risk
during llbitmap teardown?

If a user removes the bitmap via the location sysfs node,
llbitmap_destroy() deletes the pending_timer and flushes the I/O workqueue:

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

However, it doesn't seem to set a shutdown flag to prevent re-arming.
If the daemon work is executing and finds unexpired dirty pages, it will set
restart = true and unconditionally re-arm the timer:

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

Once the work finishes, the flush_workqueue() call in llbitmap_destroy()
returns and the llbitmap structure is freed. Can the re-armed timer then
fire on the freed llbitmap structure?

[Severity: High]
This is a pre-existing issue, but can concurrent array resizing cause a
use-after-free on the llbitmap->pctl array?

When expanding the array size via sysfs, update_size() invokes the
personality resize hook without suspending the array:

drivers/md/md.c:update_size() {
    ...
    rv = mddev->pers->resize(mddev, num_sectors);
    ...
}

This eventually calls llbitmap_expand_pages(), which allocates a new
pointer array and immediately frees the old one:

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

Since update_size() only holds the mddev_lock and does not drain in-flight
I/O, new I/O can still be issued.

Could a concurrent I/O path calling llbitmap_raise_barrier() fetch the old
llbitmap->pctl array pointer just before it is freed:

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

and then dereference it, accessing freed memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1782282042.git.yukuai@kernel.org?part=7