On Sun, Apr 07, 2024 at 10:02:35PM +0200, Miguel Ojeda wrote:
On Mon, Apr 1, 2024 at 8:53 PM Benno Lossin benno.lossin@proton.me wrote:
The `module!` macro creates glue code that are called by C to initialize the Rust modules using the `Module::init` function. Part of this glue code are the local functions `__init` and `__exit` that are used to initialize/destroy the Rust module. These functions are safe and also visible to the Rust mod in which the `module!` macro is invoked. This means that they can be called by other safe Rust code. But since they contain `unsafe` blocks that rely on only being called at the right time, this is a soundness issue.
Wrap these generated functions inside of two private modules, this guarantees that the public functions cannot be called from the outside. Make the safe functions `unsafe` and add SAFETY comments.
Cc: stable@vger.kernel.org Closes: https://github.com/Rust-for-Linux/linux/issues/629 Fixes: 1fbde52bde73 ("rust: add `macros` crate") Signed-off-by: Benno Lossin benno.lossin@proton.me
[ Capitalized comments, avoided newline in non-list SAFETY comments and reworded to add Reported-by and newline. ]
Applied to `rust-fixes` -- thanks everyone!
As reported by Dirk Behme:
https://rust-for-linux.zulipchat.com/#narrow/stream/291565-Help/topic/How.20...
The following is needed to allow modules using `THIS_MODULE` as a static variable. That being said, maybe we can merge this patch as it is, since it doesn't break mainline, and the following change can be done in a separate patch.
Regards, Boqun
----------------------------->8 diff --git a/rust/macros/module.rs b/rust/macros/module.rs index 293beca0a583..0664b957a70a 100644 --- a/rust/macros/module.rs +++ b/rust/macros/module.rs @@ -199,6 +199,17 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream { /// Used by the printing macros, e.g. [`info!`]. const __LOG_PREFIX: &[u8] = b"{name}\0";
+ // SAFETY: `__this_module` is constructed by the kernel at load time and will not be + // freed until the module is unloaded. + #[cfg(MODULE)] + static THIS_MODULE: kernel::ThisModule = unsafe {{ + kernel::ThisModule::from_ptr(&kernel::bindings::__this_module as *const _ as *mut _) + }}; + #[cfg(not(MODULE))] + static THIS_MODULE: kernel::ThisModule = unsafe {{ + kernel::ThisModule::from_ptr(core::ptr::null_mut()) + }}; + // Double nested modules, since then nobody can access the public items inside. mod __module_init {{ mod __module_init {{ @@ -215,17 +226,6 @@ mod __module_init {{
static mut __MOD: Option<{type_}> = None;
- // SAFETY: `__this_module` is constructed by the kernel at load time and will not be - // freed until the module is unloaded. - #[cfg(MODULE)] - static THIS_MODULE: kernel::ThisModule = unsafe {{ - kernel::ThisModule::from_ptr(&kernel::bindings::__this_module as *const _ as *mut _) - }}; - #[cfg(not(MODULE))] - static THIS_MODULE: kernel::ThisModule = unsafe {{ - kernel::ThisModule::from_ptr(core::ptr::null_mut()) - }}; - // Loadable modules need to export the `{{init,cleanup}}_module` identifiers. /// # Safety /// @@ -301,7 +301,7 @@ mod __module_init {{ /// /// This function must only be called once. unsafe fn __init() -> core::ffi::c_int {{ - match <{type_} as kernel::Module>::init(&THIS_MODULE) {{ + match <{type_} as kernel::Module>::init(&super::super::THIS_MODULE) {{ Ok(m) => {{ // SAFETY: // no data race, since `__MOD` can only be accessed by this module and