Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pinned module #997

Draft
wants to merge 2 commits into
base: rust-next
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 30 additions & 1 deletion rust/kernel/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#![feature(new_uninit)]
#![feature(pin_macro)]
#![feature(receiver_trait)]
#![feature(type_alias_impl_trait)]
#![feature(unsize)]

// Ensure conditional compilation based on the kernel configuration works;
Expand Down Expand Up @@ -59,7 +60,7 @@ const __LOG_PREFIX: &[u8] = b"rust_kernel\0";
/// 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
Expand All @@ -69,6 +70,34 @@ 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 {
/// The type that implements the pinned initialisation of the module.
type Init: init::PinInit<Self, error::Error>;

/// Creates an initialiser for the module.
///
/// It is called when the module is loaded.
fn init(module: &'static ThisModule) -> error::Result<Self::Init>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you using Result<impl Init<Self, Error>> here? There are then two places that can fail:

  • the creation of the initializer
  • the initialization itself.

Just wanted to check that you actually want that. Not really sure if we want that, I would need to view some examples to decide for myself.

}

impl<T: Module> InPlaceModule for T {
type Init = impl init::PinInit<Self, error::Error>;

fn init(module: &'static ThisModule) -> error::Result<Self::Init> {
let m = <Self as Module>::init(module)?;

let initer = move |slot: *mut Self| {
// 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`.
Ok(unsafe { init::pin_init_from_closure(initer) })
Comment on lines +87 to +97
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks to this I found out that a thing in pin-init is missing something 1, with that fixed this can become

<Self as Module>::init(module)

Footnotes

  1. the impl<T> Init<T> for T should be impl<T, E> Init<T, E> for T.

}
}

/// Equivalent to `THIS_MODULE` in the C API.
///
/// C header: `include/linux/export.h`
Expand Down
22 changes: 10 additions & 12 deletions rust/macros/module.rs
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
#[used]
static __IS_RUST_MODULE: () = ();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is all of this stuff not placed inside of const _: () = { ... };? That way it is inaccessible to the outside and we can avoid the __ naming (this also allows multiple module! invocations in the same module).

We must do this, otherwise people can just call __exit() anywhere (since it is a safe function).

Copy link
Member

@ojeda ojeda Apr 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

global_asm! cannot go there, and then others would be forced to go out too. What about a private module? I don't think we need/want several module! invocations (it could be interesting to consider, though, but it would need wider discussion).

By the way, I don't recall why we did not use a no_mangle for that static (prefixing it with the module name or similar).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is fine for global_asm! to be outside. The functions used by global_asm! are #[no_mangle], so they would not be forced out?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think eventually we'd want to switch to asm_sym and stop doing the no_mangle stuff.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yeah, it would move it a bit away though. But what is wrong with the private module? Or rather, when would you reach for const _ instead?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yeah, it would move it a bit away though. But what is wrong with the private module? Or rather, when would you reach for const _ instead?

people can still refer to __private_module::exit and call the exit function. Calling it twice will result in a UAF.

When stuff is inside of const _: () = {...}; then nobody outside of that block can refer to the things inside.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And with asm_sym, we could create a module inside of the const _ block where we put the global_asm!.

Copy link
Member

@ojeda ojeda Apr 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

people can still refer to __private_module::exit and call the exit function.

How? I am talking about making the functions fully private, i.e. private-in-private, which would still export the symbol (no_mangle/used).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that also works, but if you need them to be public, you can use const _. I think it is just better to make them fully inaccessible even if they require unsafe like the pub extern \"C\" fn __{name}_init() or pub static __{name}_initcall .
Also const _ avoids compile errors for people who name their module the same as the macro-generated one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I support making things as inaccessible as possible, but if the only difference is that we avoid a compile-time error that should not be triggered, then I think simplicity is best vs. const _: () { ... } + a mod inside for global_asm! and/or changing the order.

I see anonymous modules were considered as an alternative for unnamed constants -- that would have been cleaner for this use case and simpler to understand.


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.
Expand Down Expand Up @@ -270,23 +270,21 @@ 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 = match <{type_} as kernel::InPlaceModule>::init(&THIS_MODULE) {{
Ok(i) => i,
Err(e) => return e.to_errno(),
}};

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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the __exit() function never get called, when __init returned non-zero?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. If init fails, exit is never called.

}}
}}

Expand Down
11 changes: 11 additions & 0 deletions samples/rust/Kconfig
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,17 @@ config SAMPLE_RUST_MINIMAL

If unsure, say N.

config SAMPLE_RUST_INPLACE
tristate "Minimal in-place"
help
This option builds the Rust minimal module with in-place
initialisation.

To compile this as a module, choose M here:
the module will be called rust_inplace.

If unsure, say N.

config SAMPLE_RUST_PRINT
tristate "Printing macros"
help
Expand Down
1 change: 1 addition & 0 deletions samples/rust/Makefile
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# SPDX-License-Identifier: GPL-2.0

obj-$(CONFIG_SAMPLE_RUST_MINIMAL) += rust_minimal.o
obj-$(CONFIG_SAMPLE_RUST_INPLACE) += rust_inplace.o
obj-$(CONFIG_SAMPLE_RUST_PRINT) += rust_print.o

subdir-$(CONFIG_SAMPLE_RUST_HOSTPROGS) += hostprogs
41 changes: 41 additions & 0 deletions samples/rust/rust_inplace.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
// SPDX-License-Identifier: GPL-2.0

//! Rust minimal in-place sample.

use kernel::prelude::*;

module! {
type: RustInPlace,
name: "rust_inplace",
author: "Rust for Linux Contributors",
description: "Rust minimal in-place sample",
license: "GPL",
}

#[pin_data(PinnedDrop)]
struct RustInPlace {
numbers: Vec<i32>,
}
Comment on lines +15 to +18
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be beneficial to also put something in here that people can't put into non-in-place modules, e.g. a Mutex, of coruse sync would have to land first.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the main motivation for this PR is indeed to be able to use a module-wide Mutex without having to use ctor tricks?


impl kernel::InPlaceModule for RustInPlace {
type Init = impl PinInit<Self, Error>;
fn init(_module: &'static ThisModule) -> Result<Self::Init> {
pr_info!("Rust minimal sample (init)\n");
pr_info!("Am I built-in? {}\n", !cfg!(MODULE));

let mut numbers = Vec::new();
numbers.try_push(72)?;
numbers.try_push(108)?;
numbers.try_push(200)?;

Ok(try_pin_init!(Self { numbers }))
}
}

#[pinned_drop]
impl PinnedDrop for RustInPlace {
fn drop(self: Pin<&mut Self>) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a few folks suggest that instead of using a drop implementation, that we should do have deinit or exit method in kernel::Module.

We decided against it back then, which I think was the right decision.

But I think for the pinned/static case,the ergonomics of a method in the module to clean up (which takes a Pin<&mut self>) are better I think.

We should perhaps consider it.

What do you all think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few problems with an explicit exit/deinit function:

  • people could call it like a normal function, so you cannot do things that you can do in drop (e.g. drop other things)
  • we have to blanket impl Drop for all Modules, which would prevent people from adding their own drop functionality

We could change module! to allow custom drop impls, but I feel like keeping it like this makes more sense.

Why do you think that the ergonomics of exit would be better than PinnedDrop::drop?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With pin-ini, we have to add (PinnedDrop) to pin_data attribute of the module, then we have to add the #[pinned_drop] attribute to the PinnedDrop impl block.

These are all magic as far as I can tell with no easy way for someone who knows about regular Drop impls to find this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point, however if you have an explicit exit function on Module, then that could be called multiple times by user code.
You could make the function unsafe, but that feels weird to me, since then you could no longer grep for unsafe to check if they use unsafe code.
You could of course mimic the #[pinned_drop] macro that makes sure nobody can call the function accidentally, but then we could also just use the drop approach.

I am not sure how we could improve the situation...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make it not callable from other places, we can have an extra argument of some type T whose constructor is unsafe.

Note, however, that I'm not convinced yet that an exit function in Module is necessarily the way to go. I'm just saying we should consider it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make it not callable from other places, we can have an extra argument of some type T whose constructor is unsafe.

That is how #[pinned_drop] currently does things (it adds this parameter to the function). I think it would also be a source of confusion for users to have this visible.

Another thing to consider with the exit function approach is that we then need to have a blanket impl of Drop for all modules. Users will not be able to have a normal drop function, which I think is unfortunate, since that will also create confusion.

pr_info!("My numbers are {:?}\n", self.numbers);
pr_info!("Rust minimal inplace sample (exit)\n");
}
}
2 changes: 1 addition & 1 deletion scripts/Makefile.build
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,7 @@ $(obj)/%.lst: $(src)/%.c FORCE
# Compile Rust sources (.rs)
# ---------------------------------------------------------------------------

rust_allowed_features := core_ffi_c,explicit_generic_args_with_impl_trait,new_uninit,pin_macro
rust_allowed_features := core_ffi_c,explicit_generic_args_with_impl_trait,new_uninit,pin_macro,type_alias_impl_trait
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not informed on the status of this feature, maybe @bjorn3 knows more? I have seen some general movement in the area (e.g. impl trait in type alias) but no idea how far away this is.


rust_common_cmd = \
RUST_MODFILE=$(modfile) $(RUSTC_OR_CLIPPY) $(rust_flags) \
Expand Down