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

[Benno Lossin <benno.lossin@proton.me>]

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