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

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 [off-list ref]

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.

[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);
+	}
+}

[ ... ]

[ ... ]

+	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;

[ ... ]

[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?

[ ... ]

[ ... ]

+	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