| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAH5fLgiQkPpMUV0Bvmwd5zUsHy=GHLdoVFjRuAPxpWCbBiD2Jw@mail.gmail.com>
Date: Mon, 3 Mar 2025 14:01:44 +0100
From: Alice Ryhl <aliceryhl@...gle.com>
To: Burak Emir <bqe@...gle.com>
Cc: Yury Norov <yury.norov@...il.com>, 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>,
Trevor Gross <tmgross@...ch.edu>, rust-for-linux@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] rust: add bindings and API for bitmap.h and bitops.h.
On Mon, Mar 3, 2025 at 1:48 PM Alice Ryhl <aliceryhl@...gle.com> wrote:
>
> On Mon, Mar 3, 2025 at 12:55 PM Burak Emir <bqe@...gle.com> wrote:
> >
> > Adds a Rust bitmap API and necessary bitmap and bitops bindings.
> > These are for porting the approach from commit 15d9da3f818c ("binder:
> > use bitmap for faster descriptor lookup") to Rust. The functionality
> > in dbitmap.h makes use of bitmap and bitops.
> >
> > The Rust bitmap API provides an abstraction to underlying bitmap
> > and bitops operations. For now, we only include methods that are
> > necessary for reimplementing dbitmap.h. It is straightforward to add
> > more methods later, as needed. We offer a safe API through
> > bounds checks which panic if violated.
> >
> > We introduce bindings for the non-atomic variants __set_bit and
> > __clear_bit, and use the _find_* variants instead of the find_*
> > wrappers. The C optimizations enabled by the wrappers does not carry
> > over to Rust.
> >
> > This series uses 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.
> >
> > Suggested-by: Alice Ryhl <aliceryhl@...gle.com>
> > Signed-off-by: Burak Emir <bqe@...gle.com>
> > ---
> > This is v2 of a patch introducing Rust bitmap API [v1]. Thanks
> > for all the helpful comments!
> >
> > Changes v1 --> v2:
> > - Rebased on Yury's v2 patch [1] and Viresh's v2 patch [2]
> > - Removed import of `bindings::*`, keeping only prefix (Miguel)
> > - Renamed panic methods to make more explicit (Miguel)
> > - use markdown in doc comments and added example/kunit test (Miguel)
> > - Added maintainer section for BITOPS API BINDINGS [RUST] (Yury)
> > - Added M: entry for bitmap.rs which goes to Alice (Viresh, Alice)
> > - Changed calls from find_* to _find_*, removed helpers (Yury)
> > - Use non-atomic __set_bit and __clear_bit from Bitmap Rust API (Yury)
> >
> > Question to Yury: we could remove `bitmap_copy` helper and instead do
> > the memcpy in Rust. Should we? If so, should we expose a helper for
> > `bitmap_size` or duplicate the nbits-to-aligned-nlongs logic?
> >
> > [1] https://lore.kernel.org/all/20250224233938.3158-1-yury.norov@gmail.com/
> > [2] https://lore.kernel.org/all/cover.1740726226.git.viresh.kumar@linaro.org/
> > [v1]: https://lore.kernel.org/all/20250227101720.1811578-1-bqe@google.com/
> >
> > Thanks,
> > Burak
> >
> > ---
> > MAINTAINERS | 8 ++
> > rust/bindings/bindings_helper.h | 1 +
> > rust/helpers/bitmap.c | 8 ++
> > rust/helpers/bitops.c | 13 +++
> > rust/helpers/helpers.c | 2 +
> > rust/kernel/bitmap.rs | 191 ++++++++++++++++++++++++++++++++
> > rust/kernel/lib.rs | 1 +
> > 7 files changed, 224 insertions(+)
> > create mode 100644 rust/helpers/bitmap.c
> > create mode 100644 rust/helpers/bitops.c
> > create mode 100644 rust/kernel/bitmap.rs
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 6d6e55d8593b..8f42fb1f24c6 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -4032,12 +4032,15 @@ F: tools/lib/find_bit.c
> > BITMAP API BINDINGS [RUST]
> > M: Yury Norov <yury.norov@...il.com>
> > S: Maintained
> > +F: rust/helpers/bitmap.c
> > F: rust/helpers/cpumask.c
> >
> > BITMAP API [RUST]
> > M: Viresh Kumar <viresh.kumar@...aro.org> (cpumask)
> > +M: Alice Ryhl <aliceryhl@...gle.com> (bitmap)
> > R: Yury Norov <yury.norov@...il.com>
> > S: Maintained
> > +F: rust/kernel/bitmap.rs
> > F: rust/kernel/cpumask.rs
> >
> > BITOPS API
> > @@ -4054,6 +4057,11 @@ F: include/linux/bitops.h
> > F: lib/test_bitops.c
> > F: tools/*/bitops*
> >
> > +BITOPS API BINDINGS [RUST]
> > +M: Yury Norov <yury.norov@...il.com>
> > +S: Maintained
> > +F: rust/helpers/bitops.c
> > +
> > BLINKM RGB LED DRIVER
> > M: Jan-Simon Moeller <jansimon.moeller@....de>
> > S: Maintained
> > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> > index 673b1daa9a58..50416c1a3de9 100644
> > --- a/rust/bindings/bindings_helper.h
> > +++ b/rust/bindings/bindings_helper.h
> > @@ -7,6 +7,7 @@
> > */
> >
> > #include <kunit/test.h>
> > +#include <linux/bitmap.h>
> > #include <linux/blk-mq.h>
> > #include <linux/blk_types.h>
> > #include <linux/blkdev.h>
> > diff --git a/rust/helpers/bitmap.c b/rust/helpers/bitmap.c
> > new file mode 100644
> > index 000000000000..4fa4e4f76110
> > --- /dev/null
> > +++ b/rust/helpers/bitmap.c
> > @@ -0,0 +1,8 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <linux/bitmap.h>
> > +
> > +void rust_helper_bitmap_copy(unsigned long *dst, const unsigned long *src, unsigned int nbits)
> > +{
> > + bitmap_copy(dst, src, nbits);
> > +}
>
> I was about to say that this could just be a memcpy, but ...
>
> > + /// Copy up to `nbits` bits from this bitmap into another.
> > + ///
> > + /// # Panics
> > + ///
> > + /// Panics if `nbits` is too large for this bitmap or destination.
> > + #[inline]
> > + pub fn bitmap_copy(&self, dst: &mut Bitmap, nbits: usize) {
> > + if self.nbits < nbits {
> > + panic_not_in_bounds_le("nbits", self.nbits, nbits);
> > + }
> > + if dst.nbits < nbits {
> > + panic_not_in_bounds_le("nbits", dst.nbits, nbits);
> > + }
> > + // SAFETY: nbits == 0 is supported and access to `self` and `dst` is within bounds.
> > + unsafe { bindings::bitmap_copy(dst.as_mut_ptr(), self.ptr.as_ptr(), nbits as u32) };
> > + }
>
> ... then I realized that we're probably not using it correctly. I
> would expect this to modify the first `nbits` bits in `dst`, leaving
> any remaining bits unmodified. However, if nbits is not divisible by
> BITS_PER_LONG it might modify some bits it shouldn't.
>
> That said, Binder needs this only in the case where the sizes are
> equal. Perhaps we could rename this to `copy_from_bitmap` with this
> signature:
> fn copy_from_bitmap(&mut self, src: Bitmap)
Sorry I meant src: &Bitmap here.
Also, we could rewrite it to just call memcpy rather than go through
bitmap_copy. Though that requires us to have a Rust version of
bitmap_size, which I think it makes sense to avoid using a Rust helper
for.
We could reimplement it by first computing the number of longs and
then computing the number of bytes
const fn bitmap_size(nbits: usize) -> usize {
nbits.div_ceil(c_ulong::BITS) * size_of::<c_ulong>()
}
Thoughts?
Alice
Powered by blists - more mailing lists