| 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: <CAH5fLggXT70-V8hddtAobfgYyNOjhSYCsjrHoDWff9FZN5Y5-w@mail.gmail.com>
Date: Fri, 28 Feb 2025 10:16:41 +0100
From: Alice Ryhl <aliceryhl@...gle.com>
To: Yury Norov <yury.norov@...il.com>
Cc: Burak Emir <bqe@...gle.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] rust: add bindings and API for bitmap.h and bitops.h.
On Thu, Feb 27, 2025 at 7:30 PM Yury Norov <yury.norov@...il.com> wrote:
>
> On Thu, Feb 27, 2025 at 10:08:41AM +0000, Burak Emir wrote:
> > Adds a bitmap and bitops bindings and bitmap Rust API.
> > 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 bitmaps
> > 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.
> >
> > 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.
> >
> > Adds F: entries to MAINTAINERS.
> >
> > Suggested-by: Alice Ryhl <aliceryhl@...gle.com>
> > Signed-off-by: Burak Emir <bqe@...gle.com>
> > ---
> > This series adds a Rust abstraction for bitmap, and binding helpers
> > for inline methods of bitmap.h bitops.h.
> >
> > It depends on [1] and [2] which add bitmap helpers, MAINTAINERS entries
> > and an abstraction that is part of the bitmaps Rust API.
> >
> > Question for Yury: What would you like us to do for the MAINTAINERS
> > file? For now I just added the new files as F: entries.
> >
> > [1] https://lore.kernel.org/all/20250224233938.3158-1-yury.norov@gmail.com/
> > [2] https://lore.kernel.org/all/cover.1740475625.git.viresh.kumar@linaro.org/
> >
> > MAINTAINERS | 4 +
> > rust/bindings/bindings_helper.h | 1 +
> > rust/helpers/bitmap.c | 8 ++
> > rust/helpers/bitops.c | 13 +++
> > rust/helpers/find.c | 15 +++
> > rust/helpers/helpers.c | 3 +
> > rust/kernel/bitmap.rs | 182 ++++++++++++++++++++++++++++++++
> > rust/kernel/lib.rs | 1 +
> > 8 files changed, 227 insertions(+)
> > create mode 100644 rust/helpers/bitmap.c
> > create mode 100644 rust/helpers/bitops.c
> > create mode 100644 rust/helpers/find.c
> > create mode 100644 rust/kernel/bitmap.rs
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 6d6e55d8593b..359f09e8e2c0 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -4033,12 +4033,16 @@ BITMAP API BINDINGS [RUST]
> > M: Yury Norov <yury.norov@...il.com>
> > S: Maintained
> > F: rust/helpers/cpumask.c
> > +F: rust/helpers/find.c
> > +F: rust/helpers/bitmap.c
> > +F: rust/helpers/bitops.c
>
> bitops.c is part of BITOPS API, not the BITMAP one. I think we need a
> new record for it?
>
> >
> > BITMAP API [RUST]
> > M: Viresh Kumar <viresh.kumar@...aro.org> (cpumask)
> > R: Yury Norov <yury.norov@...il.com>
> > S: Maintained
> > F: rust/kernel/cpumask.rs
> > +F: rust/kernel/bitmap.rs
> >
> > BITOPS API
> > M: Yury Norov <yury.norov@...il.com>
> > 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);
> > +}
> > diff --git a/rust/helpers/bitops.c b/rust/helpers/bitops.c
> > new file mode 100644
> > index 000000000000..191ef0341fd5
> > --- /dev/null
> > +++ b/rust/helpers/bitops.c
> > @@ -0,0 +1,13 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <linux/bitops.h>
> > +
> > +void rust_helper_set_bit(unsigned int nr, volatile unsigned long *addr)
> > +{
> > + set_bit(nr, addr);
> > +}
> > +
> > +void rust_helper_clear_bit(unsigned int nr, volatile unsigned long *addr)
> > +{
> > + clear_bit(nr, addr);
> > +}
>
> So you mention that you're rewriting dbitmap, and I took a brief look
> at drivers/android/dbitmap.h.
>
> What I can say is that at least dbitmap_acquire_next_zero_bit() abuses
> the set_bit() API. It should use non-atomic __set_bit(). If you're
> going to re-write it, can you review the existing code and make sure
> you're using the right API in a right way?
Huh, I did not realize the method was atomic. I had assumed it wasn't
from how Binder is using it.
> > diff --git a/rust/helpers/find.c b/rust/helpers/find.c
> > new file mode 100644
> > index 000000000000..3841d3f0330f
> > --- /dev/null
> > +++ b/rust/helpers/find.c
> > @@ -0,0 +1,15 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <linux/find.h>
> > +
> > +unsigned long rust_helper_find_last_bit(const unsigned long *addr, unsigned long size)
> > +{
> > + return find_last_bit(addr, size);
> > +}
> > +
> > +
> > +unsigned long rust_helper_find_next_zero_bit(const unsigned long *addr, unsigned long size,
> > + unsigned long offset)
> > +{
> > + return find_next_zero_bit(addr, size, offset);
> > +}
>
> For those two, the find_*_bit() are the wrappers around _find_*_bit()
> in lib/find_bit.c, with a few exceptions. The reason for having the
> wrappers is an optimization that improves code generation for small
> bitmaps.
>
> In your case, when you wrap a macro, the optimization becomes impossible,
> and the macro is unneeded.
>
> From technical perspective, the underscored _find_*_bit() functions
> should be accessible by rust directly. Can you confirm that? If so,
> maybe just use them and avoid bloating?
That sounds likely.
Alice
Powered by blists - more mailing lists