From 520a56aac1d9b6719462694c2696a9ed59c86132 Mon Sep 17 00:00:00 2001 From: Boqun Feng Date: Tue, 13 Jun 2023 09:42:58 -0700 Subject: [PATCH 01/37] rust: allocator: Prevents mis-aligned allocation Currently the KernelAllocator simply passes the size of the type Layout to krealloc(), and in theory the alignment requirement from the type Layout may be larger than the guarantee provided by SLAB, which means the allocated object is mis-aligned. Fixes this by adjusting the allocation size to the nearest power of two, which SLAB always guarantees a size-aligned allocation. And because Rust guarantees that original size must be a multiple of alignment and the alignment must be a power of two, then the alignment requirement is satisfied. Suggested-by: Vlastimil Babka Co-developed-by: Andreas Hindborg (Samsung) Signed-off-by: Andreas Hindborg (Samsung) Signed-off-by: Boqun Feng Cc: stable@vger.kernel.org # v6.1+ Reviewed-by: Martin Rodriguez Reboredo Reviewed-by: Benno Lossin Reviewed-by: Gary Guo Reviewed-by: Alice Ryhl Link: https://lore.kernel.org/r/20230613164258.3831917-1-boqun.feng@gmail.com --- rust/bindings/bindings_helper.h | 1 + rust/kernel/allocator.rs | 17 ++++++++++++++++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h index 3e601ce2548d4f..6619ce95dd3743 100644 --- a/rust/bindings/bindings_helper.h +++ b/rust/bindings/bindings_helper.h @@ -15,3 +15,4 @@ /* `bindgen` gets confused at certain things. */ const gfp_t BINDINGS_GFP_KERNEL = GFP_KERNEL; const gfp_t BINDINGS___GFP_ZERO = __GFP_ZERO; +const size_t BINDINGS_ARCH_SLAB_MINALIGN = ARCH_SLAB_MINALIGN; diff --git a/rust/kernel/allocator.rs b/rust/kernel/allocator.rs index 397a3dd57a9b13..66575cf87ce2f2 100644 --- a/rust/kernel/allocator.rs +++ b/rust/kernel/allocator.rs @@ -11,9 +11,24 @@ struct KernelAllocator; unsafe impl GlobalAlloc for KernelAllocator { unsafe fn alloc(&self, layout: Layout) -> *mut u8 { + // Customized layouts from `Layout::from_size_align()` can have size < align, so pads first. + let layout = layout.pad_to_align(); + + let mut size = layout.size(); + + if layout.align() > bindings::BINDINGS_ARCH_SLAB_MINALIGN { + // The alignment requirement exceeds the slab guarantee, then tries to enlarges the size + // to use the "power-of-two" size/alignment guarantee (see comments in kmalloc() for + // more information). + // + // Note that `layout.size()` (after padding) is guaranteed to be muliples of + // `layout.align()`, so `next_power_of_two` gives enough alignment guarantee. + size = size.next_power_of_two(); + } + // `krealloc()` is used instead of `kmalloc()` because the latter is // an inline function and cannot be bound to as a result. - unsafe { bindings::krealloc(ptr::null(), layout.size(), bindings::GFP_KERNEL) as *mut u8 } + unsafe { bindings::krealloc(ptr::null(), size, bindings::GFP_KERNEL) as *mut u8 } } unsafe fn dealloc(&self, ptr: *mut u8, _layout: Layout) { From 3d8d9bc41172bcd5fb99bbcebc5b686707869dad Mon Sep 17 00:00:00 2001 From: Alice Ryhl Date: Wed, 14 Jun 2023 11:53:28 +0000 Subject: [PATCH 02/37] rust: make `UnsafeCell` the outer type in `Opaque` When combining `UnsafeCell` with `MaybeUninit`, it is idiomatic to use `UnsafeCell` as the outer type. Intuitively, this is because a `MaybeUninit` might not contain a `T`, but we always want the effect of the `UnsafeCell`, even if the inner value is uninitialized. Now, strictly speaking, this doesn't really make a difference. The compiler will always apply the `UnsafeCell` effect even if the inner value is uninitialized. But I think we should follow the convention here. Signed-off-by: Alice Ryhl Reviewed-by: Gary Guo Reviewed-by: Martin Rodriguez Reboredo Reviewed-by: Benno Lossin Link: https://lore.kernel.org/r/20230614115328.2825961-1-aliceryhl@google.com --- rust/kernel/types.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs index 1e5380b16ed553..fb41635f1e1f47 100644 --- a/rust/kernel/types.rs +++ b/rust/kernel/types.rs @@ -224,17 +224,17 @@ impl Drop for ScopeGuard { /// /// This is meant to be used with FFI objects that are never interpreted by Rust code. #[repr(transparent)] -pub struct Opaque(MaybeUninit>); +pub struct Opaque(UnsafeCell>); impl Opaque { /// Creates a new opaque value. pub const fn new(value: T) -> Self { - Self(MaybeUninit::new(UnsafeCell::new(value))) + Self(UnsafeCell::new(MaybeUninit::new(value))) } /// Creates an uninitialised value. pub const fn uninit() -> Self { - Self(MaybeUninit::uninit()) + Self(UnsafeCell::new(MaybeUninit::uninit())) } /// Creates a pin-initializer from the given initializer closure. @@ -258,7 +258,7 @@ impl Opaque { /// Returns a raw pointer to the opaque data. pub fn get(&self) -> *mut T { - UnsafeCell::raw_get(self.0.as_ptr()) + UnsafeCell::get(&self.0).cast::() } /// Gets the value behind `this`. @@ -266,7 +266,7 @@ impl Opaque { /// This function is useful to get access to the value without creating intermediate /// references. pub const fn raw_get(this: *const Self) -> *mut T { - UnsafeCell::raw_get(this.cast::>()) + UnsafeCell::raw_get(this.cast::>>()).cast::() } } From 76715278d827c965a816184270288219b9f1de5f Mon Sep 17 00:00:00 2001 From: Benno Lossin Date: Fri, 30 Jun 2023 15:03:23 +0000 Subject: [PATCH 03/37] rust: types: make `Opaque` be `!Unpin` Adds a `PhantomPinned` field to `Opaque`. This removes the last Rust guarantee: the assumption that the type `T` can be freely moved. This is not the case for many types from the C side (e.g. if they contain a `struct list_head`). This change removes the need to add a `PhantomPinned` field manually to Rust structs that contain C structs which must not be moved. Signed-off-by: Benno Lossin Reviewed-by: Gary Guo Reviewed-by: Alice Ryhl Link: https://lore.kernel.org/r/20230630150216.109789-1-benno.lossin@proton.me --- rust/kernel/types.rs | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs index fb41635f1e1f47..e664a2beef3044 100644 --- a/rust/kernel/types.rs +++ b/rust/kernel/types.rs @@ -6,7 +6,7 @@ use crate::init::{self, PinInit}; use alloc::boxed::Box; use core::{ cell::UnsafeCell, - marker::PhantomData, + marker::{PhantomData, PhantomPinned}, mem::MaybeUninit, ops::{Deref, DerefMut}, ptr::NonNull, @@ -224,17 +224,26 @@ impl Drop for ScopeGuard { /// /// This is meant to be used with FFI objects that are never interpreted by Rust code. #[repr(transparent)] -pub struct Opaque(UnsafeCell>); +pub struct Opaque { + value: UnsafeCell>, + _pin: PhantomPinned, +} impl Opaque { /// Creates a new opaque value. pub const fn new(value: T) -> Self { - Self(UnsafeCell::new(MaybeUninit::new(value))) + Self { + value: UnsafeCell::new(MaybeUninit::new(value)), + _pin: PhantomPinned, + } } /// Creates an uninitialised value. pub const fn uninit() -> Self { - Self(UnsafeCell::new(MaybeUninit::uninit())) + Self { + value: UnsafeCell::new(MaybeUninit::uninit()), + _pin: PhantomPinned, + } } /// Creates a pin-initializer from the given initializer closure. @@ -258,7 +267,7 @@ impl Opaque { /// Returns a raw pointer to the opaque data. pub fn get(&self) -> *mut T { - UnsafeCell::get(&self.0).cast::() + UnsafeCell::get(&self.value).cast::() } /// Gets the value behind `this`. From 0ad204c30ef0a27e92081b26b679ac05664b9dc0 Mon Sep 17 00:00:00 2001 From: Masahiro Yamada Date: Fri, 16 Jun 2023 02:16:21 +0200 Subject: [PATCH 04/37] kbuild: rust_is_available: remove -v option The -v option is passed when this script is invoked from Makefile, but not when invoked from Kconfig. As you can see in scripts/Kconfig.include, the 'success' macro suppresses stdout and stderr anyway, so this script does not need to be quiet. Signed-off-by: Masahiro Yamada Reviewed-by: Miguel Ojeda Tested-by: Miguel Ojeda Reviewed-by: Nathan Chancellor Link: https://lore.kernel.org/r/20230109061436.3146442-1-masahiroy@kernel.org [ Reworded prefix to match the others in the patch series. ] Signed-off-by: Miguel Ojeda Reviewed-by: Martin Rodriguez Reboredo Link: https://lore.kernel.org/r/20230616001631.463536-2-ojeda@kernel.org --- Makefile | 4 +- scripts/rust_is_available.sh | 96 +++++++++++++++--------------------- 2 files changed, 42 insertions(+), 58 deletions(-) diff --git a/Makefile b/Makefile index 836643eaefee0e..3c5775ae78b834 100644 --- a/Makefile +++ b/Makefile @@ -1289,7 +1289,7 @@ prepare0: archprepare # All the preparing.. prepare: prepare0 ifdef CONFIG_RUST - $(Q)$(CONFIG_SHELL) $(srctree)/scripts/rust_is_available.sh -v + $(Q)$(CONFIG_SHELL) $(srctree)/scripts/rust_is_available.sh $(Q)$(MAKE) $(build)=rust endif @@ -1823,7 +1823,7 @@ $(DOC_TARGETS): # "Is Rust available?" target PHONY += rustavailable rustavailable: - $(Q)$(CONFIG_SHELL) $(srctree)/scripts/rust_is_available.sh -v && echo "Rust is available!" + $(Q)$(CONFIG_SHELL) $(srctree)/scripts/rust_is_available.sh && echo "Rust is available!" # Documentation target # diff --git a/scripts/rust_is_available.sh b/scripts/rust_is_available.sh index aebbf19139709d..f43a010eaf3050 100755 --- a/scripts/rust_is_available.sh +++ b/scripts/rust_is_available.sh @@ -2,8 +2,6 @@ # SPDX-License-Identifier: GPL-2.0 # # Tests whether a suitable Rust toolchain is available. -# -# Pass `-v` for human output and more checks (as warnings). set -e @@ -23,21 +21,17 @@ get_canonical_version() # Check that the Rust compiler exists. if ! command -v "$RUSTC" >/dev/null; then - if [ "$1" = -v ]; then - echo >&2 "***" - echo >&2 "*** Rust compiler '$RUSTC' could not be found." - echo >&2 "***" - fi + echo >&2 "***" + echo >&2 "*** Rust compiler '$RUSTC' could not be found." + echo >&2 "***" exit 1 fi # Check that the Rust bindings generator exists. if ! command -v "$BINDGEN" >/dev/null; then - if [ "$1" = -v ]; then - echo >&2 "***" - echo >&2 "*** Rust bindings generator '$BINDGEN' could not be found." - echo >&2 "***" - fi + echo >&2 "***" + echo >&2 "*** Rust bindings generator '$BINDGEN' could not be found." + echo >&2 "***" exit 1 fi @@ -53,16 +47,14 @@ rust_compiler_min_version=$($min_tool_version rustc) rust_compiler_cversion=$(get_canonical_version $rust_compiler_version) rust_compiler_min_cversion=$(get_canonical_version $rust_compiler_min_version) if [ "$rust_compiler_cversion" -lt "$rust_compiler_min_cversion" ]; then - if [ "$1" = -v ]; then - echo >&2 "***" - echo >&2 "*** Rust compiler '$RUSTC' is too old." - echo >&2 "*** Your version: $rust_compiler_version" - echo >&2 "*** Minimum version: $rust_compiler_min_version" - echo >&2 "***" - fi + echo >&2 "***" + echo >&2 "*** Rust compiler '$RUSTC' is too old." + echo >&2 "*** Your version: $rust_compiler_version" + echo >&2 "*** Minimum version: $rust_compiler_min_version" + echo >&2 "***" exit 1 fi -if [ "$1" = -v ] && [ "$rust_compiler_cversion" -gt "$rust_compiler_min_cversion" ]; then +if [ "$rust_compiler_cversion" -gt "$rust_compiler_min_cversion" ]; then echo >&2 "***" echo >&2 "*** Rust compiler '$RUSTC' is too new. This may or may not work." echo >&2 "*** Your version: $rust_compiler_version" @@ -82,16 +74,14 @@ rust_bindings_generator_min_version=$($min_tool_version bindgen) rust_bindings_generator_cversion=$(get_canonical_version $rust_bindings_generator_version) rust_bindings_generator_min_cversion=$(get_canonical_version $rust_bindings_generator_min_version) if [ "$rust_bindings_generator_cversion" -lt "$rust_bindings_generator_min_cversion" ]; then - if [ "$1" = -v ]; then - echo >&2 "***" - echo >&2 "*** Rust bindings generator '$BINDGEN' is too old." - echo >&2 "*** Your version: $rust_bindings_generator_version" - echo >&2 "*** Minimum version: $rust_bindings_generator_min_version" - echo >&2 "***" - fi + echo >&2 "***" + echo >&2 "*** Rust bindings generator '$BINDGEN' is too old." + echo >&2 "*** Your version: $rust_bindings_generator_version" + echo >&2 "*** Minimum version: $rust_bindings_generator_min_version" + echo >&2 "***" exit 1 fi -if [ "$1" = -v ] && [ "$rust_bindings_generator_cversion" -gt "$rust_bindings_generator_min_cversion" ]; then +if [ "$rust_bindings_generator_cversion" -gt "$rust_bindings_generator_min_cversion" ]; then echo >&2 "***" echo >&2 "*** Rust bindings generator '$BINDGEN' is too new. This may or may not work." echo >&2 "*** Your version: $rust_bindings_generator_version" @@ -110,13 +100,11 @@ bindgen_libclang_min_version=$($min_tool_version llvm) bindgen_libclang_cversion=$(get_canonical_version $bindgen_libclang_version) bindgen_libclang_min_cversion=$(get_canonical_version $bindgen_libclang_min_version) if [ "$bindgen_libclang_cversion" -lt "$bindgen_libclang_min_cversion" ]; then - if [ "$1" = -v ]; then - echo >&2 "***" - echo >&2 "*** libclang (used by the Rust bindings generator '$BINDGEN') is too old." - echo >&2 "*** Your version: $bindgen_libclang_version" - echo >&2 "*** Minimum version: $bindgen_libclang_min_version" - echo >&2 "***" - fi + echo >&2 "***" + echo >&2 "*** libclang (used by the Rust bindings generator '$BINDGEN') is too old." + echo >&2 "*** Your version: $bindgen_libclang_version" + echo >&2 "*** Minimum version: $bindgen_libclang_min_version" + echo >&2 "***" exit 1 fi @@ -125,21 +113,19 @@ fi # # In the future, we might be able to perform a full version check, see # https://github.com/rust-lang/rust-bindgen/issues/2138. -if [ "$1" = -v ]; then - cc_name=$($(dirname $0)/cc-version.sh "$CC" | cut -f1 -d' ') - if [ "$cc_name" = Clang ]; then - clang_version=$( \ - LC_ALL=C "$CC" --version 2>/dev/null \ - | sed -nE '1s:.*version ([0-9]+\.[0-9]+\.[0-9]+).*:\1:p' - ) - if [ "$clang_version" != "$bindgen_libclang_version" ]; then - echo >&2 "***" - echo >&2 "*** libclang (used by the Rust bindings generator '$BINDGEN')" - echo >&2 "*** version does not match Clang's. This may be a problem." - echo >&2 "*** libclang version: $bindgen_libclang_version" - echo >&2 "*** Clang version: $clang_version" - echo >&2 "***" - fi +cc_name=$($(dirname $0)/cc-version.sh "$CC" | cut -f1 -d' ') +if [ "$cc_name" = Clang ]; then + clang_version=$( \ + LC_ALL=C "$CC" --version 2>/dev/null \ + | sed -nE '1s:.*version ([0-9]+\.[0-9]+\.[0-9]+).*:\1:p' + ) + if [ "$clang_version" != "$bindgen_libclang_version" ]; then + echo >&2 "***" + echo >&2 "*** libclang (used by the Rust bindings generator '$BINDGEN')" + echo >&2 "*** version does not match Clang's. This may be a problem." + echo >&2 "*** libclang version: $bindgen_libclang_version" + echo >&2 "*** Clang version: $clang_version" + echo >&2 "***" fi fi @@ -150,11 +136,9 @@ rustc_sysroot=$("$RUSTC" $KRUSTFLAGS --print sysroot) rustc_src=${RUST_LIB_SRC:-"$rustc_sysroot/lib/rustlib/src/rust/library"} rustc_src_core="$rustc_src/core/src/lib.rs" if [ ! -e "$rustc_src_core" ]; then - if [ "$1" = -v ]; then - echo >&2 "***" - echo >&2 "*** Source code for the 'core' standard library could not be found" - echo >&2 "*** at '$rustc_src_core'." - echo >&2 "***" - fi + echo >&2 "***" + echo >&2 "*** Source code for the 'core' standard library could not be found" + echo >&2 "*** at '$rustc_src_core'." + echo >&2 "***" exit 1 fi From 70a61eb35a4c6ec6df477a30816bd6008d06ab38 Mon Sep 17 00:00:00 2001 From: Russell Currey Date: Fri, 16 Jun 2023 02:16:22 +0200 Subject: [PATCH 05/37] kbuild: rust_is_available: fix version check when CC has multiple arguments rust_is_available.sh uses cc-version.sh to identify which C compiler is in use, as scripts/Kconfig.include does. cc-version.sh isn't designed to be able to handle multiple arguments in one variable, i.e. "ccache clang". Its invocation in rust_is_available.sh quotes "$CC", which makes $1 == "ccache clang" instead of the intended $1 == ccache & $2 == clang. cc-version.sh could also be changed to handle having "ccache clang" as one argument, but it only has the one consumer upstream, making it simpler to fix the caller here. Signed-off-by: Russell Currey Fixes: 78521f3399ab ("scripts: add `rust_is_available.sh`") Link: https://github.com/Rust-for-Linux/linux/pull/873 [ Reworded title prefix and reflow line to 75 columns. ] Signed-off-by: Miguel Ojeda Reviewed-by: Nathan Chancellor Reviewed-by: Martin Rodriguez Reboredo Link: https://lore.kernel.org/r/20230616001631.463536-3-ojeda@kernel.org --- scripts/rust_is_available.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/rust_is_available.sh b/scripts/rust_is_available.sh index f43a010eaf3050..0c9be438e4cd33 100755 --- a/scripts/rust_is_available.sh +++ b/scripts/rust_is_available.sh @@ -113,10 +113,10 @@ fi # # In the future, we might be able to perform a full version check, see # https://github.com/rust-lang/rust-bindgen/issues/2138. -cc_name=$($(dirname $0)/cc-version.sh "$CC" | cut -f1 -d' ') +cc_name=$($(dirname $0)/cc-version.sh $CC | cut -f1 -d' ') if [ "$cc_name" = Clang ]; then clang_version=$( \ - LC_ALL=C "$CC" --version 2>/dev/null \ + LC_ALL=C $CC --version 2>/dev/null \ | sed -nE '1s:.*version ([0-9]+\.[0-9]+\.[0-9]+).*:\1:p' ) if [ "$clang_version" != "$bindgen_libclang_version" ]; then From a250bde0ba55c1e58643fc273e7d0d74a5662103 Mon Sep 17 00:00:00 2001 From: Miguel Ojeda Date: Fri, 16 Jun 2023 02:16:23 +0200 Subject: [PATCH 06/37] docs: rust: add paragraph about finding a suitable `libclang` Sometimes users need to tweak the finding process of `libclang` for `bindgen` via the `clang-sys`-provided environment variables. Thus add a paragraph to the setting up guide, including a reference to `clang-sys`'s relevant documentation. Link: https://lore.kernel.org/rust-for-linux/CAKwvOdm5JT4wbdQQYuW+RT07rCi6whGBM2iUAyg8A1CmLXG6Nw@mail.gmail.com/ Reviewed-by: Nick Desaulniers Signed-off-by: Miguel Ojeda Reviewed-by: Nathan Chancellor Reviewed-by: Martin Rodriguez Reboredo Link: https://lore.kernel.org/r/20230616001631.463536-4-ojeda@kernel.org --- Documentation/rust/quick-start.rst | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/Documentation/rust/quick-start.rst b/Documentation/rust/quick-start.rst index a8931512ed9898..58a183bb90b1e3 100644 --- a/Documentation/rust/quick-start.rst +++ b/Documentation/rust/quick-start.rst @@ -100,6 +100,23 @@ Install it via (note that this will download and build the tool from source):: cargo install --locked --version $(scripts/min-tool-version.sh bindgen) bindgen +``bindgen`` needs to find a suitable ``libclang`` in order to work. If it is +not found (or a different ``libclang`` than the one found should be used), +the process can be tweaked using the environment variables understood by +``clang-sys`` (the Rust bindings crate that ``bindgen`` uses to access +``libclang``): + +* ``LLVM_CONFIG_PATH`` can be pointed to an ``llvm-config`` executable. + +* Or ``LIBCLANG_PATH`` can be pointed to a ``libclang`` shared library + or to the directory containing it. + +* Or ``CLANG_PATH`` can be pointed to a ``clang`` executable. + +For details, please see ``clang-sys``'s documentation at: + + https://github.com/KyleMayes/clang-sys#environment-variables + Requirements: Developing ------------------------ From 00383f709315b33365f0344469e37b1953b3a355 Mon Sep 17 00:00:00 2001 From: Miguel Ojeda Date: Fri, 16 Jun 2023 02:16:24 +0200 Subject: [PATCH 07/37] kbuild: rust_is_available: print docs reference People trying out the Rust support in the kernel may get warnings and errors from `scripts/rust_is_available.sh` from the `rustavailable` target or the build step. Some of those users may be following the Quick Start guide, but others may not (likely those getting warnings from the build step instead of the target). While the messages are fairly clear on what the problem is, it may not be clear how to solve the particular issue, especially for those not aware of the documentation. We could add all sorts of details on the script for each one, but it is better to point users to the documentation instead, where it is easily readable in different formats. It also avoids duplication. Thus add a reference to the documentation whenever the script fails or there is at least a warning. Reviewed-by: Finn Behrens Signed-off-by: Miguel Ojeda Reviewed-by: Masahiro Yamada Reviewed-by: Nathan Chancellor Reviewed-by: Martin Rodriguez Reboredo Link: https://lore.kernel.org/r/20230616001631.463536-5-ojeda@kernel.org --- scripts/rust_is_available.sh | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/scripts/rust_is_available.sh b/scripts/rust_is_available.sh index 0c9be438e4cd33..6b8131d5b54791 100755 --- a/scripts/rust_is_available.sh +++ b/scripts/rust_is_available.sh @@ -19,6 +19,20 @@ get_canonical_version() echo $((100000 * $1 + 100 * $2 + $3)) } +# Print a reference to the Quick Start guide in the documentation. +print_docs_reference() +{ + echo >&2 "***" + echo >&2 "*** Please see Documentation/rust/quick-start.rst for details" + echo >&2 "*** on how to set up the Rust support." + echo >&2 "***" +} + +# If the script fails for any reason, or if there was any warning, then +# print a reference to the documentation on exit. +warning=0 +trap 'if [ $? -ne 0 ] || [ $warning -ne 0 ]; then print_docs_reference; fi' EXIT + # Check that the Rust compiler exists. if ! command -v "$RUSTC" >/dev/null; then echo >&2 "***" @@ -60,6 +74,7 @@ if [ "$rust_compiler_cversion" -gt "$rust_compiler_min_cversion" ]; then echo >&2 "*** Your version: $rust_compiler_version" echo >&2 "*** Expected version: $rust_compiler_min_version" echo >&2 "***" + warning=1 fi # Check that the Rust bindings generator is suitable. @@ -87,6 +102,7 @@ if [ "$rust_bindings_generator_cversion" -gt "$rust_bindings_generator_min_cvers echo >&2 "*** Your version: $rust_bindings_generator_version" echo >&2 "*** Expected version: $rust_bindings_generator_min_version" echo >&2 "***" + warning=1 fi # Check that the `libclang` used by the Rust bindings generator is suitable. @@ -126,6 +142,7 @@ if [ "$cc_name" = Clang ]; then echo >&2 "*** libclang version: $bindgen_libclang_version" echo >&2 "*** Clang version: $clang_version" echo >&2 "***" + warning=1 fi fi From 8b2e73a960f09c70885810796dc3a662999cf50d Mon Sep 17 00:00:00 2001 From: Miguel Ojeda Date: Fri, 16 Jun 2023 02:16:25 +0200 Subject: [PATCH 08/37] kbuild: rust_is_available: add check for `bindgen` invocation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `scripts/rust_is_available.sh` calls `bindgen` with a special header in order to check whether the `libclang` version in use is suitable. However, the invocation itself may fail if, for instance, `bindgen` cannot locate `libclang`. This is fine for Kconfig (since the script will still fail and therefore disable Rust as it should), but it is pretty confusing for users of the `rustavailable` target given the error will be unrelated: ./scripts/rust_is_available.sh: 21: arithmetic expression: expecting primary: "100000 * + 100 * + " make: *** [Makefile:1816: rustavailable] Error 2 Instead, run the `bindgen` invocation independently in a previous step, saving its output and return code. If it fails, then show the user a proper error message. Otherwise, continue as usual with the saved output. Since the previous patch we show a reference to the docs, and the docs now explain how `bindgen` looks for `libclang`, thus the error message can leverage the documentation, avoiding duplication here (and making users aware of the setup guide in the documentation). Reported-by: Nick Desaulniers Link: https://lore.kernel.org/rust-for-linux/CAKwvOdm5JT4wbdQQYuW+RT07rCi6whGBM2iUAyg8A1CmLXG6Nw@mail.gmail.com/ Reported-by: François Valenduc Closes: https://github.com/Rust-for-Linux/linux/issues/934 Reported-by: Alexandru Radovici Closes: https://github.com/Rust-for-Linux/linux/pull/921 Reported-by: Matthew Leach Closes: https://lore.kernel.org/rust-for-linux/20230507084116.1099067-1-dev@mattleach.net/ Fixes: 78521f3399ab ("scripts: add `rust_is_available.sh`") Signed-off-by: Miguel Ojeda Reviewed-by: Masahiro Yamada Reviewed-by: Nathan Chancellor Reviewed-by: Martin Rodriguez Reboredo Link: https://lore.kernel.org/r/20230616001631.463536-6-ojeda@kernel.org --- scripts/rust_is_available.sh | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/scripts/rust_is_available.sh b/scripts/rust_is_available.sh index 6b8131d5b54791..1bdff4472cbe44 100755 --- a/scripts/rust_is_available.sh +++ b/scripts/rust_is_available.sh @@ -106,8 +106,28 @@ if [ "$rust_bindings_generator_cversion" -gt "$rust_bindings_generator_min_cvers fi # Check that the `libclang` used by the Rust bindings generator is suitable. +# +# In order to do that, first invoke `bindgen` to get the `libclang` version +# found by `bindgen`. This step may already fail if, for instance, `libclang` +# is not found, thus inform the user in such a case. +bindgen_libclang_output=$( \ + LC_ALL=C "$BINDGEN" $(dirname $0)/rust_is_available_bindgen_libclang.h 2>&1 >/dev/null +) || bindgen_libclang_code=$? +if [ -n "$bindgen_libclang_code" ]; then + echo >&2 "***" + echo >&2 "*** Running '$BINDGEN' to check the libclang version (used by the Rust" + echo >&2 "*** bindings generator) failed with code $bindgen_libclang_code. This may be caused by" + echo >&2 "*** a failure to locate libclang. See output and docs below for details:" + echo >&2 "***" + echo >&2 "$bindgen_libclang_output" + echo >&2 "***" + exit 1 +fi + +# `bindgen` returned successfully, thus use the output to check that the version +# of the `libclang` found by the Rust bindings generator is suitable. bindgen_libclang_version=$( \ - LC_ALL=C "$BINDGEN" $(dirname $0)/rust_is_available_bindgen_libclang.h 2>&1 >/dev/null \ + echo "$bindgen_libclang_output" \ | grep -F 'clang version ' \ | grep -oE '[0-9]+\.[0-9]+\.[0-9]+' \ | head -n 1 \ From eb9199d326359feda203f1c26547cfaad3136b77 Mon Sep 17 00:00:00 2001 From: Miguel Ojeda Date: Fri, 16 Jun 2023 02:16:26 +0200 Subject: [PATCH 09/37] kbuild: rust_is_available: check that environment variables are set Sometimes [1] users may attempt to setup the Rust support by checking what Kbuild does and they end up finding out about `scripts/rust_is_available.sh`. Inevitably, they run the script directly, but unless they setup the required variables, the result of the script is not meaningful. We could add some defaults to the variables, but that could be confusing for those that may override the defaults (compared to their kernel builds), and `$CC` would not be a simple default in any case. Therefore, instead, explicitly check whether the expected variables are set (`$RUSTC`, `$BINDGEN` and `$CC`). If not, print an explanation about the fact that the script is meant to be called from Kbuild, since that is the most likely cause for the variables not being set. Link: https://lore.kernel.org/oe-kbuild-all/Y6r4mXz5NS0+HVXo@zn.tnic/ [1] Signed-off-by: Miguel Ojeda Reviewed-by: Nathan Chancellor Reviewed-by: Martin Rodriguez Reboredo Link: https://lore.kernel.org/r/20230616001631.463536-7-ojeda@kernel.org --- scripts/rust_is_available.sh | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/scripts/rust_is_available.sh b/scripts/rust_is_available.sh index 1bdff4472cbe44..7e0368babe643b 100755 --- a/scripts/rust_is_available.sh +++ b/scripts/rust_is_available.sh @@ -28,11 +28,40 @@ print_docs_reference() echo >&2 "***" } +# Print an explanation about the fact that the script is meant to be called from Kbuild. +print_kbuild_explanation() +{ + echo >&2 "***" + echo >&2 "*** This script is intended to be called from Kbuild." + echo >&2 "*** Please use the 'rustavailable' target to call it instead." + echo >&2 "*** Otherwise, the results may not be meaningful." + exit 1 +} + # If the script fails for any reason, or if there was any warning, then # print a reference to the documentation on exit. warning=0 trap 'if [ $? -ne 0 ] || [ $warning -ne 0 ]; then print_docs_reference; fi' EXIT +# Check that the expected environment variables are set. +if [ -z "${RUSTC+x}" ]; then + echo >&2 "***" + echo >&2 "*** Environment variable 'RUSTC' is not set." + print_kbuild_explanation +fi + +if [ -z "${BINDGEN+x}" ]; then + echo >&2 "***" + echo >&2 "*** Environment variable 'BINDGEN' is not set." + print_kbuild_explanation +fi + +if [ -z "${CC+x}" ]; then + echo >&2 "***" + echo >&2 "*** Environment variable 'CC' is not set." + print_kbuild_explanation +fi + # Check that the Rust compiler exists. if ! command -v "$RUSTC" >/dev/null; then echo >&2 "***" From 6bed841e56c4342c272aa424be45d409e8ee8a47 Mon Sep 17 00:00:00 2001 From: Miguel Ojeda Date: Fri, 16 Jun 2023 02:16:27 +0200 Subject: [PATCH 10/37] kbuild: rust_is_available: fix confusion when a version appears in the path `bindgen`'s output for `libclang`'s version check contains paths, which in turn may contain strings that look like version numbers [1][2]: .../6.1.0-dev/.../rust_is_available_bindgen_libclang.h:2:9: warning: clang version 11.1.0 [-W#pragma-messages], err: false which the script will pick up as the version instead of the latter. It is also the case that versions may appear after the actual version (e.g. distribution's version text), which was the reason behind `head` [3]: .../rust-is-available-bindgen-libclang.h:2:9: warning: clang version 13.0.0 (Fedora 13.0.0-3.fc35) [-W#pragma-messages], err: false Thus instead ask for a match after the `clang version` string. Reported-by: Jordan Isaacs Closes: https://github.com/Rust-for-Linux/linux/issues/942 [1] Reported-by: Ethan D. Twardy Closes: https://lore.kernel.org/rust-for-linux/20230528131802.6390-2-ethan.twardy@gmail.com/ [2] Reported-by: Tiago Lam Closes: https://github.com/Rust-for-Linux/linux/pull/789 [3] Fixes: 78521f3399ab ("scripts: add `rust_is_available.sh`") Signed-off-by: Miguel Ojeda Reviewed-By: Ethan Twardy Tested-By: Ethan Twardy Reviewed-by: Nathan Chancellor Reviewed-by: Martin Rodriguez Reboredo Link: https://lore.kernel.org/r/20230616001631.463536-8-ojeda@kernel.org --- scripts/rust_is_available.sh | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/scripts/rust_is_available.sh b/scripts/rust_is_available.sh index 7e0368babe643b..810691af66ebd8 100755 --- a/scripts/rust_is_available.sh +++ b/scripts/rust_is_available.sh @@ -157,9 +157,7 @@ fi # of the `libclang` found by the Rust bindings generator is suitable. bindgen_libclang_version=$( \ echo "$bindgen_libclang_output" \ - | grep -F 'clang version ' \ - | grep -oE '[0-9]+\.[0-9]+\.[0-9]+' \ - | head -n 1 \ + | sed -nE 's:.*clang version ([0-9]+\.[0-9]+\.[0-9]+).*:\1:p' ) bindgen_libclang_min_version=$($min_tool_version llvm) bindgen_libclang_cversion=$(get_canonical_version $bindgen_libclang_version) From 600eb66899b46cb7e87e7ab01c8d01df7b29441e Mon Sep 17 00:00:00 2001 From: Miguel Ojeda Date: Fri, 16 Jun 2023 02:16:28 +0200 Subject: [PATCH 11/37] kbuild: rust_is_available: normalize version matching In order to match the version string, `sed` is used in a couple cases, and `grep` and `head` in a couple others. Make the script more consistent and easier to understand by using the same method, `sed`, for all of them. This makes the version matching also a bit more strict for the changed cases, since the strings `rustc ` and `bindgen ` will now be required, which should be fine since `rustc` complains if one attempts to call it with another program name, and `bindgen` uses a hardcoded string. In addition, clarify why one of the existing `sed` commands does not provide an address like the others. Signed-off-by: Miguel Ojeda Reviewed-by: Masahiro Yamada Reviewed-by: Nathan Chancellor Reviewed-by: Martin Rodriguez Reboredo Link: https://lore.kernel.org/r/20230616001631.463536-9-ojeda@kernel.org --- scripts/rust_is_available.sh | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/scripts/rust_is_available.sh b/scripts/rust_is_available.sh index 810691af66ebd8..b7e0781fdea982 100755 --- a/scripts/rust_is_available.sh +++ b/scripts/rust_is_available.sh @@ -83,8 +83,7 @@ fi # Non-stable and distributions' versions may have a version suffix, e.g. `-dev`. rust_compiler_version=$( \ LC_ALL=C "$RUSTC" --version 2>/dev/null \ - | head -n 1 \ - | grep -oE '[0-9]+\.[0-9]+\.[0-9]+' \ + | sed -nE '1s:.*rustc ([0-9]+\.[0-9]+\.[0-9]+).*:\1:p' ) rust_compiler_min_version=$($min_tool_version rustc) rust_compiler_cversion=$(get_canonical_version $rust_compiler_version) @@ -111,8 +110,7 @@ fi # Non-stable and distributions' versions may have a version suffix, e.g. `-dev`. rust_bindings_generator_version=$( \ LC_ALL=C "$BINDGEN" --version 2>/dev/null \ - | head -n 1 \ - | grep -oE '[0-9]+\.[0-9]+\.[0-9]+' \ + | sed -nE '1s:.*bindgen ([0-9]+\.[0-9]+\.[0-9]+).*:\1:p' ) rust_bindings_generator_min_version=$($min_tool_version bindgen) rust_bindings_generator_cversion=$(get_canonical_version $rust_bindings_generator_version) @@ -155,6 +153,9 @@ fi # `bindgen` returned successfully, thus use the output to check that the version # of the `libclang` found by the Rust bindings generator is suitable. +# +# Unlike other version checks, note that this one does not necessarily appear +# in the first line of the output, thus no `sed` address is provided. bindgen_libclang_version=$( \ echo "$bindgen_libclang_output" \ | sed -nE 's:.*clang version ([0-9]+\.[0-9]+\.[0-9]+).*:\1:p' From 6d4220b100b66b4e1e989d372cacfb67bc0a2588 Mon Sep 17 00:00:00 2001 From: Miguel Ojeda Date: Fri, 16 Jun 2023 02:16:29 +0200 Subject: [PATCH 12/37] kbuild: rust_is_available: handle failures calling `$RUSTC`/`$BINDGEN` The script already checks if `$RUSTC` and `$BINDGEN` exists via `command`, but the environment variables may point to a non-executable file, or the programs may fail for some other reason. While the script successfully exits with a failure as it should, the error given can be quite confusing depending on the shell and the behavior of its `command`. For instance, with `dash`: $ RUSTC=./mm BINDGEN=bindgen CC=clang scripts/rust_is_available.sh scripts/rust_is_available.sh: 19: arithmetic expression: expecting primary: "100000 * + 100 * + " Thus detect failure exit codes when calling `$RUSTC` and `$BINDGEN` and print a better message, in a similar way to what we do when extracting the `libclang` version found by `bindgen`. Link: https://lore.kernel.org/rust-for-linux/CAK7LNAQYk6s11MASRHW6oxtkqF00EJVqhHOP=5rynWt-QDUsXw@mail.gmail.com/ Signed-off-by: Miguel Ojeda Reviewed-by: Nathan Chancellor Reviewed-by: Martin Rodriguez Reboredo Link: https://lore.kernel.org/r/20230616001631.463536-10-ojeda@kernel.org --- scripts/rust_is_available.sh | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/scripts/rust_is_available.sh b/scripts/rust_is_available.sh index b7e0781fdea982..da8296cd9b8d1d 100755 --- a/scripts/rust_is_available.sh +++ b/scripts/rust_is_available.sh @@ -81,8 +81,20 @@ fi # Check that the Rust compiler version is suitable. # # Non-stable and distributions' versions may have a version suffix, e.g. `-dev`. +rust_compiler_output=$( \ + LC_ALL=C "$RUSTC" --version 2>/dev/null +) || rust_compiler_code=$? +if [ -n "$rust_compiler_code" ]; then + echo >&2 "***" + echo >&2 "*** Running '$RUSTC' to check the Rust compiler version failed with" + echo >&2 "*** code $rust_compiler_code. See output and docs below for details:" + echo >&2 "***" + echo >&2 "$rust_compiler_output" + echo >&2 "***" + exit 1 +fi rust_compiler_version=$( \ - LC_ALL=C "$RUSTC" --version 2>/dev/null \ + echo "$rust_compiler_output" \ | sed -nE '1s:.*rustc ([0-9]+\.[0-9]+\.[0-9]+).*:\1:p' ) rust_compiler_min_version=$($min_tool_version rustc) @@ -108,8 +120,20 @@ fi # Check that the Rust bindings generator is suitable. # # Non-stable and distributions' versions may have a version suffix, e.g. `-dev`. +rust_bindings_generator_output=$( \ + LC_ALL=C "$BINDGEN" --version 2>/dev/null +) || rust_bindings_generator_code=$? +if [ -n "$rust_bindings_generator_code" ]; then + echo >&2 "***" + echo >&2 "*** Running '$BINDGEN' to check the Rust bindings generator version failed with" + echo >&2 "*** code $rust_bindings_generator_code. See output and docs below for details:" + echo >&2 "***" + echo >&2 "$rust_bindings_generator_output" + echo >&2 "***" + exit 1 +fi rust_bindings_generator_version=$( \ - LC_ALL=C "$BINDGEN" --version 2>/dev/null \ + echo "$rust_bindings_generator_output" \ | sed -nE '1s:.*bindgen ([0-9]+\.[0-9]+\.[0-9]+).*:\1:p' ) rust_bindings_generator_min_version=$($min_tool_version bindgen) From 10452fc4a3deba904fa6807cd481c170e61d518c Mon Sep 17 00:00:00 2001 From: Miguel Ojeda Date: Fri, 16 Jun 2023 02:16:30 +0200 Subject: [PATCH 13/37] kbuild: rust_is_available: check that output looks as expected The script already checks for `$RUSTC` and `$BINDGEN` existing and exiting without failure. However, one may still pass an unexpected binary that does not output what the later parsing expects. The script still successfully reports a failure as expected, but the error is confusing. For instance: $ RUSTC=true BINDGEN=bindgen CC=clang scripts/rust_is_available.sh scripts/rust_is_available.sh: 19: arithmetic expression: expecting primary: "100000 * + 100 * + " *** *** Please see Documentation/rust/quick-start.rst for details *** on how to set up the Rust support. *** Thus add an explicit check and a proper message for unexpected output from the called command. Similarly, do so for the `libclang` version parsing, too. Link: https://lore.kernel.org/rust-for-linux/CAK7LNAQYk6s11MASRHW6oxtkqF00EJVqhHOP=5rynWt-QDUsXw@mail.gmail.com/ Signed-off-by: Miguel Ojeda Reviewed-by: Nathan Chancellor Reviewed-by: Martin Rodriguez Reboredo Link: https://lore.kernel.org/r/20230616001631.463536-11-ojeda@kernel.org --- scripts/rust_is_available.sh | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/scripts/rust_is_available.sh b/scripts/rust_is_available.sh index da8296cd9b8d1d..117018946b577a 100755 --- a/scripts/rust_is_available.sh +++ b/scripts/rust_is_available.sh @@ -97,6 +97,15 @@ rust_compiler_version=$( \ echo "$rust_compiler_output" \ | sed -nE '1s:.*rustc ([0-9]+\.[0-9]+\.[0-9]+).*:\1:p' ) +if [ -z "$rust_compiler_version" ]; then + echo >&2 "***" + echo >&2 "*** Running '$RUSTC' to check the Rust compiler version did not return" + echo >&2 "*** an expected output. See output and docs below for details:" + echo >&2 "***" + echo >&2 "$rust_compiler_output" + echo >&2 "***" + exit 1 +fi rust_compiler_min_version=$($min_tool_version rustc) rust_compiler_cversion=$(get_canonical_version $rust_compiler_version) rust_compiler_min_cversion=$(get_canonical_version $rust_compiler_min_version) @@ -136,6 +145,15 @@ rust_bindings_generator_version=$( \ echo "$rust_bindings_generator_output" \ | sed -nE '1s:.*bindgen ([0-9]+\.[0-9]+\.[0-9]+).*:\1:p' ) +if [ -z "$rust_bindings_generator_version" ]; then + echo >&2 "***" + echo >&2 "*** Running '$BINDGEN' to check the bindings generator version did not return" + echo >&2 "*** an expected output. See output and docs below for details:" + echo >&2 "***" + echo >&2 "$rust_bindings_generator_output" + echo >&2 "***" + exit 1 +fi rust_bindings_generator_min_version=$($min_tool_version bindgen) rust_bindings_generator_cversion=$(get_canonical_version $rust_bindings_generator_version) rust_bindings_generator_min_cversion=$(get_canonical_version $rust_bindings_generator_min_version) @@ -184,6 +202,16 @@ bindgen_libclang_version=$( \ echo "$bindgen_libclang_output" \ | sed -nE 's:.*clang version ([0-9]+\.[0-9]+\.[0-9]+).*:\1:p' ) +if [ -z "$bindgen_libclang_version" ]; then + echo >&2 "***" + echo >&2 "*** Running '$BINDGEN' to check the libclang version (used by the Rust" + echo >&2 "*** bindings generator) did not return an expected output. See output" + echo >&2 "*** and docs below for details:" + echo >&2 "***" + echo >&2 "$bindgen_libclang_output" + echo >&2 "***" + exit 1 +fi bindgen_libclang_min_version=$($min_tool_version llvm) bindgen_libclang_cversion=$(get_canonical_version $bindgen_libclang_version) bindgen_libclang_min_cversion=$(get_canonical_version $bindgen_libclang_min_version) From 6a4e6dc5b2c4a1fc78fcc35f8a90cbdff12371c9 Mon Sep 17 00:00:00 2001 From: Miguel Ojeda Date: Fri, 16 Jun 2023 02:16:31 +0200 Subject: [PATCH 14/37] kbuild: rust_is_available: add test suite The `rust_is_available.sh` script runs for everybody compiling the kernel, even if not using Rust. Therefore, it is important to ensure that the script is correct to avoid breaking people's compilation. In addition, the script needs to be able to handle a set of subtle cases, including parsing version strings of different tools. Therefore, maintenance of this script can be greatly eased with a set of tests. Thus add a test suite to cover hopefully most of the setups that the script may encounter in the wild. Extra setups can be easily added later on if missing. The script currently covers all the branches of the shell script, including several ways in which they may be entered. Python is used for this script, since the script under test does not depend on Rust, thus hopefully making it easier for others to use if the need arises. Signed-off-by: Miguel Ojeda Reviewed-by: Martin Rodriguez Reboredo Link: https://lore.kernel.org/r/20230616001631.463536-12-ojeda@kernel.org --- scripts/rust_is_available_test.py | 346 ++++++++++++++++++++++++++++++ 1 file changed, 346 insertions(+) create mode 100755 scripts/rust_is_available_test.py diff --git a/scripts/rust_is_available_test.py b/scripts/rust_is_available_test.py new file mode 100755 index 00000000000000..57613fe5ed7545 --- /dev/null +++ b/scripts/rust_is_available_test.py @@ -0,0 +1,346 @@ +#!/usr/bin/env python3 +# SPDX-License-Identifier: GPL-2.0 + +"""Tests the `rust_is_available.sh` script. + +Some of the tests require the real programs to be available in `$PATH` +under their canonical name (and with the expected versions). +""" + +import enum +import os +import pathlib +import stat +import subprocess +import tempfile +import unittest + +class TestRustIsAvailable(unittest.TestCase): + @enum.unique + class Expected(enum.Enum): + SUCCESS = enum.auto() + SUCCESS_WITH_WARNINGS = enum.auto() + SUCCESS_WITH_EXTRA_OUTPUT = enum.auto() + FAILURE = enum.auto() + + @classmethod + def generate_executable(cls, content): + path = pathlib.Path(cls.tempdir.name) + name = str(len(tuple(path.iterdir()))) + path = path / name + with open(path, "w") as file_: + file_.write(content) + os.chmod(path, os.stat(path).st_mode | stat.S_IXUSR) + return path + + @classmethod + def generate_clang(cls, stdout): + return cls.generate_executable(f"""#!/usr/bin/env python3 +import sys +if "-E" in " ".join(sys.argv): + print({repr("Clang " + " ".join(cls.llvm_default_version.split(" ")))}) +else: + print({repr(stdout)}) +""") + + @classmethod + def generate_rustc(cls, stdout): + return cls.generate_executable(f"""#!/usr/bin/env python3 +import sys +if "--print sysroot" in " ".join(sys.argv): + print({repr(cls.rust_default_sysroot)}) +else: + print({repr(stdout)}) +""") + + @classmethod + def generate_bindgen(cls, version_stdout, libclang_stderr): + return cls.generate_executable(f"""#!/usr/bin/env python3 +import sys +if "rust_is_available_bindgen_libclang.h" in " ".join(sys.argv): + print({repr(libclang_stderr)}, file=sys.stderr) +else: + print({repr(version_stdout)}) +""") + + @classmethod + def generate_bindgen_version(cls, stdout): + return cls.generate_bindgen(stdout, cls.bindgen_default_bindgen_libclang_stderr) + + @classmethod + def generate_bindgen_libclang(cls, stderr): + return cls.generate_bindgen(cls.bindgen_default_bindgen_version_stdout, stderr) + + @classmethod + def setUpClass(cls): + cls.tempdir = tempfile.TemporaryDirectory() + + cls.missing = pathlib.Path(cls.tempdir.name) / "missing" + + cls.nonexecutable = pathlib.Path(cls.tempdir.name) / "nonexecutable" + with open(cls.nonexecutable, "w") as file_: + file_.write("nonexecutable") + + cls.unexpected_binary = "true" + + cls.rustc_default_version = subprocess.check_output(("scripts/min-tool-version.sh", "rustc")).decode().strip() + cls.bindgen_default_version = subprocess.check_output(("scripts/min-tool-version.sh", "bindgen")).decode().strip() + cls.llvm_default_version = subprocess.check_output(("scripts/min-tool-version.sh", "llvm")).decode().strip() + cls.rust_default_sysroot = subprocess.check_output(("rustc", "--print", "sysroot")).decode().strip() + + cls.bindgen_default_bindgen_version_stdout = f"bindgen {cls.bindgen_default_version}" + cls.bindgen_default_bindgen_libclang_stderr = f"scripts/rust_is_available_bindgen_libclang.h:2:9: warning: clang version {cls.llvm_default_version} [-W#pragma-messages], err: false" + + cls.default_rustc = cls.generate_rustc(f"rustc {cls.rustc_default_version}") + cls.default_bindgen = cls.generate_bindgen(cls.bindgen_default_bindgen_version_stdout, cls.bindgen_default_bindgen_libclang_stderr) + cls.default_cc = cls.generate_clang(f"clang version {cls.llvm_default_version}") + + def run_script(self, expected, override_env): + env = { + "RUSTC": self.default_rustc, + "BINDGEN": self.default_bindgen, + "CC": self.default_cc, + } + + for key, value in override_env.items(): + if value is None: + del env[key] + continue + env[key] = value + + result = subprocess.run("scripts/rust_is_available.sh", env=env, capture_output=True) + + # The script should never output anything to `stdout`. + self.assertEqual(result.stdout, b"") + + if expected == self.Expected.SUCCESS: + # When expecting a success, the script should return 0 + # and it should not output anything to `stderr`. + self.assertEqual(result.returncode, 0) + self.assertEqual(result.stderr, b"") + elif expected == self.Expected.SUCCESS_WITH_EXTRA_OUTPUT: + # When expecting a success with extra output (that is not warnings, + # which is the common case), the script should return 0 and it + # should output at least something to `stderr` (the output should + # be checked further by the test). + self.assertEqual(result.returncode, 0) + self.assertNotEqual(result.stderr, b"") + elif expected == self.Expected.SUCCESS_WITH_WARNINGS: + # When expecting a success with warnings, the script should return 0 + # and it should output at least the instructions to `stderr`. + self.assertEqual(result.returncode, 0) + self.assertIn(b"Please see Documentation/rust/quick-start.rst for details", result.stderr) + else: + # When expecting a failure, the script should return non-0 + # and it should output at least the instructions to `stderr`. + self.assertNotEqual(result.returncode, 0) + self.assertIn(b"Please see Documentation/rust/quick-start.rst for details", result.stderr) + + # The output will generally be UTF-8 (i.e. unless the user has + # put strange values in the environment). + result.stderr = result.stderr.decode() + + return result + + def test_rustc_unset(self): + result = self.run_script(self.Expected.FAILURE, { "RUSTC": None }) + self.assertIn("Environment variable 'RUSTC' is not set.", result.stderr) + self.assertIn("This script is intended to be called from Kbuild.", result.stderr) + + def test_bindgen_unset(self): + result = self.run_script(self.Expected.FAILURE, { "BINDGEN": None }) + self.assertIn("Environment variable 'BINDGEN' is not set.", result.stderr) + self.assertIn("This script is intended to be called from Kbuild.", result.stderr) + + def test_cc_unset(self): + result = self.run_script(self.Expected.FAILURE, { "CC": None }) + self.assertIn("Environment variable 'CC' is not set.", result.stderr) + self.assertIn("This script is intended to be called from Kbuild.", result.stderr) + + def test_rustc_missing(self): + result = self.run_script(self.Expected.FAILURE, { "RUSTC": self.missing }) + self.assertIn(f"Rust compiler '{self.missing}' could not be found.", result.stderr) + + def test_bindgen_missing(self): + result = self.run_script(self.Expected.FAILURE, { "BINDGEN": self.missing }) + self.assertIn(f"Rust bindings generator '{self.missing}' could not be found.", result.stderr) + + def test_rustc_nonexecutable(self): + result = self.run_script(self.Expected.FAILURE, { "RUSTC": self.nonexecutable }) + self.assertIn(f"Running '{self.nonexecutable}' to check the Rust compiler version failed with", result.stderr) + + def test_rustc_unexpected_binary(self): + result = self.run_script(self.Expected.FAILURE, { "RUSTC": self.unexpected_binary }) + self.assertIn(f"Running '{self.unexpected_binary}' to check the Rust compiler version did not return", result.stderr) + + def test_rustc_unexpected_name(self): + rustc = self.generate_rustc(f"unexpected {self.rustc_default_version} (a8314ef7d 2022-06-27)") + result = self.run_script(self.Expected.FAILURE, { "RUSTC": rustc }) + self.assertIn(f"Running '{rustc}' to check the Rust compiler version did not return", result.stderr) + + def test_rustc_unexpected_version(self): + rustc = self.generate_rustc("rustc unexpected (a8314ef7d 2022-06-27)") + result = self.run_script(self.Expected.FAILURE, { "RUSTC": rustc }) + self.assertIn(f"Running '{rustc}' to check the Rust compiler version did not return", result.stderr) + + def test_rustc_no_minor(self): + rustc = self.generate_rustc(f"rustc {'.'.join(self.rustc_default_version.split('.')[:2])} (a8314ef7d 2022-06-27)") + result = self.run_script(self.Expected.FAILURE, { "RUSTC": rustc }) + self.assertIn(f"Running '{rustc}' to check the Rust compiler version did not return", result.stderr) + + def test_rustc_old_version(self): + rustc = self.generate_rustc("rustc 1.60.0 (a8314ef7d 2022-06-27)") + result = self.run_script(self.Expected.FAILURE, { "RUSTC": rustc }) + self.assertIn(f"Rust compiler '{rustc}' is too old.", result.stderr) + + def test_rustc_new_version(self): + rustc = self.generate_rustc("rustc 1.999.0 (a8314ef7d 2099-06-27)") + result = self.run_script(self.Expected.SUCCESS_WITH_WARNINGS, { "RUSTC": rustc }) + self.assertIn(f"Rust compiler '{rustc}' is too new. This may or may not work.", result.stderr) + + def test_bindgen_nonexecutable(self): + result = self.run_script(self.Expected.FAILURE, { "BINDGEN": self.nonexecutable }) + self.assertIn(f"Running '{self.nonexecutable}' to check the Rust bindings generator version failed with", result.stderr) + + def test_bindgen_unexpected_binary(self): + result = self.run_script(self.Expected.FAILURE, { "BINDGEN": self.unexpected_binary }) + self.assertIn(f"Running '{self.unexpected_binary}' to check the bindings generator version did not return", result.stderr) + + def test_bindgen_unexpected_name(self): + bindgen = self.generate_bindgen_version(f"unexpected {self.bindgen_default_version}") + result = self.run_script(self.Expected.FAILURE, { "BINDGEN": bindgen }) + self.assertIn(f"Running '{bindgen}' to check the bindings generator version did not return", result.stderr) + + def test_bindgen_unexpected_version(self): + bindgen = self.generate_bindgen_version("bindgen unexpected") + result = self.run_script(self.Expected.FAILURE, { "BINDGEN": bindgen }) + self.assertIn(f"Running '{bindgen}' to check the bindings generator version did not return", result.stderr) + + def test_bindgen_no_minor(self): + bindgen = self.generate_bindgen_version(f"bindgen {'.'.join(self.bindgen_default_version.split('.')[:2])}") + result = self.run_script(self.Expected.FAILURE, { "BINDGEN": bindgen }) + self.assertIn(f"Running '{bindgen}' to check the bindings generator version did not return", result.stderr) + + def test_bindgen_old_version(self): + bindgen = self.generate_bindgen_version("bindgen 0.50.0") + result = self.run_script(self.Expected.FAILURE, { "BINDGEN": bindgen }) + self.assertIn(f"Rust bindings generator '{bindgen}' is too old.", result.stderr) + + def test_bindgen_new_version(self): + bindgen = self.generate_bindgen_version("bindgen 0.999.0") + result = self.run_script(self.Expected.SUCCESS_WITH_WARNINGS, { "BINDGEN": bindgen }) + self.assertIn(f"Rust bindings generator '{bindgen}' is too new. This may or may not work.", result.stderr) + + def test_bindgen_libclang_failure(self): + for env in ( + { "LLVM_CONFIG_PATH": self.missing }, + { "LIBCLANG_PATH": self.missing }, + { "CLANG_PATH": self.missing }, + ): + with self.subTest(env=env): + result = self.run_script(self.Expected.FAILURE, env | { "PATH": os.environ["PATH"], "BINDGEN": "bindgen" }) + self.assertIn("Running 'bindgen' to check the libclang version (used by the Rust", result.stderr) + self.assertIn("bindings generator) failed with code ", result.stderr) + + def test_bindgen_libclang_unexpected_version(self): + bindgen = self.generate_bindgen_libclang("scripts/rust_is_available_bindgen_libclang.h:2:9: warning: clang version unexpected [-W#pragma-messages], err: false") + result = self.run_script(self.Expected.FAILURE, { "BINDGEN": bindgen }) + self.assertIn(f"Running '{bindgen}' to check the libclang version (used by the Rust", result.stderr) + self.assertIn("bindings generator) did not return an expected output. See output", result.stderr) + + def test_bindgen_libclang_old_version(self): + bindgen = self.generate_bindgen_libclang("scripts/rust_is_available_bindgen_libclang.h:2:9: warning: clang version 10.0.0 [-W#pragma-messages], err: false") + result = self.run_script(self.Expected.FAILURE, { "BINDGEN": bindgen }) + self.assertIn(f"libclang (used by the Rust bindings generator '{bindgen}') is too old.", result.stderr) + + def test_clang_matches_bindgen_libclang_different_bindgen(self): + bindgen = self.generate_bindgen_libclang("scripts/rust_is_available_bindgen_libclang.h:2:9: warning: clang version 999.0.0 [-W#pragma-messages], err: false") + result = self.run_script(self.Expected.SUCCESS_WITH_WARNINGS, { "BINDGEN": bindgen }) + self.assertIn("version does not match Clang's. This may be a problem.", result.stderr) + + def test_clang_matches_bindgen_libclang_different_clang(self): + cc = self.generate_clang("clang version 999.0.0") + result = self.run_script(self.Expected.SUCCESS_WITH_WARNINGS, { "CC": cc }) + self.assertIn("version does not match Clang's. This may be a problem.", result.stderr) + + def test_rustc_src_core_krustflags(self): + result = self.run_script(self.Expected.FAILURE, { "PATH": os.environ["PATH"], "RUSTC": "rustc", "KRUSTFLAGS": f"--sysroot={self.missing}" }) + self.assertIn("Source code for the 'core' standard library could not be found", result.stderr) + + def test_rustc_src_core_rustlibsrc(self): + result = self.run_script(self.Expected.FAILURE, { "RUST_LIB_SRC": self.missing }) + self.assertIn("Source code for the 'core' standard library could not be found", result.stderr) + + def test_success_cc_unknown(self): + result = self.run_script(self.Expected.SUCCESS_WITH_EXTRA_OUTPUT, { "CC": self.missing }) + self.assertIn("unknown C compiler", result.stderr) + + def test_success_cc_multiple_arguments_ccache(self): + clang = self.generate_clang(f"""Ubuntu clang version {self.llvm_default_version}-1ubuntu1 +Target: x86_64-pc-linux-gnu +Thread model: posix +InstalledDir: /usr/bin +""") + result = self.run_script(self.Expected.SUCCESS, { "CC": f"{clang} clang" }) + + def test_success_rustc_version(self): + for rustc_stdout in ( + f"rustc {self.rustc_default_version} (a8314ef7d 2022-06-27)", + f"rustc {self.rustc_default_version}-dev (a8314ef7d 2022-06-27)", + f"rustc {self.rustc_default_version}-1.60.0 (a8314ef7d 2022-06-27)", + ): + with self.subTest(rustc_stdout=rustc_stdout): + rustc = self.generate_rustc(rustc_stdout) + result = self.run_script(self.Expected.SUCCESS, { "RUSTC": rustc }) + + def test_success_bindgen_version(self): + for bindgen_stdout in ( + f"bindgen {self.bindgen_default_version}", + f"bindgen {self.bindgen_default_version}-dev", + f"bindgen {self.bindgen_default_version}-0.999.0", + ): + with self.subTest(bindgen_stdout=bindgen_stdout): + bindgen = self.generate_bindgen_version(bindgen_stdout) + result = self.run_script(self.Expected.SUCCESS, { "BINDGEN": bindgen }) + + def test_success_bindgen_libclang(self): + for stderr in ( + f"scripts/rust_is_available_bindgen_libclang.h:2:9: warning: clang version {self.llvm_default_version} (https://github.com/llvm/llvm-project.git 4a2c05b05ed07f1f620e94f6524a8b4b2760a0b1) [-W#pragma-messages], err: false", + f"/home/jd/Documents/dev/kernel-module-flake/linux-6.1/outputs/dev/lib/modules/6.1.0-development/source/scripts/rust_is_available_bindgen_libclang.h:2:9: warning: clang version {self.llvm_default_version} [-W#pragma-messages], err: false", + f"scripts/rust_is_available_bindgen_libclang.h:2:9: warning: clang version {self.llvm_default_version} (Fedora 13.0.0-3.fc35) [-W#pragma-messages], err: false", + f""" +/nix/store/dsd5gz46hdbdk2rfdimqddhq6m8m8fqs-bash-5.1-p16/bin/bash: warning: setlocale: LC_ALL: cannot change locale (c) +scripts/rust_is_available_bindgen_libclang.h:2:9: warning: clang version {self.llvm_default_version} [-W#pragma-messages], err: false +""", + f""" +/nix/store/dsd5gz46hdbdk2rfdimqddhq6m8m8fqs-bash-5.1.0-p16/bin/bash: warning: setlocale: LC_ALL: cannot change locale (c) +/home/jd/Documents/dev/kernel-module-flake/linux-6.1/outputs/dev/lib/modules/6.1.0-development/source/scripts/rust_is_available_bindgen_libclang.h:2:9: warning: clang version {self.llvm_default_version} (Fedora 13.0.0-3.fc35) [-W#pragma-messages], err: false +""" + ): + with self.subTest(stderr=stderr): + bindgen = self.generate_bindgen_libclang(stderr) + result = self.run_script(self.Expected.SUCCESS, { "BINDGEN": bindgen }) + + def test_success_clang_version(self): + for clang_stdout in ( + f"clang version {self.llvm_default_version} (https://github.com/llvm/llvm-project.git 4a2c05b05ed07f1f620e94f6524a8b4b2760a0b1)", + f"clang version {self.llvm_default_version}-dev", + f"clang version {self.llvm_default_version}-2~ubuntu20.04.1", + f"Ubuntu clang version {self.llvm_default_version}-2~ubuntu20.04.1", + ): + with self.subTest(clang_stdout=clang_stdout): + clang = self.generate_clang(clang_stdout) + result = self.run_script(self.Expected.SUCCESS, { "CC": clang }) + + def test_success_real_programs(self): + for cc in ["gcc", "clang"]: + with self.subTest(cc=cc): + result = self.run_script(self.Expected.SUCCESS, { + "PATH": os.environ["PATH"], + "RUSTC": "rustc", + "BINDGEN": "bindgen", + "CC": cc, + }) + +if __name__ == "__main__": + unittest.main() From f2f2787737dc7586e8be58676e0a73a4eb43ea6e Mon Sep 17 00:00:00 2001 From: Boqun Feng Date: Sun, 25 Jun 2023 16:25:28 -0700 Subject: [PATCH 15/37] rust: alloc: Add realloc and alloc_zeroed to the GlobalAlloc impl MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit While there are default impls for these methods, using the respective C api's is faster. Currently neither the existing nor these new GlobalAlloc method implementations are actually called. Instead the __rust_* function defined below the GlobalAlloc impl are used. With rustc 1.71 these functions will be gone and all allocation calls will go through the GlobalAlloc implementation. Link: https://github.com/Rust-for-Linux/linux/issues/68 Signed-off-by: Björn Roy Baron [boqun: add size adjustment for alignment requirement] Signed-off-by: Boqun Feng Link: https://lore.kernel.org/r/20230625232528.89306-1-boqun.feng@gmail.com --- rust/kernel/allocator.rs | 59 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 54 insertions(+), 5 deletions(-) diff --git a/rust/kernel/allocator.rs b/rust/kernel/allocator.rs index 66575cf87ce2f2..af723c2924dcc9 100644 --- a/rust/kernel/allocator.rs +++ b/rust/kernel/allocator.rs @@ -9,8 +9,17 @@ use crate::bindings; struct KernelAllocator; -unsafe impl GlobalAlloc for KernelAllocator { - unsafe fn alloc(&self, layout: Layout) -> *mut u8 { +impl KernelAllocator { + /// # Safety + /// + /// * `ptr` can be either null or a pointer which has been allocated by this allocator. + /// * `layout` must have a non-zero size. + unsafe fn krealloc_with_flags( + &self, + ptr: *mut u8, + layout: Layout, + flags: bindings::gfp_t, + ) -> *mut u8 { // Customized layouts from `Layout::from_size_align()` can have size < align, so pads first. let layout = layout.pad_to_align(); @@ -26,9 +35,22 @@ unsafe impl GlobalAlloc for KernelAllocator { size = size.next_power_of_two(); } - // `krealloc()` is used instead of `kmalloc()` because the latter is - // an inline function and cannot be bound to as a result. - unsafe { bindings::krealloc(ptr::null(), size, bindings::GFP_KERNEL) as *mut u8 } + // SAFETY: + // + // * `ptr` is either null or a pointer returned from a previous k{re}alloc() by the function + // safety requirement. + // + // * `size` is greater than 0 since it's either a `layout.size()` (which cannot be zero + // according to the function safety requirement) or a result from `next_power_of_two()`. + unsafe { bindings::krealloc(ptr as *const core::ffi::c_void, size, flags) as *mut u8 } + } +} + +unsafe impl GlobalAlloc for KernelAllocator { + unsafe fn alloc(&self, layout: Layout) -> *mut u8 { + // SAFETY: `ptr::null_mut()` is null and `layout` has a non-zero size by the function safety + // requirement. + unsafe { self.krealloc_with_flags(ptr::null_mut(), layout, bindings::GFP_KERNEL) } } unsafe fn dealloc(&self, ptr: *mut u8, _layout: Layout) { @@ -36,6 +58,33 @@ unsafe impl GlobalAlloc for KernelAllocator { bindings::kfree(ptr as *const core::ffi::c_void); } } + + unsafe fn realloc(&self, ptr: *mut u8, layout: Layout, new_size: usize) -> *mut u8 { + // SAFETY: + // * `new_size` when rounded up to the nearest multiple of `layout.align()`, will not + // overflow `isize` by the function safety requirement. + // * `layout.align()` is a proper alignment (i.e. not zero and must be a power of two). + let layout = unsafe { Layout::from_size_align_unchecked(new_size, layout.align()) }; + + // SAFETY: + // * `ptr` is either null or a pointer allocated by this allocator by function safety + // requirement. + // * the size of `layout` is not zero because `new_size` is not zero by function safety + // requirement. + unsafe { self.krealloc_with_flags(ptr, layout, bindings::GFP_KERNEL) } + } + + unsafe fn alloc_zeroed(&self, layout: Layout) -> *mut u8 { + // SAFETY: `ptr::null_mut()` is null and `layout` has a non-zero size by the function safety + // requirement. + unsafe { + self.krealloc_with_flags( + ptr::null_mut(), + layout, + bindings::GFP_KERNEL | bindings::__GFP_ZERO, + ) + } + } } #[global_allocator] From 53dd54f5ff9930e410db3c1c0419adbc1c3fa00c Mon Sep 17 00:00:00 2001 From: Aakash Sen Sharma Date: Tue, 13 Jun 2023 01:13:11 +0530 Subject: [PATCH 16/37] rust: bindgen: upgrade to 0.65.1 * Rationale: Upgrades bindgen to code-generation for anonymous unions, structs, and enums [7] for LLVM-16 based toolchains. The following upgrade also incorporates `noreturn` support from bindgen allowing us to remove useless `loop` calls which was placed as a workaround. * Truncated build logs on using bindgen `v0.56.0` prior to LLVM-16 toolchain: ``` $ make rustdoc LLVM=1 CLIPPY=1 -j$(nproc) RUSTC L rust/core.o BINDGEN rust/bindings/bindings_generated.rs BINDGEN rust/bindings/bindings_helpers_generated.rs BINDGEN rust/uapi/uapi_generated.rs thread 'main' panicked at '"ftrace_branch_data_union_(anonymous_at__/_/include/linux/compiler_types_h_146_2)" is not a valid Ident', .../proc-macro2-1.0.24/src/fallback.rs:693:9 ... thread 'main' panicked at '"ftrace_branch_data_union_(anonymous_at__/_/include/linux/compiler_types_h_146_2)" is not a valid Ident', .../proc-macro2-1.0.24/src/fallback.rs:693:9 ... ``` * LLVM-16 Changes: API changes [1] were introduced such that libclang would emit names like "(unnamed union at compiler_types.h:146:2)" for unnamed unions, structs, and enums whereas it previously returned an empty string. * Bindgen Changes: Bindgen `v0.56.0` on LLVM-16 based toolchains hence was unable to process the anonymous union in `include/linux/compiler_types` `struct ftrace_branch_data` and caused subsequent panics as the new `libclang` API emitted name was not being handled. The following issue was fixed in Bindgen `v0.62.0` [2]. Bindgen `v0.58.0` changed the flags `--whitelist-*` and `--blacklist-*` to `--allowlist-*` and `--blocklist-*` respectively [3]. Bindgen `v0.61.0` added support for `_Noreturn`, `[[noreturn]]`, `__attribute__((noreturn))` [4], hence the empty `loop`s used to circumvent bindgen returning `!` in place of `()` for noreturn attributes have been removed completely. Bindgen `v0.61.0` also changed default functionality to bind `size_t` to `usize` [5] and added the `--no-size_t-is-usize` [5] flag to not bind `size_t` as `usize`. Bindgen `v0.65.0` removed `--size_t-is-usize` flag [6]. Link: https://github.com/llvm/llvm-project/commit/19e984ef8f49bc3ccced15621989fa9703b2cd5b [1] Link: https://github.com/rust-lang/rust-bindgen/pull/2319 [2] Link: https://github.com/rust-lang/rust-bindgen/pull/1990 [3] Link: https://github.com/rust-lang/rust-bindgen/issues/2094 [4] Link: https://github.com/rust-lang/rust-bindgen/commit/cc78b6fdb6e829e5fb8fa1639f2182cb49333569 [5] Link: https://github.com/rust-lang/rust-bindgen/pull/2408 [6] Closes: https://github.com/Rust-for-Linux/linux/issues/1013 [7] Signed-off-by: Aakash Sen Sharma Reviewed-by: Gary Guo Tested-by: Ariel Miculas Link: https://lore.kernel.org/r/20230612194311.24826-1-aakashsensharma@gmail.com [boqun: resolve conflicts because of Rust version bump] Signed-off-by: Boqun Feng --- Documentation/process/changes.rst | 2 +- rust/Makefile | 6 +++--- rust/helpers.c | 13 ++++++------- rust/kernel/lib.rs | 3 --- scripts/min-tool-version.sh | 2 +- 5 files changed, 11 insertions(+), 15 deletions(-) diff --git a/Documentation/process/changes.rst b/Documentation/process/changes.rst index 5cf6a5f8ca5741..4e27fb32f8e6d3 100644 --- a/Documentation/process/changes.rst +++ b/Documentation/process/changes.rst @@ -32,7 +32,7 @@ you probably needn't concern yourself with pcmciautils. GNU C 5.1 gcc --version Clang/LLVM (optional) 11.0.0 clang --version Rust (optional) 1.68.2 rustc --version -bindgen (optional) 0.56.0 bindgen --version +bindgen (optional) 0.65.1 bindgen --version GNU make 3.82 make --version bash 4.2 bash --version binutils 2.25 ld -v diff --git a/rust/Makefile b/rust/Makefile index 7c9d9f11aec505..fc8cdcfcc9e598 100644 --- a/rust/Makefile +++ b/rust/Makefile @@ -300,7 +300,7 @@ quiet_cmd_bindgen = BINDGEN $@ $(BINDGEN) $< $(bindgen_target_flags) \ --use-core --with-derive-default --ctypes-prefix core::ffi --no-layout-tests \ --no-debug '.*' \ - --size_t-is-usize -o $@ -- $(bindgen_c_flags_final) -DMODULE \ + -o $@ -- $(bindgen_c_flags_final) -DMODULE \ $(bindgen_target_cflags) $(bindgen_target_extra) $(obj)/bindings/bindings_generated.rs: private bindgen_target_flags = \ @@ -320,8 +320,8 @@ $(obj)/uapi/uapi_generated.rs: $(src)/uapi/uapi_helper.h \ # given it is `libclang`; but for consistency, future Clang changes and/or # a potential future GCC backend for `bindgen`, we disable it too. $(obj)/bindings/bindings_helpers_generated.rs: private bindgen_target_flags = \ - --blacklist-type '.*' --whitelist-var '' \ - --whitelist-function 'rust_helper_.*' + --blocklist-type '.*' --allowlist-var '' \ + --allowlist-function 'rust_helper_.*' $(obj)/bindings/bindings_helpers_generated.rs: private bindgen_target_cflags = \ -I$(objtree)/$(obj) -Wno-missing-prototypes -Wno-missing-declarations $(obj)/bindings/bindings_helpers_generated.rs: private bindgen_target_extra = ; \ diff --git a/rust/helpers.c b/rust/helpers.c index bb594da56137ef..d8cf773d966e43 100644 --- a/rust/helpers.c +++ b/rust/helpers.c @@ -136,19 +136,18 @@ void rust_helper_put_task_struct(struct task_struct *t) EXPORT_SYMBOL_GPL(rust_helper_put_task_struct); /* - * We use `bindgen`'s `--size_t-is-usize` option to bind the C `size_t` type - * as the Rust `usize` type, so we can use it in contexts where Rust - * expects a `usize` like slice (array) indices. `usize` is defined to be - * the same as C's `uintptr_t` type (can hold any pointer) but not - * necessarily the same as `size_t` (can hold the size of any single + * `bindgen` binds the C `size_t` type the Rust `usize` type, so we can + * use it in contexts where Rust expects a `usize` like slice (array) indices. + * `usize` is defined to be the same as C's `uintptr_t` type (can hold any pointer) + * but not necessarily the same as `size_t` (can hold the size of any single * object). Most modern platforms use the same concrete integer type for * both of them, but in case we find ourselves on a platform where * that's not true, fail early instead of risking ABI or * integer-overflow issues. * * If your platform fails this assertion, it means that you are in - * danger of integer-overflow bugs (even if you attempt to remove - * `--size_t-is-usize`). It may be easiest to change the kernel ABI on + * danger of integer-overflow bugs (even if you attempt to add + * `--no-size_t-is-usize`). It may be easiest to change the kernel ABI on * your platform such that `size_t` matches `uintptr_t` (i.e., to increase * `size_t`, because `uintptr_t` has to be at least as big as `size_t`). */ diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index 85b26120997758..d59041ff5ff211 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -93,7 +93,4 @@ fn panic(info: &core::panic::PanicInfo<'_>) -> ! { pr_emerg!("{}\n", info); // SAFETY: FFI call. unsafe { bindings::BUG() }; - // Bindgen currently does not recognize `__noreturn` so `BUG` returns `()` - // instead of `!`. See . - loop {} } diff --git a/scripts/min-tool-version.sh b/scripts/min-tool-version.sh index 131be76d21305e..5895aaa1fd5869 100755 --- a/scripts/min-tool-version.sh +++ b/scripts/min-tool-version.sh @@ -30,7 +30,7 @@ rustc) echo 1.68.2 ;; bindgen) - echo 0.56.0 + echo 0.65.1 ;; *) echo "$1: unknown tool" >&2 From 02ddf5a9e1aafed9ee081d21bbbfacff83eb6b3d Mon Sep 17 00:00:00 2001 From: Miguel Ojeda Date: Wed, 14 Jun 2023 20:08:25 +0200 Subject: [PATCH 17/37] rust: init: make doctests compilable/testable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rust documentation tests are going to be build/run-tested with the KUnit integration added in a future patch, thus update them to make them compilable/testable so that we may start enforcing it. Signed-off-by: Miguel Ojeda Reviewed-by: Benno Lossin Reviewed-by: Björn Roy Baron Reviewed-by: David Gow Reviewed-by: Vincenzo Palazzo Reviewed-by: Martin Rodriguez Reboredo Reviewed-by: Alice Ryhl Link: https://lore.kernel.org/r/20230614180837.630180-2-ojeda@kernel.org --- rust/kernel/init.rs | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/rust/kernel/init.rs b/rust/kernel/init.rs index b4332a4ec1f4d5..1073515ed40e41 100644 --- a/rust/kernel/init.rs +++ b/rust/kernel/init.rs @@ -120,14 +120,23 @@ //! `slot` gets called. //! //! ```rust -//! use kernel::{prelude::*, init}; +//! # #![allow(unreachable_pub, clippy::disallowed_names)] +//! use kernel::{prelude::*, init, types::Opaque}; //! use core::{ptr::addr_of_mut, marker::PhantomPinned, pin::Pin}; //! # mod bindings { +//! # #![allow(non_camel_case_types)] //! # pub struct foo; //! # pub unsafe fn init_foo(_ptr: *mut foo) {} //! # pub unsafe fn destroy_foo(_ptr: *mut foo) {} //! # pub unsafe fn enable_foo(_ptr: *mut foo, _flags: u32) -> i32 { 0 } //! # } +//! # trait FromErrno { +//! # fn from_errno(errno: core::ffi::c_int) -> Error { +//! # // Dummy error that can be constructed outside the `kernel` crate. +//! # Error::from(core::fmt::Error) +//! # } +//! # } +//! # impl FromErrno for Error {} //! /// # Invariants //! /// //! /// `foo` is always initialized @@ -158,7 +167,7 @@ //! if err != 0 { //! // Enabling has failed, first clean up the foo and then return the error. //! bindings::destroy_foo(Opaque::raw_get(foo)); -//! return Err(Error::from_kernel_errno(err)); +//! return Err(Error::from_errno(err)); //! } //! //! // All fields of `RawFoo` have been initialized, since `_p` is a ZST. @@ -226,8 +235,7 @@ pub mod macros; /// /// ```rust /// # #![allow(clippy::disallowed_names, clippy::new_ret_no_self)] -/// # use kernel::{init, pin_init, stack_pin_init, init::*, sync::Mutex, new_mutex}; -/// # use macros::pin_data; +/// # use kernel::{init, macros::pin_data, pin_init, stack_pin_init, init::*, sync::Mutex, new_mutex}; /// # use core::pin::Pin; /// #[pin_data] /// struct Foo { @@ -277,7 +285,7 @@ macro_rules! stack_pin_init { /// /// # Examples /// -/// ```rust +/// ```rust,ignore /// # #![allow(clippy::disallowed_names, clippy::new_ret_no_self)] /// # use kernel::{init, pin_init, stack_try_pin_init, init::*, sync::Mutex, new_mutex}; /// # use macros::pin_data; @@ -303,7 +311,7 @@ macro_rules! stack_pin_init { /// pr_info!("a: {}", &*foo.a.lock()); /// ``` /// -/// ```rust +/// ```rust,ignore /// # #![allow(clippy::disallowed_names, clippy::new_ret_no_self)] /// # use kernel::{init, pin_init, stack_try_pin_init, init::*, sync::Mutex, new_mutex}; /// # use macros::pin_data; @@ -513,8 +521,7 @@ macro_rules! stack_try_pin_init { /// For instance: /// /// ```rust -/// # use kernel::pin_init; -/// # use macros::pin_data; +/// # use kernel::{macros::pin_data, pin_init}; /// # use core::{ptr::addr_of_mut, marker::PhantomPinned}; /// #[pin_data] /// struct Buf { @@ -841,7 +848,7 @@ macro_rules! init { /// # Examples /// /// ```rust -/// use kernel::{init::PinInit, error::Error, InPlaceInit}; +/// use kernel::{init::{PinInit, zeroed}, error::Error}; /// struct BigBuf { /// big: Box<[u8; 1024 * 1024 * 1024]>, /// small: [u8; 1024 * 1024], From 276f14da0543946bda854fa2ea60534813ca89a4 Mon Sep 17 00:00:00 2001 From: Miguel Ojeda Date: Wed, 14 Jun 2023 20:08:26 +0200 Subject: [PATCH 18/37] rust: str: make doctests compilable/testable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rust documentation tests are going to be build/run-tested with the KUnit integration added in a future patch, thus update them to make them compilable/testable so that we may start enforcing it. Signed-off-by: Miguel Ojeda Reviewed-by: Björn Roy Baron Reviewed-by: David Gow Reviewed-by: Vincenzo Palazzo Reviewed-by: Martin Rodriguez Reboredo Reviewed-by: Alice Ryhl Link: https://lore.kernel.org/r/20230614180837.630180-3-ojeda@kernel.org --- rust/kernel/str.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs index c9dd3bf59e34c6..c41607b2e4fe93 100644 --- a/rust/kernel/str.rs +++ b/rust/kernel/str.rs @@ -213,6 +213,7 @@ impl fmt::Display for CStr { /// /// ``` /// # use kernel::c_str; + /// # use kernel::fmt; /// # use kernel::str::CStr; /// # use kernel::str::CString; /// let penguin = c_str!("🐧"); @@ -241,6 +242,7 @@ impl fmt::Debug for CStr { /// /// ``` /// # use kernel::c_str; + /// # use kernel::fmt; /// # use kernel::str::CStr; /// # use kernel::str::CString; /// let penguin = c_str!("🐧"); @@ -529,7 +531,7 @@ impl fmt::Write for Formatter { /// # Examples /// /// ``` -/// use kernel::str::CString; +/// use kernel::{str::CString, fmt}; /// /// let s = CString::try_from_fmt(fmt!("{}{}{}", "abc", 10, 20)).unwrap(); /// assert_eq!(s.as_bytes_with_nul(), "abc1020\0".as_bytes()); From c30db857da6ce62fe653f7e32ee476040bd35dfe Mon Sep 17 00:00:00 2001 From: Miguel Ojeda Date: Wed, 14 Jun 2023 20:08:27 +0200 Subject: [PATCH 19/37] rust: sync: make doctests compilable/testable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rust documentation tests are going to be build/run-tested with the KUnit integration added in a future patch, thus update them to make them compilable/testable so that we may start enforcing it. Signed-off-by: Miguel Ojeda Reviewed-by: Björn Roy Baron Reviewed-by: David Gow Reviewed-by: Martin Rodriguez Reboredo Reviewed-by: Alice Ryhl Link: https://lore.kernel.org/r/20230614180837.630180-4-ojeda@kernel.org --- rust/kernel/sync/arc.rs | 9 +++++++-- rust/kernel/sync/lock/mutex.rs | 1 + rust/kernel/sync/lock/spinlock.rs | 1 + 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs index a89843cacaad07..1ecb2efab51e48 100644 --- a/rust/kernel/sync/arc.rs +++ b/rust/kernel/sync/arc.rs @@ -73,6 +73,7 @@ mod std_vendor; /// assert_eq!(cloned.b, 20); /// /// // The refcount drops to zero when `cloned` goes out of scope, and the memory is freed. +/// # Ok::<(), Error>(()) /// ``` /// /// Using `Arc` as the type of `self`: @@ -98,6 +99,7 @@ mod std_vendor; /// let obj = Arc::try_new(Example { a: 10, b: 20 })?; /// obj.use_reference(); /// obj.take_over(); +/// # Ok::<(), Error>(()) /// ``` /// /// Coercion from `Arc` to `Arc`: @@ -121,6 +123,7 @@ mod std_vendor; /// /// // `coerced` has type `Arc`. /// let coerced: Arc = obj; +/// # Ok::<(), Error>(()) /// ``` pub struct Arc { ptr: NonNull>, @@ -337,7 +340,7 @@ impl From>> for Arc { /// # Example /// /// ``` -/// use crate::sync::{Arc, ArcBorrow}; +/// use kernel::sync::{Arc, ArcBorrow}; /// /// struct Example; /// @@ -350,12 +353,13 @@ impl From>> for Arc { /// /// // Assert that both `obj` and `cloned` point to the same underlying object. /// assert!(core::ptr::eq(&*obj, &*cloned)); +/// # Ok::<(), Error>(()) /// ``` /// /// Using `ArcBorrow` as the type of `self`: /// /// ``` -/// use crate::sync::{Arc, ArcBorrow}; +/// use kernel::sync::{Arc, ArcBorrow}; /// /// struct Example { /// a: u32, @@ -370,6 +374,7 @@ impl From>> for Arc { /// /// let obj = Arc::try_new(Example { a: 10, b: 20 })?; /// obj.as_arc_borrow().use_reference(); +/// # Ok::<(), Error>(()) /// ``` pub struct ArcBorrow<'a, T: ?Sized + 'a> { inner: NonNull>, diff --git a/rust/kernel/sync/lock/mutex.rs b/rust/kernel/sync/lock/mutex.rs index 923472f04af4bd..09276fedc091b8 100644 --- a/rust/kernel/sync/lock/mutex.rs +++ b/rust/kernel/sync/lock/mutex.rs @@ -63,6 +63,7 @@ macro_rules! new_mutex { /// assert_eq!(e.c, 10); /// assert_eq!(e.d.lock().a, 20); /// assert_eq!(e.d.lock().b, 30); +/// # Ok::<(), Error>(()) /// ``` /// /// The following example shows how to use interior mutability to modify the contents of a struct diff --git a/rust/kernel/sync/lock/spinlock.rs b/rust/kernel/sync/lock/spinlock.rs index 979b56464a4e9f..91eb2c9e9123f7 100644 --- a/rust/kernel/sync/lock/spinlock.rs +++ b/rust/kernel/sync/lock/spinlock.rs @@ -61,6 +61,7 @@ macro_rules! new_spinlock { /// assert_eq!(e.c, 10); /// assert_eq!(e.d.lock().a, 20); /// assert_eq!(e.d.lock().b, 30); +/// # Ok::<(), Error>(()) /// ``` /// /// The following example shows how to use interior mutability to modify the contents of a struct From 4009de18739c4e7b436c80ac9872d7f1ba6b3341 Mon Sep 17 00:00:00 2001 From: Miguel Ojeda Date: Wed, 14 Jun 2023 20:08:28 +0200 Subject: [PATCH 20/37] rust: types: make doctests compilable/testable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rust documentation tests are going to be build/run-tested with the KUnit integration added in a future patch, thus update them to make them compilable/testable so that we may start enforcing it. Signed-off-by: Miguel Ojeda Reviewed-by: Björn Roy Baron Reviewed-by: David Gow Reviewed-by: Martin Rodriguez Reboredo Reviewed-by: Alice Ryhl Link: https://lore.kernel.org/r/20230614180837.630180-5-ojeda@kernel.org --- rust/kernel/types.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs index e664a2beef3044..169066030367d8 100644 --- a/rust/kernel/types.rs +++ b/rust/kernel/types.rs @@ -109,7 +109,7 @@ impl ForeignOwnable for () { /// In the example below, we have multiple exit paths and we want to log regardless of which one is /// taken: /// ``` -/// # use kernel::ScopeGuard; +/// # use kernel::types::ScopeGuard; /// fn example1(arg: bool) { /// let _log = ScopeGuard::new(|| pr_info!("example1 completed\n")); /// @@ -127,7 +127,7 @@ impl ForeignOwnable for () { /// In the example below, we want to log the same message on all early exits but a different one on /// the main exit path: /// ``` -/// # use kernel::ScopeGuard; +/// # use kernel::types::ScopeGuard; /// fn example2(arg: bool) { /// let log = ScopeGuard::new(|| pr_info!("example2 returned early\n")); /// @@ -148,7 +148,7 @@ impl ForeignOwnable for () { /// In the example below, we need a mutable object (the vector) to be accessible within the log /// function, so we wrap it in the [`ScopeGuard`]: /// ``` -/// # use kernel::ScopeGuard; +/// # use kernel::types::ScopeGuard; /// fn example3(arg: bool) -> Result { /// let mut vec = /// ScopeGuard::new_with_data(Vec::new(), |v| pr_info!("vec had {} elements\n", v.len())); From e9e95cd57cf17bd5a09a6235dff470e230d046c4 Mon Sep 17 00:00:00 2001 From: Miguel Ojeda Date: Wed, 14 Jun 2023 20:08:29 +0200 Subject: [PATCH 21/37] rust: support running Rust documentation tests as KUnit ones MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rust has documentation tests: these are typically examples of usage of any item (e.g. function, struct, module...). They are very convenient because they are just written alongside the documentation. For instance: /// Sums two numbers. /// /// ``` /// assert_eq!(mymod::f(10, 20), 30); /// ``` pub fn f(a: i32, b: i32) -> i32 { a + b } In userspace, the tests are collected and run via `rustdoc`. Using the tool as-is would be useful already, since it allows to compile-test most tests (thus enforcing they are kept in sync with the code they document) and run those that do not depend on in-kernel APIs. However, by transforming the tests into a KUnit test suite, they can also be run inside the kernel. Moreover, the tests get to be compiled as other Rust kernel objects instead of targeting userspace. On top of that, the integration with KUnit means the Rust support gets to reuse the existing testing facilities. For instance, the kernel log would look like: KTAP version 1 1..1 KTAP version 1 # Subtest: rust_doctests_kernel 1..59 # Doctest from line 13 ok 1 rust_doctest_kernel_build_assert_rs_0 # Doctest from line 56 ok 2 rust_doctest_kernel_build_assert_rs_1 # Doctest from line 122 ok 3 rust_doctest_kernel_init_rs_0 ... # Doctest from line 150 ok 59 rust_doctest_kernel_types_rs_2 # rust_doctests_kernel: pass:59 fail:0 skip:0 total:59 # Totals: pass:59 fail:0 skip:0 total:59 ok 1 rust_doctests_kernel Therefore, add support for running Rust documentation tests in KUnit. Some other notes about the current implementation and support follow. The transformation is performed by a couple scripts written as Rust hostprogs. Tests using the `?` operator are also supported as usual, e.g.: /// ``` /// # use kernel::{spawn_work_item, workqueue}; /// spawn_work_item!(workqueue::system(), || pr_info!("x"))?; /// # Ok::<(), Error>(()) /// ``` The tests are also compiled with Clippy under `CLIPPY=1`, just like normal code, thus also benefitting from extra linting. The names of the tests are currently automatically generated. This allows to reduce the burden for documentation writers, while keeping them fairly stable for bisection. This is an improvement over the `rustdoc`-generated names, which include the line number; but ideally we would like to get `rustdoc` to provide the Rust item path and a number (for multiple examples in a single documented Rust item). In order for developers to easily see from which original line a failed doctests came from, a KTAP diagnostic line is printed to the log. In the future, we may be able to use a proper KUnit facility to append this sort of information instead. A notable difference from KUnit C tests is that the Rust tests appear to assert using the usual `assert!` and `assert_eq!` macros from the Rust standard library (`core`). We provide a custom version that forwards the call to KUnit instead. Importantly, these macros do not require passing context, unlike the KUnit C ones (i.e. `struct kunit *`). This makes them easier to use, and readers of the documentation do not need to care about which testing framework is used. In addition, it may allow us to test third-party code more easily in the future. However, a current limitation is that KUnit does not support assertions in other tasks. Thus we presently simply print an error to the kernel log if an assertion actually failed. This should be revisited to properly fail the test, perhaps saving the context somewhere else, or letting KUnit handle it. Signed-off-by: Miguel Ojeda Tested-by: Sergio González Collado Reviewed-by: David Gow Reviewed-by: Vincenzo Palazzo Reviewed-by: Martin Rodriguez Reboredo Tested-by: Matt Gilbride Reviewed-by: Alice Ryhl Link: https://lore.kernel.org/r/20230614180837.630180-6-ojeda@kernel.org [boqun: Add #include ] --- lib/Kconfig.debug | 13 +++ rust/.gitignore | 2 + rust/Makefile | 29 ++++++ rust/bindings/bindings_helper.h | 1 + rust/helpers.c | 8 ++ rust/kernel/kunit.rs | 156 ++++++++++++++++++++++++++++++ rust/kernel/lib.rs | 2 + scripts/.gitignore | 2 + scripts/Makefile | 4 + scripts/rustdoc_test_builder.rs | 73 ++++++++++++++ scripts/rustdoc_test_gen.rs | 162 ++++++++++++++++++++++++++++++++ 11 files changed, 452 insertions(+) create mode 100644 rust/kernel/kunit.rs create mode 100644 scripts/rustdoc_test_builder.rs create mode 100644 scripts/rustdoc_test_gen.rs diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index ce51d4dc6803ed..49f5e9c4220061 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -2911,6 +2911,19 @@ config RUST_BUILD_ASSERT_ALLOW If unsure, say N. +config RUST_KERNEL_KUNIT_TEST + bool "KUnit test for the `kernel` crate" if !KUNIT_ALL_TESTS + depends on RUST && KUNIT=y + default KUNIT_ALL_TESTS + help + This builds the documentation tests of the `kernel` crate + as KUnit tests. + + For more information on KUnit and unit tests in general, + please refer to the KUnit documentation in Documentation/dev-tools/kunit/. + + If unsure, say N. + endmenu # "Rust" endmenu # Kernel hacking diff --git a/rust/.gitignore b/rust/.gitignore index 21552992b401fa..d3829ffab80ba0 100644 --- a/rust/.gitignore +++ b/rust/.gitignore @@ -2,6 +2,8 @@ bindings_generated.rs bindings_helpers_generated.rs +doctests_kernel_generated.rs +doctests_kernel_generated_kunit.c uapi_generated.rs exports_*_generated.h doc/ diff --git a/rust/Makefile b/rust/Makefile index fc8cdcfcc9e598..e8f2e0b14fd038 100644 --- a/rust/Makefile +++ b/rust/Makefile @@ -27,6 +27,12 @@ endif obj-$(CONFIG_RUST) += exports.o +always-$(CONFIG_RUST_KERNEL_KUNIT_TEST) += doctests_kernel_generated.rs +always-$(CONFIG_RUST_KERNEL_KUNIT_TEST) += doctests_kernel_generated_kunit.c + +obj-$(CONFIG_RUST_KERNEL_KUNIT_TEST) += doctests_kernel_generated.o +obj-$(CONFIG_RUST_KERNEL_KUNIT_TEST) += doctests_kernel_generated_kunit.o + # Avoids running `$(RUSTC)` for the sysroot when it may not be available. ifdef CONFIG_RUST @@ -39,9 +45,11 @@ ifeq ($(quiet),silent_) cargo_quiet=-q rust_test_quiet=-q rustdoc_test_quiet=--test-args -q +rustdoc_test_kernel_quiet=>/dev/null else ifeq ($(quiet),quiet_) rust_test_quiet=-q rustdoc_test_quiet=--test-args -q +rustdoc_test_kernel_quiet=>/dev/null else cargo_quiet=--verbose endif @@ -157,6 +165,27 @@ quiet_cmd_rustdoc_test = RUSTDOC T $< -L$(objtree)/$(obj)/test --output $(objtree)/$(obj)/doc \ --crate-name $(subst rusttest-,,$@) $< +quiet_cmd_rustdoc_test_kernel = RUSTDOC TK $< + cmd_rustdoc_test_kernel = \ + rm -rf $(objtree)/$(obj)/test/doctests/kernel; \ + mkdir -p $(objtree)/$(obj)/test/doctests/kernel; \ + OBJTREE=$(abspath $(objtree)) \ + $(RUSTDOC) --test $(rust_flags) \ + @$(objtree)/include/generated/rustc_cfg \ + -L$(objtree)/$(obj) --extern alloc --extern kernel \ + --extern build_error --extern macros \ + --extern bindings --extern uapi \ + --no-run --crate-name kernel -Zunstable-options \ + --test-builder $(objtree)/scripts/rustdoc_test_builder \ + $< $(rustdoc_test_kernel_quiet); \ + $(objtree)/scripts/rustdoc_test_gen + +%/doctests_kernel_generated.rs %/doctests_kernel_generated_kunit.c: \ + $(src)/kernel/lib.rs $(obj)/kernel.o \ + $(objtree)/scripts/rustdoc_test_builder \ + $(objtree)/scripts/rustdoc_test_gen FORCE + $(call if_changed,rustdoc_test_kernel) + # We cannot use `-Zpanic-abort-tests` because some tests are dynamic, # so for the moment we skip `-Cpanic=abort`. quiet_cmd_rustc_test = RUSTC T $< diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h index 6619ce95dd3743..5db2940b2379a0 100644 --- a/rust/bindings/bindings_helper.h +++ b/rust/bindings/bindings_helper.h @@ -6,6 +6,7 @@ * Sorted alphabetically. */ +#include #include #include #include diff --git a/rust/helpers.c b/rust/helpers.c index d8cf773d966e43..64b118fae7ab10 100644 --- a/rust/helpers.c +++ b/rust/helpers.c @@ -18,6 +18,8 @@ * accidentally exposed. */ +#include +#include #include #include #include @@ -135,6 +137,12 @@ void rust_helper_put_task_struct(struct task_struct *t) } EXPORT_SYMBOL_GPL(rust_helper_put_task_struct); +struct kunit *rust_helper_kunit_get_current_test(void) +{ + return kunit_get_current_test(); +} +EXPORT_SYMBOL_GPL(rust_helper_kunit_get_current_test); + /* * `bindgen` binds the C `size_t` type the Rust `usize` type, so we can * use it in contexts where Rust expects a `usize` like slice (array) indices. diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs new file mode 100644 index 00000000000000..3c94efcd7f76e3 --- /dev/null +++ b/rust/kernel/kunit.rs @@ -0,0 +1,156 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! KUnit-based macros for Rust unit tests. +//! +//! C header: [`include/kunit/test.h`](../../../../../include/kunit/test.h) +//! +//! Reference: + +use core::{ffi::c_void, fmt}; + +/// Prints a KUnit error. +/// +/// Public but hidden since it should only be used from KUnit generated code. +#[doc(hidden)] +pub fn err(args: fmt::Arguments<'_>) { + // SAFETY: The format string is null-terminated and the `%pA` specifier matches the argument we + // are passing. + #[cfg(CONFIG_PRINTK)] + unsafe { + bindings::_printk( + b"\x013%pA\0".as_ptr() as _, + &args as *const _ as *const c_void, + ); + } +} + +/// Prints a KUnit error. +/// +/// Public but hidden since it should only be used from KUnit generated code. +#[doc(hidden)] +pub fn info(args: fmt::Arguments<'_>) { + // SAFETY: The format string is null-terminated and the `%pA` specifier matches the argument we + // are passing. + #[cfg(CONFIG_PRINTK)] + unsafe { + bindings::_printk( + b"\x016%pA\0".as_ptr() as _, + &args as *const _ as *const c_void, + ); + } +} + +/// Asserts that a boolean expression is `true` at runtime. +/// +/// Public but hidden since it should only be used from generated tests. +/// +/// Unlike the one in `core`, this one does not panic; instead, it is mapped to the KUnit +/// facilities. See [`assert!`] for more details. +#[doc(hidden)] +#[macro_export] +macro_rules! kunit_assert { + ($name:literal, $condition:expr $(,)?) => { + 'out: { + // Do nothing if the condition is `true`. + if $condition { + break 'out; + } + + static LINE: i32 = core::line!() as i32; + static FILE: &'static $crate::str::CStr = $crate::c_str!(core::file!()); + static CONDITION: &'static $crate::str::CStr = $crate::c_str!(stringify!($condition)); + + // SAFETY: FFI call without safety requirements. + let kunit_test = unsafe { $crate::bindings::kunit_get_current_test() }; + if kunit_test.is_null() { + // The assertion failed but this task is not running a KUnit test, so we cannot call + // KUnit, but at least print an error to the kernel log. This may happen if this + // macro is called from an spawned thread in a test (see + // `scripts/rustdoc_test_gen.rs`) or if some non-test code calls this macro by + // mistake (it is hidden to prevent that). + // + // This mimics KUnit's failed assertion format. + $crate::kunit::err(format_args!( + " # {}: ASSERTION FAILED at {FILE}:{LINE}\n", + $name + )); + $crate::kunit::err(format_args!( + " Expected {CONDITION} to be true, but is false\n" + )); + $crate::kunit::err(format_args!( + " Failure not reported to KUnit since this is a non-KUnit task\n" + )); + break 'out; + } + + #[repr(transparent)] + struct Location($crate::bindings::kunit_loc); + + #[repr(transparent)] + struct UnaryAssert($crate::bindings::kunit_unary_assert); + + // SAFETY: There is only a static instance and in that one the pointer field points to + // an immutable C string. + unsafe impl Sync for Location {} + + // SAFETY: There is only a static instance and in that one the pointer field points to + // an immutable C string. + unsafe impl Sync for UnaryAssert {} + + static LOCATION: Location = Location($crate::bindings::kunit_loc { + file: FILE.as_char_ptr(), + line: LINE, + }); + static ASSERTION: UnaryAssert = UnaryAssert($crate::bindings::kunit_unary_assert { + assert: $crate::bindings::kunit_assert {}, + condition: CONDITION.as_char_ptr(), + expected_true: true, + }); + + // SAFETY: + // - FFI call. + // - The `kunit_test` pointer is valid because we got it from + // `kunit_get_current_test()` and it was not null. This means we are in a KUnit + // test, and that the pointer can be passed to KUnit functions and assertions. + // - The string pointers (`file` and `condition` above) point to null-terminated + // strings since they are `CStr`s. + // - The function pointer (`format`) points to the proper function. + // - The pointers passed will remain valid since they point to `static`s. + // - The format string is allowed to be null. + // - There are, however, problems with this: first of all, this will end up stopping + // the thread, without running destructors. While that is problematic in itself, + // it is considered UB to have what is effectively a forced foreign unwind + // with `extern "C"` ABI. One could observe the stack that is now gone from + // another thread. We should avoid pinning stack variables to prevent library UB, + // too. For the moment, given that test failures are reported immediately before the + // next test runs, that test failures should be fixed and that KUnit is explicitly + // documented as not suitable for production environments, we feel it is reasonable. + unsafe { + $crate::bindings::kunit_do_failed_assertion( + kunit_test, + core::ptr::addr_of!(LOCATION.0), + $crate::bindings::kunit_assert_type_KUNIT_ASSERTION, + core::ptr::addr_of!(ASSERTION.0.assert), + Some($crate::bindings::kunit_unary_assert_format), + core::ptr::null(), + ); + } + } + }; +} + +/// Asserts that two expressions are equal to each other (using [`PartialEq`]). +/// +/// Public but hidden since it should only be used from generated tests. +/// +/// Unlike the one in `core`, this one does not panic; instead, it is mapped to the KUnit +/// facilities. See [`assert!`] for more details. +#[doc(hidden)] +#[macro_export] +macro_rules! kunit_assert_eq { + ($name:literal, $left:expr, $right:expr $(,)?) => {{ + // For the moment, we just forward to the expression assert because, for binary asserts, + // KUnit supports only a few types (e.g. integers). + $crate::kunit_assert!($name, $left == $right); + }}; +} diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index d59041ff5ff211..e8811700239aaf 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -34,6 +34,8 @@ mod build_assert; pub mod error; pub mod init; pub mod ioctl; +#[cfg(CONFIG_KUNIT)] +pub mod kunit; pub mod prelude; pub mod print; mod static_assert; diff --git a/scripts/.gitignore b/scripts/.gitignore index 6e9ce6720a05aa..3dbb8bb2457bca 100644 --- a/scripts/.gitignore +++ b/scripts/.gitignore @@ -5,6 +5,8 @@ /kallsyms /module.lds /recordmcount +/rustdoc_test_builder +/rustdoc_test_gen /sign-file /sorttable /target.json diff --git a/scripts/Makefile b/scripts/Makefile index 32b6ba7227284c..d5a5382e753c90 100644 --- a/scripts/Makefile +++ b/scripts/Makefile @@ -9,6 +9,8 @@ hostprogs-always-$(CONFIG_BUILDTIME_TABLE_SORT) += sorttable hostprogs-always-$(CONFIG_ASN1) += asn1_compiler hostprogs-always-$(CONFIG_MODULE_SIG_FORMAT) += sign-file hostprogs-always-$(CONFIG_SYSTEM_EXTRA_CERTIFICATE) += insert-sys-cert +hostprogs-always-$(CONFIG_RUST_KERNEL_KUNIT_TEST) += rustdoc_test_builder +hostprogs-always-$(CONFIG_RUST_KERNEL_KUNIT_TEST) += rustdoc_test_gen always-$(CONFIG_RUST) += target.json filechk_rust_target = $< < include/config/auto.conf @@ -18,6 +20,8 @@ $(obj)/target.json: scripts/generate_rust_target include/config/auto.conf FORCE hostprogs += generate_rust_target generate_rust_target-rust := y +rustdoc_test_builder-rust := y +rustdoc_test_gen-rust := y HOSTCFLAGS_sorttable.o = -I$(srctree)/tools/include HOSTLDLIBS_sorttable = -lpthread diff --git a/scripts/rustdoc_test_builder.rs b/scripts/rustdoc_test_builder.rs new file mode 100644 index 00000000000000..e3b7138fb4f98b --- /dev/null +++ b/scripts/rustdoc_test_builder.rs @@ -0,0 +1,73 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! Test builder for `rustdoc`-generated tests. +//! +//! This script is a hack to extract the test from `rustdoc`'s output. Ideally, `rustdoc` would +//! have an option to generate this information instead, e.g. as JSON output. +//! +//! The `rustdoc`-generated test names look like `{file}_{line}_{number}`, e.g. +//! `...path_rust_kernel_sync_arc_rs_42_0`. `number` is the "test number", needed in cases like +//! a macro that expands into items with doctests is invoked several times within the same line. +//! +//! However, since these names are used for bisection in CI, the line number makes it not stable +//! at all. In the future, we would like `rustdoc` to give us the Rust item path associated with +//! the test, plus a "test number" (for cases with several examples per item) and generate a name +//! from that. For the moment, we generate ourselves a new name, `{file}_{number}` instead, in +//! the `gen` script (done there since we need to be aware of all the tests in a given file). + +use std::fs::File; +use std::io::{BufWriter, Read, Write}; + +fn main() { + let mut stdin = std::io::stdin().lock(); + let mut body = String::new(); + stdin.read_to_string(&mut body).unwrap(); + + // Find the generated function name looking for the inner function inside `main()`. + // + // The line we are looking for looks like one of the following: + // + // ``` + // fn main() { #[allow(non_snake_case)] fn _doctest_main_rust_kernel_file_rs_28_0() { + // fn main() { #[allow(non_snake_case)] fn _doctest_main_rust_kernel_file_rs_37_0() -> Result<(), impl core::fmt::Debug> { + // ``` + // + // It should be unlikely that doctest code matches such lines (when code is formatted properly). + let rustdoc_function_name = body + .lines() + .find_map(|line| { + Some( + line.split_once("fn main() {")? + .1 + .split_once("fn ")? + .1 + .split_once("()")? + .0, + ) + .filter(|x| x.chars().all(|c| c.is_alphanumeric() || c == '_')) + }) + .expect("No test function found in `rustdoc`'s output."); + + // Qualify `Result` to avoid the collision with our own `Result` coming from the prelude. + let body = body.replace( + &format!("{rustdoc_function_name}() -> Result<(), impl core::fmt::Debug> {{"), + &format!("{rustdoc_function_name}() -> core::result::Result<(), impl core::fmt::Debug> {{"), + ); + + // For tests that get generated with `Result`, like above, `rustdoc` generates an `unwrap()` on + // the return value to check there were no returned errors. Instead, we use our assert macro + // since we want to just fail the test, not panic the kernel. + // + // We save the result in a variable so that the failed assertion message looks nicer. + let body = body.replace( + &format!("}} {rustdoc_function_name}().unwrap() }}"), + &format!("}} let test_return_value = {rustdoc_function_name}(); assert!(test_return_value.is_ok()); }}"), + ); + + // Figure out a smaller test name based on the generated function name. + let name = rustdoc_function_name.split_once("_rust_kernel_").unwrap().1; + + let path = format!("rust/test/doctests/kernel/{name}"); + + write!(BufWriter::new(File::create(path).unwrap()), "{body}").unwrap(); +} diff --git a/scripts/rustdoc_test_gen.rs b/scripts/rustdoc_test_gen.rs new file mode 100644 index 00000000000000..793885c32c0df5 --- /dev/null +++ b/scripts/rustdoc_test_gen.rs @@ -0,0 +1,162 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! Generates KUnit tests from saved `rustdoc`-generated tests. +//! +//! KUnit passes a context (`struct kunit *`) to each test, which should be forwarded to the other +//! KUnit functions and macros. +//! +//! However, we want to keep this as an implementation detail because: +//! +//! - Test code should not care about the implementation. +//! +//! - Documentation looks worse if it needs to carry extra details unrelated to the piece +//! being described. +//! +//! - Test code should be able to define functions and call them, without having to carry +//! the context. +//! +//! - Later on, we may want to be able to test non-kernel code (e.g. `core`, `alloc` or +//! third-party crates) which likely use the standard library `assert*!` macros. +//! +//! For this reason, instead of the passed context, `kunit_get_current_test()` is used instead +//! (i.e. `current->kunit_test`). +//! +//! Note that this means other threads/tasks potentially spawned by a given test, if failing, will +//! report the failure in the kernel log but will not fail the actual test. Saving the pointer in +//! e.g. a `static` per test does not fully solve the issue either, because currently KUnit does +//! not support assertions (only expectations) from other tasks. Thus leave that feature for +//! the future, which simplifies the code here too. We could also simply not allow `assert`s in +//! other tasks, but that seems overly constraining, and we do want to support them, eventually. + +use std::io::{BufWriter, Read, Write}; +use std::{fs, fs::File}; + +fn main() { + let mut paths = fs::read_dir("rust/test/doctests/kernel") + .unwrap() + .map(|entry| entry.unwrap().path()) + .collect::>(); + + // Sort paths for clarity. + paths.sort(); + + let mut rust_tests = String::new(); + let mut c_test_declarations = String::new(); + let mut c_test_cases = String::new(); + let mut body = String::new(); + let mut last_file = String::new(); + let mut number = 0; + for path in paths { + // The `name` follows the `{file}_{line}_{number}` pattern (see description in + // `scripts/rustdoc_test_builder.rs`). Discard the `number`. + let name = path.file_name().unwrap().to_str().unwrap().to_string(); + + // Extract the `file` and the `line`, discarding the `number`. + let (file, line) = name.rsplit_once('_').unwrap().0.rsplit_once('_').unwrap(); + + // Generate an ID sequence ("test number") for each one in the file. + if file == last_file { + number += 1; + } else { + number = 0; + last_file = file.to_string(); + } + + // Generate a KUnit name (i.e. test name and C symbol) for this test. + // + // We avoid the line number, like `rustdoc` does, to make things slightly more stable for + // bisection purposes. However, to aid developers in mapping back what test failed, we will + // print a diagnostics line in the KTAP report. + let kunit_name = format!("rust_doctest_kernel_{file}_{number}"); + + // Read the test's text contents to dump it below. + body.clear(); + File::open(path).unwrap().read_to_string(&mut body).unwrap(); + + let line = line.parse::().unwrap(); + + use std::fmt::Write; + write!( + rust_tests, + r#"/// Generated `{name}` KUnit test case from a Rust documentation test. +#[no_mangle] +pub extern "C" fn {kunit_name}(__kunit_test: *mut kernel::bindings::kunit) {{ + /// Overrides the usual [`assert!`] macro with one that calls KUnit instead. + #[allow(unused)] + macro_rules! assert {{ + ($cond:expr $(,)?) => {{{{ + kernel::kunit_assert!("{kunit_name}", $cond); + }}}} + }} + + /// Overrides the usual [`assert_eq!`] macro with one that calls KUnit instead. + #[allow(unused)] + macro_rules! assert_eq {{ + ($left:expr, $right:expr $(,)?) => {{{{ + kernel::kunit_assert_eq!("{kunit_name}", $left, $right); + }}}} + }} + + // Many tests need the prelude, so provide it by default. + #[allow(unused)] + use kernel::prelude::*; + + // Display line number so that developers can map the test easily to the source code. + kernel::kunit::info(format_args!(" # Doctest from line {line}\n")); + + {{ + {body} + main(); + }} +}} + +"# + ) + .unwrap(); + + write!(c_test_declarations, "void {kunit_name}(struct kunit *);\n").unwrap(); + write!(c_test_cases, " KUNIT_CASE({kunit_name}),\n").unwrap(); + } + + let rust_tests = rust_tests.trim(); + let c_test_declarations = c_test_declarations.trim(); + let c_test_cases = c_test_cases.trim(); + + write!( + BufWriter::new(File::create("rust/doctests_kernel_generated.rs").unwrap()), + r#"//! `kernel` crate documentation tests. + +const __LOG_PREFIX: &[u8] = b"rust_doctests_kernel\0"; + +{rust_tests} +"# + ) + .unwrap(); + + write!( + BufWriter::new(File::create("rust/doctests_kernel_generated_kunit.c").unwrap()), + r#"/* + * `kernel` crate documentation tests. + */ + +#include + +{c_test_declarations} + +static struct kunit_case test_cases[] = {{ + {c_test_cases} + {{ }} +}}; + +static struct kunit_suite test_suite = {{ + .name = "rust_doctests_kernel", + .test_cases = test_cases, +}}; + +kunit_test_suite(test_suite); + +MODULE_LICENSE("GPL"); +"# + ) + .unwrap(); +} From eff7a66161e9583c8220991daf13e3e95c7163fc Mon Sep 17 00:00:00 2001 From: Miguel Ojeda Date: Wed, 14 Jun 2023 20:08:30 +0200 Subject: [PATCH 22/37] MAINTAINERS: add Rust KUnit files to the KUnit entry The KUnit maintainers would like to maintain these files on their side too (thanks!), so add them to their entry. With this in place, `scripts/get_maintainer.pl` prints both sets of maintainers/reviewers (i.e. KUnit and Rust) for those files, which is the behavior we are looking for. Signed-off-by: Miguel Ojeda Reviewed-by: David Gow Reviewed-by: Vincenzo Palazzo Reviewed-by: Martin Rodriguez Reboredo Link: https://lore.kernel.org/r/20230614180837.630180-7-ojeda@kernel.org --- MAINTAINERS | 2 ++ 1 file changed, 2 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 250518fc70ff5f..f4c9ce1b685f7d 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11323,6 +11323,8 @@ W: https://google.github.io/kunit-docs/third_party/kernel/docs/ F: Documentation/dev-tools/kunit/ F: include/kunit/ F: lib/kunit/ +F: rust/kernel/kunit.rs +F: scripts/rustdoc_test_* F: tools/testing/kunit/ KERNEL USERMODE HELPER From 72fbe4a4de2d53599cecda660a2f78c2ee66e4bb Mon Sep 17 00:00:00 2001 From: Qingsong Chen Date: Mon, 26 Jun 2023 15:42:42 +0800 Subject: [PATCH 23/37] rust: macros: fix redefine const_name in `vtable` If the trait has same function name, the `vtable` macro will redefine its `gen_const_name`, e.g.: ```rust #[vtable] pub trait Foo { #[cfg(CONFIG_X)] fn bar(); #[cfg(not(CONFIG_X))] fn bar(x: usize); } ``` Use `HashSet` to avoid this. Signed-off-by: Qingsong Chen Reviewed-by: Gary Guo Reviewed-by: Benno Lossin Reviewed-by: Martin Rodriguez Reboredo Link: https://lore.kernel.org/r/20230626074242.3945398-2-changxian.cqs@antgroup.com --- rust/macros/vtable.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rust/macros/vtable.rs b/rust/macros/vtable.rs index 34d5e7fb5768a2..08eb0355f99bf7 100644 --- a/rust/macros/vtable.rs +++ b/rust/macros/vtable.rs @@ -27,7 +27,7 @@ pub(crate) fn vtable(_attr: TokenStream, ts: TokenStream) -> TokenStream { }; let mut body_it = body.stream().into_iter(); - let mut functions = Vec::new(); + let mut functions = HashSet::new(); let mut consts = HashSet::new(); while let Some(token) = body_it.next() { match token { @@ -37,7 +37,7 @@ pub(crate) fn vtable(_attr: TokenStream, ts: TokenStream) -> TokenStream { // Possibly we've encountered a fn pointer type instead. _ => continue, }; - functions.push(fn_name); + functions.insert(fn_name); } TokenTree::Ident(ident) if ident.to_string() == "const" => { let const_name = match body_it.next() { From 8e87a0dffe01b5d15ef37496ea3191a0b8eeb4d7 Mon Sep 17 00:00:00 2001 From: Gary Guo Date: Wed, 28 Jun 2023 18:11:01 +0100 Subject: [PATCH 24/37] rust: macros: add `paste!` proc macro MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This macro provides a flexible way to concatenated identifiers together and it allows the resulting identifier to be used to declare new items, which `concat_idents!` does not allow. It also allows identifiers to be transformed before concatenated. The `concat_idents!` example let x_1 = 42; let x_2 = concat_idents!(x, _1); assert!(x_1 == x_2); can be written with `paste!` macro like this: let x_1 = 42; let x_2 = paste!([]); assert!(x_1 == x_2); However `paste!` macro is more flexible because it can be used to create a new variable: let x_1 = 42; paste!(let [] = [];); assert!(x_1 == x_2); While this is not possible with `concat_idents!`. This macro is similar to the `paste!` crate [1], but this is a fresh implementation to avoid vendoring large amount of code directly. Also, I have augmented it to provide a way to specify span of the resulting token, allowing precise control. For example, this code is broken because the variable is declared inside the macro, so Rust macro hygiene rules prevents access from the outside: macro_rules! m { ($id: ident) => { // The resulting token has hygiene of the macro. paste!(let [<$id>] = 1;) } } m!(a); let _ = a; In this versionn of `paste!` macro I added a `span` modifier to allow this: macro_rules! m { ($id: ident) => { // The resulting token has hygiene of `$id`. paste!(let [<$id:span>] = 1;) } } m!(a); let _ = a; Link: http://docs.rs/paste/ [1] Signed-off-by: Gary Guo Reviewed-by: Björn Roy Baron Reviewed-by: Benno Lossin Link: https://lore.kernel.org/r/20230628171108.1150742-1-gary@garyguo.net --- rust/macros/lib.rs | 97 ++++++++++++++++++++++++++++++++++++++++++++ rust/macros/paste.rs | 94 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 191 insertions(+) create mode 100644 rust/macros/paste.rs diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs index 3fc74cb4ea1903..b4bc44c27bd4c6 100644 --- a/rust/macros/lib.rs +++ b/rust/macros/lib.rs @@ -7,6 +7,7 @@ mod quote; mod concat_idents; mod helpers; mod module; +mod paste; mod pin_data; mod pinned_drop; mod vtable; @@ -246,3 +247,99 @@ pub fn pin_data(inner: TokenStream, item: TokenStream) -> TokenStream { pub fn pinned_drop(args: TokenStream, input: TokenStream) -> TokenStream { pinned_drop::pinned_drop(args, input) } + +/// Paste identifiers together. +/// +/// Within the `paste!` macro, identifiers inside `[<` and `>]` are concatenated together to form a +/// single identifier. +/// +/// This is similar to the [`paste`] crate, but with pasting feature limited to identifiers +/// (literals, lifetimes and documentation strings are not supported). There is a difference in +/// supported modifiers as well. +/// +/// # Example +/// +/// ```ignore +/// use kernel::macro::paste; +/// +/// macro_rules! pub_no_prefix { +/// ($prefix:ident, $($newname:ident),+) => { +/// paste! { +/// $(pub(crate) const $newname: u32 = [<$prefix $newname>];)+ +/// } +/// }; +/// } +/// +/// pub_no_prefix!( +/// binder_driver_return_protocol_, +/// BR_OK, +/// BR_ERROR, +/// BR_TRANSACTION, +/// BR_REPLY, +/// BR_DEAD_REPLY, +/// BR_TRANSACTION_COMPLETE, +/// BR_INCREFS, +/// BR_ACQUIRE, +/// BR_RELEASE, +/// BR_DECREFS, +/// BR_NOOP, +/// BR_SPAWN_LOOPER, +/// BR_DEAD_BINDER, +/// BR_CLEAR_DEATH_NOTIFICATION_DONE, +/// BR_FAILED_REPLY +/// ); +/// +/// assert_eq!(BR_OK, binder_driver_return_protocol_BR_OK); +/// ``` +/// +/// # Modifiers +/// +/// For each identifier, it is possible to attach one or multiple modifiers to +/// it. +/// +/// Currently supported modifiers are: +/// * `span`: change the span of concatenated identifier to the span of the specified token. By +/// default the span of the `[< >]` group is used. +/// * `lower`: change the identifier to lower case. +/// * `upper`: change the identifier to upper case. +/// +/// ```ignore +/// use kernel::macro::paste; +/// +/// macro_rules! pub_no_prefix { +/// ($prefix:ident, $($newname:ident),+) => { +/// kernel::macros::paste! { +/// $(pub(crate) const fn [<$newname:lower:span>]: u32 = [<$prefix $newname:span>];)+ +/// } +/// }; +/// } +/// +/// pub_no_prefix!( +/// binder_driver_return_protocol_, +/// BR_OK, +/// BR_ERROR, +/// BR_TRANSACTION, +/// BR_REPLY, +/// BR_DEAD_REPLY, +/// BR_TRANSACTION_COMPLETE, +/// BR_INCREFS, +/// BR_ACQUIRE, +/// BR_RELEASE, +/// BR_DECREFS, +/// BR_NOOP, +/// BR_SPAWN_LOOPER, +/// BR_DEAD_BINDER, +/// BR_CLEAR_DEATH_NOTIFICATION_DONE, +/// BR_FAILED_REPLY +/// ); +/// +/// assert_eq!(br_ok(), binder_driver_return_protocol_BR_OK); +/// ``` +/// +/// [`paste`]: https://docs.rs/paste/ +#[proc_macro] +pub fn paste(input: TokenStream) -> TokenStream { + let mut tokens = input.into_iter().collect(); + paste::expand(&mut tokens); + tokens.into_iter().collect() +} diff --git a/rust/macros/paste.rs b/rust/macros/paste.rs new file mode 100644 index 00000000000000..42fde0930b0589 --- /dev/null +++ b/rust/macros/paste.rs @@ -0,0 +1,94 @@ +use proc_macro::{Delimiter, Group, Ident, Spacing, Span, TokenTree}; + +fn concat(tokens: &[TokenTree], group_span: Span) -> TokenTree { + let mut tokens = tokens.iter(); + let mut segments = Vec::new(); + let mut span = None; + loop { + match tokens.next() { + None => break, + Some(TokenTree::Literal(lit)) => segments.push((lit.to_string(), lit.span())), + Some(TokenTree::Ident(ident)) => { + let mut value = ident.to_string(); + if value.starts_with("r#") { + value.replace_range(0..2, ""); + } + segments.push((value, ident.span())); + } + Some(TokenTree::Punct(p)) if p.as_char() == ':' => { + let Some(TokenTree::Ident(ident)) = tokens.next() else { + panic!("expected identifier as modifier"); + }; + + let (mut value, sp) = segments.pop().expect("expected identifier before modifier"); + match ident.to_string().as_str() { + // Set the overall span of concatenated token as current span + "span" => { + assert!( + span.is_none(), + "span modifier should only appear at most once" + ); + span = Some(sp); + } + "lower" => value = value.to_lowercase(), + "upper" => value = value.to_uppercase(), + v => panic!("unknown modifier `{v}`"), + }; + segments.push((value, sp)); + } + _ => panic!("unexpected token in paste segments"), + }; + } + + let pasted: String = segments.into_iter().map(|x| x.0).collect(); + TokenTree::Ident(Ident::new(&pasted, span.unwrap_or(group_span))) +} + +pub(crate) fn expand(tokens: &mut Vec) { + for token in tokens.iter_mut() { + if let TokenTree::Group(group) = token { + let delimiter = group.delimiter(); + let span = group.span(); + let mut stream: Vec<_> = group.stream().into_iter().collect(); + // Find groups that looks like `[< A B C D >]` + if delimiter == Delimiter::Bracket + && stream.len() >= 3 + && matches!(&stream[0], TokenTree::Punct(p) if p.as_char() == '<') + && matches!(&stream[stream.len() - 1], TokenTree::Punct(p) if p.as_char() == '>') + { + // Replace the group with concatenated token + *token = concat(&stream[1..stream.len() - 1], span); + } else { + // Recursively expand tokens inside the group + expand(&mut stream); + let mut group = Group::new(delimiter, stream.into_iter().collect()); + group.set_span(span); + *token = TokenTree::Group(group); + } + } + } + + // Path segments cannot contain invisible delimiter group, so remove them if any. + for i in (0..tokens.len().saturating_sub(3)).rev() { + // Looking for a double colon + if matches!( + (&tokens[i + 1], &tokens[i + 2]), + (TokenTree::Punct(a), TokenTree::Punct(b)) + if a.as_char() == ':' && a.spacing() == Spacing::Joint && b.as_char() == ':' + ) { + match &tokens[i + 3] { + TokenTree::Group(group) if group.delimiter() == Delimiter::None => { + tokens.splice(i + 3..i + 4, group.stream()); + } + _ => (), + } + + match &tokens[i] { + TokenTree::Group(group) if group.delimiter() == Delimiter::None => { + tokens.splice(i..i + 1, group.stream()); + } + _ => (), + } + } + } +} From 7c6e564f3f62d3ec05abf1a1e4cbf27595177d63 Mon Sep 17 00:00:00 2001 From: Jamie Cunliffe Date: Tue, 6 Jun 2023 15:56:04 +0100 Subject: [PATCH 25/37] arm64: rust: Enable Rust support for AArch64 This commit provides the build flags for Rust for AArch64. The core Rust support already in the kernel does the rest. When disabling the neon and fp target features to avoid fp & simd registers. The use of fp-armv8 will cause a warning from rustc about an unknown feature that is specified. The target feature is still passed through to LLVM, this behaviour is documented as part of the warning. This will be fixed in a future version of the rustc toolchain. The Rust samples have been tested with this commit. Signed-off-by: Jamie Cunliffe Link: https://lore.kernel.org/r/20230606145606.1153715-2-Jamie.Cunliffe@arm.com [boqun: resolve the conflicts with kunit test enablement] --- Documentation/rust/arch-support.rst | 1 + Makefile | 1 - arch/arm64/Kconfig | 1 + arch/arm64/Makefile | 2 ++ arch/x86/Makefile | 1 + rust/Makefile | 6 +++++- scripts/Makefile | 5 +++-- scripts/generate_rust_target.rs | 4 +++- 8 files changed, 16 insertions(+), 5 deletions(-) diff --git a/Documentation/rust/arch-support.rst b/Documentation/rust/arch-support.rst index b91e9ef4d0c21e..9b022af2f649c4 100644 --- a/Documentation/rust/arch-support.rst +++ b/Documentation/rust/arch-support.rst @@ -15,6 +15,7 @@ support corresponds to ``S`` values in the ``MAINTAINERS`` file. ============ ================ ============================================== Architecture Level of support Constraints ============ ================ ============================================== +``arm64`` Maintained None. ``um`` Maintained ``x86_64`` only. ``x86`` Maintained ``x86_64`` only. ============ ================ ============================================== diff --git a/Makefile b/Makefile index 3c5775ae78b834..9364cc4e853a96 100644 --- a/Makefile +++ b/Makefile @@ -561,7 +561,6 @@ KBUILD_CFLAGS := -Wall -Wundef -Werror=strict-prototypes -Wno-trigraphs \ -std=gnu11 KBUILD_CPPFLAGS := -D__KERNEL__ KBUILD_RUSTFLAGS := $(rust_common_flags) \ - --target=$(objtree)/scripts/target.json \ -Cpanic=abort -Cembed-bitcode=n -Clto=n \ -Cforce-unwind-tables=n -Ccodegen-units=1 \ -Csymbol-mangling-version=v0 \ diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index b1201d25a8a4ee..10e9399e8e2f26 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -217,6 +217,7 @@ config ARM64 select HAVE_FUNCTION_ARG_ACCESS_API select MMU_GATHER_RCU_TABLE_FREE select HAVE_RSEQ + select HAVE_RUST select HAVE_STACKPROTECTOR select HAVE_SYSCALL_TRACEPOINTS select HAVE_KPROBES diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile index 2d49aea0ff67a8..2ce1555e9fc540 100644 --- a/arch/arm64/Makefile +++ b/arch/arm64/Makefile @@ -41,6 +41,8 @@ KBUILD_CFLAGS += -mgeneral-regs-only \ KBUILD_CFLAGS += $(call cc-disable-warning, psabi) KBUILD_AFLAGS += $(compat_vdso) +KBUILD_RUSTFLAGS += --target aarch64-unknown-none -C target-feature="-neon,-fp-armv8" + KBUILD_CFLAGS += $(call cc-option,-mabi=lp64) KBUILD_AFLAGS += $(call cc-option,-mabi=lp64) diff --git a/arch/x86/Makefile b/arch/x86/Makefile index b39975977c037c..79cffe92c91605 100644 --- a/arch/x86/Makefile +++ b/arch/x86/Makefile @@ -68,6 +68,7 @@ export BITS # https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53383 # KBUILD_CFLAGS += -mno-sse -mno-mmx -mno-sse2 -mno-3dnow -mno-avx +KBUILD_RUSTFLAGS += --target=$(objtree)/scripts/target.json KBUILD_RUSTFLAGS += -Ctarget-feature=-sse,-sse2,-sse3,-ssse3,-sse4.1,-sse4.2,-avx,-avx2 ifeq ($(CONFIG_X86_KERNEL_IBT),y) diff --git a/rust/Makefile b/rust/Makefile index e8f2e0b14fd038..2e95487810caa4 100644 --- a/rust/Makefile +++ b/rust/Makefile @@ -291,6 +291,7 @@ bindgen_skip_c_flags := -mno-fp-ret-in-387 -mpreferred-stack-boundary=% \ # Derived from `scripts/Makefile.clang`. BINDGEN_TARGET_x86 := x86_64-linux-gnu +BINDGEN_TARGET_arm64 := aarch64-linux-gnu BINDGEN_TARGET := $(BINDGEN_TARGET_$(SRCARCH)) # All warnings are inhibited since GCC builds are very experimental, @@ -422,8 +423,11 @@ $(obj)/core.o: private skip_clippy = 1 $(obj)/core.o: private skip_flags = -Dunreachable_pub $(obj)/core.o: private rustc_objcopy = $(foreach sym,$(redirect-intrinsics),--redefine-sym $(sym)=__rust$(sym)) $(obj)/core.o: private rustc_target_flags = $(core-cfgs) -$(obj)/core.o: $(RUST_LIB_SRC)/core/src/lib.rs scripts/target.json FORCE +$(obj)/core.o: $(RUST_LIB_SRC)/core/src/lib.rs FORCE $(call if_changed_dep,rustc_library) +ifeq ($(ARCH),x86_64) +$(obj)/core.o: scripts/target.json +endif $(obj)/compiler_builtins.o: private rustc_objcopy = -w -W '__*' $(obj)/compiler_builtins.o: $(src)/compiler_builtins.rs $(obj)/core.o FORCE diff --git a/scripts/Makefile b/scripts/Makefile index d5a5382e753c90..281eabecffd1e6 100644 --- a/scripts/Makefile +++ b/scripts/Makefile @@ -11,12 +11,13 @@ hostprogs-always-$(CONFIG_MODULE_SIG_FORMAT) += sign-file hostprogs-always-$(CONFIG_SYSTEM_EXTRA_CERTIFICATE) += insert-sys-cert hostprogs-always-$(CONFIG_RUST_KERNEL_KUNIT_TEST) += rustdoc_test_builder hostprogs-always-$(CONFIG_RUST_KERNEL_KUNIT_TEST) += rustdoc_test_gen -always-$(CONFIG_RUST) += target.json +ifeq ($(ARCH),x86_64) +always-$(CONFIG_RUST) += target.json filechk_rust_target = $< < include/config/auto.conf - $(obj)/target.json: scripts/generate_rust_target include/config/auto.conf FORCE $(call filechk,rust_target) +endif hostprogs += generate_rust_target generate_rust_target-rust := y diff --git a/scripts/generate_rust_target.rs b/scripts/generate_rust_target.rs index 3c6cbe2b278d30..ec5ef35dbe52db 100644 --- a/scripts/generate_rust_target.rs +++ b/scripts/generate_rust_target.rs @@ -148,7 +148,9 @@ fn main() { let mut ts = TargetSpec::new(); // `llvm-target`s are taken from `scripts/Makefile.clang`. - if cfg.has("X86_64") { + if cfg.has("ARM64") { + panic!("arm64 uses the builtin rustc aarch64-unknown-none target"); + } else if cfg.has("X86_64") { ts.push("arch", "x86_64"); ts.push( "data-layout", From 522ba057fc1fb6224404b0d43fdcec26a624f79b Mon Sep 17 00:00:00 2001 From: Jamie Cunliffe Date: Tue, 6 Jun 2023 15:56:05 +0100 Subject: [PATCH 26/37] arm64: rust: Enable PAC support for Rust. Enable the PAC ret and BTI options in the Rust build flags to match the options that are used when building C. Signed-off-by: Jamie Cunliffe Link: https://lore.kernel.org/r/20230606145606.1153715-3-Jamie.Cunliffe@arm.com --- arch/arm64/Makefile | 2 ++ 1 file changed, 2 insertions(+) diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile index 2ce1555e9fc540..4a2c807d65dbb8 100644 --- a/arch/arm64/Makefile +++ b/arch/arm64/Makefile @@ -67,7 +67,9 @@ endif ifeq ($(CONFIG_ARM64_BTI_KERNEL),y) KBUILD_CFLAGS += -mbranch-protection=pac-ret+bti + KBUILD_RUSTFLAGS += -Z branch-protection=bti,pac-ret else ifeq ($(CONFIG_ARM64_PTR_AUTH_KERNEL),y) + KBUILD_RUSTFLAGS += -Z branch-protection=pac-ret ifeq ($(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET),y) KBUILD_CFLAGS += -mbranch-protection=pac-ret else From b1555e1336fb60308ccaa83cf31e3fae54bce225 Mon Sep 17 00:00:00 2001 From: Jamie Cunliffe Date: Tue, 6 Jun 2023 15:56:06 +0100 Subject: [PATCH 27/37] arm64: Restrict Rust support to little endian only. Signed-off-by: Jamie Cunliffe Link: https://lore.kernel.org/r/20230606145606.1153715-4-Jamie.Cunliffe@arm.com --- Documentation/rust/arch-support.rst | 2 +- arch/arm64/Kconfig | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Documentation/rust/arch-support.rst b/Documentation/rust/arch-support.rst index 9b022af2f649c4..6bcb3b97c5b610 100644 --- a/Documentation/rust/arch-support.rst +++ b/Documentation/rust/arch-support.rst @@ -15,7 +15,7 @@ support corresponds to ``S`` values in the ``MAINTAINERS`` file. ============ ================ ============================================== Architecture Level of support Constraints ============ ================ ============================================== -``arm64`` Maintained None. +``arm64`` Maintained Little Endian only. ``um`` Maintained ``x86_64`` only. ``x86`` Maintained ``x86_64`` only. ============ ================ ============================================== diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 10e9399e8e2f26..02187dd3b839f3 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -217,7 +217,7 @@ config ARM64 select HAVE_FUNCTION_ARG_ACCESS_API select MMU_GATHER_RCU_TABLE_FREE select HAVE_RSEQ - select HAVE_RUST + select HAVE_RUST if CPU_LITTLE_ENDIAN select HAVE_STACKPROTECTOR select HAVE_SYSCALL_TRACEPOINTS select HAVE_KPROBES From 19b33fc2b9334084308ca0c733145b6c3bd87a32 Mon Sep 17 00:00:00 2001 From: Matt Gilbride Date: Thu, 29 Jun 2023 13:24:46 +0000 Subject: [PATCH 28/37] rust: bindgen: update installation instructions As of v0.61.0 the bindgen executable is installed with `cargo install bindgen-cli` instead of `cargo install bindgen` --- Documentation/rust/quick-start.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/rust/quick-start.rst b/Documentation/rust/quick-start.rst index 58a183bb90b1e3..2cdf75eeae1d80 100644 --- a/Documentation/rust/quick-start.rst +++ b/Documentation/rust/quick-start.rst @@ -98,7 +98,7 @@ the ``bindgen`` tool. A particular version is required. Install it via (note that this will download and build the tool from source):: - cargo install --locked --version $(scripts/min-tool-version.sh bindgen) bindgen + cargo install --locked --version $(scripts/min-tool-version.sh bindgen) bindgen-cli ``bindgen`` needs to find a suitable ``libclang`` in order to work. If it is not found (or a different ``libclang`` than the one found should be used), From c1c510f82e4eefae30d6832ca45ba866d0f8475d Mon Sep 17 00:00:00 2001 From: Asahi Lina Date: Tue, 27 Jun 2023 17:31:45 +0000 Subject: [PATCH 29/37] rust: xarray: Add an abstraction for XArray The XArray is an abstract data type which behaves like a very large array of pointers. Add a Rust abstraction for this data type. The initial implementation uses explicit locking on get operations and returns a guard which blocks mutation, ensuring that the referenced object remains alive. To avoid excessive serialization, users are expected to use an inner type that can be efficiently cloned (such as Arc), and eagerly clone and drop the guard to unblock other users after a lookup. Future variants may support using RCU instead to avoid mutex locking. Co-Authored-by: Matt Gilbride Signed-off-by: Matt Gilbride --- rust/bindings/bindings_helper.h | 17 +++ rust/helpers.c | 36 +++++ rust/kernel/lib.rs | 1 + rust/kernel/xarray.rs | 235 ++++++++++++++++++++++++++++++++ scripts/Makefile.build | 2 +- 5 files changed, 290 insertions(+), 1 deletion(-) create mode 100644 rust/kernel/xarray.rs diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h index 5db2940b2379a0..ae0c0f3c65fb41 100644 --- a/rust/bindings/bindings_helper.h +++ b/rust/bindings/bindings_helper.h @@ -12,8 +12,25 @@ #include #include #include +#include /* `bindgen` gets confused at certain things. */ const gfp_t BINDINGS_GFP_KERNEL = GFP_KERNEL; const gfp_t BINDINGS___GFP_ZERO = __GFP_ZERO; const size_t BINDINGS_ARCH_SLAB_MINALIGN = ARCH_SLAB_MINALIGN; + +const gfp_t BINDINGS_XA_FLAGS_LOCK_IRQ = XA_FLAGS_LOCK_IRQ; +const gfp_t BINDINGS_XA_FLAGS_LOCK_BH = XA_FLAGS_LOCK_BH; +const gfp_t BINDINGS_XA_FLAGS_TRACK_FREE = XA_FLAGS_TRACK_FREE; +const gfp_t BINDINGS_XA_FLAGS_ZERO_BUSY = XA_FLAGS_ZERO_BUSY; +const gfp_t BINDINGS_XA_FLAGS_ALLOC_WRAPPED = XA_FLAGS_ALLOC_WRAPPED; +const gfp_t BINDINGS_XA_FLAGS_ACCOUNT = XA_FLAGS_ACCOUNT; +const gfp_t BINDINGS_XA_FLAGS_ALLOC = XA_FLAGS_ALLOC; +const gfp_t BINDINGS_XA_FLAGS_ALLOC1 = XA_FLAGS_ALLOC1; + +const xa_mark_t BINDINGS_XA_MARK_0 = XA_MARK_0; +const xa_mark_t BINDINGS_XA_MARK_1 = XA_MARK_1; +const xa_mark_t BINDINGS_XA_MARK_2 = XA_MARK_2; +const xa_mark_t BINDINGS_XA_PRESENT = XA_PRESENT; +const xa_mark_t BINDINGS_XA_MARK_MAX = XA_MARK_MAX; +const xa_mark_t BINDINGS_XA_FREE_MARK = XA_FREE_MARK; diff --git a/rust/helpers.c b/rust/helpers.c index 64b118fae7ab10..49fa81f6ee62aa 100644 --- a/rust/helpers.c +++ b/rust/helpers.c @@ -29,6 +29,7 @@ #include #include #include +#include __noreturn void rust_helper_BUG(void) { @@ -142,6 +143,41 @@ struct kunit *rust_helper_kunit_get_current_test(void) return kunit_get_current_test(); } EXPORT_SYMBOL_GPL(rust_helper_kunit_get_current_test); +void rust_helper_xa_init_flags(struct xarray *xa, gfp_t flags) +{ + xa_init_flags(xa, flags); +} +EXPORT_SYMBOL_GPL(rust_helper_xa_init_flags); + +bool rust_helper_xa_empty(struct xarray *xa) +{ + return xa_empty(xa); +} +EXPORT_SYMBOL_GPL(rust_helper_xa_empty); + +int rust_helper_xa_alloc(struct xarray *xa, u32 *id, void *entry, struct xa_limit limit, gfp_t gfp) +{ + return xa_alloc(xa, id, entry, limit, gfp); +} +EXPORT_SYMBOL_GPL(rust_helper_xa_alloc); + +void rust_helper_xa_lock(struct xarray *xa) +{ + xa_lock(xa); +} +EXPORT_SYMBOL_GPL(rust_helper_xa_lock); + +void rust_helper_xa_unlock(struct xarray *xa) +{ + xa_unlock(xa); +} +EXPORT_SYMBOL_GPL(rust_helper_xa_unlock); + +int rust_helper_xa_err(void *entry) +{ + return xa_err(entry); +} +EXPORT_SYMBOL_GPL(rust_helper_xa_err); /* * `bindgen` binds the C `size_t` type the Rust `usize` type, so we can diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index e8811700239aaf..5127555ff5ec74 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -45,6 +45,7 @@ pub mod str; pub mod sync; pub mod task; pub mod types; +pub mod xarray; #[doc(hidden)] pub use bindings; diff --git a/rust/kernel/xarray.rs b/rust/kernel/xarray.rs new file mode 100644 index 00000000000000..83bc0f7a829b73 --- /dev/null +++ b/rust/kernel/xarray.rs @@ -0,0 +1,235 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! XArray abstraction. +//! +//! C header: [`include/linux/xarray.h`](../../include/linux/xarray.h) + +use crate::{ + bindings, + error::{Error, Result}, + types::{ForeignOwnable, Opaque, ScopeGuard}, +}; +use core::{ + marker::{PhantomData, PhantomPinned}, + pin::Pin, + ptr::NonNull, +}; + +/// Flags passed to `XArray::new` to configure the `XArray`. +type Flags = bindings::gfp_t; + +/// Flag values passed to `XArray::new` to configure the `XArray`. +pub mod flags { + /// Use IRQ-safe locking. + pub const LOCK_IRQ: super::Flags = bindings::BINDINGS_XA_FLAGS_LOCK_IRQ; + /// Use softirq-safe locking. + pub const LOCK_BH: super::Flags = bindings::BINDINGS_XA_FLAGS_LOCK_BH; + /// Track which entries are free (distinct from None). + pub const TRACK_FREE: super::Flags = bindings::BINDINGS_XA_FLAGS_TRACK_FREE; + /// Initialize array index 0 as busy. + pub const ZERO_BUSY: super::Flags = bindings::BINDINGS_XA_FLAGS_ZERO_BUSY; + /// Use GFP_ACCOUNT for internal memory allocations. + pub const ACCOUNT: super::Flags = bindings::BINDINGS_XA_FLAGS_ACCOUNT; + /// Create an allocating `XArray` starting at index 0. + pub const ALLOC: super::Flags = bindings::BINDINGS_XA_FLAGS_ALLOC; + /// Create an allocating `XArray` starting at index 1. + pub const ALLOC1: super::Flags = bindings::BINDINGS_XA_FLAGS_ALLOC1; +} + +/// Wrapper for a value owned by the `XArray` which holds the `XArray` lock until dropped. +pub struct ValueGuard<'a, T: ForeignOwnable>(NonNull, Pin<&'a XArray>); + +impl<'a, T: ForeignOwnable> ValueGuard<'a, T> { + /// Borrow the underlying value wrapped by the `ValueGuard`. + /// + /// Returns a `T::Borrowed` type for the owned `ForeignOwnable` type. + pub fn borrow(&self) -> T::Borrowed<'_> { + // SAFETY: The value is owned by the `XArray`, the lifetime it is borrowed for must not + // outlive the `XArray` itself, nor the `ValueGuard` that holds the lock ensuring the value + // remains in the `XArray`. + unsafe { T::borrow(self.0.as_ptr() as _) } + } +} + +impl<'a, T: ForeignOwnable> Drop for ValueGuard<'a, T> { + fn drop(&mut self) { + // SAFETY: The XArray we have a reference to owns the C xarray object. + unsafe { bindings::xa_unlock(self.1.xa.get()) }; + } +} + +/// An array which efficiently maps sparse integer indices to owned objects. +/// +/// This is similar to a `Vec>`, but more efficient when there are holes in the +/// index space, and can be efficiently grown. +/// +/// This structure is expected to often be used with an inner type that can be efficiently +/// cloned, such as an `Arc`. +/// +pub struct XArray { + xa: Opaque, + _p: PhantomData, + _q: PhantomPinned, +} + +impl XArray { + /// Creates a new `XArray` with the given flags. + pub fn new(flags: Flags) -> XArray { + let xa = Opaque::uninit(); + + // SAFETY: We have just created `xa`. This data structure does not require + // pinning. + unsafe { bindings::xa_init_flags(xa.get(), flags) }; + + // INVARIANT: Initialize the `XArray` with a valid `xa`. + XArray { + xa, + _p: PhantomData, + _q: PhantomPinned, + } + } + + /// Replaces an entry with a new value, returning the old value (if any). + pub fn replace(self: Pin<&Self>, index: usize, value: T) -> Result> { + let new = value.into_foreign(); + // SAFETY: `new` just came from into_foreign(), and we dismiss this guard if + // the xa_store operation succeeds and takes ownership of the pointer. + let guard = ScopeGuard::new(|| unsafe { + T::from_foreign(new); + }); + + // SAFETY: `self.xa` is always valid by the type invariant, and we are storing + // a `T::into_foreign()` result which upholds the later invariants. + let old = unsafe { + bindings::xa_store( + self.xa.get(), + index.try_into()?, + new as *mut _, + bindings::GFP_KERNEL, + ) + }; + + let ret = unsafe { bindings::xa_err(old) }; + if ret != 0 { + Err(Error::from_errno(ret)) + } else if old.is_null() { + guard.dismiss(); + Ok(None) + } else { + guard.dismiss(); + // SAFETY: The old value must have been stored by either this function or + // `alloc_limits_opt`, both of which ensure non-NULL entries are valid + // ForeignOwnable pointers. + Ok(Some(unsafe { T::from_foreign(old) })) + } + } + + /// Replaces an entry with a new value, dropping the old value (if any). + pub fn set(self: Pin<&Self>, index: usize, value: T) -> Result { + self.replace(index, value)?; + Ok(()) + } + + /// Looks up and returns a reference to an entry in the array, returning a `ValueGuard` if it + /// exists. + /// + /// This guard blocks all other actions on the `XArray`. Callers are expected to drop the + /// `ValueGuard` eagerly to avoid blocking other users, such as by taking a clone of the value. + pub fn get(self: Pin<&Self>, index: usize) -> Option> { + // SAFETY: `self.xa` is always valid by the type invariant. + unsafe { bindings::xa_lock(self.xa.get()) }; + + // SAFETY: `self.xa` is always valid by the type invariant. + let guard = ScopeGuard::new(|| unsafe { bindings::xa_unlock(self.xa.get()) }); + + // SAFETY: `self.xa` is always valid by the type invariant. + let p = unsafe { bindings::xa_load(self.xa.get(), index.try_into().ok()?) }; + + NonNull::new(p as *mut T).map(|p| { + guard.dismiss(); + ValueGuard(p, self) + }) + } + + /// Looks up and returns a reference to an entry in the array, returning `(index, ValueGuard)` + /// if it exists. If the index doesn't exist, returns the next larger index/value pair, + /// else `None`. + /// + /// This guard blocks all other actions on the `XArray`. Callers are expected to drop the + /// `ValueGuard` eagerly to avoid blocking other users, such as by taking a clone of the value. + pub fn find(self: Pin<&Self>, index: usize) -> Option<(usize, ValueGuard<'_, T>)> { + // SAFETY: `self.xa` is always valid by the type invariant. + unsafe { bindings::xa_lock(self.xa.get()) }; + + // SAFETY: `self.xa` is always valid by the type invariant. + let guard = ScopeGuard::new(|| unsafe { bindings::xa_unlock(self.xa.get()) }); + + let indexp = &mut index.try_into().ok()?; + // SAFETY: `self.xa` is always valud by the type invariant. + unsafe { + bindings::xa_find( + self.xa.get(), + indexp, + core::ffi::c_ulong::MAX, + bindings::BINDINGS_XA_PRESENT, + ) + }; + + let new_index = *indexp; + + // SAFETY: `self.xa` is always valid by the type invariant. + let p = unsafe { bindings::xa_load(self.xa.get(), new_index) }; + + let new_index: usize = new_index.try_into().ok()?; + + NonNull::new(p as *mut T).map(|p| { + guard.dismiss(); + (new_index, ValueGuard(p, self)) + }) + } + + /// Removes and returns an entry, returning it if it existed. + pub fn remove(self: Pin<&Self>, index: usize) -> Option { + let p = unsafe { bindings::xa_erase(self.xa.get(), index.try_into().ok()?) }; + if p.is_null() { + None + } else { + Some(unsafe { T::from_foreign(p) }) + } + } +} + +impl Drop for XArray { + fn drop(&mut self) { + // SAFETY: `self.xa` is valid by the type invariant, and as we have the only reference to + // the `XArray` we can safely iterate its contents and drop everything. + unsafe { + let mut index: core::ffi::c_ulong = 0; + let mut entry = bindings::xa_find( + self.xa.get(), + &mut index, + core::ffi::c_ulong::MAX, + bindings::BINDINGS_XA_PRESENT, + ); + while !entry.is_null() { + T::from_foreign(entry); + entry = bindings::xa_find_after( + self.xa.get(), + &mut index, + core::ffi::c_ulong::MAX, + bindings::BINDINGS_XA_PRESENT, + ); + } + + // Locked locks are not safe to drop. Normally we would want to try_lock()/unlock() here + // for safety or something similar, but in this case xa_destroy() is guaranteed to + // acquire the lock anyway. This will deadlock if a lock guard was improperly dropped, + // but that is not UB, so it's sufficient for soundness purposes. + bindings::xa_destroy(self.xa.get()); + } + } +} + +// SAFETY: XArray is thread-safe and all mutation operations are internally locked. +unsafe impl Send for XArray {} +unsafe impl Sync for XArray {} diff --git a/scripts/Makefile.build b/scripts/Makefile.build index 78175231c9699c..1404967e908e73 100644 --- a/scripts/Makefile.build +++ b/scripts/Makefile.build @@ -277,7 +277,7 @@ $(obj)/%.lst: $(src)/%.c FORCE # Compile Rust sources (.rs) # --------------------------------------------------------------------------- -rust_allowed_features := new_uninit +rust_allowed_features := allocator_api,new_uninit rust_common_cmd = \ RUST_MODFILE=$(modfile) $(RUSTC_OR_CLIPPY) $(rust_flags) \ From 91f414785c7e1aa0f9c940edb9d8b0282cc55de6 Mon Sep 17 00:00:00 2001 From: Matt Gilbride Date: Tue, 27 Jun 2023 18:24:45 +0000 Subject: [PATCH 30/37] rust: xarray: add rudimentary doctest Add simple example of xarray usage with `get`, `set`, `replace`, `remove`, and `find` operations. Signed-off-by: Matt Gilbride --- rust/kernel/xarray.rs | 43 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/rust/kernel/xarray.rs b/rust/kernel/xarray.rs index 83bc0f7a829b73..b869df58948abc 100644 --- a/rust/kernel/xarray.rs +++ b/rust/kernel/xarray.rs @@ -66,6 +66,49 @@ impl<'a, T: ForeignOwnable> Drop for ValueGuard<'a, T> { /// This structure is expected to often be used with an inner type that can be efficiently /// cloned, such as an `Arc`. /// +/// # Examples +/// +/// In this example, we create a new `XArray` and demonstrate +/// rudimentary read/write operations. +/// +/// ``` +/// use kernel::xarray::{XArray, flags::LOCK_IRQ}; +/// +/// let xarr: Box>> = Box::try_new(XArray::new(LOCK_IRQ))?; +/// let xarr: Pin>>> = Box::into_pin(xarr); +/// let xarr: Pin<&XArray>> = xarr.as_ref(); +/// +/// assert!(xarr.get(0).is_none()); +/// +/// xarr.set(0, Box::try_new(0)?); +/// assert_eq!(xarr.get(0).unwrap().borrow(), &0); +/// +/// // `replace` is like `set`, but returns the old value. +/// let old = xarr.replace(0, Box::try_new(1)?)?.unwrap(); +/// assert_eq!(old.as_ref(), &0); +/// assert_eq!(xarr.get(0).unwrap().borrow(), &1); +/// +/// // `replace` returns `None` if there was no previous value. +/// assert!(xarr.replace(1, Box::try_new(1)?)?.is_none()); +/// assert_eq!(xarr.get(1).unwrap().borrow(), &1); +/// +/// // Similarly, `remove` returns the old value, or `None` if it didn't exist. +/// assert_eq!(xarr.remove(0).unwrap().as_ref(), &1); +/// assert!(xarr.get(0).is_none()); +/// assert!(xarr.remove(0).is_none()); +/// +/// // `find` returns the index/value pair matching the index, the next larger +/// // index/value pair if it doesn't exist, or `None` of no larger index exists. +/// let (found_index, found_value) = xarr.find(1).unwrap(); +/// assert_eq!(found_index, 1); +/// assert_eq!(found_value.borrow(), &1); +/// let (found_index, found_value) = xarr.find(0).unwrap(); +/// assert_eq!(found_index, 1); +/// assert_eq!(found_value.borrow(), &1); +/// assert!(xarr.find(2).is_none()); +/// +/// # Ok::<(), Error>(()) +/// ``` pub struct XArray { xa: Opaque, _p: PhantomData, From 6c19acb7dc3b13ea7c38b6606e1530cb0ba3fad8 Mon Sep 17 00:00:00 2001 From: Matt Gilbride Date: Tue, 27 Jun 2023 18:40:31 +0000 Subject: [PATCH 31/37] rust: xarray: add `get_mutable` and `find_mut` Add equivalents to `get` and `find` for mutable access to the values stored inside an `XArray`. `get_mutable` is named as such to avoid a name collission with `Pin::get_mut`. Signed-off-by: Matt Gilbride --- rust/kernel/xarray.rs | 132 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 132 insertions(+) diff --git a/rust/kernel/xarray.rs b/rust/kernel/xarray.rs index b869df58948abc..6c4cc7b06b0613 100644 --- a/rust/kernel/xarray.rs +++ b/rust/kernel/xarray.rs @@ -58,6 +58,28 @@ impl<'a, T: ForeignOwnable> Drop for ValueGuard<'a, T> { } } +/// Wrapper for a *mutable* value owned by the `XArray` which holds the `XArray` lock until dropped. +pub struct ValueGuardMut<'a, T: ForeignOwnable>(NonNull, Pin<&'a mut XArray>); + +impl<'a, T: ForeignOwnable> ValueGuardMut<'a, T> { + /// Borrow the underlying value wrapped by the `ValueGuard`. + /// + /// Returns a `T::Borrowed` type for the owned `ForeignOwnable` type. + pub fn borrow_mut(&mut self) -> ScopeGuard { + // SAFETY: The value is owned by the `XArray`, the lifetime it is borrowed for must not + // outlive the `XArray` itself, nor the `ValueGuard` that holds the lock ensuring the value + // remains in the `XArray`. + unsafe { T::borrow_mut(self.0.as_ptr() as _) } + } +} + +impl<'a, T: ForeignOwnable> Drop for ValueGuardMut<'a, T> { + fn drop(&mut self) { + // SAFETY: The XArray we have a reference to owns the C xarray object. + unsafe { bindings::xa_unlock(self.1.xa.get()) }; + } +} + /// An array which efficiently maps sparse integer indices to owned objects. /// /// This is similar to a `Vec>`, but more efficient when there are holes in the @@ -109,6 +131,52 @@ impl<'a, T: ForeignOwnable> Drop for ValueGuard<'a, T> { /// /// # Ok::<(), Error>(()) /// ``` +/// +/// In this example, we create a new `XArray` and demonstrate +/// how to mutably access values. +/// +/// ``` +/// use kernel::xarray::{XArray, flags::LOCK_IRQ}; +/// +/// let xarr: Box>> = Box::try_new(XArray::new(LOCK_IRQ))?; +/// let mut xarr: Pin>>> = Box::into_pin(xarr); +/// +/// // Create scopes so that references can be dropped +/// // in order to take a new, mutable/immutable ones afterwards. +/// { +/// let xarr = xarr.as_ref(); +/// assert!(xarr.get(1).is_none()); +/// xarr.set(1, Box::try_new(1)?); +/// assert_eq!(xarr.get(1).unwrap().borrow(), &1); +/// } +/// +/// { +/// let xarr = xarr.as_mut(); +/// let mut value = xarr.get_mutable(1).unwrap().borrow_mut(); +/// *(value.as_mut()) = 2; +/// } +/// +/// { +/// let xarr = xarr.as_ref(); +/// assert_eq!(xarr.get(1).unwrap().borrow(), &2); +/// } +/// +/// // `find_mut` can equivalently be used for mutation. +/// { +/// let xarr = xarr.as_mut(); +/// let (index, mut value_guard) = xarr.find_mut(0).unwrap(); +/// assert_eq!(index, 1); +/// *(value_guard.borrow_mut().as_mut()) = 3; +/// } +/// +/// { +/// let xarr = xarr.as_ref(); +/// assert_eq!(xarr.get(1).unwrap().borrow(), &3); +/// } +/// +/// # Ok::<(), Error>(()) +/// ``` +/// pub struct XArray { xa: Opaque, _p: PhantomData, @@ -194,6 +262,30 @@ impl XArray { }) } + /// Looks up and returns a *mutable* reference to an entry in the array, returning a `ValueGuardMut` if it + /// exists. + /// + /// This guard blocks all other actions on the `XArray`. Callers are expected to drop the + /// `ValueGuardMut` eagerly to avoid blocking other users, such as by taking a clone of the value. + pub fn get_mutable(self: Pin<&mut Self>, index: usize) -> Option> { + // SAFETY: `self.xa` is always valid by the type invariant. + unsafe { bindings::xa_lock(self.xa.get()) }; + + // SAFETY: `self.xa` is always valid by the type invariant. + let guard = ScopeGuard::new(|| unsafe { bindings::xa_unlock(self.xa.get()) }); + + // SAFETY: `self.xa` is always valid by the type invariant. + let p = unsafe { bindings::xa_load(self.xa.get(), index.try_into().ok()?) }; + + match NonNull::new(p as *mut T) { + Some(p) => { + guard.dismiss(); + Some(ValueGuardMut(p, self)) + } + None => None, + } + } + /// Looks up and returns a reference to an entry in the array, returning `(index, ValueGuard)` /// if it exists. If the index doesn't exist, returns the next larger index/value pair, /// else `None`. @@ -231,6 +323,46 @@ impl XArray { }) } + /// Looks up and returns a *mutable* reference to an entry in the array, returning `(index, ValueGuardMut)` + /// if it exists. If the index doesn't exist, returns the next larger index/value pair, + /// else `None`. + /// + /// This guard blocks all other actions on the `XArray`. Callers are expected to drop the + /// `ValueGuardMut` eagerly to avoid blocking other users, such as by taking a clone of the value. + pub fn find_mut(self: Pin<&mut Self>, index: usize) -> Option<(usize, ValueGuardMut<'_, T>)> { + // SAFETY: `self.xa` is always valid by the type invariant. + unsafe { bindings::xa_lock(self.xa.get()) }; + + // SAFETY: `self.xa` is always valid by the type invariant. + let guard = ScopeGuard::new(|| unsafe { bindings::xa_unlock(self.xa.get()) }); + + let indexp = &mut index.try_into().ok()?; + // SAFETY: `self.xa` is always valud by the type invariant. + unsafe { + bindings::xa_find( + self.xa.get(), + indexp, + core::ffi::c_ulong::MAX, + bindings::BINDINGS_XA_PRESENT, + ) + }; + + let new_index = *indexp; + + // SAFETY: `self.xa` is always valid by the type invariant. + let p = unsafe { bindings::xa_load(self.xa.get(), new_index) }; + + let new_index: usize = new_index.try_into().ok()?; + + match NonNull::new(p as *mut T) { + Some(p) => { + guard.dismiss(); + Some((new_index, ValueGuardMut(p, self))) + } + None => None, + } + } + /// Removes and returns an entry, returning it if it existed. pub fn remove(self: Pin<&Self>, index: usize) -> Option { let p = unsafe { bindings::xa_erase(self.xa.get(), index.try_into().ok()?) }; From 3016eacc3a77f927010396c0aad96980aaefa05a Mon Sep 17 00:00:00 2001 From: Asahi Lina Date: Tue, 27 Jun 2023 19:57:30 +0000 Subject: [PATCH 32/37] rust: xarray: add reservation mechanism Introduces a reservation mechanism, which can be used by alloc-capable `XArrays` to reserve a free slot without immediately filling it, and then do so at a later time. If the reservation is dropped without being filled, the slot is freed again for other users, which eliminates the need for explicit cleanup code. Signed-off-by: Matt Gilbride --- rust/kernel/xarray.rs | 105 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 105 insertions(+) diff --git a/rust/kernel/xarray.rs b/rust/kernel/xarray.rs index 6c4cc7b06b0613..2e29070427bd97 100644 --- a/rust/kernel/xarray.rs +++ b/rust/kernel/xarray.rs @@ -80,6 +80,41 @@ impl<'a, T: ForeignOwnable> Drop for ValueGuardMut<'a, T> { } } +/// Represents a reserved slot in an `XArray`, which does not yet have a value but has an assigned +/// index and may not be allocated by any other user. If the Reservation is dropped without +/// being filled, the entry is marked as available again. +/// +/// Users must ensure that reserved slots are not filled by other mechanisms, or otherwise their +/// contents may be dropped and replaced (which will print a warning). +pub struct Reservation<'a, T: ForeignOwnable>(Pin<&'a XArray>, usize, PhantomData); + +impl<'a, T: ForeignOwnable> Reservation<'a, T> { + /// Stores a value into the reserved slot. + pub fn store(self, value: T) -> Result { + if self.0.replace(self.1, value)?.is_some() { + crate::pr_err!("XArray: Reservation stored but the entry already had data!\n"); + // Consider it a success anyway, not much we can do + } + let index = self.1; + // The reservation is now fulfilled, so do not run our destructor. + core::mem::forget(self); + Ok(index) + } + + /// Returns the index of this reservation. + pub fn index(&self) -> usize { + self.1 + } +} + +impl<'a, T: ForeignOwnable> Drop for Reservation<'a, T> { + fn drop(&mut self) { + if self.0.remove(self.1).is_some() { + crate::pr_err!("XArray: Reservation dropped but the entry was not empty!\n"); + } + } +} + /// An array which efficiently maps sparse integer indices to owned objects. /// /// This is similar to a `Vec>`, but more efficient when there are holes in the @@ -372,6 +407,76 @@ impl XArray { Some(unsafe { T::from_foreign(p) }) } } + + /// Allocates a new index in the array, optionally storing a new value into it, with + /// configurable bounds for the index range to allocate from. + /// + /// If `value` is `None`, then the index is reserved from further allocation but remains + /// free for storing a value into it. + fn alloc_limits_opt(self: Pin<&Self>, value: Option, min: u32, max: u32) -> Result { + let new = value.map_or(core::ptr::null(), |a| a.into_foreign()); + let mut id: u32 = 0; + + // SAFETY: `self.xa` is always valid by the type invariant. If this succeeds, it + // takes ownership of the passed `T` (if any). If it fails, we must drop the + // `T` again. + let ret = unsafe { + bindings::xa_alloc( + self.xa.get(), + &mut id, + new as *mut _, + bindings::xa_limit { min, max }, + bindings::GFP_KERNEL, + ) + }; + + if ret < 0 { + // Make sure to drop the value we failed to store + if !new.is_null() { + // SAFETY: If `new` is not NULL, it came from the `ForeignOwnable` we got + // from the caller. + unsafe { T::from_foreign(new) }; + } + Err(Error::from_errno(ret)) + } else { + Ok(id as usize) + } + } + + /// Allocates a new index in the array, storing a new value into it, with configurable + /// bounds for the index range to allocate from. + pub fn alloc_limits(self: Pin<&Self>, value: T, min: u32, max: u32) -> Result { + self.alloc_limits_opt(Some(value), min, max) + } + + /// Allocates a new index in the array, storing a new value into it. + pub fn alloc(self: Pin<&Self>, value: T) -> Result { + self.alloc_limits(value, 0, u32::MAX) + } + + /// Reserves a new index in the array within configurable bounds for the index. + /// + /// Returns a `Reservation` object, which can then be used to store a value at this index or + /// otherwise free it for reuse. + pub fn reserve_limits(self: Pin<&Self>, min: u32, max: u32) -> Result> { + Ok(Reservation( + self, + self.alloc_limits_opt(None, min, max)?, + PhantomData, + )) + } + + /// Reserves a new index in the array. + /// + /// Returns a `Reservation` object, which can then be used to store a value at this index or + /// otherwise free it for reuse. + pub fn reserve(self: Pin<&Self>) -> Result> { + Ok(Reservation( + self, + self.alloc_limits_opt(None, 0, u32::MAX)?, + PhantomData, + )) + } } impl Drop for XArray { From 90ddba239bf3b8ba333b2708ddceb03cf2a87edc Mon Sep 17 00:00:00 2001 From: Matt Gilbride Date: Sun, 2 Jul 2023 20:03:49 +0000 Subject: [PATCH 33/37] Why does this cuase NULL pointer dereference? --- rust/kernel/xarray.rs | 154 +++++++++++++++--------------------------- 1 file changed, 54 insertions(+), 100 deletions(-) diff --git a/rust/kernel/xarray.rs b/rust/kernel/xarray.rs index 2e29070427bd97..e22cabc3ceb34c 100644 --- a/rust/kernel/xarray.rs +++ b/rust/kernel/xarray.rs @@ -129,85 +129,24 @@ impl<'a, T: ForeignOwnable> Drop for Reservation<'a, T> { /// rudimentary read/write operations. /// /// ``` -/// use kernel::xarray::{XArray, flags::LOCK_IRQ}; +/// use core::{ +/// borrow::Borrow, +/// ops::Deref +/// }; +/// use kernel::{ +/// sync::Arc, +/// xarray::{XArray, flags::LOCK_IRQ} +/// }; /// -/// let xarr: Box>> = Box::try_new(XArray::new(LOCK_IRQ))?; -/// let xarr: Pin>>> = Box::into_pin(xarr); -/// let xarr: Pin<&XArray>> = xarr.as_ref(); +/// let xarr: Box>> = Box::try_new(XArray::new(LOCK_IRQ))?; +/// let xarr: Pin>>> = Box::into_pin(xarr); +/// let xarr: Pin<&XArray>> = xarr.as_ref(); /// /// assert!(xarr.get(0).is_none()); /// -/// xarr.set(0, Box::try_new(0)?); -/// assert_eq!(xarr.get(0).unwrap().borrow(), &0); -/// -/// // `replace` is like `set`, but returns the old value. -/// let old = xarr.replace(0, Box::try_new(1)?)?.unwrap(); -/// assert_eq!(old.as_ref(), &0); -/// assert_eq!(xarr.get(0).unwrap().borrow(), &1); -/// -/// // `replace` returns `None` if there was no previous value. -/// assert!(xarr.replace(1, Box::try_new(1)?)?.is_none()); -/// assert_eq!(xarr.get(1).unwrap().borrow(), &1); -/// -/// // Similarly, `remove` returns the old value, or `None` if it didn't exist. -/// assert_eq!(xarr.remove(0).unwrap().as_ref(), &1); -/// assert!(xarr.get(0).is_none()); -/// assert!(xarr.remove(0).is_none()); -/// -/// // `find` returns the index/value pair matching the index, the next larger -/// // index/value pair if it doesn't exist, or `None` of no larger index exists. -/// let (found_index, found_value) = xarr.find(1).unwrap(); -/// assert_eq!(found_index, 1); -/// assert_eq!(found_value.borrow(), &1); -/// let (found_index, found_value) = xarr.find(0).unwrap(); -/// assert_eq!(found_index, 1); -/// assert_eq!(found_value.borrow(), &1); -/// assert!(xarr.find(2).is_none()); -/// -/// # Ok::<(), Error>(()) -/// ``` -/// -/// In this example, we create a new `XArray` and demonstrate -/// how to mutably access values. -/// -/// ``` -/// use kernel::xarray::{XArray, flags::LOCK_IRQ}; -/// -/// let xarr: Box>> = Box::try_new(XArray::new(LOCK_IRQ))?; -/// let mut xarr: Pin>>> = Box::into_pin(xarr); -/// -/// // Create scopes so that references can be dropped -/// // in order to take a new, mutable/immutable ones afterwards. -/// { -/// let xarr = xarr.as_ref(); -/// assert!(xarr.get(1).is_none()); -/// xarr.set(1, Box::try_new(1)?); -/// assert_eq!(xarr.get(1).unwrap().borrow(), &1); -/// } -/// -/// { -/// let xarr = xarr.as_mut(); -/// let mut value = xarr.get_mutable(1).unwrap().borrow_mut(); -/// *(value.as_mut()) = 2; -/// } -/// -/// { -/// let xarr = xarr.as_ref(); -/// assert_eq!(xarr.get(1).unwrap().borrow(), &2); -/// } -/// -/// // `find_mut` can equivalently be used for mutation. -/// { -/// let xarr = xarr.as_mut(); -/// let (index, mut value_guard) = xarr.find_mut(0).unwrap(); -/// assert_eq!(index, 1); -/// *(value_guard.borrow_mut().as_mut()) = 3; -/// } -/// -/// { -/// let xarr = xarr.as_ref(); -/// assert_eq!(xarr.get(1).unwrap().borrow(), &3); -/// } +/// xarr.set(0, Arc::try_new(0)?); +/// let wut_null_pointer_deref = xarr.get(0).unwrap(); +/// assert_eq!(wut_null_pointer_deref.as_ref(), &0); /// /// # Ok::<(), Error>(()) /// ``` @@ -276,49 +215,64 @@ impl XArray { Ok(()) } - /// Looks up and returns a reference to an entry in the array, returning a `ValueGuard` if it - /// exists. + /// Looks up a reference to an entry in the array, cloning it + /// and returning the cloned value to the user. /// - /// This guard blocks all other actions on the `XArray`. Callers are expected to drop the - /// `ValueGuard` eagerly to avoid blocking other users, such as by taking a clone of the value. - pub fn get(self: Pin<&Self>, index: usize) -> Option> { + pub fn get(self: Pin<&Self>, index: u64) -> Option + where + T: Clone, + { // SAFETY: `self.xa` is always valid by the type invariant. unsafe { bindings::xa_lock(self.xa.get()) }; // SAFETY: `self.xa` is always valid by the type invariant. - let guard = ScopeGuard::new(|| unsafe { bindings::xa_unlock(self.xa.get()) }); + let p = unsafe { bindings::xa_load(self.xa.get(), index) } as *const T; + + let t = NonNull::new(p as *mut T).map(|p| unsafe { p.as_ref() }).cloned(); // SAFETY: `self.xa` is always valid by the type invariant. - let p = unsafe { bindings::xa_load(self.xa.get(), index.try_into().ok()?) }; + unsafe { bindings::xa_unlock(self.xa.get()) }; - NonNull::new(p as *mut T).map(|p| { - guard.dismiss(); - ValueGuard(p, self) - }) + t } - /// Looks up and returns a *mutable* reference to an entry in the array, returning a `ValueGuardMut` if it - /// exists. + /// Looks up and a reference to an entry in the array, calling the user + /// provided function on the resulting `Option<&T>` to return a value + /// computed from the reference. Use this function if you need shared + /// access to a `&T` that is not `Clone`. /// - /// This guard blocks all other actions on the `XArray`. Callers are expected to drop the - /// `ValueGuardMut` eagerly to avoid blocking other users, such as by taking a clone of the value. - pub fn get_mutable(self: Pin<&mut Self>, index: usize) -> Option> { + pub fn get_shared(self: Pin<&mut Self>, index: u64, f: F) -> R + where + F: FnOnce(Option<&T>) -> R, + { // SAFETY: `self.xa` is always valid by the type invariant. unsafe { bindings::xa_lock(self.xa.get()) }; // SAFETY: `self.xa` is always valid by the type invariant. - let guard = ScopeGuard::new(|| unsafe { bindings::xa_unlock(self.xa.get()) }); + let p = unsafe { bindings::xa_load(self.xa.get(), index) }; + let t: Option<&T> = unsafe { NonNull::new(p as *mut T).map(|p| p.as_ref()) }; + let r = f(t); // SAFETY: `self.xa` is always valid by the type invariant. - let p = unsafe { bindings::xa_load(self.xa.get(), index.try_into().ok()?) }; + unsafe { bindings::xa_unlock(self.xa.get()) }; - match NonNull::new(p as *mut T) { - Some(p) => { - guard.dismiss(); - Some(ValueGuardMut(p, self)) - } - None => None, - } + r + } + + /// Looks up and returns a *mutable* reference to an entry in the array. + pub fn get_mutable(self: Pin<&mut Self>, index: u64) -> Option<&mut T> { + // SAFETY: `self.xa` is always valid by the type invariant. + unsafe { bindings::xa_lock(self.xa.get()) }; + + // SAFETY: `self.xa` is always valid by the type invariant. + let p = unsafe { bindings::xa_load(self.xa.get(), index) }; + + let t = NonNull::new(p as *mut T).map(|mut p| unsafe { p.as_mut() }); + + // SAFETY: `self.xa` is always valid by the type invariant. + unsafe { bindings::xa_unlock(self.xa.get()) }; + + t } /// Looks up and returns a reference to an entry in the array, returning `(index, ValueGuard)` From 702746d9bc6c7b981e45c9ffbed146d601d31af7 Mon Sep 17 00:00:00 2001 From: Matt Gilbride Date: Tue, 4 Jul 2023 19:50:05 +0000 Subject: [PATCH 34/37] Just me thrashing about - figuring out NPE --- rust/kernel/xarray.rs | 301 +++++++++--------------------------------- 1 file changed, 61 insertions(+), 240 deletions(-) diff --git a/rust/kernel/xarray.rs b/rust/kernel/xarray.rs index e22cabc3ceb34c..e552641ab19daa 100644 --- a/rust/kernel/xarray.rs +++ b/rust/kernel/xarray.rs @@ -36,85 +36,6 @@ pub mod flags { pub const ALLOC1: super::Flags = bindings::BINDINGS_XA_FLAGS_ALLOC1; } -/// Wrapper for a value owned by the `XArray` which holds the `XArray` lock until dropped. -pub struct ValueGuard<'a, T: ForeignOwnable>(NonNull, Pin<&'a XArray>); - -impl<'a, T: ForeignOwnable> ValueGuard<'a, T> { - /// Borrow the underlying value wrapped by the `ValueGuard`. - /// - /// Returns a `T::Borrowed` type for the owned `ForeignOwnable` type. - pub fn borrow(&self) -> T::Borrowed<'_> { - // SAFETY: The value is owned by the `XArray`, the lifetime it is borrowed for must not - // outlive the `XArray` itself, nor the `ValueGuard` that holds the lock ensuring the value - // remains in the `XArray`. - unsafe { T::borrow(self.0.as_ptr() as _) } - } -} - -impl<'a, T: ForeignOwnable> Drop for ValueGuard<'a, T> { - fn drop(&mut self) { - // SAFETY: The XArray we have a reference to owns the C xarray object. - unsafe { bindings::xa_unlock(self.1.xa.get()) }; - } -} - -/// Wrapper for a *mutable* value owned by the `XArray` which holds the `XArray` lock until dropped. -pub struct ValueGuardMut<'a, T: ForeignOwnable>(NonNull, Pin<&'a mut XArray>); - -impl<'a, T: ForeignOwnable> ValueGuardMut<'a, T> { - /// Borrow the underlying value wrapped by the `ValueGuard`. - /// - /// Returns a `T::Borrowed` type for the owned `ForeignOwnable` type. - pub fn borrow_mut(&mut self) -> ScopeGuard { - // SAFETY: The value is owned by the `XArray`, the lifetime it is borrowed for must not - // outlive the `XArray` itself, nor the `ValueGuard` that holds the lock ensuring the value - // remains in the `XArray`. - unsafe { T::borrow_mut(self.0.as_ptr() as _) } - } -} - -impl<'a, T: ForeignOwnable> Drop for ValueGuardMut<'a, T> { - fn drop(&mut self) { - // SAFETY: The XArray we have a reference to owns the C xarray object. - unsafe { bindings::xa_unlock(self.1.xa.get()) }; - } -} - -/// Represents a reserved slot in an `XArray`, which does not yet have a value but has an assigned -/// index and may not be allocated by any other user. If the Reservation is dropped without -/// being filled, the entry is marked as available again. -/// -/// Users must ensure that reserved slots are not filled by other mechanisms, or otherwise their -/// contents may be dropped and replaced (which will print a warning). -pub struct Reservation<'a, T: ForeignOwnable>(Pin<&'a XArray>, usize, PhantomData); - -impl<'a, T: ForeignOwnable> Reservation<'a, T> { - /// Stores a value into the reserved slot. - pub fn store(self, value: T) -> Result { - if self.0.replace(self.1, value)?.is_some() { - crate::pr_err!("XArray: Reservation stored but the entry already had data!\n"); - // Consider it a success anyway, not much we can do - } - let index = self.1; - // The reservation is now fulfilled, so do not run our destructor. - core::mem::forget(self); - Ok(index) - } - - /// Returns the index of this reservation. - pub fn index(&self) -> usize { - self.1 - } -} - -impl<'a, T: ForeignOwnable> Drop for Reservation<'a, T> { - fn drop(&mut self) { - if self.0.remove(self.1).is_some() { - crate::pr_err!("XArray: Reservation dropped but the entry was not empty!\n"); - } - } -} - /// An array which efficiently maps sparse integer indices to owned objects. /// /// This is similar to a `Vec>`, but more efficient when there are holes in the @@ -129,10 +50,6 @@ impl<'a, T: ForeignOwnable> Drop for Reservation<'a, T> { /// rudimentary read/write operations. /// /// ``` -/// use core::{ -/// borrow::Borrow, -/// ops::Deref -/// }; /// use kernel::{ /// sync::Arc, /// xarray::{XArray, flags::LOCK_IRQ} @@ -142,11 +59,62 @@ impl<'a, T: ForeignOwnable> Drop for Reservation<'a, T> { /// let xarr: Pin>>> = Box::into_pin(xarr); /// let xarr: Pin<&XArray>> = xarr.as_ref(); /// +/// // `get` and `set` are used to read/write values. /// assert!(xarr.get(0).is_none()); -/// /// xarr.set(0, Arc::try_new(0)?); -/// let wut_null_pointer_deref = xarr.get(0).unwrap(); -/// assert_eq!(wut_null_pointer_deref.as_ref(), &0); +/// assert_eq!(*xarr.get(0).unwrap(), 0); +/// +/// // `replace` is like `set`, but returns the old value. +/// let old = xarr.replace(0, Arc::try_new(1)?)?.unwrap(); +/// assert_eq!(*old, 0); +/// assert_eq!(*xarr.get(0).unwrap(), 1); +/// +/// // `replace` returns `None` if there was no previous value. +/// assert!(xarr.replace(1, Arc::try_new(1)?)?.is_none()); +/// assert_eq!(*xarr.get(1).unwrap(), 1); +/// +/// // Similarly, `remove` returns the old value, or `None` if it didn't exist. +/// assert_eq!(*xarr.remove(0).unwrap(), 1); +/// assert!(xarr.get(0).is_none()); +/// assert!(xarr.remove(0).is_none()); +/// +/// # Ok::<(), Error>(()) +/// ``` +/// +/// In this example, we create a new `XArray` and demonstrate +/// reading values that are not `Clone`, as well as mutating values. +/// +/// ``` +/// use core::{ +/// borrow::Borrow, +/// ops::{ Deref, DerefMut } +/// }; +/// use kernel::xarray::{XArray, flags::LOCK_IRQ}; +/// +/// let xarr: Box>> = Box::try_new(XArray::new(LOCK_IRQ))?; +/// let mut xarr: Pin>>> = Box::into_pin(xarr); +/// +/// // Create scopes so that references can be dropped +/// // in order to take a new, mutable/immutable ones afterwards. +/// { +/// let xarr: Pin<&XArray>> = xarr.as_ref(); +/// // If the type is not `Clone`, use `get_shared` to access a shared reference +/// // from a closure. +/// assert!(xarr.get_shared(0, |x| x.is_none())); +/// xarr.set(0, Box::try_new(0)?); +/// xarr.get_shared(0, |x| assert_eq!(*x.unwrap(), 0)); +/// } +/// { +/// let mut xarr = xarr.as_mut(); +/// let mut value = xarr.get_mutable(0).unwrap(); +/// let mut value = value.deref_mut().as_mut(); +/// assert_eq!(value, &mut 0); +/// *value = 1; +/// } +/// { +/// let xarr = xarr.as_ref(); +/// xarr.get_shared(0, |x| assert_eq!(*x.unwrap(), 1)); +/// } /// /// # Ok::<(), Error>(()) /// ``` @@ -218,7 +186,7 @@ impl XArray { /// Looks up a reference to an entry in the array, cloning it /// and returning the cloned value to the user. /// - pub fn get(self: Pin<&Self>, index: u64) -> Option + pub fn get(self: Pin<&Self>, index: u64) -> Option> where T: Clone, { @@ -228,7 +196,7 @@ impl XArray { // SAFETY: `self.xa` is always valid by the type invariant. let p = unsafe { bindings::xa_load(self.xa.get(), index) } as *const T; - let t = NonNull::new(p as *mut T).map(|p| unsafe { p.as_ref() }).cloned(); + let t = NonNull::new(p as *mut T).map(|p| unsafe { T::borrow(p.clone().as_ptr() as _) }); // SAFETY: `self.xa` is always valid by the type invariant. unsafe { bindings::xa_unlock(self.xa.get()) }; @@ -241,16 +209,16 @@ impl XArray { /// computed from the reference. Use this function if you need shared /// access to a `&T` that is not `Clone`. /// - pub fn get_shared(self: Pin<&mut Self>, index: u64, f: F) -> R + pub fn get_shared(self: Pin<&Self>, index: u64, f: F) -> R where - F: FnOnce(Option<&T>) -> R, + F: FnOnce(Option>) -> R, { // SAFETY: `self.xa` is always valid by the type invariant. unsafe { bindings::xa_lock(self.xa.get()) }; // SAFETY: `self.xa` is always valid by the type invariant. let p = unsafe { bindings::xa_load(self.xa.get(), index) }; - let t: Option<&T> = unsafe { NonNull::new(p as *mut T).map(|p| p.as_ref()) }; + let t = NonNull::new(p as *mut T).map(|p| unsafe { T::borrow(p.as_ptr() as _) }); let r = f(t); // SAFETY: `self.xa` is always valid by the type invariant. @@ -260,14 +228,14 @@ impl XArray { } /// Looks up and returns a *mutable* reference to an entry in the array. - pub fn get_mutable(self: Pin<&mut Self>, index: u64) -> Option<&mut T> { + pub fn get_mutable(self: Pin<&mut Self>, index: u64) -> Option> { // SAFETY: `self.xa` is always valid by the type invariant. unsafe { bindings::xa_lock(self.xa.get()) }; // SAFETY: `self.xa` is always valid by the type invariant. let p = unsafe { bindings::xa_load(self.xa.get(), index) }; - let t = NonNull::new(p as *mut T).map(|mut p| unsafe { p.as_mut() }); + let t = NonNull::new(p as *mut T).map(|p| unsafe { T::borrow_mut(p.as_ptr() as _) }); // SAFETY: `self.xa` is always valid by the type invariant. unsafe { bindings::xa_unlock(self.xa.get()) }; @@ -275,83 +243,6 @@ impl XArray { t } - /// Looks up and returns a reference to an entry in the array, returning `(index, ValueGuard)` - /// if it exists. If the index doesn't exist, returns the next larger index/value pair, - /// else `None`. - /// - /// This guard blocks all other actions on the `XArray`. Callers are expected to drop the - /// `ValueGuard` eagerly to avoid blocking other users, such as by taking a clone of the value. - pub fn find(self: Pin<&Self>, index: usize) -> Option<(usize, ValueGuard<'_, T>)> { - // SAFETY: `self.xa` is always valid by the type invariant. - unsafe { bindings::xa_lock(self.xa.get()) }; - - // SAFETY: `self.xa` is always valid by the type invariant. - let guard = ScopeGuard::new(|| unsafe { bindings::xa_unlock(self.xa.get()) }); - - let indexp = &mut index.try_into().ok()?; - // SAFETY: `self.xa` is always valud by the type invariant. - unsafe { - bindings::xa_find( - self.xa.get(), - indexp, - core::ffi::c_ulong::MAX, - bindings::BINDINGS_XA_PRESENT, - ) - }; - - let new_index = *indexp; - - // SAFETY: `self.xa` is always valid by the type invariant. - let p = unsafe { bindings::xa_load(self.xa.get(), new_index) }; - - let new_index: usize = new_index.try_into().ok()?; - - NonNull::new(p as *mut T).map(|p| { - guard.dismiss(); - (new_index, ValueGuard(p, self)) - }) - } - - /// Looks up and returns a *mutable* reference to an entry in the array, returning `(index, ValueGuardMut)` - /// if it exists. If the index doesn't exist, returns the next larger index/value pair, - /// else `None`. - /// - /// This guard blocks all other actions on the `XArray`. Callers are expected to drop the - /// `ValueGuardMut` eagerly to avoid blocking other users, such as by taking a clone of the value. - pub fn find_mut(self: Pin<&mut Self>, index: usize) -> Option<(usize, ValueGuardMut<'_, T>)> { - // SAFETY: `self.xa` is always valid by the type invariant. - unsafe { bindings::xa_lock(self.xa.get()) }; - - // SAFETY: `self.xa` is always valid by the type invariant. - let guard = ScopeGuard::new(|| unsafe { bindings::xa_unlock(self.xa.get()) }); - - let indexp = &mut index.try_into().ok()?; - // SAFETY: `self.xa` is always valud by the type invariant. - unsafe { - bindings::xa_find( - self.xa.get(), - indexp, - core::ffi::c_ulong::MAX, - bindings::BINDINGS_XA_PRESENT, - ) - }; - - let new_index = *indexp; - - // SAFETY: `self.xa` is always valid by the type invariant. - let p = unsafe { bindings::xa_load(self.xa.get(), new_index) }; - - let new_index: usize = new_index.try_into().ok()?; - - match NonNull::new(p as *mut T) { - Some(p) => { - guard.dismiss(); - Some((new_index, ValueGuardMut(p, self))) - } - None => None, - } - } - /// Removes and returns an entry, returning it if it existed. pub fn remove(self: Pin<&Self>, index: usize) -> Option { let p = unsafe { bindings::xa_erase(self.xa.get(), index.try_into().ok()?) }; @@ -361,76 +252,6 @@ impl XArray { Some(unsafe { T::from_foreign(p) }) } } - - /// Allocates a new index in the array, optionally storing a new value into it, with - /// configurable bounds for the index range to allocate from. - /// - /// If `value` is `None`, then the index is reserved from further allocation but remains - /// free for storing a value into it. - fn alloc_limits_opt(self: Pin<&Self>, value: Option, min: u32, max: u32) -> Result { - let new = value.map_or(core::ptr::null(), |a| a.into_foreign()); - let mut id: u32 = 0; - - // SAFETY: `self.xa` is always valid by the type invariant. If this succeeds, it - // takes ownership of the passed `T` (if any). If it fails, we must drop the - // `T` again. - let ret = unsafe { - bindings::xa_alloc( - self.xa.get(), - &mut id, - new as *mut _, - bindings::xa_limit { min, max }, - bindings::GFP_KERNEL, - ) - }; - - if ret < 0 { - // Make sure to drop the value we failed to store - if !new.is_null() { - // SAFETY: If `new` is not NULL, it came from the `ForeignOwnable` we got - // from the caller. - unsafe { T::from_foreign(new) }; - } - Err(Error::from_errno(ret)) - } else { - Ok(id as usize) - } - } - - /// Allocates a new index in the array, storing a new value into it, with configurable - /// bounds for the index range to allocate from. - pub fn alloc_limits(self: Pin<&Self>, value: T, min: u32, max: u32) -> Result { - self.alloc_limits_opt(Some(value), min, max) - } - - /// Allocates a new index in the array, storing a new value into it. - pub fn alloc(self: Pin<&Self>, value: T) -> Result { - self.alloc_limits(value, 0, u32::MAX) - } - - /// Reserves a new index in the array within configurable bounds for the index. - /// - /// Returns a `Reservation` object, which can then be used to store a value at this index or - /// otherwise free it for reuse. - pub fn reserve_limits(self: Pin<&Self>, min: u32, max: u32) -> Result> { - Ok(Reservation( - self, - self.alloc_limits_opt(None, min, max)?, - PhantomData, - )) - } - - /// Reserves a new index in the array. - /// - /// Returns a `Reservation` object, which can then be used to store a value at this index or - /// otherwise free it for reuse. - pub fn reserve(self: Pin<&Self>) -> Result> { - Ok(Reservation( - self, - self.alloc_limits_opt(None, 0, u32::MAX)?, - PhantomData, - )) - } } impl Drop for XArray { From 976067befc5e11fbaa70b3f9bb495baddf7be501 Mon Sep 17 00:00:00 2001 From: Alice Ryhl Date: Thu, 6 Jul 2023 09:46:15 +0000 Subject: [PATCH 35/37] rust: delete `ForeignOwnable::borrow_mut` We discovered that the current design of `borrow_mut` is problematic. This patch removes it until a better solution can be found. Specifically, the current design gives you access to a `&mut T`, which lets you change where the `ForeignOwnable` points (e.g., with `core::mem::swap`). No upcoming user of this API intended to make that possible, making all of them unsound. Signed-off-by: Alice Ryhl Reviewed-by: Martin Rodriguez Reboredo Reviewed-by: Gary Guo Reviewed-by: Benno Lossin --- rust/kernel/sync/arc.rs | 3 +-- rust/kernel/types.rs | 22 ++-------------------- 2 files changed, 3 insertions(+), 22 deletions(-) diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs index 1ecb2efab51e48..3d496391a9bd86 100644 --- a/rust/kernel/sync/arc.rs +++ b/rust/kernel/sync/arc.rs @@ -246,8 +246,7 @@ impl ForeignOwnable for Arc { let inner = NonNull::new(ptr as *mut ArcInner).unwrap(); // SAFETY: The safety requirements of `from_foreign` ensure that the object remains alive - // for the lifetime of the returned value. Additionally, the safety requirements of - // `ForeignOwnable::borrow_mut` ensure that no new mutable references are created. + // for the lifetime of the returned value. unsafe { ArcBorrow::new(inner) } } diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs index 169066030367d8..fdb778e65d79d3 100644 --- a/rust/kernel/types.rs +++ b/rust/kernel/types.rs @@ -35,34 +35,16 @@ pub trait ForeignOwnable: Sized { /// /// `ptr` must have been returned by a previous call to [`ForeignOwnable::into_foreign`] for /// which a previous matching [`ForeignOwnable::from_foreign`] hasn't been called yet. - /// Additionally, all instances (if any) of values returned by [`ForeignOwnable::borrow_mut`] - /// for this object must have been dropped. unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> Self::Borrowed<'a>; - /// Mutably borrows a foreign-owned object. - /// - /// # Safety - /// - /// `ptr` must have been returned by a previous call to [`ForeignOwnable::into_foreign`] for - /// which a previous matching [`ForeignOwnable::from_foreign`] hasn't been called yet. - /// Additionally, all instances (if any) of values returned by [`ForeignOwnable::borrow`] and - /// [`ForeignOwnable::borrow_mut`] for this object must have been dropped. - unsafe fn borrow_mut(ptr: *const core::ffi::c_void) -> ScopeGuard { - // SAFETY: The safety requirements ensure that `ptr` came from a previous call to - // `into_foreign`. - ScopeGuard::new_with_data(unsafe { Self::from_foreign(ptr) }, |d| { - d.into_foreign(); - }) - } - /// Converts a foreign-owned object back to a Rust-owned one. /// /// # Safety /// /// `ptr` must have been returned by a previous call to [`ForeignOwnable::into_foreign`] for /// which a previous matching [`ForeignOwnable::from_foreign`] hasn't been called yet. - /// Additionally, all instances (if any) of values returned by [`ForeignOwnable::borrow`] and - /// [`ForeignOwnable::borrow_mut`] for this object must have been dropped. + /// Additionally, all instances (if any) of values returned by [`ForeignOwnable::borrow`] for + /// this object must have been dropped. unsafe fn from_foreign(ptr: *const core::ffi::c_void) -> Self; } From ac520c2166c9a64e7ea37665d9df82251852778f Mon Sep 17 00:00:00 2001 From: Alice Ryhl Date: Mon, 10 Jul 2023 07:46:42 +0000 Subject: [PATCH 36/37] rust: add improved version of `ForeignOwnable::borrow_mut` Previously, the `ForeignOwnable` trait had a method called `borrow_mut` that was intended to provide mutable access to the inner value. However, the method accidentally made it possible to change the address of the object being modified, which usually isn't what we want. (And when we want that, it can be done by calling `from_foreign` and `into_foreign`, like how the old `borrow_mut` was implemented.) In this patch, we introduce an alternate definition of `borrow_mut` that solves the previous problem. Conceptually, given a pointer type `P` that implements `ForeignOwnable`, the `borrow_mut` method gives you the same kind of access as an `&mut P` would, except that it does not let you change the pointer `P` itself. This is analogous to how the existing `borrow` method provides the same kind of access to the inner value as an `&P`. Note that for types like `Arc`, having an `&mut Arc` only gives you immutable access to the inner `T`. This is because mutable references assume exclusive access, but there might be other handles to the same reference counted value, so the access isn't exclusive. The `Arc` type implements this by making `borrow_mut` return the same type as `borrow`. Signed-off-by: Alice Ryhl Reviewed-by: Boqun Feng --- rust/kernel/sync/arc.rs | 25 ++++++++---- rust/kernel/types.rs | 87 +++++++++++++++++++++++++++++++---------- 2 files changed, 83 insertions(+), 29 deletions(-) diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs index 3d496391a9bd86..ce1fa58c71cb4a 100644 --- a/rust/kernel/sync/arc.rs +++ b/rust/kernel/sync/arc.rs @@ -235,26 +235,35 @@ impl Arc { impl ForeignOwnable for Arc { type Borrowed<'a> = ArcBorrow<'a, T>; + // Mutable access to the `Arc` does not give any extra abilities over + // immutable access. + type BorrowedMut<'a> = ArcBorrow<'a, T>; fn into_foreign(self) -> *const core::ffi::c_void { ManuallyDrop::new(self).ptr.as_ptr() as _ } + unsafe fn from_foreign(ptr: *const core::ffi::c_void) -> Self { + // SAFETY: By the safety requirement of this function, we know that `ptr` came from + // a previous call to `Arc::into_foreign`, which guarantees that `ptr` is valid and + // holds a reference count increment that is transferrable to us. + unsafe { Self::from_inner(NonNull::new_unchecked(ptr as _)) } + } + unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> ArcBorrow<'a, T> { // SAFETY: By the safety requirement of this function, we know that `ptr` came from // a previous call to `Arc::into_foreign`. - let inner = NonNull::new(ptr as *mut ArcInner).unwrap(); + let inner = unsafe { NonNull::new_unchecked(ptr as *mut ArcInner) }; - // SAFETY: The safety requirements of `from_foreign` ensure that the object remains alive - // for the lifetime of the returned value. + // SAFETY: The safety requirements ensure that we will not give up our + // foreign-owned refcount while the `ArcBorrow` is still live. unsafe { ArcBorrow::new(inner) } } - unsafe fn from_foreign(ptr: *const core::ffi::c_void) -> Self { - // SAFETY: By the safety requirement of this function, we know that `ptr` came from - // a previous call to `Arc::into_foreign`, which guarantees that `ptr` is valid and - // holds a reference count increment that is transferrable to us. - unsafe { Self::from_inner(NonNull::new(ptr as _).unwrap()) } + unsafe fn borrow_mut<'a>(ptr: *const core::ffi::c_void) -> ArcBorrow<'a, T> { + // SAFETY: The safety requirements for `borrow_mut` are a superset of the safety + // requirements for `borrow`. + unsafe { Self::borrow(ptr) } } } diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs index fdb778e65d79d3..d849e1979ac707 100644 --- a/rust/kernel/types.rs +++ b/rust/kernel/types.rs @@ -20,66 +20,111 @@ use core::{ /// This trait is meant to be used in cases when Rust objects are stored in C objects and /// eventually "freed" back to Rust. pub trait ForeignOwnable: Sized { - /// Type of values borrowed between calls to [`ForeignOwnable::into_foreign`] and - /// [`ForeignOwnable::from_foreign`]. + /// Type used to immutably borrow a value that is currently foreign-owned. type Borrowed<'a>; + /// Type used to mutably borrow a value that is currently foreign-owned. + type BorrowedMut<'a>; + /// Converts a Rust-owned object to a foreign-owned one. /// /// The foreign representation is a pointer to void. fn into_foreign(self) -> *const core::ffi::c_void; - /// Borrows a foreign-owned object. + /// Converts a foreign-owned object back to a Rust-owned one. + /// + /// # Safety + /// + /// The provided pointer must have been returned by a previous call to [`into_foreign`], and it + /// must not be passed to `from_foreign` more than once. + /// + /// [`into_foreign`]: Self::into_foreign + unsafe fn from_foreign(ptr: *const core::ffi::c_void) -> Self; + + /// Borrows a foreign-owned object immutably. + /// + /// This method provides a way to access a foreign-owned value from Rust immutably. It provides + /// you with exactly the same abilities as an `&Self` when the value is Rust-owned. /// /// # Safety /// - /// `ptr` must have been returned by a previous call to [`ForeignOwnable::into_foreign`] for - /// which a previous matching [`ForeignOwnable::from_foreign`] hasn't been called yet. + /// The provided pointer must have been returned by a previous call to [`into_foreign`], and if + /// the pointer is ever passed to [`from_foreign`], then that call must happen after the end of + /// the lifetime 'a. + /// + /// [`into_foreign`]: Self::into_foreign + /// [`from_foreign`]: Self::from_foreign unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> Self::Borrowed<'a>; - /// Converts a foreign-owned object back to a Rust-owned one. + /// Borrows a foreign-owned object mutably. + /// + /// This method provides a way to access a foreign-owned value from Rust mutably. It provides + /// you with exactly the same abilities as an `&mut Self` when the value is Rust-owned, except + /// that this method does not let you swap the foreign-owned object for another. (That is, it + /// does not let you change the address of the void pointer that the foreign code is storing.) + /// + /// Note that for types like [`Arc`], an `&mut Arc` only gives you immutable access to the + /// inner value, so this method also only provides immutable access in that case. + /// + /// In the case of `Box`, this method gives you the ability to modify the inner `T`, but it + /// does not let you change the box itself. That is, you cannot change which allocation the box + /// points at. /// /// # Safety /// - /// `ptr` must have been returned by a previous call to [`ForeignOwnable::into_foreign`] for - /// which a previous matching [`ForeignOwnable::from_foreign`] hasn't been called yet. - /// Additionally, all instances (if any) of values returned by [`ForeignOwnable::borrow`] for - /// this object must have been dropped. - unsafe fn from_foreign(ptr: *const core::ffi::c_void) -> Self; + /// The provided pointer must have been returned by a previous call to [`into_foreign`], and if + /// the pointer is ever passed to [`from_foreign`], then that call must happen after the end of + /// the lifetime 'a. + /// + /// The lifetime 'a must not overlap with the lifetime of any other call to [`borrow`] or + /// `borrow_mut` on the same object. + /// + /// [`into_foreign`]: Self::into_foreign + /// [`from_foreign`]: Self::from_foreign + /// [`borrow`]: Self::borrow + /// [`Arc`]: crate::sync::Arc + unsafe fn borrow_mut<'a>(ptr: *const core::ffi::c_void) -> Self::BorrowedMut<'a>; } impl ForeignOwnable for Box { type Borrowed<'a> = &'a T; + type BorrowedMut<'a> = &'a mut T; fn into_foreign(self) -> *const core::ffi::c_void { Box::into_raw(self) as _ } - unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> &'a T { - // SAFETY: The safety requirements for this function ensure that the object is still alive, - // so it is safe to dereference the raw pointer. - // The safety requirements of `from_foreign` also ensure that the object remains alive for - // the lifetime of the returned value. - unsafe { &*ptr.cast() } - } - unsafe fn from_foreign(ptr: *const core::ffi::c_void) -> Self { // SAFETY: The safety requirements of this function ensure that `ptr` comes from a previous // call to `Self::into_foreign`. unsafe { Box::from_raw(ptr as _) } } + + unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> &'a T { + // SAFETY: The safety requirements of this method ensure that the object remains alive and + // immutable for the duration of 'a. + unsafe { &*ptr.cast() } + } + + unsafe fn borrow_mut<'a>(ptr: *const core::ffi::c_void) -> &'a mut T { + // SAFETY: The safety requirements of this method ensure that the pointer is valid and that + // nothing else will access the value for the duration of 'a. + unsafe { &mut *ptr.cast_mut().cast() } + } } impl ForeignOwnable for () { type Borrowed<'a> = (); + type BorrowedMut<'a> = (); fn into_foreign(self) -> *const core::ffi::c_void { core::ptr::NonNull::dangling().as_ptr() } - unsafe fn borrow<'a>(_: *const core::ffi::c_void) -> Self::Borrowed<'a> {} - unsafe fn from_foreign(_: *const core::ffi::c_void) -> Self {} + + unsafe fn borrow<'a>(_: *const core::ffi::c_void) -> Self::Borrowed<'a> {} + unsafe fn borrow_mut<'a>(_: *const core::ffi::c_void) -> Self::BorrowedMut<'a> {} } /// Runs a cleanup function/closure when dropped. From 642ff49d21e45075e6d0df9428651c3242639d0c Mon Sep 17 00:00:00 2001 From: Matt Gilbride Date: Mon, 17 Jul 2023 14:23:27 +0000 Subject: [PATCH 37/37] rust: xarray: figure out pin and get_scoped This needs to be rebased --- rust/kernel/xarray.rs | 134 +++++++++++++++--------------------------- 1 file changed, 47 insertions(+), 87 deletions(-) diff --git a/rust/kernel/xarray.rs b/rust/kernel/xarray.rs index e552641ab19daa..57ab4b9a4136ee 100644 --- a/rust/kernel/xarray.rs +++ b/rust/kernel/xarray.rs @@ -7,12 +7,13 @@ use crate::{ bindings, error::{Error, Result}, + prelude::*, types::{ForeignOwnable, Opaque, ScopeGuard}, }; use core::{ marker::{PhantomData, PhantomPinned}, pin::Pin, - ptr::NonNull, + ptr::{addr_of_mut, NonNull}, }; /// Flags passed to `XArray::new` to configure the `XArray`. @@ -50,14 +51,18 @@ pub mod flags { /// rudimentary read/write operations. /// /// ``` +/// use core::{ +/// borrow::Borrow, +/// ops::{ Deref, DerefMut } +/// }; /// use kernel::{ +/// pin_init, /// sync::Arc, -/// xarray::{XArray, flags::LOCK_IRQ} +/// xarray::{ XArray, flags::LOCK_IRQ } /// }; /// -/// let xarr: Box>> = Box::try_new(XArray::new(LOCK_IRQ))?; -/// let xarr: Pin>>> = Box::into_pin(xarr); -/// let xarr: Pin<&XArray>> = xarr.as_ref(); +/// let xarr = Box::pin_init(XArray::>::new(LOCK_IRQ)).unwrap(); +/// let xarr = xarr.as_ref(); /// /// // `get` and `set` are used to read/write values. /// assert!(xarr.get(0).is_none()); @@ -82,7 +87,7 @@ pub mod flags { /// ``` /// /// In this example, we create a new `XArray` and demonstrate -/// reading values that are not `Clone`, as well as mutating values. +/// reading and/or mutating values that are not `T::Borrowed<'a>: Into`. /// /// ``` /// use core::{ @@ -91,30 +96,23 @@ pub mod flags { /// }; /// use kernel::xarray::{XArray, flags::LOCK_IRQ}; /// -/// let xarr: Box>> = Box::try_new(XArray::new(LOCK_IRQ))?; -/// let mut xarr: Pin>>> = Box::into_pin(xarr); +/// let xarr = Box::pin_init(XArray::>::new(LOCK_IRQ)).unwrap(); +/// let xarr = xarr.as_ref(); +/// +/// // If the type is not `T::Borrowed<'a>: Into`, use `get_scoped` to access a shared reference +/// // from a closure. +/// assert!(xarr.get_scoped(0, |x| x.is_none())); +/// xarr.set(0, Box::try_new(0)?); +/// xarr.get_scoped(0, |x| assert_eq!(*x.unwrap(), 0)); /// -/// // Create scopes so that references can be dropped -/// // in order to take a new, mutable/immutable ones afterwards. -/// { -/// let xarr: Pin<&XArray>> = xarr.as_ref(); -/// // If the type is not `Clone`, use `get_shared` to access a shared reference -/// // from a closure. -/// assert!(xarr.get_shared(0, |x| x.is_none())); -/// xarr.set(0, Box::try_new(0)?); -/// xarr.get_shared(0, |x| assert_eq!(*x.unwrap(), 0)); -/// } -/// { -/// let mut xarr = xarr.as_mut(); -/// let mut value = xarr.get_mutable(0).unwrap(); -/// let mut value = value.deref_mut().as_mut(); -/// assert_eq!(value, &mut 0); -/// *value = 1; -/// } -/// { -/// let xarr = xarr.as_ref(); -/// xarr.get_shared(0, |x| assert_eq!(*x.unwrap(), 1)); -/// } +/// // `get_scoped` also gives mutable access inside the closure. +/// xarr.get_scoped(0, |x| { +/// let mut_x = x.unwrap(); +/// assert_eq!(mut_x, &0); +/// *mut_x = 1; +/// }); +/// +/// xarr.get_scoped(0, |x| assert_eq!(*x.unwrap(), 1)); /// /// # Ok::<(), Error>(()) /// ``` @@ -127,39 +125,28 @@ pub struct XArray { impl XArray { /// Creates a new `XArray` with the given flags. - pub fn new(flags: Flags) -> XArray { - let xa = Opaque::uninit(); - - // SAFETY: We have just created `xa`. This data structure does not require - // pinning. - unsafe { bindings::xa_init_flags(xa.get(), flags) }; - - // INVARIANT: Initialize the `XArray` with a valid `xa`. - XArray { - xa, - _p: PhantomData, - _q: PhantomPinned, + pub fn new(flags: Flags) -> impl PinInit { + unsafe { + kernel::init::pin_init_from_closure(move |slot: *mut XArray| { + bindings::xa_init_flags(Opaque::raw_get(addr_of_mut!((*slot).xa)), flags); + Ok(()) + }) } } /// Replaces an entry with a new value, returning the old value (if any). - pub fn replace(self: Pin<&Self>, index: usize, value: T) -> Result> { + pub fn replace(&self, index: u64, value: T) -> Result> { let new = value.into_foreign(); // SAFETY: `new` just came from into_foreign(), and we dismiss this guard if // the xa_store operation succeeds and takes ownership of the pointer. let guard = ScopeGuard::new(|| unsafe { - T::from_foreign(new); + drop(T::from_foreign(new)); }); // SAFETY: `self.xa` is always valid by the type invariant, and we are storing // a `T::into_foreign()` result which upholds the later invariants. let old = unsafe { - bindings::xa_store( - self.xa.get(), - index.try_into()?, - new as *mut _, - bindings::GFP_KERNEL, - ) + bindings::xa_store(self.xa.get(), index, new as *mut _, bindings::GFP_KERNEL) }; let ret = unsafe { bindings::xa_err(old) }; @@ -178,7 +165,7 @@ impl XArray { } /// Replaces an entry with a new value, dropping the old value (if any). - pub fn set(self: Pin<&Self>, index: usize, value: T) -> Result { + pub fn set(&self, index: u64, value: T) -> Result { self.replace(index, value)?; Ok(()) } @@ -186,39 +173,28 @@ impl XArray { /// Looks up a reference to an entry in the array, cloning it /// and returning the cloned value to the user. /// - pub fn get(self: Pin<&Self>, index: u64) -> Option> + pub fn get(&self, index: u64) -> Option where - T: Clone, + for<'a> T::BorrowedMut<'a>: Into, { - // SAFETY: `self.xa` is always valid by the type invariant. - unsafe { bindings::xa_lock(self.xa.get()) }; - - // SAFETY: `self.xa` is always valid by the type invariant. - let p = unsafe { bindings::xa_load(self.xa.get(), index) } as *const T; - - let t = NonNull::new(p as *mut T).map(|p| unsafe { T::borrow(p.clone().as_ptr() as _) }); - - // SAFETY: `self.xa` is always valid by the type invariant. - unsafe { bindings::xa_unlock(self.xa.get()) }; - - t + self.get_scoped(index, |x| x.map(|v| v.into())) } /// Looks up and a reference to an entry in the array, calling the user /// provided function on the resulting `Option<&T>` to return a value - /// computed from the reference. Use this function if you need shared - /// access to a `&T` that is not `Clone`. + /// computed from the reference. Use this function if you need + /// (possibly mutable) access to a `&T` that is not `T::Borrowed<'a>: Into`. /// - pub fn get_shared(self: Pin<&Self>, index: u64, f: F) -> R + pub fn get_scoped(&self, index: u64, f: F) -> R where - F: FnOnce(Option>) -> R, + F: FnOnce(Option>) -> R, { // SAFETY: `self.xa` is always valid by the type invariant. unsafe { bindings::xa_lock(self.xa.get()) }; // SAFETY: `self.xa` is always valid by the type invariant. let p = unsafe { bindings::xa_load(self.xa.get(), index) }; - let t = NonNull::new(p as *mut T).map(|p| unsafe { T::borrow(p.as_ptr() as _) }); + let t = NonNull::new(p as *mut T).map(|p| unsafe { T::borrow_mut(p.as_ptr() as _) }); let r = f(t); // SAFETY: `self.xa` is always valid by the type invariant. @@ -227,25 +203,9 @@ impl XArray { r } - /// Looks up and returns a *mutable* reference to an entry in the array. - pub fn get_mutable(self: Pin<&mut Self>, index: u64) -> Option> { - // SAFETY: `self.xa` is always valid by the type invariant. - unsafe { bindings::xa_lock(self.xa.get()) }; - - // SAFETY: `self.xa` is always valid by the type invariant. - let p = unsafe { bindings::xa_load(self.xa.get(), index) }; - - let t = NonNull::new(p as *mut T).map(|p| unsafe { T::borrow_mut(p.as_ptr() as _) }); - - // SAFETY: `self.xa` is always valid by the type invariant. - unsafe { bindings::xa_unlock(self.xa.get()) }; - - t - } - /// Removes and returns an entry, returning it if it existed. - pub fn remove(self: Pin<&Self>, index: usize) -> Option { - let p = unsafe { bindings::xa_erase(self.xa.get(), index.try_into().ok()?) }; + pub fn remove(&self, index: u64) -> Option { + let p = unsafe { bindings::xa_erase(self.xa.get(), index) }; if p.is_null() { None } else {