| 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: <Z8CvLBwKeH2gM-2u@thinkpad>
Date: Thu, 27 Feb 2025 13:30:04 -0500
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] rust: add bindings and API for bitmap.h and bitops.h.
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?
> 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?
Thanks,
Yury
Powered by blists - more mailing lists