Re: [PATCH v3 2/4] rust: bitmap: add contiguous area operations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The `set` and `clear` methods incorrectly restrict the area's `end` coordinate to `i32::MAX`, artificially limiting bitmap operations and causing silent data corruption on large bitmaps.
- [High] The safe function `next_zero_area` fails to prevent integer overflow for `self.len() + align - 1`, leading to a documented deterministic kernel hang.
--

--- Patch [2]: [PATCH v3 2/4] rust: bitmap: add contiguous area operations ---
commit 6adb3d6be8231d8ad4e8a7ca4a15f16e149caa2c
Author: Eliot Courtney <ecourtney@nvidia.com>

rust: bitmap: add contiguous area operations

This commit introduces bindings for area operations on bitmaps, such as finding
the next zero area, setting, and clearing contiguous bits. It also adds bounds
checking and other safety mechanisms to catch erroneous usage.

> diff --git a/rust/kernel/bitmap.rs b/rust/kernel/bitmap.rs
> index a43bfe0ec3dc2..f4b0b8ae39d88 100644
> --- a/rust/kernel/bitmap.rs
> +++ b/rust/kernel/bitmap.rs

[ ... ]

> @@ -497,6 +498,116 @@ pub fn next_zero_bit(&self, start: usize) -> Option<usize> {
>              Some(index)
>          }
>      }
> +
> +    /// Finds a contiguous area of `nbits` zero bits at or after `start`, aligned to `align`.
> +    ///
> +    /// Returns the bit index of the start of the area, or [`None`] if no such area fitting in
> +    /// the bitmap exists.
> +    ///
> +    /// The returned index is a multiple of `align`. Alignments where `self.len() + align - 1`
> +    /// overflows a `usize` can hang the underlying C code.

[ ... ]

> +    #[inline]
> +    pub fn next_zero_area(&self, start: usize, nbits: usize, align: Alignment) -> Option<usize> {

[ ... ]

> +        let nr = u32::try_from(nbits).ok()?;
> +
> +        // SAFETY: `bitmap_find_next_zero_area_off` is safe to use with an out of bounds `start`
> +        // value and never reads beyond `self.len()` bits.
> +        let index = unsafe {
> +            bindings::bitmap_find_next_zero_area_off(
> +                self.as_ptr().cast_mut(),
> +                self.len(),
> +                start,
> +                nr,
> +                align.as_usize() - 1,
> +                0,
> +            )
> +        };

[Severity: High]
The documentation notes that an alignment where self.len() + align - 1
overflows can hang the C code.

Since next_zero_area() is exposed as a safe API, shouldn't it validate this
overflow condition before calling the unsafe FFI function to prevent a
deterministic kernel hang from safe Rust? A caller providing an alignment
and bitmap length that overflows usize will cause this function to hang
indefinitely.

[ ... ]

> +    /// Sets a contiguous area of `nbits` bits starting at `start`.
> +    ///
> +    /// If CONFIG_RUST_BITMAP_HARDENED is not enabled and the area `start..start + nbits` is out of
> +    /// bounds, does nothing.

[ ... ]

> +    #[inline]
> +    pub fn set(&mut self, start: usize, nbits: usize) {
> +        bitmap_assert_return!(
> +            start
> +                .checked_add(nbits)
> +                .is_some_and(|end| end <= self.len() && end <= i32::MAX as usize),

[Severity: High]
Does this bounds check in set() restrict the sum start + nbits to i32::MAX
instead of just restricting nbits?

The underlying C function __bitmap_set() takes unsigned int start and
int len. By bounding the end coordinate, a caller attempting to set bits
past index i32::MAX on a valid large bitmap will experience silent failure,
dropping state updates and potentially leading to data corruption like
double-allocations.

[ ... ]

> +    /// Clears a contiguous area of `nbits` bits starting at `start`.
> +    ///
> +    /// If CONFIG_RUST_BITMAP_HARDENED is not enabled and the area `start..start + nbits` is out of
> +    /// bounds, does nothing.

[ ... ]

> +    #[inline]
> +    pub fn clear(&mut self, start: usize, nbits: usize) {
> +        bitmap_assert_return!(
> +            start
> +                .checked_add(nbits)
> +                .is_some_and(|end| end <= self.len() && end <= i32::MAX as usize),

[Severity: High]
Like the set() function, does this bounds check in clear() also improperly
bound start + nbits to i32::MAX?

This would discard valid clear operations for any caller modifying bits in
the upper half of the addressable bit space.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729-chid-v3-0-20cc08032bbc@nvidia.com?part=2