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 caus=
e 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 pla=
ce for reshape ---
commit 73a7214b36a3cd36e140de64fe3e4cd6cfe6795a
Author: Yu Kuai <[email protected]>

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.

> diff --git a/drivers/md/md-llbitmap.c b/drivers/md/md-llbitmap.c
> index 300dd8b93b01e..ddeea20989873 100644
> --- a/drivers/md/md-llbitmap.c
> +++ b/drivers/md/md-llbitmap.c
> @@ -416,6 +416,19 @@ static char state_machine[BitStateCount][BitmapActio=
nCount] =3D {
>  };
> =20
>  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 bloc=
ks,
> +				      unsigned long *chunksize,
> +				      unsigned long *chunks)
> +{
> +	*chunks =3D DIV_ROUND_UP_SECTOR_T(blocks, *chunksize);
> +	while (*chunks > mddev->bitmap_info.space << SECTOR_SHIFT) {

[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 chun=
ks
without reserving space for BITMAP_DATA_OFFSET, which could allow the requi=
red
size to silently exceed the reserved disk space. If the array size forces t=
he
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 =3D *chunksize << 1;
> +		*chunks =3D DIV_ROUND_UP_SECTOR_T(blocks, *chunksize);
> +	}
> +}

[ ... ]

> @@ -655,6 +668,48 @@ static unsigned int llbitmap_reserved_pages(struct l=
lbitmap *llbitmap)
>  			    PAGE_SIZE);
>  }
> =20
> +static int llbitmap_expand_pages(struct llbitmap *llbitmap,
> +				 unsigned long chunks)
> +{
> +	struct llbitmap_page_ctl **pctl;
[ ... ]
> +	for (i =3D old_nr_pages; i < nr_pages; i++) {
> +		pctl[i] =3D llbitmap_alloc_page_ctl(llbitmap, i);
> +		if (IS_ERR(pctl[i]))
> +			goto err_alloc_ptr;
> +	}
> +
> +	kfree(llbitmap->pctl);
> +	llbitmap->pctl =3D 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_BUS=
Y,
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 =3D nr_pages;
> +	return 0;

[ ... ]

> @@ -730,6 +785,34 @@ static bool llbitmap_zero_all_disks(struct llbitmap =
*llbitmap)
>  	return true;
>  }
> =20
> +static void llbitmap_mark_range(struct llbitmap *llbitmap,
> +				unsigned long start,
> +				unsigned long end,
> +				enum llbitmap_state state)
> +{
> +	while (start <=3D end) {
> +		llbitmap_write(llbitmap, state, start);
> +		start++;
> +	}
> +}

[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 l=
oop,
could this monopolize the CPU for an extended period and trigger a soft loc=
kup?

[ ... ]

> @@ -1184,24 +1267,62 @@ static int llbitmap_create(struct mddev *mddev)
>  static int llbitmap_resize(struct mddev *mddev, sector_t blocks, int chu=
nksize)
>  {
>  	struct llbitmap *llbitmap =3D mddev->bitmap;
[ ... ]
> +	mutex_lock(&mddev->bitmap_info.mutex);
> +	cache_chunks =3D reshape ? max(old_chunks, chunks) : chunks;
> +	ret =3D llbitmap_prepare_resize(llbitmap, old_chunks, chunks, cache_chu=
nks);
> +	if (ret)
> +		goto out;
> +
> +	if (reshape) {
> +		llbitmap->chunks =3D 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 =3D bitmap_chunksize;
> +		llbitmap->chunks =3D chunks;

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260802195038.1642=
[email protected]?part=3D15