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`. + 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`. + 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
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
On 31.03.24 03:00, Wedson Almeida Filho wrote:
On Wed, 27 Mar 2024 at 13:04, Benno Lossin benno.lossin@proton.me wrote:
#[cfg(not(MODULE))]#[doc(hidden)]#[no_mangle]pub extern \"C\" fn __{name}_exit() {{__exit()
I just noticed this should be wrapped in an `unsafe` block with a SAFETY comment. Will fix this in v2.
}}
#[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`.
Fixed.
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*.
Indeed.
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`.
I would love to drop that requirement, but I am not sure we can. With that requirement, I wanted to ensure that no data race on `__MOD` can happen. If you need to verify that `__init` succeeded, one might think that it is not possible to call `__exit` such that a data race occurs, but I think it could theoretically be done if the concrete `Module` implementation never failed.
Do you have any suggestion for what I could add to the "be called after `__init` was called and returned `0`" requirement to make a data race impossible?
On Sun, 31 Mar 2024 at 07:27, Benno Lossin benno.lossin@proton.me wrote:
On 31.03.24 03:00, Wedson Almeida Filho wrote:
On Wed, 27 Mar 2024 at 13:04, Benno Lossin benno.lossin@proton.me wrote:
#[cfg(not(MODULE))]#[doc(hidden)]#[no_mangle]pub extern \"C\" fn __{name}_exit() {{__exit()I just noticed this should be wrapped in an `unsafe` block with a SAFETY comment. Will fix this in v2.
}}
#[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`.
Fixed.
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*.
Indeed.
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`.
I would love to drop that requirement, but I am not sure we can. With that requirement, I wanted to ensure that no data race on `__MOD` can happen. If you need to verify that `__init` succeeded, one might think that it is not possible to call `__exit` such that a data race occurs, but I think it could theoretically be done if the concrete `Module` implementation never failed.
I see. If you're concerned about compiler reordering, then we need compiler barriers.
Do you have any suggestion for what I could add to the "be called after `__init` was called and returned `0`" requirement to make a data race impossible?
If you're concerned with reordering from the processor as well, then we need cpu barriers. You'd have to say that the cpu/thread executing `__init` must have a release barrier after `__init` completes, and the thread/cpu doing `__exit` must have an acquire barrier before starting `__exit`.
But I'm not sure we need to go that far. Mostly because C is going to guarantee that ordering for us, so I'd say we can just omit this or perhaps say "This function must only be called from the exit module implementation".
-- Cheers, Benno
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
On 01.04.24 21:10, Wedson Almeida Filho wrote:
On Sun, 31 Mar 2024 at 07:27, Benno Lossin benno.lossin@proton.me wrote:
On 31.03.24 03:00, Wedson Almeida Filho wrote:
On Wed, 27 Mar 2024 at 13:04, Benno Lossin benno.lossin@proton.me wrote:
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*.
Indeed.
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`.
I would love to drop that requirement, but I am not sure we can. With that requirement, I wanted to ensure that no data race on `__MOD` can happen. If you need to verify that `__init` succeeded, one might think that it is not possible to call `__exit` such that a data race occurs, but I think it could theoretically be done if the concrete `Module` implementation never failed.
I see. If you're concerned about compiler reordering, then we need compiler barriers.
Do you have any suggestion for what I could add to the "be called after `__init` was called and returned `0`" requirement to make a data race impossible?
If you're concerned with reordering from the processor as well, then we need cpu barriers. You'd have to say that the cpu/thread executing `__init` must have a release barrier after `__init` completes, and the thread/cpu doing `__exit` must have an acquire barrier before starting `__exit`.
But I'm not sure we need to go that far. Mostly because C is going to guarantee that ordering for us, so I'd say we can just omit this or perhaps say "This function must only be called from the exit module implementation".
Yeah, though I do not exactly know where or what the "exit module implementation" is. If you are happy with v2, then I think we can go with that. This piece of code is also not really something people will need to read.
linux-stable-mirror@lists.linaro.org
-
Benno Lossin -
Wedson Almeida Filho