Re: [PATCH v2] rust: macros: fix soundness issue in `module!` macro

[Benno Lossin]

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
>