Re: [PATCH v4 02/25] md/md-llbitmap: use GFP_NOIO for cache allocations

[sashiko-bot@kernel.org]

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

New issues:
- [High] Large contiguous memory allocation failure using `kmalloc_array` and `GFP_NOIO`.

Pre-existing issues:
- [Critical] Unprotected read-modify-write race condition corrupts lockless bitmap state.
- [High] Out-of-bounds array access due to missing page cache reallocation during array resize.
- [High] Use-After-Free of `llbitmap` in the `llbitmap_create` error path.
--

--- Patch [2]: [PATCH v4 02/25] md/md-llbitmap: use GFP_NOIO for cache allocations ---
commit e93c6e98fd387cb6b6c743dfb60b192e52bca4d1
Author: Yu Kuai <yukuai@fygo.io>

md/md-llbitmap: use GFP_NOIO for cache allocations

This commit changes the allocation context from GFP_KERNEL to GFP_NOIO
for several llbitmap cache and object structures. This is done to prevent
direct reclaim from entering filesystem or block I/O while MD resize state
is locked, avoiding recursive deadlocks.

> update_size() calls the personality resize method, and llbitmap_resize()
> can grow the page cache through llbitmap_prepare_resize().

[Severity: High]
This isn't a bug introduced by this patch, but the commit message mentions
llbitmap_prepare_resize(), which appears to be entirely missing from the
code.

In llbitmap_resize(), llbitmap->chunks is increased, but the underlying
page cache arrays (pctl) are never actually reallocated to match the new
capacity:

drivers/md/md-llbitmap.c:llbitmap_resize() {
    ...
	llbitmap->chunkshift = ffz(~chunksize);
	llbitmap->chunksize = chunksize;
	llbitmap->chunks = chunks;
    ...
}

Will subsequent I/O to the newly resized space calculate a page_start
index exceeding nr_pages and cause an out-of-bounds access?

drivers/md/md-llbitmap.c:llbitmap_start_write() {
    ...
	int page_start = (start + BITMAP_DATA_OFFSET) >> PAGE_SHIFT;
    ...
	while (page_start <= page_end) {
		llbitmap_raise_barrier(llbitmap, page_start);
    ...
}


diff --git a/drivers/md/md-llbitmap.c b/drivers/md/md-llbitmap.c
index 131582724e7e9..6ab2188bba279 100644
--- a/drivers/md/md-llbitmap.c
+++ b/drivers/md/md-llbitmap.c

[ ... ]

> @@ -616,12 +616,12 @@ static int llbitmap_cache_pages(struct llbitmap *llbitmap)
>  	int i;
>  
>  	llbitmap->pctl = kmalloc_array(nr_pages, sizeof(void *),
> -				       GFP_KERNEL | __GFP_ZERO);
> +				       GFP_NOIO | __GFP_ZERO);
>  	if (!llbitmap->pctl)
>  		return -ENOMEM;
>  
>  	size = round_up(size, cache_line_size());
> -	pctl = kmalloc_array(nr_pages, size, GFP_KERNEL | __GFP_ZERO);
> +	pctl = kmalloc_array(nr_pages, size, GFP_NOIO | __GFP_ZERO);

[Severity: High]
Will this strict GFP_NOIO constraint cause large contiguous memory
allocations to deterministically fail due to physical memory fragmentation?

For large MD arrays, allocating the pctl array scales linearly with disk
size. This allocation can reach several megabytes, which will exceed
KMALLOC_MAX_SIZE or frequently fail because GFP_NOIO prevents direct
filesystem and I/O reclaim.

Should this allocation be migrated to kvmalloc_array() to handle larger
sizes gracefully?

>  	if (!pctl) {
>  		kfree(llbitmap->pctl);
>  		return -ENOMEM;

[ ... ]

> @@ -1110,7 +1110,7 @@ static int llbitmap_create(struct mddev *mddev)
>  	if (ret)
>  		return ret;
>  
> -	llbitmap = kzalloc_obj(*llbitmap);
> +	llbitmap = kzalloc_obj(*llbitmap, GFP_NOIO);
>  	if (!llbitmap)
>  		return -ENOMEM;

[ ... ]

>  	mutex_unlock(&mddev->bitmap_info.mutex);
>  	if (ret) {
>  		kfree(llbitmap);
>  		mddev->bitmap = NULL;
>  	}

[Severity: High]
This is a pre-existing issue, but does dropping the mutex before clearing
mddev->bitmap open a race window for concurrent sysfs readers?

If llbitmap_read_sb() fails during llbitmap_create(), the code drops
mddev->bitmap_info.mutex, calls kfree(llbitmap), and only then sets
mddev->bitmap to NULL.

Could a concurrent sysfs read (such as metadata_show()) acquire the lock
while mddev->bitmap is still non-NULL and read from the freed memory?

[Severity: Critical]
This is a pre-existing issue, but can the lockless bitmap state be
corrupted by a read-modify-write race condition here?

When multiple threads write to the same sector concurrently,
llbitmap_infect_dirty_bits() iterates over the chunks and performs an
unlocked read-modify-write on the byte array:

drivers/md/md-llbitmap.c:llbitmap_infect_dirty_bits() {
    ...
		switch (pctl->state[pos]) {
		case BitUnwritten:
			pctl->state[pos] = level_456 ? BitNeedSync : BitDirty;
    ...
}

Simultaneously, another thread may execute llbitmap_write() and assign a
new state directly:

drivers/md/md-llbitmap.c:llbitmap_write() {
    ...
	llbitmap->pctl[idx]->state[bit] = state;
    ...
}

Could the modifying thread in llbitmap_infect_dirty_bits() overwrite the
direct assignment in llbitmap_write() with a stale value, permanently
losing state updates and leading to silent data corruption (e.g., dirty
regions failing to resync after a crash)?

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