Re: [PATCH v2 05/20] md/md-llbitmap: allocate page controls independently

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

New issues:
- [High] Eager allocation of physical memory and disk I/O for the entire unused reserved bitmap space causes severe memory consumption and I/O stall regressions.

Pre-existing issues:
- [Critical] The bitmap space validation omits BITMAP_DATA_OFFSET, potentially allowing the bitmap to overflow its physical bounds and corrupt user data.
--

commit 880054f58bccd2442566ac1e584d6d9c8fc01e7e
Author: Yu Kuai <[email protected]>

md/md-llbitmap: allocate page controls independently

Allocate one llbitmap page-control object at a time and free each
object through the same model.

> diff --git a/drivers/md/md-llbitmap.c b/drivers/md/md-llbitmap.c
> index e3500a1a52c07..4c3fc4b35aff0 100644
> --- a/drivers/md/md-llbitmap.c
> +++ b/drivers/md/md-llbitmap.c

[ ... ]

> +static int llbitmap_alloc_pages(struct llbitmap *llbitmap)
> +{
> +	unsigned int used_pages = llbitmap_used_pages(llbitmap, llbitmap->chunks);
> +	unsigned int nr_pages = max(used_pages, llbitmap_reserved_pages(llbitmap));
> +	int i;

[Severity: High]
Does this unconditionally eager-allocate memory and write out the entire 
reserved space?

By setting nr_pages to the maximum reserved space:
nr_pages = max(used_pages, llbitmap_reserved_pages(llbitmap));

and unconditionally allocating a physical 4KB page for every index up to 
this limit in llbitmap_read_page(), this seems to allocate RAM for all 
reserved, unused pages.

Furthermore, will functions like __llbitmap_flush() now iterate over all 
these allocated nr_pages, marking them dirty and synchronously writing them 
to disk, potentially causing severe I/O stalls for arrays with large 
reserved bitmap spaces?

> +
> +	llbitmap->pctl = kcalloc(nr_pages, sizeof(*llbitmap->pctl), GFP_KERNEL);
> +	if (!llbitmap->pctl)
> +		return -ENOMEM;

[ ... ]

> @@ -923,7 +948,7 @@ static int llbitmap_init(struct llbitmap *llbitmap)
>  	llbitmap->sync_size = blocks;
>  	mddev->bitmap_info.daemon_sleep = DEFAULT_DAEMON_SLEEP;
>  
> -	ret = llbitmap_cache_pages(llbitmap);
> +	ret = llbitmap_alloc_pages(llbitmap);

[Severity: Critical]
This is a pre-existing issue, but does the bitmap space validation here
omit BITMAP_DATA_OFFSET?

In llbitmap_init() and llbitmap_read_sb(), the validation checks ensure
the total bitmap size (chunks) doesn't exceed the reserved byte length
(space). However, they don't seem to account for the 1024-byte
BITMAP_DATA_OFFSET that is prepended to the bitmap data on disk.

If chunks is close to or exactly equal to space, the total byte size
required (chunks + BITMAP_DATA_OFFSET) could exceed the physical bounds
of the reserved space. During flushes, could llbitmap_write_page() write
across the reserved bitmap boundary and overwrite adjacent user data?

>  	if (ret)
>  		return ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5