[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20fcbbd0-4a7a-49b1-a383-f8b388153066@proton.me>
Date: Mon, 01 Apr 2024 22:01:34 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Boqun Feng <boqun.feng@...il.com>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, Wedson Almeida Filho <wedsonaf@...il.com>, Gary Guo <gary@...yguo.net>, Björn Roy Baron <bjorn3_gh@...tonmail.com>, Andreas Hindborg <a.hindborg@...sung.com>, Alice Ryhl <aliceryhl@...gle.com>, Martin Rodriguez Reboredo <yakoyoku@...il.com>, Asahi Lina <lina@...hilina.net>, Sumera Priyadarsini <sylphrenadin@...il.com>, Neal Gompa <neal@...pa.dev>, Thomas Bertschinger <tahbertschinger@...il.com>, Andrea Righi <andrea.righi@...onical.com>, Matthew Bakhtiari <dev@...k.me>, Adam Bratschi-Kaye <ark.email@...il.com>, stable@...r.kernel.org, Masahiro Yamada <masahiroy@...nel.org>, Wedson Almeida Filho <wedsonaf@...gle.com>, Finn Behrens <me@...enk.dev>, rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] rust: macros: fix soundness issue in `module!` macro
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
>
Powered by blists - more mailing lists