Pinned module #997

wedsonaf · 2023-04-12T19:38:19Z

No description provided.

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 <[email protected]>

This is a modifid version of rust_minimal that is initialised in-place. Signed-off-by: Wedson Almeida Filho <[email protected]>

alex · 2023-04-12T21:50:08Z

Do we expect eventually all modules will just use the in-place interface, or will there always be a use case for the "simple" version?

wedsonaf · 2023-04-13T04:49:03Z

Do we expect eventually all modules will just use the in-place interface, or will there always be a use case for the "simple" version?

There's very little cost to having the "simple" case, so I think we should keep it.

If we eventually get to a state where the ergonomics of the pinned one are close enough to the unpinned version (they're a bit cumbersome now, as we can see in the sample), then I think we could reconsider.

wedsonaf · 2023-04-13T04:52:28Z

samples/rust/rust_inplace.rs

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


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?

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?

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.

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...

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.

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.

y86-dev · 2023-04-13T07:01:18Z

rust/kernel/lib.rs

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


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.

y86-dev · 2023-04-13T07:08:57Z

rust/kernel/lib.rs

+    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) })


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

<Self as Module>::init(module)

Footnotes

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

y86-dev · 2023-04-13T07:41:05Z

rust/macros/module.rs

@@ -208,7 +208,7 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
            #[used]
            static __IS_RUST_MODULE: () = ();


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).

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).

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?

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

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?

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.

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

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).

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.

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.

y86-dev · 2023-04-13T07:42:03Z

rust/macros/module.rs

                }}
            }}

            fn __exit() {{
                unsafe {{
                    // Invokes `drop()` on `__MOD`, which should be used for cleanup.
-                    __MOD = None;
+                    __MOD.assume_init_drop();


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

Correct. If init fails, exit is never called.

y86-dev · 2023-04-13T07:46:27Z

scripts/Makefile.build

@@ -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


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.

y86-dev · 2023-04-13T07:48:19Z

samples/rust/rust_inplace.rs

+#[pin_data(PinnedDrop)]
+struct RustInPlace {
+    numbers: Vec<i32>,
+}


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.

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?

wedsonaf added 2 commits April 12, 2023 16:45

samples: rust: add in-place initilisation sample

14cccdb

This is a modifid version of rust_minimal that is initialised in-place. Signed-off-by: Wedson Almeida Filho <[email protected]>

wedsonaf force-pushed the pinned-module branch from 1c03ee0 to 14cccdb Compare April 12, 2023 19:47

wedsonaf commented Apr 13, 2023

View reviewed changes

y86-dev reviewed Apr 13, 2023

View reviewed changes

ojeda force-pushed the rust-next branch from bee23a1 to 341faf2 Compare July 19, 2023 18:06

ojeda force-pushed the rust-next branch from bd83e43 to 49a9ef7 Compare August 7, 2023 09:39

ojeda force-pushed the rust-next branch from 71004ce to 4af84c6 Compare August 21, 2023 12:32

ojeda force-pushed the rust-next branch from 2392e2a to e3c3d34 Compare February 18, 2024 20:22

ojeda force-pushed the rust-next branch from a419846 to 6d1c22d Compare August 21, 2024 22:16

ojeda force-pushed the rust-next branch 3 times, most recently from c9b5ce6 to ce1c54f Compare October 9, 2024 22:37

ojeda force-pushed the rust-next branch 3 times, most recently from 9ee7197 to 6ce162a Compare October 15, 2024 21:11

ojeda force-pushed the rust-next branch from 0675624 to d072acd Compare November 11, 2024 22:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pinned module #997

Pinned module #997

wedsonaf commented Apr 12, 2023

alex commented Apr 12, 2023

wedsonaf commented Apr 13, 2023

wedsonaf Apr 13, 2023

y86-dev Apr 13, 2023

wedsonaf Mar 27, 2024

y86-dev Mar 27, 2024

wedsonaf Mar 27, 2024

y86-dev Mar 27, 2024

y86-dev Apr 13, 2023

y86-dev Apr 13, 2023

y86-dev Apr 13, 2023

ojeda Apr 13, 2023 •

edited

Loading

y86-dev Apr 13, 2023

nbdd0121 Apr 13, 2023

ojeda Apr 13, 2023

y86-dev Apr 13, 2023

y86-dev Apr 13, 2023

ojeda Apr 13, 2023 •

edited

Loading

y86-dev Apr 13, 2023

ojeda Apr 14, 2023

y86-dev Apr 13, 2023

wedsonaf Mar 27, 2024

y86-dev Apr 13, 2023

y86-dev Apr 13, 2023

nbdd0121 Apr 13, 2023

		@@ -208,7 +208,7 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
		#[used]
		static __IS_RUST_MODULE: () = ();

Pinned module #997

Are you sure you want to change the base?

Pinned module #997

Conversation

wedsonaf commented Apr 12, 2023

alex commented Apr 12, 2023

wedsonaf commented Apr 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Footnotes

Choose a reason for hiding this comment

ojeda Apr 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ojeda Apr 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ojeda Apr 13, 2023 •

edited

Loading

ojeda Apr 13, 2023 •

edited

Loading