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

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 <[email protected]>

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/[email protected]?part=2