Message-Id: <DATW0XWNN45X.1L2WMZ41JJ5O8@kernel.org>
Date: Mon, 23 Jun 2025 13:48:42 +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 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.

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

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

Is parameter accessing used that often in hot paths? Can't you just copy
the value into your `Module` struct?

---
Cheers,
Benno

Powered by blists - more mailing lists