Re: [PATCH 1/2] md: change chunk_sectors and stripe cache counts to unsigned int

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

New issues:
- [Medium] Lockless read race on total_cached evaluates to a negative value, triggering unsigned promotion and false cache pressure signals.

Pre-existing issues:
- [Critical] The calculation of total sectors per chunk stripe (chunk size multiplied by data disks) is performed using 32-bit arithmetic, which can overflow. This leads to a divide-by-zero kernel panic or incorrect boundary math if a maliciously large chunk size is provided.
--

--- Patch [1]: [PATCH 1/2] md: change chunk_sectors and stripe cache counts to unsigned int ---
commit 6fa5b599e890f5ca04407ab1005af1cc096d709e
Author: Hiroshi Nishida <[email protected]>

md: change chunk_sectors and stripe cache counts to unsigned int

This commit changes various chunk and stripe counts from signed to
unsigned int in the md/raid5 subsystem to prevent negative values.
It allows the compiler to optimize division and modulo operations by
power-of-two chunk sizes to right shifts in the hot I/O path.

> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 0c5c9fb0606ee..28828e083c2b3 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
[ ... ]
> @@ -7929,7 +7929,7 @@ static int raid5_run(struct mddev *mddev)
>  		sector_t here_new, here_old;
>  		int old_disks;
>  		int max_degraded = (mddev->level == 6 ? 2 : 1);
> -		int chunk_sectors;
> +		unsigned int chunk_sectors;
>  		int new_data_disks;

[Severity: Critical]
This isn't a bug introduced by this patch, but changing the types to
unsigned int fails to resolve an underlying 32-bit wrapping regression.

Does this code overflow when calculating total sectors per chunk stripe?

In raid5_run(), if a maliciously crafted RAID superblock sets chunksize
to 0x08000000 (64 GiB) and raid_disks to 33, the multiplication
(chunk_sectors * new_data_disks) evaluates to 0x08000000 * 32.

This wraps to 0 in 32-bit arithmetic, which is then passed to sector_div():

    if (sector_div(here_new, chunk_sectors * new_data_disks)) {

This triggers a divide-by-zero panic. A similar issue occurs in
make_discard_request() where DIV_ROUND_UP_SECTOR_T() is called with a
wrapped stripe_sectors value.

> diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
> index cb5feae04db27..5cd9d0f36b6e0 100644
> --- a/drivers/md/raid5.h
> +++ b/drivers/md/raid5.h
[ ... ]
> @@ -572,12 +572,12 @@ struct r5conf {
>  	/* only protect corresponding hash list and inactive_list */
>  	spinlock_t		hash_locks[NR_STRIPE_HASH_LOCKS];
>  	struct mddev		*mddev;
> -	int			chunk_sectors;
> +	unsigned int		chunk_sectors;
>  	int			level, algorithm, rmw_level;
>  	int			max_degraded;
>  	int			raid_disks;
> -	int			max_nr_stripes;
> -	int			min_nr_stripes;
> +	unsigned int		max_nr_stripes;
> +	unsigned int		min_nr_stripes;

[Severity: Medium]
Does changing min_nr_stripes to unsigned int introduce a spurious cache
flush regression due to type promotion?

In drivers/md/raid5-cache.c:r5c_do_reclaim(), total_cached can evaluate
to -1 if r5c_finish_stripe_write_out() concurrently decrements
r5c_flushing_partial_stripes and r5c_cached_partial_stripes during a
lockless read.

Since min_nr_stripes is now unsigned, the -1 is promoted to UINT_MAX in
this condition:

    if (total_cached > conf->min_nr_stripes * 3 / 4 ||

This makes the cache pressure check spuriously evaluate to true, leading
to unnecessary CPU wakeups and a performance regression during writeback
workloads.

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