Re: [PATCH v2 11/20] md/md-llbitmap: don't skip reshape ranges from bitmap state

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

Pre-existing issues:
- [Critical] Use-After-Free (UAF) of `llbitmap` memory during dynamic bitmap unregistration.
- [High] Signed integer overflow in `llbitmap_skip_sync_blocks` can cause premature resync termination.
--

commit 008d5398fa7aa1473594e503af861f03ef4f2548
Author: Yu Kuai <[email protected]>

md/md-llbitmap: don't skip reshape ranges from bitmap state

Reshape progress is tracked by array metadata rather than llbitmap.
Do not let llbitmap skip_sync_blocks() suppress reshape ranges based on
stale bitmap state before the corresponding checkpoint is persisted.

> diff --git a/drivers/md/md-llbitmap.c b/drivers/md/md-llbitmap.c
> index d27993cfb13da..c62397743d9a0 100644
> --- a/drivers/md/md-llbitmap.c
> +++ b/drivers/md/md-llbitmap.c
> @@ -1647,6 +1647,14 @@ static sector_t llbitmap_skip_sync_blocks(struct mddev *mddev, sector_t offset)
>  		return 0;
>  	c = llbitmap_read(llbitmap, p);
>  
> +	/*
> +	 * Reshape progress is tracked by array metadata rather than llbitmap.
> +	 * Skipping reshape ranges from stale bitmap state can lose data after a
> +	 * restart before the corresponding bits are checkpointed to disk.
> +	 */
> +	if (test_bit(MD_RECOVERY_RESHAPE, &mddev->recovery))
> +		return 0;
> +
>  	/* always skip unwritten blocks */
>  	if (c == BitUnwritten)
>  		return blocks;

[Severity: High]
This is a pre-existing issue, but since we are modifying this function, is
there a risk of signed integer overflow when returning blocks here?

If the array is extremely large and reserved bitmap space is small,
llbitmap->chunksize can be doubled during initialization until it exceeds
INT_MAX.

Because blocks is calculated earlier in this function as an int:

    int blocks = llbitmap->chunksize - (offset & (llbitmap->chunksize - 1));

If chunksize exceeds INT_MAX, blocks could become negative.

When this negative value is implicitly cast to the sector_t (unsigned 64-bit)
return type, it becomes a massive positive number.

Could this cause md_do_sync() to jump to the end of the array and
prematurely abort the synchronization?

Would it be safer to change the type of blocks to unsigned long or sector_t?


[Severity: Critical]
This isn't a bug introduced by this patch, but looking at the teardown
sequence elsewhere in this file for dynamic bitmap unregistration, does
llbitmap_destroy() have a use-after-free vulnerability?

When clearing the bitmap at runtime, llbitmap_destroy() deletes the timer
and flushes the workqueues without setting the LLPageFlush flag:

    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 worker (md_llbitmap_daemon_fn) is running on the workqueue,
timer_delete_sync() might do nothing if the timer already fired. The
flush_workqueue() call then waits for the worker to finish.

Inside the worker, because LLPageFlush is not set and idle pages have an
expire of LONG_MAX, it sets restart = true and re-arms the timer:

    if (!test_bit(LLPageFlush, &pctl->flags) &&
        time_before(jiffies, pctl->expire)) {
        restart = true;
        continue;
    }
    ...
    if (restart)
        mod_timer(&llbitmap->pending_timer, ...);

Once the worker finishes, flush_workqueue() returns and llbitmap is freed.
Does this leave the timer armed on freed memory, potentially causing memory
corruption when it fires?

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