Re: [PATCH v5 15/29] md/md-llbitmap: grow the page cache in place for reshape

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Mismatched chunkshift and page allocations during reshape cause out-of-bounds dereferences.
- [High] Concurrent access to `llbitmap->pctl` during array resize leads to Use-After-Free.
- [Medium] Missing `cond_resched()` in `llbitmap_mark_range` loop can cause soft lockups during large array resizing.

Pre-existing issues:
- [Critical] Bitmap metadata writes can overflow the reserved bitmap space on disk because it ignores `BITMAP_DATA_OFFSET`.
--

--- Patch [15]: [PATCH v5 15/29] md/md-llbitmap: grow the page cache in place for reshape ---
commit 73a7214b36a3cd36e140de64fe3e4cd6cfe6795a
Author: Yu Kuai <yukuai@fygo.io>

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 300dd8b93b01e..ddeea20989873 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) {

[Severity: Critical]
This is a pre-existing issue, but does this calculation allow the metadata
writes to overflow the reserved disk space? It calculates the required chunks
without reserving space for BITMAP_DATA_OFFSET, which could allow the required
size to silently exceed the reserved disk space. If the array size forces the
chunk calculation to be exactly at or near the reserved space limit, could
used_pages exceed the reserved limit and overwrite sectors past the end of the
reserved space during a flush?

> +		*chunksize = *chunksize << 1;
> +		*chunks = DIV_ROUND_UP_SECTOR_T(blocks, *chunksize);
> +	}
> +}

[ ... ]

> @@ -655,6 +668,48 @@ static unsigned int llbitmap_reserved_pages(struct llbitmap *llbitmap)
>  			    PAGE_SIZE);
>  }
>  
> +static int llbitmap_expand_pages(struct llbitmap *llbitmap,
> +				 unsigned long chunks)
> +{
> +	struct llbitmap_page_ctl **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;

[Severity: High]
Can this sequence create a use-after-free window? If the daemon timer had
previously fired while the flush work was running and set BITMAP_DAEMON_BUSY,
the daemon could re-arm pending_timer and exit. Could the timer then fire
concurrently while llbitmap->pctl is being freed here, causing the daemon to
wake up and dereference the freed pctl array?

> +	llbitmap->nr_pages = nr_pages;
> +	return 0;

[ ... ]

> @@ -730,6 +785,34 @@ static bool llbitmap_zero_all_disks(struct llbitmap *llbitmap)
>  	return true;
>  }
>  
> +static void llbitmap_mark_range(struct llbitmap *llbitmap,
> +				unsigned long start,
> +				unsigned long end,
> +				enum llbitmap_state state)
> +{
> +	while (start <= end) {
> +		llbitmap_write(llbitmap, state, start);
> +		start++;
> +	}
> +}

[Severity: Medium]
If the array is resized significantly, the difference between start and end
could be hundreds of millions of chunks. Without a cond_resched() in this loop,
could this monopolize the CPU for an extended period and trigger a soft lockup?

[ ... ]

> @@ -1184,24 +1267,62 @@ static int llbitmap_create(struct mddev *mddev)
>  static int llbitmap_resize(struct mddev *mddev, sector_t blocks, int chunksize)
>  {
>  	struct llbitmap *llbitmap = mddev->bitmap;
[ ... ]
> +	mutex_lock(&mddev->bitmap_info.mutex);
> +	cache_chunks = reshape ? max(old_chunks, chunks) : chunks;
> +	ret = llbitmap_prepare_resize(llbitmap, old_chunks, chunks, cache_chunks);
> +	if (ret)
> +		goto out;
> +
> +	if (reshape) {
> +		llbitmap->chunks = max(old_chunks, chunks);
> +	} else {

[Severity: Critical]
Does the reshape path here fail to update the chunkshift and chunksize?
It expands the page cache based on the new chunks count, but doesn't seem to
update llbitmap->chunksize and llbitmap->chunkshift. When subsequent I/O
operations are processed, could they calculate their chunk index using the old,
smaller shift, resulting in chunk indices that exceed the newly allocated
pages? Would this lead to an out-of-bounds access on llbitmap->pctl?

> +		if (blocks < old_blocks && chunks < old_chunks)
> +			llbitmap_mark_range(llbitmap, chunks, old_chunks - 1,
> +					    BitUnwritten);
> +		mddev->bitmap_info.chunksize = bitmap_chunksize;
> +		llbitmap->chunks = chunks;

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