* [PATCH] rust: macros: fix soundness issue in `module!` macro
@ 2024-03-27 16:04 Benno Lossin
2024-03-31 1:00 ` Wedson Almeida Filho
0 siblings, 1 reply; 5+ messages in thread
From: Benno Lossin @ 2024-03-27 16:04 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Alice Ryhl, Martin Rodriguez Reboredo, Asahi Lina, Eric Curtin,
Neal Gompa, Thomas Bertschinger, Andrea Righi,
Sumera Priyadarsini, Finn Behrens, Adam Bratschi-Kaye
Cc: stable, Daniel Xu, rust-for-linux, linux-kernel
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
--
2.44.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] rust: macros: fix soundness issue in `module!` macro
2024-03-27 16:04 [PATCH] rust: macros: fix soundness issue in `module!` macro Benno Lossin
@ 2024-03-31 1:00 ` Wedson Almeida Filho
2024-03-31 10:27 ` Benno Lossin
0 siblings, 1 reply; 5+ messages in thread
From: Wedson Almeida Filho @ 2024-03-31 1:00 UTC (permalink / raw)
To: Benno Lossin
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Andreas Hindborg, Alice Ryhl,
Martin Rodriguez Reboredo, Asahi Lina, Eric Curtin, Neal Gompa,
Thomas Bertschinger, Andrea Righi, Sumera Priyadarsini,
Finn Behrens, Adam Bratschi-Kaye, stable, Daniel Xu,
rust-for-linux, linux-kernel
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
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] rust: macros: fix soundness issue in `module!` macro
2024-03-31 1:00 ` Wedson Almeida Filho
@ 2024-03-31 10:27 ` Benno Lossin
2024-04-01 19:10 ` Wedson Almeida Filho
0 siblings, 1 reply; 5+ messages in thread
From: Benno Lossin @ 2024-03-31 10:27 UTC (permalink / raw)
To: Wedson Almeida Filho
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Andreas Hindborg, Alice Ryhl,
Martin Rodriguez Reboredo, Asahi Lina, Eric Curtin, Neal Gompa,
Thomas Bertschinger, Andrea Righi, Sumera Priyadarsini,
Finn Behrens, Adam Bratschi-Kaye, stable, Daniel Xu,
rust-for-linux, linux-kernel
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?
--
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
>>
>>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] rust: macros: fix soundness issue in `module!` macro
2024-03-31 10:27 ` Benno Lossin
@ 2024-04-01 19:10 ` Wedson Almeida Filho
2024-04-01 19:27 ` Benno Lossin
0 siblings, 1 reply; 5+ messages in thread
From: Wedson Almeida Filho @ 2024-04-01 19:10 UTC (permalink / raw)
To: Benno Lossin
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Andreas Hindborg, Alice Ryhl,
Martin Rodriguez Reboredo, Asahi Lina, Eric Curtin, Neal Gompa,
Thomas Bertschinger, Andrea Righi, Sumera Priyadarsini,
Finn Behrens, Adam Bratschi-Kaye, stable, Daniel Xu,
rust-for-linux, linux-kernel
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
> >>
> >>
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] rust: macros: fix soundness issue in `module!` macro
2024-04-01 19:10 ` Wedson Almeida Filho
@ 2024-04-01 19:27 ` Benno Lossin
0 siblings, 0 replies; 5+ messages in thread
From: Benno Lossin @ 2024-04-01 19:27 UTC (permalink / raw)
To: Wedson Almeida Filho
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Andreas Hindborg, Alice Ryhl,
Martin Rodriguez Reboredo, Asahi Lina, Eric Curtin, Neal Gompa,
Thomas Bertschinger, Andrea Righi, Sumera Priyadarsini,
Finn Behrens, Adam Bratschi-Kaye, stable, Daniel Xu,
rust-for-linux, linux-kernel
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.
--
Cheers,
Benno
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-04-01 19:28 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-03-27 16:04 [PATCH] rust: macros: fix soundness issue in `module!` macro Benno Lossin
2024-03-31 1:00 ` Wedson Almeida Filho
2024-03-31 10:27 ` Benno Lossin
2024-04-01 19:10 ` Wedson Almeida Filho
2024-04-01 19:27 ` Benno Lossin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox