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: [email protected]
Closes: https://github.com/Rust-for-Linux/linux/issues/629
Fixes: 1fbde52bde73 ("rust: add `macros` crate")
Signed-off-by: Benno Lossin <[email protected]>
---
v1: https://lore.kernel.org/rust-for-linux/[email protected]/
v1 -> v2:
- wrapped `__init` and `__exit` calls in `unsafe` blocks and added
SAFETY comments,
- fixed safety requirement on `__exit` and `__init`,
- rebased onto rust-next.
rust/macros/module.rs | 213 +++++++++++++++++++++++++-----------------
1 file changed, 127 insertions(+), 86 deletions(-)
diff --git a/rust/macros/module.rs b/rust/macros/module.rs
index 27979e582e4b..293beca0a583 100644
--- a/rust/macros/module.rs
+++ b/rust/macros/module.rs
@@ -199,103 +199,144 @@ 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 {{
+ // SAFETY: this function is inaccessible to the outside due to the double
+ // module wrapping it. It is called exactly once by the C side via its
+ // unique name.
+ unsafe {{ __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() {{
+ // SAFETY:
+ // - this function is inaccessible to the outside due to the double
+ // module wrapping it. It is called exactly once by the C side via its
+ // unique name,
+ // - furthermore it is only called after `init_module` has returned `0`
+ // (which delegates to `__init`).
+ unsafe {{ __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 {{
+ // SAFETY: this function is inaccessible to the outside due to the double
+ // module wrapping it. It is called exactly once by the C side via its
+ // placement above in the initcall section.
+ unsafe {{ __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() {{
+ // SAFETY:
+ // - this function is inaccessible to the outside due to the double
+ // module wrapping it. It is called exactly once by the C side via its
+ // unique name,
+ // - furthermore it is only called after `__{name}_init` has returned `0`
+ // (which delegates to `__init`).
+ unsafe {{ __exit() }}
+ }}
- #[cfg(not(MODULE))]
- #[doc(hidden)]
- #[no_mangle]
- pub extern \"C\" fn __{name}_exit() {{
- __exit()
- }}
+ /// # Safety
+ ///
+ /// This function must only be called once.
+ 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` has been called and returned `0`.
+ 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: 9ffe2a730313f27cebd0859ea856247ac59c576c
--
2.44.0
On Mon, 1 Apr 2024 at 15:52, Benno Lossin <[email protected]> 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: [email protected]
> Closes: https://github.com/Rust-for-Linux/linux/issues/629
> Fixes: 1fbde52bde73 ("rust: add `macros` crate")
> Signed-off-by: Benno Lossin <[email protected]>
Reviewed-by: Wedson Almeida Filho <[email protected]>
On Mon, Apr 01, 2024 at 06:52:50PM +0000, Benno Lossin wrote:
[...]
> + // 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 _)
While we're at it, probably we want the following as well? I.e. using
`Opaque` and extern block, because __this_module is certainly something
interior mutable and !Unpin.
diff --git a/rust/macros/module.rs b/rust/macros/module.rs
index 293beca0a583..8aa4eed6578c 100644
--- a/rust/macros/module.rs
+++ b/rust/macros/module.rs
@@ -219,7 +219,11 @@ mod __module_init {{
// 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 _)
+ extern \"C\" {{
+ static __this_module: kernel::types::Opaque<kernel::bindings::module>;
+ }}
+
+ kernel::ThisModule::from_ptr(__this_module.get())
}};
#[cfg(not(MODULE))]
static THIS_MODULE: kernel::ThisModule = unsafe {{
Thoughts?
Note this requires `Opaque::get` to be `const`, which I will send out
shortly, I think it's a good change regardless of the usage here.
Regards,
Boqun
> + }};
> + #[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 {{
> + // SAFETY: this function is inaccessible to the outside due to the double
> + // module wrapping it. It is called exactly once by the C side via its
> + // unique name.
> + unsafe {{ __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() {{
> + // SAFETY:
> + // - this function is inaccessible to the outside due to the double
> + // module wrapping it. It is called exactly once by the C side via its
> + // unique name,
> + // - furthermore it is only called after `init_module` has returned `0`
> + // (which delegates to `__init`).
> + unsafe {{ __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 {{
> + // SAFETY: this function is inaccessible to the outside due to the double
> + // module wrapping it. It is called exactly once by the C side via its
> + // placement above in the initcall section.
> + unsafe {{ __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() {{
> + // SAFETY:
> + // - this function is inaccessible to the outside due to the double
> + // module wrapping it. It is called exactly once by the C side via its
> + // unique name,
> + // - furthermore it is only called after `__{name}_init` has returned `0`
> + // (which delegates to `__init`).
> + unsafe {{ __exit() }}
> + }}
>
> - #[cfg(not(MODULE))]
> - #[doc(hidden)]
> - #[no_mangle]
> - pub extern \"C\" fn __{name}_exit() {{
> - __exit()
> - }}
> + /// # Safety
> + ///
> + /// This function must only be called once.
> + 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` has been called and returned `0`.
> + 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: 9ffe2a730313f27cebd0859ea856247ac59c576c
> --
> 2.44.0
>
>
On 01.04.24 23:10, Boqun Feng wrote:
> On Mon, Apr 01, 2024 at 06:52:50PM +0000, Benno Lossin wrote:
> [...]
>> + // 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 _)
>
> While we're at it, probably we want the following as well? I.e. using
> `Opaque` and extern block, because __this_module is certainly something
> interior mutable and !Unpin.
>
> diff --git a/rust/macros/module.rs b/rust/macros/module.rs
> index 293beca0a583..8aa4eed6578c 100644
> --- a/rust/macros/module.rs
> +++ b/rust/macros/module.rs
> @@ -219,7 +219,11 @@ mod __module_init {{
> // 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 _)
> + extern \"C\" {{
> + static __this_module: kernel::types::Opaque<kernel::bindings::module>;
> + }}
> +
> + kernel::ThisModule::from_ptr(__this_module.get())
> }};
> #[cfg(not(MODULE))]
> static THIS_MODULE: kernel::ThisModule = unsafe {{
>
> Thoughts?
I am not sure we need it. Bindgen generates
extern "C" {
pub static mut __this_module: module;
}
And the `mut` should take care of the "it might be modified by other
threads".
The only thing that sticks out to me is the borrow, it should probably
be using `addr_of_mut!` instead. Then we don't need to re-import it
again manually.
I think it should be a separate patch though.
--
Cheers,
Benno
>
> Note this requires `Opaque::get` to be `const`, which I will send out
> shortly, I think it's a good change regardless of the usage here.
>
> Regards,
> Boqun
>
On Mon, Apr 01, 2024 at 10:01:34PM +0000, Benno Lossin wrote:
> On 01.04.24 23:10, Boqun Feng wrote:
> > On Mon, Apr 01, 2024 at 06:52:50PM +0000, Benno Lossin wrote:
> > [...]
> >> + // 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 _)
> >
> > While we're at it, probably we want the following as well? I.e. using
> > `Opaque` and extern block, because __this_module is certainly something
> > interior mutable and !Unpin.
> >
> > diff --git a/rust/macros/module.rs b/rust/macros/module.rs
> > index 293beca0a583..8aa4eed6578c 100644
> > --- a/rust/macros/module.rs
> > +++ b/rust/macros/module.rs
> > @@ -219,7 +219,11 @@ mod __module_init {{
> > // 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 _)
> > + extern \"C\" {{
> > + static __this_module: kernel::types::Opaque<kernel::bindings::module>;
> > + }}
> > +
> > + kernel::ThisModule::from_ptr(__this_module.get())
> > }};
> > #[cfg(not(MODULE))]
> > static THIS_MODULE: kernel::ThisModule = unsafe {{
> >
> > Thoughts?
>
> I am not sure we need it. Bindgen generates
>
> extern "C" {
> pub static mut __this_module: module;
> }
>
> And the `mut` should take care of the "it might be modified by other
> threads".
Hmm.. but there could a C thread modifies some field of __this_module
while Rust code uses it, e.g. struct module has a list_head in it, which
could be used by C code to put another module next to it.
> The only thing that sticks out to me is the borrow, it should probably
> be using `addr_of_mut!` instead. Then we don't need to re-import it
> again manually.
>
> I think it should be a separate patch though.
>
Yes, agreed.
Regards,
Boqun
> --
> Cheers,
> Benno
>
> >
> > Note this requires `Opaque::get` to be `const`, which I will send out
> > shortly, I think it's a good change regardless of the usage here.
> >
> > Regards,
> > Boqun
> >
>
On 02.04.24 00:17, Boqun Feng wrote:
> On Mon, Apr 01, 2024 at 10:01:34PM +0000, Benno Lossin wrote:
>> On 01.04.24 23:10, Boqun Feng wrote:
>>> On Mon, Apr 01, 2024 at 06:52:50PM +0000, Benno Lossin wrote:
>>> [...]
>>>> + // 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 _)
>>>
>>> While we're at it, probably we want the following as well? I.e. using
>>> `Opaque` and extern block, because __this_module is certainly something
>>> interior mutable and !Unpin.
>>>
>>> diff --git a/rust/macros/module.rs b/rust/macros/module.rs
>>> index 293beca0a583..8aa4eed6578c 100644
>>> --- a/rust/macros/module.rs
>>> +++ b/rust/macros/module.rs
>>> @@ -219,7 +219,11 @@ mod __module_init {{
>>> // 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 _)
>>> + extern \"C\" {{
>>> + static __this_module: kernel::types::Opaque<kernel::bindings::module>;
>>> + }}
>>> +
>>> + kernel::ThisModule::from_ptr(__this_module.get())
>>> }};
>>> #[cfg(not(MODULE))]
>>> static THIS_MODULE: kernel::ThisModule = unsafe {{
>>>
>>> Thoughts?
>>
>> I am not sure we need it. Bindgen generates
>>
>> extern "C" {
>> pub static mut __this_module: module;
>> }
>>
>> And the `mut` should take care of the "it might be modified by other
>> threads".
>
> Hmm.. but there could a C thread modifies some field of __this_module
> while Rust code uses it, e.g. struct module has a list_head in it, which
> could be used by C code to put another module next to it.
This still should not be a problem, since we never actually read or
write to the mutable static. The only thing we are doing is taking its
address. `addr_of_mut!` should be sufficient. (AFAIK `static mut` is
designed such that it can be mutated at any time by any thread. Maybe
Gary knows more?)
--
Cheers,
Benno
On Mon, Apr 1, 2024 at 8:53 PM Benno Lossin <[email protected]> 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: [email protected]
> Closes: https://github.com/Rust-for-Linux/linux/issues/629
> Fixes: 1fbde52bde73 ("rust: add `macros` crate")
> Signed-off-by: Benno Lossin <[email protected]>
[ Capitalized comments, avoided newline in non-list SAFETY comments
and reworded to add Reported-by and newline. ]
Applied to `rust-fixes` -- thanks everyone!
Cheers,
Miguel
On Sun, Apr 07, 2024 at 10:02:35PM +0200, Miguel Ojeda wrote:
> On Mon, Apr 1, 2024 at 8:53 PM Benno Lossin <[email protected]> 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: [email protected]
> > Closes: https://github.com/Rust-for-Linux/linux/issues/629
> > Fixes: 1fbde52bde73 ("rust: add `macros` crate")
> > Signed-off-by: Benno Lossin <[email protected]>
>
> [ 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.20to.20use.20THIS_MODULE.20with.20.22.20rust.3A.20macros.3A.20fix.20soundness.20.2E.22/near/433512583
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
On Tue, Apr 16, 2024 at 7:07 PM Boqun Feng <[email protected]> wrote:
>
> As reported by Dirk Behme:
>
> https://rust-for-linux.zulipchat.com/#narrow/stream/291565-Help/topic/How.20to.20use.20THIS_MODULE.20with.20.22.20rust.3A.20macros.3A.20fix.20soundness.20.2E.22/near/433512583
>
> 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.
Fixed in `rust-fixes` now.
[ Moved `THIS_MODULE` out of the private-in-private modules since it
should remain public, as Dirk Behme noticed [1]. Capitalized comments,
avoided newline in non-list SAFETY comments and reworded to add
Reported-by and newline. ]
Link: https://rust-for-linux.zulipchat.com/#narrow/stream/291565-Help/topic/x/near/433512583
[1]
Cheers,
Miguel