Re: [PATCH v5 02/29] md/md-llbitmap: use GFP_NOIO for cache allocations

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] `llbitmap_resize()` recalculates capacity limits but fails to reallocate the `pctl` array, causing out-of-bounds pointer dereferences during subsequent operations.
- [High] `blkdev_issue_zeroout()` is called with `GFP_KERNEL` while `mddev_lock()` is held, risking a direct reclaim deadlock.
- [High] The background timer and workqueue can re-arm each other during teardown, leading to a Use-After-Free of the `llbitmap` struct.
--

--- Patch [2]: [PATCH v5 02/29] md/md-llbitmap: use GFP_NOIO for cache allocations ---
commit 0ace115175e6f1ed40865ea8b812e7ca53aadaed
Author: Yu Kuai <yukuai@fygo.io>

md/md-llbitmap: use GFP_NOIO for cache allocations

This commit changes the memory allocation flags from GFP_KERNEL to GFP_NOIO for
the llbitmap object and its related structures. This change prevents direct
memory reclaim from triggering block I/O while MD resize state locks are held,
avoiding potential deadlocks.

> llbitmap allocates its in-memory page cache and page-control structures from
> paths that can already be holding MD reconfiguration or bitmap state locks.
> For example, component_size_store() takes mddev_lock(), update_size() calls
> the personality resize method, and llbitmap_resize() can grow the page cache
> through llbitmap_prepare_resize().

[Severity: High]
This is a pre-existing issue, but the commit message claims that
llbitmap_resize() grows the page cache through llbitmap_prepare_resize().

Looking at llbitmap_resize():

llbitmap_resize(struct mddev *mddev, sector_t blocks, int chunksize)
{
	...
	llbitmap->chunks = chunks;

	return 0;
}

The function updates the capacity limits but does not update llbitmap->nr_pages
or reallocate the llbitmap->pctl array. The referenced llbitmap_prepare_resize()
function does not seem to exist.

Because the pctl array is not reallocated, won't subsequent I/O operations call
llbitmap_write() with an index that exceeds the original nr_pages length?

llbitmap_write() calculates the index:

	idx = pos >> PAGE_SHIFT;
	bit = offset_in_page(pos);

	llbitmap->pctl[idx]->state[bit] = state;

Could this lead to an out-of-bounds pointer dereference when accessing the new
chunks limit?

> Use GFP_NOIO for the llbitmap object, cached bitmap pages, page controls,
> page-control arrays, and percpu_ref initialization. Leave the explicit
> metadata zeroout path unchanged because it is intentional bitmap I/O rather
> than reclaim-driven allocation.

[Severity: High]
This isn't a bug introduced by this patch, but the commit message explicitly
notes that the metadata zeroout path was left unchanged.

Looking at llbitmap_zero_all_disks(), which is called during array
initialization or reconfiguration with mddev_lock() held:

	ret = blkdev_issue_zeroout(rdev->bdev,
				   rdev->data_offset,
				   dev_sectors,
				   GFP_KERNEL, 0);

While the I/O itself is intentional, blkdev_issue_zeroout() uses the provided
GFP flags internally to allocate the bio and payload pages. If memory is tight,
won't GFP_KERNEL enter direct reclaim, potentially submitting a bio back to the
same mddev and causing a deadlock?

Should this also be updated to use GFP_NOIO?

> diff --git a/drivers/md/md-llbitmap.c b/drivers/md/md-llbitmap.c
> index 71e9a21b98b2e..3cd8373bc9b22 100644
> --- a/drivers/md/md-llbitmap.c
> +++ b/drivers/md/md-llbitmap.c

[ ... ]

[Severity: High]
This is a pre-existing issue, but there appears to be a resource lifecycle
regression during teardown.

Looking at llbitmap_destroy():

	timer_delete_sync(&llbitmap->pending_timer);
	flush_workqueue(md_llbitmap_io_wq);
	...
	kfree(llbitmap);

If the timer had already fired and queued the daemon_work, timer_delete_sync()
returns immediately. flush_workqueue() then blocks waiting for the work item to
execute.

Inside md_llbitmap_daemon_fn():

	if (restart)
		mod_timer(&llbitmap->pending_timer,
			  jiffies + llbitmap->mddev->bitmap_info.daemon_sleep * HZ);

If unexpired dirty pages are found, it sets restart = true and unconditionally
calls mod_timer(), re-arming the timer while the subsystem is undergoing
destruction.

After flush_workqueue() finishes, llbitmap_destroy() frees the llbitmap object.

When the newly re-armed timer fires, won't it access the freed object?

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