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

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

[Benno Lossin]

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>
---
v1: https://lore.kernel.org/rust-for-linux/20240327160346.22442-1-benno.lossin@proton.me/
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

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

[Wedson Almeida Filho]

On Mon, 1 Apr 2024 at 15:52, 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>

Reviewed-by: Wedson Almeida Filho <walmeida@microsoft.com>

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

[Boqun Feng]

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
> 
> 

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
> 

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

[Boqun Feng]

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
> > 
> 

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

[Benno Lossin]

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

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

[Miguel Ojeda]

On Mon, Apr 1, 2024 at 8:53 PM 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>

[ 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

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

[Boqun Feng]

On Sun, Apr 07, 2024 at 10:02:35PM +0200, Miguel Ojeda wrote:
> On Mon, Apr 1, 2024 at 8:53 PM 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>
> 
> [ 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

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

[Miguel Ojeda]

On Tue, Apr 16, 2024 at 7:07 PM Boqun Feng <boqun.feng@gmail.com> 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