Re: [PATCH v4 2/5] rust: bitmap: restrict bitmap length to at most i32::MAX

[sashiko-bot@kernel.org]

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 <ecourtney@nvidia.com>

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/DKG0U8RLO7LZ.2I1AIH0S38PAP@nvidia.com

> 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/20260810-chid-v4-0-c9f206fdcb97@nvidia.com?part=2