Message-ID: <aAkZZd8QRXKejVV0@yury>
Date: Wed, 23 Apr 2025 12:46:38 -0400
From: Yury Norov <yury.norov@...il.com>
To: Burak Emir <bqe@...gle.com>
Cc: Rasmus Villemoes <linux@...musvillemoes.dk>,
	Viresh Kumar <viresh.kumar@...aro.org>,
	Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...il.com>,
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Benno Lossin <benno.lossin@...ton.me>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 3/5] rust: add bitmap API.

On Wed, Apr 23, 2025 at 01:43:35PM +0000, Burak Emir wrote:
> 
> Provides an abstraction for C bitmap API and bitops operations.
> We follow the C Bitmap API closely in naming and semantics, with
> a few differences that take advantage of Rust language facilities
> and idioms:
> 
>   * not all operations are exposed yet, to avoid dead code. This commit
>     includes enough to implement an Android Binder data structure that
>     was introduced in commit 15d9da3f818c ("binder: use bitmap for
>     faster descriptor lookup"), namely drivers/android/dbitmap.h. This
>     is part of upstreaming the Android Binder driver.
> 
>   * API uses Option types where appropriate
> 
>   * bound checks are used to treat out-of-bounds access as bug
>     (hardening). The C operations treat out-of-bounds parameters
>     as a default case e.g. "not found" which is safe (avoids UB) but
>     may hide bugs.

Few paragraphs later you say:

> It enables Rust
> code to be a lot more similar and predictable with respect to C code

If you want to make Rust similar to C (which is good), you need to
make all that Panic'ing stuff configurable, right?

This is what I suggested originally, and this is the right way to go,
to me.

There was a long discussion few years ago regarding BUG_ON()s in the
kernel, and the bottomline for it is simple: the kernel has many nice
isolating and self-debugging features that help to resolve (hopefully)
non-fatal errors like out-of-bond access, so we should let it try.

Just halting the kernel helps little, particularly it prevents from
collecting syslogs and other debugging info. You can check git history
and see how many BUG()s were demoted to WARN()s, or simply dropped.

So, this is again my suggestion: it's OK to have a hardening feature,
but the panic should be configurable for the reasons:
 - robustness;
 - compatibility;
 - debugability.

To me, this should end up with something like:

  fn bitmap_assert(bool, msg) 
  {
  #if CONFIG_RUST_HARDENING_PANIC && CONFIG_RUST_BITMAP_HARDENING
        assert(bool, msg)
  #elif CONFIG_RUST_BITMAP_HARDENING
        if (!bool)
                pr_err(msg)
  #else
        // do nothing; for the best performance.
  #endif
  }

This doesn't look difficult anyhow right? But if you find it
impractical, you can just replace the panic with printing an error.

After all, this # Panic simply breaks your own coding rules:

  Please note that panicking should be very rare and used only with a good
  reason. In almost all cases, a fallible approach should be used, typically
  returning a ``Result``.

>   * better naming convention to distinguish non-atomic from atomic
>     operations: {set,clear}_bit vs {set,clear}_bit_atomic.
>     The Rust type system ensures that all non-atomic operations
>     require a &mut reference which amounts to exclusive access.
>     Using the atomic variants only makes sense when multiple threads
>     have a shared reference &bitmap which amounts to the interior
>     mutability pattern.
> 
>   * optimized to represent the bitmap inline if it would take the space
>     of a pointer. This saves allocations which is relevant in the
>     Binder use case.
> 
> The underlying C bitmap is *not* exposed. This would lose all static
> guarantees.
> 
> An alternative route of vendoring an existing Rust bitmap package was
> considered but suboptimal overall. Reusing the C implementation is
> preferable for a basic data structure like bitmaps. It enables Rust
> code to be a lot more similar and predictable with respect to C code
> that uses the same data structures and enables the use of code that
> has been tried-and-tested in the kernel.
> 
> We use the `usize` type for sizes and indices into the bitmap,
> because Rust generally always uses that type for indices and lengths
> and it will be more convenient if the API accepts that type. This means
> that we need to perform some casts to/from u32 and usize, since the C
> headers use unsigned int instead of size_t/unsigned long for these
> numbers in some places.
> 
> Adds new MAINTAINERS section BITMAP API [RUST].
> 
> Suggested-by: Alice Ryhl <aliceryhl@...gle.com>
> Suggested-by: Yury Norov <yury.norov@...il.com>
> Signed-off-by: Burak Emir <bqe@...gle.com>
> ---
>  MAINTAINERS           |   7 +
>  rust/kernel/bitmap.rs | 396 ++++++++++++++++++++++++++++++++++++++++++
>  rust/kernel/lib.rs    |   1 +
>  3 files changed, 404 insertions(+)
>  create mode 100644 rust/kernel/bitmap.rs
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1f162f64eded..7d107dc91390 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4135,6 +4135,13 @@ S:	Maintained
>  F:	rust/helpers/bitmap.c
>  F:	rust/helpers/cpumask.c
>  
> +BITMAP API [RUST]
> +M:	Alice Ryhl <aliceryhl@...gle.com>
> +M:	Burak Emir <bqe@...gle.com>
> +R:	Yury Norov <yury.norov@...il.com>
> +S:	Maintained
> +F:	rust/kernel/bitmap.rs
> +
>  BITOPS API
>  M:	Yury Norov <yury.norov@...il.com>
>  R:	Rasmus Villemoes <linux@...musvillemoes.dk>
> diff --git a/rust/kernel/bitmap.rs b/rust/kernel/bitmap.rs
> new file mode 100644
> index 000000000000..79ddbef2b028
> --- /dev/null
> +++ b/rust/kernel/bitmap.rs
> @@ -0,0 +1,396 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +// Copyright (C) 2025 Google LLC.
> +
> +//! Rust API for bitmap.
> +//!
> +//! C headers: [`include/linux/bitmap.h`](srctree/include/linux/bitmap.h).
> +
> +use crate::alloc::{AllocError, Flags};
> +use crate::bindings;
> +use core::ptr::NonNull;
> +
> +/// Holds either a pointer to array of `unsigned long` or a small bitmap.
> +#[repr(C)]
> +union BitmapRepr {
> +    bitmap: usize,
> +    ptr: NonNull<usize>,
> +}
> +
> +    pub fn last_bit(&self) -> Option<usize> {

[...]

> +        // SAFETY: access is within bounds.
> +        let index = unsafe { bindings::_find_last_bit(self.as_ptr(), self.nbits) };
> +        if index == self.nbits {

Here and everywhere, can you please:

        if index >= self.nbits {

I know that bitmap documentation says that index would be exactly
nbits, but really it was a bad decision. If one day I'll find a safe
way to relax this to >=, your code will stay untouched.

Thanks,
Yury

Powered by blists - more mailing lists