Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [High] This problem wasn't introduced by this patch, but the safe Rust API for `Bitmap` allows data races (Undefined Behavior). The methods `set_bit_atomic` and `clear_bit_atomic` are marked as `safe` and take a shared reference `&self` to perform atomic mutations. However, `Bitmap` (and `BitmapVec` via `Deref`) implements `Sync`, meaning `&Bitmap` can be shared across multiple threads. Other safe methods like `next_bit`, `last_bit`, and `copy_and_extend` (via the `src` parameter) also take `&self` but perform non-atomic reads. This allows safe Rust code to execute concurrent atomic writes and non-atomic reads on the same memory, violating Rust's memory model and causing Undefined Behavior (a data race). The safety comment in `set_bit_atomic` incorrectly states 'the caller has ensured that there is no mix of non-atomic and atomic operations', but since the method is marked safe, the compiler cannot enforce this. -- --- Patch [2]: [PATCH v4 2/5] rust: bitmap: restrict bitmap length to at most i32::MAX --- commit d989729ee19564d93c5d59130c2e1ac3974ea804 Author: Eliot Courtney <[email protected]> rust: bitmap: restrict bitmap length to at most i32::MAX This patch restricts the maximum size of a Bitmap to i32::MAX bits to match BitmapVec. It updates the unsafe requirements for Bitmap::from_raw and from_raw_mut to reflect this limit. It also corrects potential out-of-bounds writes in copy_and_extend caused by u32 cast truncations. Link: https://lore.kernel.org/[email protected] > diff --git a/rust/kernel/bitmap.rs b/rust/kernel/bitmap.rs > index a43bfe0ec3dc2..0d481d761f2ac 100644 > --- a/rust/kernel/bitmap.rs > +++ b/rust/kernel/bitmap.rs [ ... ] > @@ -415,7 +420,8 @@ pub fn clear_bit_atomic(&self, index: usize) { > #[inline] > pub fn copy_and_extend(&mut self, src: &Bitmap) { [Severity: High] This isn't a bug introduced by this patch, but does the safe Rust API for Bitmap allow data races? The set_bit_atomic() and clear_bit_atomic() methods are marked as safe and take a shared reference to perform atomic mutations. However, Bitmap implements Sync, meaning it can be shared across multiple threads. Other safe methods like next_bit(), last_bit(), and copy_and_extend() (via the src parameter here) also take a shared reference but perform non-atomic reads. This allows safe Rust code to execute concurrent atomic writes and non-atomic reads on the same memory, violating the memory model. The safety comment in set_bit_atomic() states the caller has ensured there is no mix of non-atomic and atomic operations, but since the method is marked safe, the compiler cannot enforce this. Could the API be updated to prevent safe code from triggering a data race? > let len = core::cmp::min(src.len(), self.len()); > - // SAFETY: access to `self` and `src` is within bounds. > + // SAFETY: access to `self` and `src` is within bounds. Both lengths > fit in `u32` > + // because a `Bitmap` is at most `i32::MAX` bits, so the casts are > lossless. > unsafe { > bindings::bitmap_copy_and_extend( > self.as_mut_ptr(), -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
