Re: [PATCH 5/6] md/raid5: scale the default stripe cache size with system memory

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] 32-bit arithmetic overflow in `setup_conf` causes incorrect auto-sizing of the RAID5 stripe cache on 32-bit systems.
- [Medium] TOCTOU race condition when reading the module parameter `stripe_cache_size_default` allows bypassing intended bounds.
- [Medium] The commit message incorrectly claims that 'existing arrays are unaffected' by the new auto-sizing default.
--

--- Patch [5]: [PATCH 5/6] md/raid5: scale the default stripe cache size with system memory ---
commit 879380beda117fd029123c90fae16a214ae91020
Author: Hiroshi Nishida <[email protected]>

md/raid5: scale the default stripe cache size with system memory

setup_conf() starts every array with min_nr_stripes = NR_STRIPES (256),
about 12MB of stripe cache on a 12-disk array. That fixed default was
chosen for small systems and is never revisited. Auto-size the initial
count from memory when the new stripe_cache_size_default module parameter
is 0.

> Auto-size the initial count from memory when the new
> stripe_cache_size_default module parameter is 0 (the default): keep the
> historical NR_STRIPES up to RAID5_CACHE_DEFAULT_BASE_GB (8GB) of RAM, then
> grow the count using about 1/512 of the RAM above that baseline, capped at
> RAID5_CACHE_DEFAULT_MAX (4096).  A system with 8GB or less is therefore
> unchanged -- same count, same footprint -- while larger ones scale up
> smoothly (no jump at the baseline) to a bounded maximum.  A non-zero
> stripe_cache_size_default sets a fixed initial size for administrators who
> want one; existing arrays are unaffected, and a reshape still forces at
> least its window's worth of stripes.

[Severity: Medium]
Is the claim that existing arrays are unaffected accurate?

Because setup_conf() is invoked during array assembly and the
stripe_cache_size is not persisted in the superblock, it appears an
existing array will boot with the dynamically scaled cache size instead
of the historical 256 stripes.

> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 5f0825c5effec..8e1c2eba42417 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c

[ ... ]

> @@ -7801,15 +7817,37 @@ static struct r5conf *setup_conf(struct mddev *mddev)

[ ... ]

> +	if (stripe_cache_size_default) {
> +		conf->min_nr_stripes = clamp_t(unsigned long,
> +					stripe_cache_size_default, 16, INT_MAX);

[Severity: Medium]
Could this read of stripe_cache_size_default race with a concurrent sysfs
modification?

If stripe_cache_size_default is changed to 0 exactly between the if
condition and the clamp_t evaluation, conf->min_nr_stripes would be clamped
to 16 rather than the historical 256.

Would it be safer to read this module parameter once into a local variable?

> +	} else {
> +		unsigned long per_stripe = sizeof(struct stripe_head) +
> +			max_disks * (sizeof(struct bio) + PAGE_SIZE);
> +		unsigned long ram = totalram_pages() << PAGE_SHIFT;
> +		unsigned long base = (unsigned long)RAID5_CACHE_DEFAULT_BASE_GB << 30;

[Severity: High]
Do these calculations overflow on 32-bit systems?

Since unsigned long is 32 bits on those platforms, calculating base as
8 << 30 will wrap to 0. 

Similarly, calculating ram by shifting totalram_pages() by PAGE_SHIFT will
wrap for systems with 4GB or more of memory, potentially breaking the
scaling logic for higher memory systems.

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