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

[sashiko-bot@kernel.org]

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->sync_size`.
- [High] The `chunksize` validation in `llbitmap_read_sb()` incorrectly checks 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 consistent 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 explicitly ---
commit 19aa94c506f3423cf682ebddc1ee49bc66629dd1
Author: Yu Kuai <yukuai@fygo.io>
Subject: md/md-llbitmap: track bitmap sync_size explicitly

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

> 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 = -EINVAL;
[ ... ]
> @@ -988,6 +992,14 @@ static int llbitmap_read_sb(struct llbitmap *llbitmap)
>  		goto out_put_page;
>  	}
>  
> +	sync_size = le64_to_cpu(sb->sync_size);
> +	if (!sync_size)
> +		sync_size = 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 = 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 *llbitmap)
>  
>  	llbitmap->barrier_idle = DEFAULT_BARRIER_IDLE;
>  	llbitmap->chunksize = chunksize;
> -	llbitmap->chunks = DIV_ROUND_UP_SECTOR_T(mddev->resync_max_sectors, chunksize);
> +	llbitmap->chunks = 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 = (start + BITMAP_DATA_OFFSET) >> PAGE_SHIFT;
while (page_start <= 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 = ffz(~chunksize);
> +	llbitmap->sync_size = sync_size;
>  	ret = llbitmap_cache_pages(llbitmap);
>  
>  out_put_page:
[ ... ]
> @@ -1161,6 +1174,7 @@ static int llbitmap_resize(struct mddev *mddev, sector_t blocks, int chunksize)
>  	llbitmap->chunkshift = ffz(~chunksize);
>  	llbitmap->chunksize = chunksize;
>  	llbitmap->chunks = chunks;
> +	llbitmap->sync_size = 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.

>  
>  	return 0;
>  }
[ ... ]
> @@ -1541,7 +1555,7 @@ static void llbitmap_update_sb(void *data)
>  	sb->events = cpu_to_le64(mddev->events);
>  	sb->state = cpu_to_le32(llbitmap->flags & ~BIT(BITMAP_SHUTDOWN));
>  	sb->chunksize = cpu_to_le32(llbitmap->chunksize);
> -	sb->sync_size = cpu_to_le64(mddev->resync_max_sectors);
> +	sb->sync_size = 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 = cpu_to_le64(llbitmap->events_cleared);
>  	sb->sectors_reserved = cpu_to_le32(mddev->bitmap_info.space);

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