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