linux-kernel - Re: [PATCH 1/2] rust: introduce `InPlaceModule`

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 27 Mar 2024 16:16:33 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Wedson Almeida Filho <wedsonaf@...il.com>, rust-for-linux@...r.kernel.org
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, Björn Roy Baron <bjorn3_gh@...tonmail.com>, Andreas Hindborg <a.hindborg@...sung.com>, Alice Ryhl <aliceryhl@...gle.com>, linux-kernel@...r.kernel.org, Wedson Almeida Filho <walmeida@...rosoft.com>
Subject: Re: [PATCH 1/2] rust: introduce `InPlaceModule`

On 27.03.24 04:23, Wedson Almeida Filho wrote:
> From: Wedson Almeida Filho <walmeida@...rosoft.com>
> 
> This allows modules to be initialised in-place in pinned memory, which
> enables the usage of pinned types (e.g., mutexes, spinlocks, driver
> registrations, etc.) in modules without any extra allocations.
> 
> Drivers that don't need this may continue to implement `Module` without
> any changes.
> 
> Signed-off-by: Wedson Almeida Filho <walmeida@...rosoft.com>

I have some suggestions below, with those fixed,

Reviewed-by: Benno Lossin <benno.lossin@...ton.me>

> ---
>  rust/kernel/lib.rs    | 25 ++++++++++++++++++++++++-
>  rust/macros/module.rs | 18 ++++++------------
>  2 files changed, 30 insertions(+), 13 deletions(-)
> 
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 5c641233e26d..64aee4fbc53b 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -62,7 +62,7 @@
>  /// The top level entrypoint to implementing a kernel module.
>  ///
>  /// For any teardown or cleanup operations, your type may implement [`Drop`].
> -pub trait Module: Sized + Sync {
> +pub trait Module: Sized + Sync + Send {
>      /// Called at module initialization time.
>      ///
>      /// Use this method to perform whatever setup or registration your module
> @@ -72,6 +72,29 @@ pub trait Module: Sized + Sync {
>      fn init(module: &'static ThisModule) -> error::Result<Self>;
>  }
> 
> +/// A module that is pinned and initialised in-place.
> +pub trait InPlaceModule: Sync + Send {
> +    /// Creates an initialiser for the module.
> +    ///
> +    /// It is called when the module is loaded.
> +    fn init(module: &'static ThisModule) -> impl init::PinInit<Self, error::Error>;
> +}
> +
> +impl<T: Module> InPlaceModule for T {
> +    fn init(module: &'static ThisModule) -> impl init::PinInit<Self, error::Error> {
> +        let initer = move |slot: *mut Self| {
> +            let m = <Self as Module>::init(module)?;
> +
> +            // SAFETY: `slot` is valid for write per the contract with `pin_init_from_closure`.
> +            unsafe { slot.write(m) };
> +            Ok(())
> +        };
> +
> +        // SAFETY: On success, `initer` always fully initialises an instance of `Self`.
> +        unsafe { init::pin_init_from_closure(initer) }
> +    }
> +}
> +
>  /// Equivalent to `THIS_MODULE` in the C API.
>  ///
>  /// C header: [`include/linux/export.h`](srctree/include/linux/export.h)
> diff --git a/rust/macros/module.rs b/rust/macros/module.rs
> index 27979e582e4b..0b2bb4ec2fba 100644
> --- a/rust/macros/module.rs
> +++ b/rust/macros/module.rs
> @@ -208,7 +208,7 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
>              #[used]
>              static __IS_RUST_MODULE: () = ();
> 
> -            static mut __MOD: Option<{type_}> = None;
> +            static mut __MOD: core::mem::MaybeUninit<{type_}> = core::mem::MaybeUninit::uninit();

I would prefer `::core::mem::MaybeUninit`, since that prevents
accidentally referring to a module named `core`.

> 
>              // SAFETY: `__this_module` is constructed by the kernel at load time and will not be
>              // freed until the module is unloaded.
> @@ -275,23 +275,17 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
>              }}
> 
>              fn __init() -> core::ffi::c_int {{
> -                match <{type_} as kernel::Module>::init(&THIS_MODULE) {{
> -                    Ok(m) => {{
> -                        unsafe {{
> -                            __MOD = Some(m);
> -                        }}
> -                        return 0;
> -                    }}
> -                    Err(e) => {{
> -                        return e.to_errno();
> -                    }}
> +                let initer = <{type_} as kernel::InPlaceModule>::init(&THIS_MODULE);

Ditto with `::kernel::InPlaceModule`.

> +                match unsafe {{ initer.__pinned_init(__MOD.as_mut_ptr()) }} {{

This requires that the `PinInit` trait is in scope, I would use:

     match unsafe {{ ::kernel::init::PinInit::__pinned_init(initer, __MOD.as_mut_ptr()) }} {{

-- 
Cheers,
Benno

> +                    Ok(m) => 0,
> +                    Err(e) => e.to_errno(),
>                  }}
>              }}
> 
>              fn __exit() {{
>                  unsafe {{
>                      // Invokes `drop()` on `__MOD`, which should be used for cleanup.
> -                    __MOD = None;
> +                    __MOD.assume_init_drop();
>                  }}
>              }}
> 
> --
> 2.34.1
>