Re: [PATCH v5 13/29] md/md-llbitmap: track bitmap sync_size explicitly

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

New issues:
- [High] Missing bounds check in IO fast paths allows out-of-bounds access =
to the `llbitmap->pctl` array when writing to regions beyond `llbitmap->syn=
c_size`.
- [High] The `chunksize` validation in `llbitmap_read_sb()` incorrectly che=
cks against `mddev->resync_max_sectors` instead of the newly tracked `sync_=
size`.
- [High] Strict validation of `sync_size` against `resync_max_sectors` in `=
llbitmap_read_sb` permanently fails array assembly during shrink operations=
 and crash recovery.
- [Medium] `llbitmap->sync_size` and `chunksize` are modified without consi=
stent locking, causing a data race and potential 64-bit torn reads.

Pre-existing issues:
- [High] `llbitmap_resize` increases tracking limits (`chunks`, `sync_size`=
) but fails to reallocate the `llbitmap->pctl` array to accommodate the new=
 capacity.
--

--- Patch [13]: [PATCH v5 13/29] md/md-llbitmap: track bitmap sync_size exp=
licitly ---
commit 19aa94c506f3423cf682ebddc1ee49bc66629dd1
Author: Yu Kuai <[email protected]>
Subject: md/md-llbitmap: track bitmap sync_size explicitly

This patch modifies the llbitmap layer to track its own sync_size explicitl=
y=20
rather than relying directly on mddev->resync_max_sectors. It updates the=20
superblock reader and writer to store and load this tracked sync_size. This=
=20
lays the groundwork for tracking old and new reshape geometries independent=
ly.

> diff --git a/drivers/md/md-llbitmap.c b/drivers/md/md-llbitmap.c
> index 4583bbc37c2e7..0813cebfbdeb3 100644
> --- a/drivers/md/md-llbitmap.c
> +++ b/drivers/md/md-llbitmap.c
[ ... ]
> @@ -939,6 +942,7 @@ static int llbitmap_read_sb(struct llbitmap *llbitmap)
>  	unsigned long daemon_sleep;
>  	unsigned long chunksize;
>  	unsigned long events;
> +	sector_t sync_size;
>  	struct page *sb_page;
>  	bitmap_super_t *sb;
>  	int ret =3D -EINVAL;
[ ... ]
> @@ -988,6 +992,14 @@ static int llbitmap_read_sb(struct llbitmap *llbitma=
p)
>  		goto out_put_page;
>  	}
> =20
> +	sync_size =3D le64_to_cpu(sb->sync_size);
> +	if (!sync_size)
> +		sync_size =3D mddev->resync_max_sectors;

[Severity: High]
Does decoupling the bitmap geometry from the array size require updating
the chunksize validation check?

In llbitmap_read_sb(), the chunksize validation still checks if it is large
enough using mddev->resync_max_sectors instead of the newly tracked
sync_size:

if (chunksize < DIV_ROUND_UP_SECTOR_T(mddev->resync_max_sectors,
                      mddev->bitmap_info.space << SECTOR_SHIFT)) {
    pr_err("md/llbitmap: %s: chunksize too small %lu < %llu / %lu",

If an array was recently expanded, could the old geometry's chunksize
falsely appear too small for the new array size, breaking array assembly?

> +	if (sync_size > mddev->resync_max_sectors) {
> +		pr_err("md/llbitmap: %s: sync_size %llu exceeds array sync size %llu",
> +		       mdname(mddev), sync_size, mddev->resync_max_sectors);
> +		goto out_put_page;
> +	}

[Severity: High]
Will this strict check break array assembly during a crash recovery?

If an array is shrunk and the system crashes after the array superblocks
are updated but before the bitmap superblocks are written, the on-disk
bitmap's sync_size will be larger than mddev->resync_max_sectors.

Does permanently rejecting a larger sync_size here prevent the array
from assembling in this scenario?

>  	chunksize =3D le32_to_cpu(sb->chunksize);
>  	if (!is_power_of_2(chunksize)) {
>  		pr_err("md/llbitmap: %s: chunksize not a power of 2",
[ ... ]
> @@ -1023,8 +1035,9 @@ static int llbitmap_read_sb(struct llbitmap *llbitm=
ap)
> =20
>  	llbitmap->barrier_idle =3D DEFAULT_BARRIER_IDLE;
>  	llbitmap->chunksize =3D chunksize;
> -	llbitmap->chunks =3D DIV_ROUND_UP_SECTOR_T(mddev->resync_max_sectors, c=
hunksize);
> +	llbitmap->chunks =3D DIV_ROUND_UP_SECTOR_T(sync_size, chunksize);

[Severity: High]
Could a maliciously crafted superblock with a small sync_size cause an
out-of-bounds dereference?

Because chunks is calculated from the untrusted sync_size, the allocated
pctl array will be small. The md core can still issue I/O up to the full
array size (mddev->resync_max_sectors).

Fast paths like llbitmap_start_write() shift the offset to determine the
page_start index without checking against nr_pages:

int page_start =3D (start + BITMAP_DATA_OFFSET) >> PAGE_SHIFT;
while (page_start <=3D page_end) {
    llbitmap_raise_barrier(llbitmap, page_start);
    page_start++;
}

Will this sequence lead to accessing memory past the end of llbitmap->pctl?

>  	llbitmap->chunkshift =3D ffz(~chunksize);
> +	llbitmap->sync_size =3D sync_size;
>  	ret =3D llbitmap_cache_pages(llbitmap);
> =20
>  out_put_page:
[ ... ]
> @@ -1161,6 +1174,7 @@ static int llbitmap_resize(struct mddev *mddev, sec=
tor_t blocks, int chunksize)
>  	llbitmap->chunkshift =3D ffz(~chunksize);
>  	llbitmap->chunksize =3D chunksize;
>  	llbitmap->chunks =3D chunks;
> +	llbitmap->sync_size =3D blocks;

[Severity: High]
This is a pre-existing issue, but does llbitmap_resize() safely expand the
tracking capacity?

When the array is expanded, chunks and sync_size are increased, but there
does not appear to be any code to reallocate the underlying llbitmap->pctl
array.

Subsequent I/O to the newly grown sectors maps to an index that exceeds
the originally allocated nr_pages, which might cause an out-of-bounds
memory access.

> =20
>  	return 0;
>  }
[ ... ]
> @@ -1541,7 +1555,7 @@ static void llbitmap_update_sb(void *data)
>  	sb->events =3D cpu_to_le64(mddev->events);
>  	sb->state =3D cpu_to_le32(llbitmap->flags & ~BIT(BITMAP_SHUTDOWN));
>  	sb->chunksize =3D cpu_to_le32(llbitmap->chunksize);
> -	sb->sync_size =3D cpu_to_le64(mddev->resync_max_sectors);
> +	sb->sync_size =3D cpu_to_le64(llbitmap->sync_size);

[Severity: Medium]
Is there adequate locking for the 64-bit sync_size field?

In llbitmap_resize(), llbitmap->chunksize and llbitmap->sync_size are
updated while holding only mddev->reconfig_mutex.

Concurrently, md_update_sb() can acquire mddev->lock and call
llbitmap_update_sb() without holding reconfig_mutex (such as during
background metadata updates).

Since these accesses do not share a common lock, can llbitmap_update_sb()
experience a torn read of the 64-bit sync_size on 32-bit systems, or read
an inconsistent state where chunksize and sync_size mismatch?

>  	sb->events_cleared =3D cpu_to_le64(llbitmap->events_cleared);
>  	sb->sectors_reserved =3D cpu_to_le32(mddev->bitmap_info.space);

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