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 <[email protected]>

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.

> diff --git a/drivers/md/md-llbitmap.c b/drivers/md/md-llbitmap.c
> index 4c3fc4b35aff0..1ab9b1689fd80 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: 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);
> +	}
> +}

[ ... ]

> @@ -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;
> +	unsigned int old_nr_pages = llbitmap->nr_pages;
> +	unsigned int nr_pages = llbitmap_used_pages(llbitmap, chunks);
> +	unsigned int i;
> +	int ret;
> +
> +	if (nr_pages <= old_nr_pages)
> +		return 0;
> +
> +	pctl = kcalloc(nr_pages, sizeof(*pctl), GFP_KERNEL);

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

[ ... ]

> @@ -1171,24 +1254,53 @@ static int llbitmap_create(struct mddev *mddev)
>  static int llbitmap_resize(struct mddev *mddev, sector_t blocks, int chunksize)
>  {

[ ... ]

> +	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/[email protected]?part=7