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
