[PATCH 0/2] md: widen size/count types for large arrays

[PATCH 0/2] md: widen size/count types for large arrays

[Hiroshi Nishida]

Two small type-widening fixes for large arrays.  Neither changes
behaviour or memory use on any current configuration.

  1/2 changes chunk_sectors and the stripe-cache count fields to unsigned
      int, matching how they are used and avoiding implicit sign
      extension in the cache shrinker's subtraction.

  2/2 widens the badblocks sector parameters from int to sector_t so the
      helpers do not narrow a sector_t argument on very large devices.

These are independent of the other md/raid5 patches I'm sending and can
be applied on their own.

Hiroshi Nishida (2):
  md: change chunk_sectors and stripe cache counts to unsigned int
  md: widen badblock sectors param from int to sector_t

 drivers/md/md.c    |  4 ++--
 drivers/md/md.h    | 10 +++++-----
 drivers/md/raid5.c | 14 +++++++-------
 drivers/md/raid5.h |  8 ++++----
 4 files changed, 18 insertions(+), 18 deletions(-)


base-commit: 55b77337bdd088c77461588e5ec094421b89911b
-- 
2.43.0

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

[Hiroshi Nishida]

chunk_sectors, new_chunk_sectors, prev_chunk_sectors, max_nr_stripes,
and min_nr_stripes are never negative. Using signed int is semantically
wrong and prevents the compiler from optimizing division/modulo by
power-of-two chunk sizes to right shifts in the hot I/O path.

Change all struct fields and derived local variables to unsigned int:
  mddev->chunk_sectors
  mddev->new_chunk_sectors
  r5conf->chunk_sectors
  r5conf->prev_chunk_sectors
  r5conf->max_nr_stripes
  r5conf->min_nr_stripes
  Local: sectors_per_chunk, new_chunk, chunk_sectors

The min() in r5c_check_cached_full_stripe() required both operands to
match signedness; this is now satisfied with max_nr_stripes unsigned.

Signed-off-by: Hiroshi Nishida <nishidafmly@gmail.com>
---
 drivers/md/md.h    |  4 ++--
 drivers/md/raid5.c | 14 +++++++-------
 drivers/md/raid5.h |  8 ++++----
 3 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/md/md.h b/drivers/md/md.h
index d8daf0f75cbb..b9ad26844799 100644
--- a/drivers/md/md.h
+++ b/drivers/md/md.h
@@ -437,7 +437,7 @@ struct mddev {
 	int				external;	/* metadata is
 							 * managed externally */
 	char				metadata_type[17]; /* externally set*/
-	int				chunk_sectors;
+	unsigned int			chunk_sectors;
 	time64_t			ctime, utime;
 	int				level, layout;
 	char				clevel[16];
@@ -466,7 +466,7 @@ struct mddev {
 	 */
 	sector_t			reshape_position;
 	int				delta_disks, new_level, new_layout;
-	int				new_chunk_sectors;
+	unsigned int			new_chunk_sectors;
 	int				reshape_backwards;
 
 	struct md_thread __rcu		*thread;	/* management thread */
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 0c5c9fb0606e..28828e083c2b 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -2970,7 +2970,7 @@ sector_t raid5_compute_sector(struct r5conf *conf, sector_t r_sector,
 	sector_t new_sector;
 	int algorithm = previous ? conf->prev_algo
 				 : conf->algorithm;
-	int sectors_per_chunk = previous ? conf->prev_chunk_sectors
+	unsigned int sectors_per_chunk = previous ? conf->prev_chunk_sectors
 					 : conf->chunk_sectors;
 	int raid_disks = previous ? conf->previous_raid_disks
 				  : conf->raid_disks;
@@ -3166,7 +3166,7 @@ sector_t raid5_compute_blocknr(struct stripe_head *sh, int i, int previous)
 	int raid_disks = sh->disks;
 	int data_disks = raid_disks - conf->max_degraded;
 	sector_t new_sector = sh->sector, check;
-	int sectors_per_chunk = previous ? conf->prev_chunk_sectors
+	unsigned int sectors_per_chunk = previous ? conf->prev_chunk_sectors
 					 : conf->chunk_sectors;
 	int algorithm = previous ? conf->prev_algo
 				 : conf->algorithm;
@@ -3584,7 +3584,7 @@ static void end_reshape(struct r5conf *conf);
 static void stripe_set_idx(sector_t stripe, struct r5conf *conf, int previous,
 			    struct stripe_head *sh)
 {
-	int sectors_per_chunk =
+	unsigned int sectors_per_chunk =
 		previous ? conf->prev_chunk_sectors : conf->chunk_sectors;
 	int dd_idx;
 	int chunk_offset = sector_div(stripe, sectors_per_chunk);
@@ -6103,7 +6103,7 @@ static enum stripe_result make_stripe_request(struct mddev *mddev,
 static sector_t raid5_bio_lowest_chunk_sector(struct r5conf *conf,
 					      struct bio *bi)
 {
-	int sectors_per_chunk = conf->chunk_sectors;
+	unsigned int sectors_per_chunk = conf->chunk_sectors;
 	int raid_disks = conf->raid_disks;
 	int dd_idx;
 	struct stripe_head sh;
@@ -7930,7 +7930,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;
 
 		if (journal_dev) {
@@ -8832,7 +8832,7 @@ static int raid5_check_reshape(struct mddev *mddev)
 	 * to be used by a reshape pass.
 	 */
 	struct r5conf *conf = mddev->private;
-	int new_chunk = mddev->new_chunk_sectors;
+	unsigned int new_chunk = mddev->new_chunk_sectors;
 
 	if (mddev->new_layout >= 0 && !algorithm_valid_raid5(mddev->new_layout))
 		return -EINVAL;
@@ -8866,7 +8866,7 @@ static int raid5_check_reshape(struct mddev *mddev)
 
 static int raid6_check_reshape(struct mddev *mddev)
 {
-	int new_chunk = mddev->new_chunk_sectors;
+	unsigned int new_chunk = mddev->new_chunk_sectors;
 
 	if (mddev->new_layout >= 0 && !algorithm_valid_raid6(mddev->new_layout))
 		return -EINVAL;
diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
index cb5feae04db2..5cd9d0f36b6e 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;
 #if PAGE_SIZE != DEFAULT_STRIPE_SIZE
 	unsigned long	stripe_size;
 	unsigned int	stripe_shift;
@@ -595,7 +595,7 @@ struct r5conf {
 	 */
 	sector_t		reshape_safe;
 	int			previous_raid_disks;
-	int			prev_chunk_sectors;
+	unsigned int		prev_chunk_sectors;
 	int			prev_algo;
 	short			generation; /* increments with every reshape */
 	seqcount_spinlock_t	gen_lock;	/* lock against generation changes */
-- 
2.43.0

[PATCH 2/2] md: widen badblock sectors param from int to sector_t

[Hiroshi Nishida]

The badblocks core API -- badblocks_set(), badblocks_clear() and
badblocks_check() -- and the is_badblock() helper all take the range
length as sector_t.  The md wrappers rdev_set_badblocks(),
rdev_clear_badblocks() and rdev_has_badblock(), however, declared the
same length as int, narrowing sector_t to int and back again in the
middle of an otherwise 64-bit clean path.

Change the sectors parameter to sector_t in these three wrappers so it
matches the core API and is_badblock().  No functional change: current
callers pass per-I/O or per-resync-chunk lengths well within int range.
This just removes a gratuitous truncation point and keeps the type
consistent end to end.

Signed-off-by: Hiroshi Nishida <nishidafmly@gmail.com>
---
 drivers/md/md.c | 4 ++--
 drivers/md/md.h | 6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index d1465bcd86c8..61f40fa41e78 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -10553,7 +10553,7 @@ EXPORT_SYMBOL(md_finish_reshape);
 /* Bad block management */
 
 /* Returns true on success, false on failure */
-bool rdev_set_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
+bool rdev_set_badblocks(struct md_rdev *rdev, sector_t s, sector_t sectors,
 			int is_new)
 {
 	struct mddev *mddev = rdev->mddev;
@@ -10593,7 +10593,7 @@ bool rdev_set_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
 }
 EXPORT_SYMBOL_GPL(rdev_set_badblocks);
 
-void rdev_clear_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
+void rdev_clear_badblocks(struct md_rdev *rdev, sector_t s, sector_t sectors,
 			  int is_new)
 {
 	if (is_new)
diff --git a/drivers/md/md.h b/drivers/md/md.h
index b9ad26844799..95835a3286aa 100644
--- a/drivers/md/md.h
+++ b/drivers/md/md.h
@@ -311,7 +311,7 @@ static inline int is_badblock(struct md_rdev *rdev, sector_t s, sector_t sectors
 }
 
 static inline int rdev_has_badblock(struct md_rdev *rdev, sector_t s,
-				    int sectors)
+				    sector_t sectors)
 {
 	sector_t first_bad;
 	sector_t bad_sectors;
@@ -319,9 +319,9 @@ static inline int rdev_has_badblock(struct md_rdev *rdev, sector_t s,
 	return is_badblock(rdev, s, sectors, &first_bad, &bad_sectors);
 }
 
-extern bool rdev_set_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
+extern bool rdev_set_badblocks(struct md_rdev *rdev, sector_t s, sector_t sectors,
 			       int is_new);
-extern void rdev_clear_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
+extern void rdev_clear_badblocks(struct md_rdev *rdev, sector_t s, sector_t sectors,
 				 int is_new);
 struct md_cluster_info;
 struct md_cluster_operations;
-- 
2.43.0

Re: [PATCH 2/2] md: widen badblock sectors param from int to sector_t

[sashiko-bot]

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

Pre-existing issues:
- [High] Arithmetic underflow in `is_badblock()` when a physical bad block overlaps `data_offset`.
- [High] Discarded macro results in `block/badblocks.c` make block alignment a no-op.
--

--- Patch [2]: [PATCH 2/2] md: widen badblock sectors param from int to sector_t ---
commit 19521e4f1c32c01e7a6122a185436691618d94c1
Author: Hiroshi Nishida <nishidafmly@gmail.com>

md: widen badblock sectors param from int to sector_t
    
This commit changes the sectors parameter in md wrappers (rdev_set_badblocks,
rdev_clear_badblocks, and rdev_has_badblock) from int to sector_t. This
matches the core API and is_badblock() to prevent a truncation point.

> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index d1465bcd86c81..61f40fa41e781 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c

[ ... ]

> @@ -10553,7 +10553,7 @@ EXPORT_SYMBOL(md_finish_reshape);
>  /* Bad block management */
>  
>  /* Returns true on success, false on failure */
> -bool rdev_set_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
> +bool rdev_set_badblocks(struct md_rdev *rdev, sector_t s, sector_t sectors,
>  			int is_new)
>  {

[Severity: High]
This isn't a bug introduced by this patch, but since this code interacts
with the core badblocks API, I noticed a pre-existing bug in how block
alignment is handled in block/badblocks.c.

In _badblocks_set(), the code attempts to align sectors:

block/badblocks.c:_badblocks_set() {
    ...
    rounddown(s, 1 << bb->shift);
    roundup(next, 1 << bb->shift);
    sectors = next - s;
    ...
}

Similar logic exists in _badblocks_clear() and badblocks_check().

Because rounddown() and roundup() are macros that evaluate to the rounded
value but do not modify their arguments in-place, aren't the alignment
results being ignored here, leaving s and next unaligned?

If bb->shift > 0, wouldn't this break bad block alignment, fragment the
bad block table, and risk premature table exhaustion?

[ ... ]

> diff --git a/drivers/md/md.h b/drivers/md/md.h
> index b9ad26844799d..95835a3286aac 100644
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h

[ ... ]

> @@ -311,7 +311,7 @@ static inline int is_badblock(struct md_rdev *rdev, sector_t s, sector_t sectors
>  }
>  
>  static inline int rdev_has_badblock(struct md_rdev *rdev, sector_t s,
> -				    int sectors)
> +				    sector_t sectors)
>  {
>  	sector_t first_bad;

[Severity: High]
This is a pre-existing issue, but while reviewing the badblock sector types,
I noticed a logic error in is_badblock() located just above this function.

drivers/md/md.h:is_badblock() {
    ...
    int rv = badblocks_check(&rdev->badblocks, rdev->data_offset + s,
                             sectors, first_bad, bad_sectors);
    if (rv)
        *first_bad -= rdev->data_offset;
    ...
}

If userspace tools (e.g., mdadm via sysfs bb_store) write a physical bad
block that starts before rdev->data_offset but extends into the array's data
area, badblocks_check() returns a *first_bad physical sector less than
data_offset. 

Could the unconditional subtraction (*first_bad -= rdev->data_offset)
underflow here?

Would this create a massive false first_bad sector value, causing callers
like raid1 to mistakenly issue I/O to a known bad sector?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710132329.7273-1-nishidafmly@gmail.com?part=2

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

[sashiko-bot]

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 <nishidafmly@gmail.com>

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/20260710132329.7273-1-nishidafmly@gmail.com?part=1