On Wed, 27 Mar 2024 at 13:04, 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
This patch is best viewed with `git show --ignore-space-change`, since I also adjusted the indentation.
rust/macros/module.rs | 198 ++++++++++++++++++++++++------------------ 1 file changed, 112 insertions(+), 86 deletions(-)
diff --git a/rust/macros/module.rs b/rust/macros/module.rs index 27979e582e4b..16c4921a08f2 100644 --- a/rust/macros/module.rs +++ b/rust/macros/module.rs @@ -199,103 +199,129 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream { /// Used by the printing macros, e.g. [`info!`]. const __LOG_PREFIX: &[u8] = b"{name}\0";
/// The \"Rust loadable module\" mark.//// This may be best done another way later on, e.g. as a new modinfo// key or a new section. For the moment, keep it simple.#[cfg(MODULE)]#[doc(hidden)]#[used]static __IS_RUST_MODULE: () = ();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////// This function must not be called after module initialization, because it may be/// freed after that completes.#[cfg(MODULE)]#[doc(hidden)]#[no_mangle]#[link_section = \".init.text\"]pub unsafe extern \"C\" fn init_module() -> core::ffi::c_int {{__init()}}
// Double nested modules, since then nobody can access the public items inside.mod __module_init {{mod __module_init {{use super::super::{type_};/// The \"Rust loadable module\" mark.//// This may be best done another way later on, e.g. as a new modinfo// key or a new section. For the moment, keep it simple.#[cfg(MODULE)]#[doc(hidden)]#[used]static __IS_RUST_MODULE: () = ();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////// This function must not be called after module initialization, because it may be/// freed after that completes.#[cfg(MODULE)]#[doc(hidden)]#[no_mangle]#[link_section = \".init.text\"]pub unsafe extern \"C\" fn init_module() -> core::ffi::c_int {{__init()}}
#[cfg(MODULE)]#[doc(hidden)]#[no_mangle]pub extern \"C\" fn cleanup_module() {{__exit()}}
#[cfg(MODULE)]#[doc(hidden)]#[no_mangle]pub extern \"C\" fn cleanup_module() {{__exit()}}
// Built-in modules are initialized through an initcall pointer// and the identifiers need to be unique.#[cfg(not(MODULE))]#[cfg(not(CONFIG_HAVE_ARCH_PREL32_RELOCATIONS))]#[doc(hidden)]#[link_section = \"{initcall_section}\"]#[used]pub static __{name}_initcall: extern \"C\" fn() -> core::ffi::c_int = __{name}_init;#[cfg(not(MODULE))]#[cfg(CONFIG_HAVE_ARCH_PREL32_RELOCATIONS)]core::arch::global_asm!(r#\".section \"{initcall_section}\", \"a\"__{name}_initcall:.long __{name}_init - ..previous\"#);
// Built-in modules are initialized through an initcall pointer// and the identifiers need to be unique.#[cfg(not(MODULE))]#[cfg(not(CONFIG_HAVE_ARCH_PREL32_RELOCATIONS))]#[doc(hidden)]#[link_section = \"{initcall_section}\"]#[used]pub static __{name}_initcall: extern \"C\" fn() -> core::ffi::c_int = __{name}_init;#[cfg(not(MODULE))]#[cfg(CONFIG_HAVE_ARCH_PREL32_RELOCATIONS)]core::arch::global_asm!(r#\".section \"{initcall_section}\", \"a\"__{name}_initcall:.long __{name}_init - ..previous\"#);#[cfg(not(MODULE))]#[doc(hidden)]#[no_mangle]pub extern \"C\" fn __{name}_init() -> core::ffi::c_int {{__init()}}
#[cfg(not(MODULE))]#[doc(hidden)]#[no_mangle]pub extern \"C\" fn __{name}_init() -> core::ffi::c_int {{__init()}}
#[cfg(not(MODULE))]#[doc(hidden)]#[no_mangle]pub extern \"C\" fn __{name}_exit() {{__exit()}}
#[cfg(not(MODULE))]#[doc(hidden)]#[no_mangle]pub extern \"C\" fn __{name}_exit() {{__exit()}}
/// # Safety////// This function must/// - only be called once,/// - not be called concurrently with `__exit`.
I don't think the second item is needed here, it really is a requirement on `__exit`.
unsafe fn __init() -> core::ffi::c_int {{match <{type_} as kernel::Module>::init(&THIS_MODULE) {{Ok(m) => {{// SAFETY:// no data race, since `__MOD` can only be accessed by this module and// there only `__init` and `__exit` access it. These functions are only// called once and `__exit` cannot be called before or during `__init`.unsafe {{__MOD = Some(m);}}return 0;}}Err(e) => {{return e.to_errno();}}}}}}
fn __init() -> core::ffi::c_int {{match <{type_} as kernel::Module>::init(&THIS_MODULE) {{Ok(m) => {{
/// # Safety////// This function must/// - only be called once,/// - be called after `__init`,/// - not be called concurrently with `__init`.
The second item is incomplete: it must be called after `__init` *succeeds*.
With that added (which is a different precondition), I think the third item can be dropped because if you have to wait to see whether `__init` succeeded or failed before you can call `__exit`, then certainly you cannot call it concurrently with `__init`.
unsafe fn __exit() {{// SAFETY:// no data race, since `__MOD` can only be accessed by this module and there// only `__init` and `__exit` access it. These functions are only called once// and `__init` was already called. unsafe {{
__MOD = Some(m);
// Invokes `drop()` on `__MOD`, which should be used for cleanup.__MOD = None; }}
return 0; }}Err(e) => {{return e.to_errno();}}}}}}fn __exit() {{unsafe {{// Invokes `drop()` on `__MOD`, which should be used for cleanup.__MOD = None;
{modinfo} }} }}
{modinfo} ", type_ = info.type_, name = info.name,base-commit: 4cece764965020c22cff7665b18a012006359095
2.44.0