Message-ID: <877c126bce.fsf@kernel.org>
Date: Mon, 23 Jun 2025 11:44:49 +0200
From: Andreas Hindborg <a.hindborg@...nel.org>
To: "Benno Lossin" <lossin@...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

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


Best regards,
Andreas Hindborg

Powered by blists - more mailing lists