| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CANeycqo9noMJGSCsjCoH29xuPTJ7NEmYnFBE__9D+tvaqsiLdQ@mail.gmail.com>
Date: Wed, 27 Mar 2024 11:23:59 -0300
From: Wedson Almeida Filho <wedsonaf@...il.com>
To: Valentin Obst <kernel@...entinobst.de>
Cc: a.hindborg@...sung.com, alex.gaynor@...il.com, aliceryhl@...gle.com,
benno.lossin@...ton.me, bjorn3_gh@...tonmail.com, boqun.feng@...il.com,
gary@...yguo.net, linux-kernel@...r.kernel.org, ojeda@...nel.org,
rust-for-linux@...r.kernel.org, walmeida@...rosoft.com,
Trevor Gross <tmgross@...ch.edu>, FUJITA Tomonori <fujita.tomonori@...il.com>
Subject: Re: [PATCH 1/2] rust: introduce `InPlaceModule`
On Wed, 27 Mar 2024 at 05:13, Valentin Obst <kernel@...entinobst.de> wrote:
>
> > 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>
> > ---
> > 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 {
>
> This does not compile with `CONFIG_AX88796B_RUST_PHY=y || m` (or the
> phylib abstractions' doctests) since the module `Registration` is not
> `Send`.
Thanks for the heads up. I thought I had enabled all rust code but
indeed I was missing this. I will fix it in v2.
> I remember Trevor raising the question whether we want to require modules
> to be `Send`. I am not aware of any examples of `!Send` modules but I guess
> it would be possible to write code that is only correct under the
> assumption that it is loaded/unloaded in the same context.
It might be possible in the future, but I don't believe it is now
because all rust modules support unloading. And there is no guarantee
that the thread unloading (and therefore calling module_exit) is the
same that loaded (and called module_init), so a module must be Send to
properly handle drop being called from a different thread.
Not requiring Send on the original Module trait was an oversight that
I don't want to repeat in InPlaceModule.
>
> @Trevor: Are you aware of any modules with that requirement?
>
> I have been using this patch for quite a while with my TCP CCAs now
> (without the `Send` bound) and did not experience any other issues; thus
> offering:
> Tested-by: Valentin Obst <kernel@...entinobst.de>
Thanks!
>
> - Best Valentin
>
> > /// 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();
> >
> > // 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);
> > + match unsafe {{ initer.__pinned_init(__MOD.as_mut_ptr()) }} {{
> > + 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();
> > }}
> > }}
> >
> > --
Powered by blists - more mailing lists