Message-ID: <CACQBu=VBvxyNvwSV8_gMuqNv49Ht+7aiUZwHD9Xh-TGoMTQQsw@mail.gmail.com>
Date: Mon, 28 Apr 2025 12:21:38 +0200
From: Burak Emir <bqe@...gle.com>
To: Yury Norov <yury.norov@...il.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 6:46 PM Yury Norov <yury.norov@...il.com> wrote:
>
> 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?

Yes, I agree with this reasoning in combination with BUG_ON.

> 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.

In my own words: using BUG_ON to prevent undefined behavior is almost
always going to be more useful than panic.

> 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.

There are several things at play:
- for the C methods that tolerate out-of-bounds access, the
bounds-check is redundant for safety
- but it is not redundant in terms of API: tolerating out-of-bounds
access is the spec, someone may rely on it
- if we would like a Rust API that is stricter (hardening), it is an
API change (or: a Rust and C API difference).

I will go with your suggestion and do a local (bitmap-specific)
configurable assert (=panic).
This could then later be changed to use a BUG macro.

> 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``.
>

Bitmap operations are so basic and the API that fallibility does not
make sense to me here.
The Rust API is also constrained by what the C API provides: if C
handles out-of-bounds arguments gracefully, then Rust should, too.
And panicking has the problems you described, so I have come around to
think that Rust needs a BUG_ON macro.

> >   * 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.

Done, going to be in v8.

Thanks,
Burak

>
> Thanks,
> Yury

Powered by blists - more mailing lists