Message-Id: <DAU0J3T0IEVM.2K7ZRQOVOHF8H@kernel.org>
Date: Mon, 23 Jun 2025 17:20:30 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Andreas Hindborg" <a.hindborg@...nel.org>
Cc: "Miguel Ojeda" <ojeda@...nel.org>, "Alex Gaynor"
 <alex.gaynor@...il.com>, "Boqun Feng" <boqun.feng@...il.com>, "Gary Guo"
 <gary@...yguo.net>, Björn Roy Baron
 <bjorn3_gh@...tonmail.com>, "Alice Ryhl" <aliceryhl@...gle.com>, "Masahiro
 Yamada" <masahiroy@...nel.org>, "Nathan Chancellor" <nathan@...nel.org>,
 "Luis Chamberlain" <mcgrof@...nel.org>, "Danilo Krummrich"
 <dakr@...nel.org>, "Nicolas Schier" <nicolas.schier@...ux.dev>, "Trevor
 Gross" <tmgross@...ch.edu>, "Adam Bratschi-Kaye" <ark.email@...il.com>,
 <rust-for-linux@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
 <linux-kbuild@...r.kernel.org>, "Petr Pavlu" <petr.pavlu@...e.com>, "Sami
 Tolvanen" <samitolvanen@...gle.com>, "Daniel Gomez" <da.gomez@...sung.com>,
 "Simona Vetter" <simona.vetter@...ll.ch>, "Greg KH"
 <gregkh@...uxfoundation.org>, "Fiona Behrens" <me@...enk.dev>, "Daniel
 Almeida" <daniel.almeida@...labora.com>, <linux-modules@...r.kernel.org>
Subject: Re: [PATCH v13 2/6] rust: introduce module_param module

On Mon Jun 23, 2025 at 4:31 PM CEST, Andreas Hindborg wrote:
> "Benno Lossin" <lossin@...nel.org> writes:
>
>> On Mon Jun 23, 2025 at 11:44 AM CEST, Andreas Hindborg wrote:
>>> "Benno Lossin" <lossin@...nel.org> writes:
>>>
>>>> On Fri Jun 20, 2025 at 1:29 PM CEST, Andreas Hindborg wrote:
>>>>> "Benno Lossin" <lossin@...nel.org> writes:
>>>>>> On Thu Jun 12, 2025 at 3:40 PM CEST, Andreas Hindborg wrote:
>>>>>>> +/// A wrapper for kernel parameters.
>>>>>>> +///
>>>>>>> +/// This type is instantiated by the [`module!`] macro when module parameters are
>>>>>>> +/// defined. You should never need to instantiate this type directly.
>>>>>>> +///
>>>>>>> +/// Note: This type is `pub` because it is used by module crates to access
>>>>>>> +/// parameter values.
>>>>>>> +#[repr(transparent)]
>>>>>>> +pub struct ModuleParamAccess<T> {
>>>>>>> +    data: core::cell::UnsafeCell<T>,
>>>>>>> +}
>>>>>>> +
>>>>>>> +// SAFETY: We only create shared references to the contents of this container,
>>>>>>> +// so if `T` is `Sync`, so is `ModuleParamAccess`.
>>>>>>> +unsafe impl<T: Sync> Sync for ModuleParamAccess<T> {}
>>>>>>> +
>>>>>>> +impl<T> ModuleParamAccess<T> {
>>>>>>> +    #[doc(hidden)]
>>>>>>> +    pub const fn new(value: T) -> Self {
>>>>>>> +        Self {
>>>>>>> +            data: core::cell::UnsafeCell::new(value),
>>>>>>> +        }
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    /// Get a shared reference to the parameter value.
>>>>>>> +    // Note: When sysfs access to parameters are enabled, we have to pass in a
>>>>>>> +    // held lock guard here.
>>>>>>> +    pub fn get(&self) -> &T {
>>>>>>> +        // SAFETY: As we only support read only parameters with no sysfs
>>>>>>> +        // exposure, the kernel will not touch the parameter data after module
>>>>>>> +        // initialization.
>>>>>>
>>>>>> This should be a type invariant. But I'm having difficulty defining one
>>>>>> that's actually correct: after parsing the parameter, this is written
>>>>>> to, but when is that actually?
>>>>>
>>>>> For built-in modules it is during kernel initialization. For loadable
>>>>> modules, it during module load. No code from the module will execute
>>>>> before parameters are set.
>>>>
>>>> Gotcha and there never ever will be custom code that is executed
>>>> before/during parameter setting (so code aside from code in `kernel`)?
>>>>
>>>>>> Would we eventually execute other Rust
>>>>>> code during that time? (for example when we allow custom parameter
>>>>>> parsing)
>>>>>
>>>>> I don't think we will need to synchronize because of custom parameter
>>>>> parsing. Parameters are initialized sequentially. It is not a problem if
>>>>> the custom parameter parsing code name other parameters, because they
>>>>> are all initialized to valid values (as they are statics).
>>>>
>>>> If you have `&'static i64`, then the value at that reference is never
>>>> allowed to change.
>>>>
>>>>>> This function also must never be `const` because of the following:
>>>>>>
>>>>>>     module! {
>>>>>>         // ...
>>>>>>         params: {
>>>>>>             my_param: i64 {
>>>>>>                 default: 0,
>>>>>>                 description: "",
>>>>>>             },
>>>>>>         },
>>>>>>     }
>>>>>>
>>>>>>     static BAD: &'static i64 = module_parameters::my_param.get();
>>>>>>
>>>>>> AFAIK, this static will be executed before loading module parameters and
>>>>>> thus it makes writing to the parameter UB.
>>>>>
>>>>> As I understand, the static will be initialized by a constant expression
>>>>> evaluated at compile time. I am not sure what happens when this is
>>>>> evaluated in const context:
>>>>>
>>>>>     pub fn get(&self) -> &T {
>>>>>         // SAFETY: As we only support read only parameters with no sysfs
>>>>>         // exposure, the kernel will not touch the parameter data after module
>>>>>         // initialization.
>>>>>         unsafe { &*self.data.get() }
>>>>>     }
>>>>>
>>>>> Why would that not be OK? I would assume the compiler builds a dependency graph
>>>>> when initializing statics?
>>>>
>>>> Yes it builds a dependency graph, but that is irrelevant? The problem is
>>>> that I can create a `'static` reference to the inner value *before* the
>>>> parameter is written-to (as the static is initialized before the
>>>> parameters).
>>>
>>> I see, I did not consider this situation. Thanks for pointing this out.
>>>
>>> Could we get around this without a lock maybe? If we change
>>> `ModuleParamAccess::get` to take a closure instead:
>>>
>>>     /// Call `func` with a reference to the parameter value stored in `Self`.
>>>     pub fn read(&self, func: impl FnOnce(&T)) {
>>>         // SAFETY: As we only support read only parameters with no sysfs
>>>         // exposure, the kernel will not touch the parameter data after module
>>>         // initialization.
>>>         let data = unsafe { &*self.data.get() };
>>>
>>>         func(data)
>>>     }
>>>
>>> I think this would bound the lifetime of the reference passed to the
>>> closure to the duration of the call, right?
>>
>> Yes that is correct. Now you can't assign the reference to a static.
>> However, this API is probably very clunky to use, since you always have
>> to create a closure etc.
>>
>> Since you mentioned in the other reply that one could spin up a thread
>> and do something simultaneously, I don't think this is enough. You could
>> have a loop spin over the new `read` function and read the value and
>> then the write happens.
>
> Yes you are right, we have to treat it as if it could be written at any
> point in time.
>
>> One way to fix this issue would be to use atomics to read the value and
>> to not create a reference to it. So essentially have
>>
>>     pub fn read(&self) -> T {
>>         unsafe { atomic_read_unsafe_cell(&self.data) }
>>     }
>
> That could work.
>
>> Another way would be to use a `Once`-like type (does that exist on the C
>> side?) so a type that can be initialized once and then never changes.
>> While it doesn't have a value set, we return some default value for the
>> param and print a warning, when it's set, we just return the value. But
>> this probably also requires atomics...
>
> I think atomic bool is not that far away. Either that, or we can lock.
>
>> Is parameter accessing used that often in hot paths? Can't you just copy
>> the value into your `Module` struct?
>
> I don't imagine this being read in a hot path. If so, the user could
> make a copy.

That's good to know, then let's try to go for something simple.

I don't think that we can just use a `Mutex<T>`, because we don't have a
way to create it at const time... I guess we could have

    impl<T> Mutex<T>
        /// # Safety
        ///
        /// The returned value needs to be pinned and then `init` needs
        /// to be called before any other methods are called on this.
        pub unsafe const fn const_new() -> Self;

        pub unsafe fn init(&self);
    }

But that seems like a bad idea, because where would we call the `init`
function? That also needs to be synchronized...

Maybe we can just like you said use an atomic bool?

---
Cheers,
Benno

Powered by blists - more mailing lists