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]>
---
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
--
2.44.0
On Wed, 27 Mar 2024 at 13:04, 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]>
> ---
> 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 <[email protected]> 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?
--
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 Sun, 31 Mar 2024 at 07:27, Benno Lossin <[email protected]> wrote:
>
> On 31.03.24 03:00, Wedson Almeida Filho wrote:
> > On Wed, 27 Mar 2024 at 13:04, Benno Lossin <[email protected]> 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 <[email protected]> wrote:
>>
>> On 31.03.24 03:00, Wedson Almeida Filho wrote:
>>> On Wed, 27 Mar 2024 at 13:04, Benno Lossin <[email protected]> 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.
--
Cheers,
Benno