Re: [PATCH v2 06/20] md/md-llbitmap: grow the page cache in place for reshape

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

New issues:
- [High] The `LLPageFlush` bit on existing cached pages is set during `llbitmap_flush()` but never cleared, permanently bypassing the barrier idle expiration timer and causing severe metadata write amplification.
- [High] Reallocating `llbitmap->pctl` locklessly in place causes a Use-After-Free for concurrent I/O paths and the background daemon.

Pre-existing issues:
- [High] An integer overflow in chunk size calculation on 32-bit systems causes a fatal divide-by-zero for extremely large arrays.
--

commit 06efa66cdd24523c6bea3b2de073842d179f3506
Author: Yu Kuai <[email protected]>

md/md-llbitmap: grow the page cache in place for reshape

Use the page-control helpers to grow llbitmap's cached pages in place
for resize and later reshape preparation, instead of rebuilding the
whole cache.

> diff --git a/drivers/md/md-llbitmap.c b/drivers/md/md-llbitmap.c
> index 4c3fc4b35aff0..98e7824c7362e 100644
> --- a/drivers/md/md-llbitmap.c
> +++ b/drivers/md/md-llbitmap.c
> @@ -416,6 +416,19 @@ static char state_machine[BitStateCount][BitmapActionCount] = {
>  };
>  
>  static void __llbitmap_flush(struct mddev *mddev);
> +static void llbitmap_flush(struct mddev *mddev);
> +static void llbitmap_update_sb(void *data);
> +
> +static void llbitmap_calculate_chunks(struct mddev *mddev, sector_t blocks,
> +				      unsigned long *chunksize,
> +				      unsigned long *chunks)
> +{
> +	*chunks = DIV_ROUND_UP_SECTOR_T(blocks, *chunksize);
> +	while (*chunks > mddev->bitmap_info.space << SECTOR_SHIFT) {
> +		*chunksize = *chunksize << 1;
> +		*chunks = DIV_ROUND_UP_SECTOR_T(blocks, *chunksize);

[Severity: High]
This isn't a bug introduced by this patch, but since `chunksize` is an
`unsigned long` (32 bits on 32-bit architectures) and `blocks` is a 64-bit
`sector_t`, could the left shift push `*chunksize` past 31 bits and overflow
it to 0 for extremely large arrays?

If `*chunksize` becomes 0, does this cause a divide-by-zero panic in the
`DIV_ROUND_UP_SECTOR_T()` macro?

> +	}
> +}

[ ... ]

> +static int llbitmap_expand_pages(struct llbitmap *llbitmap,
> +				 unsigned long chunks)
> +{
> +	struct llbitmap_page_ctl **pctl;
> +	unsigned int old_nr_pages = llbitmap->nr_pages;
> +	unsigned int nr_pages = llbitmap_used_pages(llbitmap, chunks);
> +	unsigned int i;
> +	int ret;
> +
> +	if (nr_pages <= old_nr_pages)
> +		return 0;
> +
> +	pctl = kcalloc(nr_pages, sizeof(*pctl), GFP_KERNEL);
> +	if (!pctl)
> +		return -ENOMEM;
> +
> +	if (llbitmap->pctl)
> +		memcpy(pctl, llbitmap->pctl,
> +		       array_size(old_nr_pages, sizeof(*pctl)));
> +
> +	for (i = old_nr_pages; i < nr_pages; i++) {
> +		pctl[i] = llbitmap_alloc_page_ctl(llbitmap, i);
> +		if (IS_ERR(pctl[i]))
> +			goto err_alloc_ptr;
> +	}
> +
> +	kfree(llbitmap->pctl);
> +	llbitmap->pctl = pctl;
> +	llbitmap->nr_pages = nr_pages;

[Severity: High]
Is it safe to reallocate and update `llbitmap->pctl` and `nr_pages` locklessly
in place like this while the array is processing I/O?

`llbitmap_resize()` holds only `mddev->bitmap_info.mutex`, but concurrent
lockless readers like `md_llbitmap_daemon_fn()` and the I/O fastpath
`llbitmap_start_write()` dereference `llbitmap->pctl[idx]`.

Could this lead to a Use-After-Free if a concurrent reader accesses the old
`llbitmap->pctl` array just as it gets freed?

> +	return 0;
> +
> +err_alloc_ptr:
> +	ret = PTR_ERR(pctl[i]);

[ ... ]

> +static int llbitmap_prepare_resize(struct llbitmap *llbitmap,
> +				   unsigned long old_chunks,
> +				   unsigned long new_chunks,
> +				   unsigned long cache_chunks)
> +{
> +	int ret;
> +
> +	llbitmap_flush(llbitmap->mddev);
> +	ret = llbitmap_expand_pages(llbitmap, cache_chunks);

[Severity: High]
Does this sequence cause the daemon thread to bypass the barrier idle timer and
create severe metadata write amplification?

`llbitmap_flush()` sets the `LLPageFlush` bit on all existing pages. Since
`llbitmap_expand_pages()` reuses the existing `pctl` structures, the active
pages are retained with the `LLPageFlush` bit still set.

Since this bit is never cleared, the check in `md_llbitmap_daemon_fn()`:

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

will permanently fail for these pages, forcing the daemon to immediately flush
them and skip re-arming the pending timer.

> +	if (ret)
> +		return ret;
> +	if (new_chunks > old_chunks)
> +		llbitmap_mark_range(llbitmap, old_chunks, new_chunks - 1,
> +				    BitUnwritten);
> +	return 0;
> +}

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