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

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?