Re: [PATCH v3 07/21] md/md-llbitmap: grow the page cache in place for reshape

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

New issues:
- [High] Use-After-Free on `llbitmap->pctl` array during bitmap resize.
- [High] Block IO recursion deadlock due to `GFP_KERNEL` allocation under `reconfig_mutex`.
- [High] Out-Of-Bounds array access during resize due to stale `chunkshift`.

Pre-existing issues:
- [High] Divide-by-zero crash in `llbitmap_calculate_chunks` on 32-bit systems due to `chunksize` overflow.
- [High] Use-After-Free timer race during `llbitmap_destroy`.
- [High] Out-of-Bounds disk write due to missing `BITMAP_DATA_OFFSET` in calculation.
--

--- Patch [7]: [PATCH v3 07/21] md/md-llbitmap: grow the page cache in place for reshape ---

commit 7e002f8cabd473784285f63600d75c1d7d7d30c4
Author: Yu Kuai [off-list ref]

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

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

[Severity: High]
This is a pre-existing issue, but does this calculation allow the on-disk
footprint to exceed the reserved space limit?

The check ensures *chunks does not exceed mddev->bitmap_info.space, but
the on-disk layout includes an additional 1024 bytes (BITMAP_DATA_OFFSET)
for the superblock before the raw bitmap state data.

When llbitmap_write_page() writes the tail chunks to disk, could it write
1024 bytes past the reserved bitmap region and corrupt adjacent data?

[Severity: High]
This is a pre-existing issue, but does this loop risk a divide by zero on
32-bit systems?

If *chunksize is 32 bits, and the array is extremely large with very small
reserved space, *chunksize could shift until it overflows to 0.

Would the subsequent call to DIV_ROUND_UP_SECTOR_T(blocks, 0) cause a crash
when the sysfs size is written?

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

[ ... ]

[Severity: High]
Can this allocation trigger a block IO recursion deadlock?

This is reached via array_size_store() which acquires mddev_lock()
(taking reconfig_mutex) without setting memalloc_noio_save() context.

Under memory pressure, GFP_KERNEL can enter direct reclaim and issue I/O
writeback to the MD array. If the array superblock needs updating,
md_write_start() waits on mddev->sb_wait for MD_SB_CHANGE_PENDING to clear.

Since the management thread must clear this flag but blocks trying to acquire
reconfig_mutex held by this resizing thread, does this result in a deadlock?

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

[Severity: High]
Could this cause a use-after-free if concurrent IO is happening?

The MD array does not suspend IO for sysfs-triggered resize operations.
The concurrent IO path accesses this array in llbitmap_start_write():

llbitmap_start_write()
    llbitmap_raise_barrier(llbitmap, page_start);

This happens without holding mddev->bitmap_info.mutex and without RCU
protection. If the IO path dereferences the array just after this kfree()
completes, will it trigger a panic?

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

[ ... ]

[ ... ]

+	if (reshape) {
+		llbitmap->chunks = max(old_chunks, chunks);
+	} else {

[Severity: High]
Does this cause an out-of-bounds array access during a reshape?

During reshape, this path updates llbitmap->chunks to a potentially larger
capacity but does not update llbitmap->chunkshift.

The active IO path continues to map array offsets to bitmap chunk indices
using the smaller old chunkshift:

llbitmap_start_write()
    start = offset >> llbitmap->chunkshift;
    ...
    page_start = (start + BITMAP_DATA_OFFSET) >> PAGE_SHIFT;

Because the old chunkshift maps to a larger number of total chunks than the
new chunks capacity (which was reduced by doubling bitmap_chunksize), could
an IO towards the end of the new array compute a page_start index that
exceeds the capacity of the newly reallocated llbitmap->pctl array?

+		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;
+		llbitmap->sync_size = blocks;
+		llbitmap_update_sb(llbitmap);
+	}
+	__llbitmap_flush(mddev);
+	mutex_unlock(&mddev->bitmap_info.mutex);
 	return 0;

[Severity: High]
This is a pre-existing issue, but is there a use-after-free race between
llbitmap_destroy() and the daemon work queue?

llbitmap_destroy() deletes the timer and then flushes the workqueue:

llbitmap_destroy()
    timer_delete_sync(&llbitmap->pending_timer);
    flush_workqueue(md_llbitmap_io_wq);

If md_llbitmap_daemon_fn() is actively executing in the workqueue while
teardown happens, the timer is not pending, so timer_delete_sync() returns
immediately.

The running daemon work then evaluates restart = true and re-arms the timer:

md_llbitmap_daemon_fn()
    if (restart)
        mod_timer(&llbitmap->pending_timer, ...);

Could flush_workqueue() complete, allowing llbitmap_destroy() to free the
llbitmap structure, while the re-armed timer later fires and accesses the
freed memory?

+
+out:
+	mutex_unlock(&mddev->bitmap_info.mutex);
+	return ret;
 }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1785206690.git.yukuai@fygo.io?part=7