Add types and traits for interfacing the C moduleparam API.
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
rust/kernel/lib.rs | 1 +
rust/kernel/module_param.rs | 201 ++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 202 insertions(+)
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 6b4774b2b1c3..2b439ea06185 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -87,6 +87,7 @@
pub mod list;
pub mod miscdevice;
pub mod mm;
+pub mod module_param;
#[cfg(CONFIG_NET)]
pub mod net;
pub mod of;
diff --git a/rust/kernel/module_param.rs b/rust/kernel/module_param.rs
new file mode 100644
index 000000000000..fd167df8e53d
--- /dev/null
+++ b/rust/kernel/module_param.rs
@@ -0,0 +1,201 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Support for module parameters.
+//!
+//! C header: [`include/linux/moduleparam.h`](srctree/include/linux/moduleparam.h)
+
+use crate::prelude::*;
+use crate::str::BStr;
+
+/// Newtype to make `bindings::kernel_param` [`Sync`].
+#[repr(transparent)]
+#[doc(hidden)]
+pub struct RacyKernelParam(pub ::kernel::bindings::kernel_param);
+
+// SAFETY: C kernel handles serializing access to this type. We never access it
+// from Rust module.
+unsafe impl Sync for RacyKernelParam {}
+
+/// Types that can be used for module parameters.
+pub trait ModuleParam: Sized + Copy {
+ /// The [`ModuleParam`] will be used by the kernel module through this type.
+ ///
+ /// This may differ from `Self` if, for example, `Self` needs to track
+ /// ownership without exposing it or allocate extra space for other possible
+ /// parameter values.
+ // This is required to support string parameters in the future.
+ type Value: ?Sized;
+
+ /// Parse a parameter argument into the parameter value.
+ ///
+ /// `Err(_)` should be returned when parsing of the argument fails.
+ ///
+ /// Parameters passed at boot time will be set before [`kmalloc`] is
+ /// available (even if the module is loaded at a later time). However, in
+ /// this case, the argument buffer will be valid for the entire lifetime of
+ /// the kernel. So implementations of this method which need to allocate
+ /// should first check that the allocator is available (with
+ /// [`crate::bindings::slab_is_available`]) and when it is not available
+ /// provide an alternative implementation which doesn't allocate. In cases
+ /// where the allocator is not available it is safe to save references to
+ /// `arg` in `Self`, but in other cases a copy should be made.
+ ///
+ /// [`kmalloc`]: srctree/include/linux/slab.h
+ fn try_from_param_arg(arg: &'static BStr) -> Result<Self>;
+}
+
+/// Set the module parameter from a string.
+///
+/// Used to set the parameter value at kernel initialization, when loading
+/// the module or when set through `sysfs`.
+///
+/// See `struct kernel_param_ops.set`.
+///
+/// # Safety
+///
+/// - If `val` is non-null then it must point to a valid null-terminated string that must be valid
+/// for reads for the duration of the call.
+/// - `parm` must be a pointer to a `bindings::kernel_param` that is valid for reads for the
+/// duration of the call.
+/// - `param.arg` must be a pointer to an initialized `T` that is valid for writes for the duration
+/// of the function.
+///
+/// # Note
+///
+/// - The safety requirements are satisfied by C API contract when this function is invoked by the
+/// module subsystem C code.
+/// - Currently, we only support read-only parameters that are not readable from `sysfs`. Thus, this
+/// function is only called at kernel initialization time, or at module load time, and we have
+/// exclusive access to the parameter for the duration of the function.
+///
+/// [`module!`]: macros::module
+unsafe extern "C" fn set_param<T>(
+ val: *const c_char,
+ param: *const crate::bindings::kernel_param,
+) -> c_int
+where
+ T: ModuleParam,
+{
+ // NOTE: If we start supporting arguments without values, val _is_ allowed
+ // to be null here.
+ if val.is_null() {
+ // TODO: Use pr_warn_once available.
+ crate::pr_warn!("Null pointer passed to `module_param::set_param`");
+ return EINVAL.to_errno();
+ }
+
+ // SAFETY: By function safety requirement, val is non-null, null-terminated
+ // and valid for reads for the duration of this function.
+ let arg = unsafe { CStr::from_char_ptr(val) };
+
+ crate::error::from_result(|| {
+ let new_value = T::try_from_param_arg(arg)?;
+
+ // SAFETY: By function safety requirements `param` is be valid for reads.
+ let old_value = unsafe { (*param).__bindgen_anon_1.arg as *mut T };
+
+ // SAFETY: By function safety requirements, the target of `old_value` is valid for writes
+ // and is initialized.
+ unsafe { *old_value = new_value };
+ Ok(0)
+ })
+}
+
+macro_rules! impl_int_module_param {
+ ($ty:ident) => {
+ impl ModuleParam for $ty {
+ type Value = $ty;
+
+ fn try_from_param_arg(arg: &'static BStr) -> Result<Self> {
+ <$ty as crate::str::parse_int::ParseInt>::from_str(arg)
+ }
+ }
+ };
+}
+
+impl_int_module_param!(i8);
+impl_int_module_param!(u8);
+impl_int_module_param!(i16);
+impl_int_module_param!(u16);
+impl_int_module_param!(i32);
+impl_int_module_param!(u32);
+impl_int_module_param!(i64);
+impl_int_module_param!(u64);
+impl_int_module_param!(isize);
+impl_int_module_param!(usize);
+
+/// 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.
+ unsafe { &*self.data.get() }
+ }
+
+ /// Get a mutable pointer to the parameter value.
+ pub const fn as_mut_ptr(&self) -> *mut T {
+ self.data.get()
+ }
+}
+
+#[doc(hidden)]
+#[macro_export]
+/// Generate a static [`kernel_param_ops`](srctree/include/linux/moduleparam.h) struct.
+///
+/// # Examples
+///
+/// ```ignore
+/// make_param_ops!(
+/// /// Documentation for new param ops.
+/// PARAM_OPS_MYTYPE, // Name for the static.
+/// MyType // A type which implements [`ModuleParam`].
+/// );
+/// ```
+macro_rules! make_param_ops {
+ ($ops:ident, $ty:ty) => {
+ #[doc(hidden)]
+ pub static $ops: $crate::bindings::kernel_param_ops = $crate::bindings::kernel_param_ops {
+ flags: 0,
+ set: Some(set_param::<$ty>),
+ get: None,
+ free: None,
+ };
+ };
+}
+
+make_param_ops!(PARAM_OPS_I8, i8);
+make_param_ops!(PARAM_OPS_U8, u8);
+make_param_ops!(PARAM_OPS_I16, i16);
+make_param_ops!(PARAM_OPS_U16, u16);
+make_param_ops!(PARAM_OPS_I32, i32);
+make_param_ops!(PARAM_OPS_U32, u32);
+make_param_ops!(PARAM_OPS_I64, i64);
+make_param_ops!(PARAM_OPS_U64, u64);
+make_param_ops!(PARAM_OPS_ISIZE, isize);
+make_param_ops!(PARAM_OPS_USIZE, usize);
--
2.47.2On 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? Would we eventually execute other Rust
code during that time? (for example when we allow custom parameter
parsing)
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.
So maybe we should just use some sort of synchronization tool here...
---
Cheers,
Benno
> + unsafe { &*self.data.get() }
> + }
> +
> + /// Get a mutable pointer to the parameter value.
> + pub const fn as_mut_ptr(&self) -> *mut T {
> + self.data.get()
> + }
> +}
"Benno Lossin" <lossin@kernel.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.
> 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).
>
> 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?
Best regards,
Andreas Hindborg
On Fri Jun 20, 2025 at 1:29 PM CEST, Andreas Hindborg wrote:
> "Benno Lossin" <lossin@kernel.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).
---
Cheers,
Benno
"Benno Lossin" <lossin@kernel.org> writes:
> On Fri Jun 20, 2025 at 1:29 PM CEST, Andreas Hindborg wrote:
>> "Benno Lossin" <lossin@kernel.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`)?
Not with the parameter parsers we provide now. In the case of custom
parsing code, I suppose there is nothing preventing the parsing code
from spinning up a thread that could do stuff while more parameters are
initialized by the kernel.
Best regards,
Andreas Hindborg
"Benno Lossin" <lossin@kernel.org> writes:
> On Fri Jun 20, 2025 at 1:29 PM CEST, Andreas Hindborg wrote:
>> "Benno Lossin" <lossin@kernel.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
On Mon Jun 23, 2025 at 11:44 AM CEST, Andreas Hindborg wrote:
> "Benno Lossin" <lossin@kernel.org> writes:
>
>> On Fri Jun 20, 2025 at 1:29 PM CEST, Andreas Hindborg wrote:
>>> "Benno Lossin" <lossin@kernel.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
"Benno Lossin" <lossin@kernel.org> writes:
> On Mon Jun 23, 2025 at 11:44 AM CEST, Andreas Hindborg wrote:
>> "Benno Lossin" <lossin@kernel.org> writes:
>>
>>> On Fri Jun 20, 2025 at 1:29 PM CEST, Andreas Hindborg wrote:
>>>> "Benno Lossin" <lossin@kernel.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.
Best regards,
Andreas Hindborg
On Mon Jun 23, 2025 at 4:31 PM CEST, Andreas Hindborg wrote:
> "Benno Lossin" <lossin@kernel.org> writes:
>
>> On Mon Jun 23, 2025 at 11:44 AM CEST, Andreas Hindborg wrote:
>>> "Benno Lossin" <lossin@kernel.org> writes:
>>>
>>>> On Fri Jun 20, 2025 at 1:29 PM CEST, Andreas Hindborg wrote:
>>>>> "Benno Lossin" <lossin@kernel.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
"Benno Lossin" <lossin@kernel.org> writes:
> On Mon Jun 23, 2025 at 4:31 PM CEST, Andreas Hindborg wrote:
>> "Benno Lossin" <lossin@kernel.org> writes:
>>
>>> On Mon Jun 23, 2025 at 11:44 AM CEST, Andreas Hindborg wrote:
>>>> "Benno Lossin" <lossin@kernel.org> writes:
>>>>
>>>>> On Fri Jun 20, 2025 at 1:29 PM CEST, Andreas Hindborg wrote:
>>>>>> "Benno Lossin" <lossin@kernel.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...
Ah, that is unfortunate. The init function will not run before this, so
we would need a `Once` or an atomic anyway to initialize the lock.
I am not sure if we are allowed to sleep during this, I would have to
check. But then we could use a spin lock.
We will need the locking anyway, when we want to enable sysfs write
access to the parameters.
>
> Maybe we can just like you said use an atomic bool?
Sigh, I will have to check how far that series has come.
Best regards,
Andreas Hindborg
Andreas Hindborg <a.hindborg@kernel.org> writes:
> "Benno Lossin" <lossin@kernel.org> writes:
>
>> On Mon Jun 23, 2025 at 4:31 PM CEST, Andreas Hindborg wrote:
>>> "Benno Lossin" <lossin@kernel.org> writes:
>>>
>>>> On Mon Jun 23, 2025 at 11:44 AM CEST, Andreas Hindborg wrote:
>>>>> "Benno Lossin" <lossin@kernel.org> writes:
>>>>>
>>>>>> On Fri Jun 20, 2025 at 1:29 PM CEST, Andreas Hindborg wrote:
>>>>>>> "Benno Lossin" <lossin@kernel.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...
>
> Ah, that is unfortunate. The init function will not run before this, so
> we would need a `Once` or an atomic anyway to initialize the lock.
>
> I am not sure if we are allowed to sleep during this, I would have to
> check. But then we could use a spin lock.
>
> We will need the locking anyway, when we want to enable sysfs write
> access to the parameters.
>
>>
>> Maybe we can just like you said use an atomic bool?
>
> Sigh, I will have to check how far that series has come.
>
I think I am going to build some kind of `Once` feature on top of
Boqun's atomic series [1], so that we can initialize a lock in these
statics. We can't use `global_lock!`, because that depends on module
init to initialize the lock before first use.
As far as I can tell, atomics may not land in v6.17, so this series
will probably not be ready for merge until v6.18 at the earliest.
Thanks for the input, Benno!
Best regards,
Andreas Hindborg
[1] https://lore.kernel.org/all/20250618164934.19817-1-boqun.feng@gmail.com
On Fri Jun 27, 2025 at 9:57 AM CEST, Andreas Hindborg wrote:
> Andreas Hindborg <a.hindborg@kernel.org> writes:
>> "Benno Lossin" <lossin@kernel.org> writes:
>>> 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...
>>
>> Ah, that is unfortunate. The init function will not run before this, so
>> we would need a `Once` or an atomic anyway to initialize the lock.
>>
>> I am not sure if we are allowed to sleep during this, I would have to
>> check. But then we could use a spin lock.
>>
>> We will need the locking anyway, when we want to enable sysfs write
>> access to the parameters.
>>
>>>
>>> Maybe we can just like you said use an atomic bool?
>>
>> Sigh, I will have to check how far that series has come.
>>
>
> I think I am going to build some kind of `Once` feature on top of
> Boqun's atomic series [1], so that we can initialize a lock in these
> statics. We can't use `global_lock!`, because that depends on module
> init to initialize the lock before first use.
Sounds good, though we probably don't want to name it `Once`. Since it
is something that will be populated in the future, but not by some
random accessor, but rather a specific populator.
So maybe:
pub struct Delayed<T> {
dummy: T,
real: Opaque<T>,
populated: Atomic<bool>, // or Atomic<Flag>
writing: Atomic<bool>, // or Atomic<Flag>
}
impl<T> Delayed<T> {
pub fn new(dummy: T) -> Self {
Self {
dummy,
real: Opaque::uninit(),
populated: Atomic::new(false),
writing: Atomic::new(false),
}
}
pub fn get(&self) -> &T {
if self.populated.load(Acquire) {
unsafe { &*self.real.get() }
} else {
// maybe print a warning here?
// or maybe let the user configure this in `new()`?
&self.dummy
}
}
pub fn populate(&self, value: T) {
if self.writing.cmpxchg(false, true, Release) {
unsafe { *self.real.get() = value };
self.populated.store(true, Release);
} else {
pr_warn!("`Delayed<{}>` written to twice!\n", core::any::type_name::<T>());
}
}
}
(no idea if the orderings are correct, I always have to think way to
much about that... especially since our atomics seem to only take one
ordering in compare_exchange?)
> As far as I can tell, atomics may not land in v6.17, so this series
> will probably not be ready for merge until v6.18 at the earliest.
Yeah, sorry about that :(
> Thanks for the input, Benno!
My pleasure!
---
Cheers,
Benno
"Benno Lossin" <lossin@kernel.org> writes:
> On Fri Jun 27, 2025 at 9:57 AM CEST, Andreas Hindborg wrote:
>> Andreas Hindborg <a.hindborg@kernel.org> writes:
>>> "Benno Lossin" <lossin@kernel.org> writes:
>>>> 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...
>>>
>>> Ah, that is unfortunate. The init function will not run before this, so
>>> we would need a `Once` or an atomic anyway to initialize the lock.
>>>
>>> I am not sure if we are allowed to sleep during this, I would have to
>>> check. But then we could use a spin lock.
>>>
>>> We will need the locking anyway, when we want to enable sysfs write
>>> access to the parameters.
>>>
>>>>
>>>> Maybe we can just like you said use an atomic bool?
>>>
>>> Sigh, I will have to check how far that series has come.
>>>
>>
>> I think I am going to build some kind of `Once` feature on top of
>> Boqun's atomic series [1], so that we can initialize a lock in these
>> statics. We can't use `global_lock!`, because that depends on module
>> init to initialize the lock before first use.
>
> Sounds good, though we probably don't want to name it `Once`. Since it
> is something that will be populated in the future, but not by some
> random accessor, but rather a specific populator.
>
> So maybe:
>
> pub struct Delayed<T> {
> dummy: T,
> real: Opaque<T>,
> populated: Atomic<bool>, // or Atomic<Flag>
> writing: Atomic<bool>, // or Atomic<Flag>
> }
>
> impl<T> Delayed<T> {
> pub fn new(dummy: T) -> Self {
> Self {
> dummy,
> real: Opaque::uninit(),
> populated: Atomic::new(false),
> writing: Atomic::new(false),
> }
> }
>
> pub fn get(&self) -> &T {
> if self.populated.load(Acquire) {
> unsafe { &*self.real.get() }
> } else {
> // maybe print a warning here?
> // or maybe let the user configure this in `new()`?
> &self.dummy
> }
> }
>
> pub fn populate(&self, value: T) {
> if self.writing.cmpxchg(false, true, Release) {
> unsafe { *self.real.get() = value };
> self.populated.store(true, Release);
> } else {
> pr_warn!("`Delayed<{}>` written to twice!\n", core::any::type_name::<T>());
> }
> }
> }
>
> (no idea if the orderings are correct, I always have to think way to
> much about that... especially since our atomics seem to only take one
> ordering in compare_exchange?)
>
>> As far as I can tell, atomics may not land in v6.17, so this series
>> will probably not be ready for merge until v6.18 at the earliest.
>
> Yeah, sorry about that :(
Actually, perhaps we could aim at merging this code without this
synchronization? The lack of synchronization is only a problem if we
support custom parsing. This patch set does not allow custom parsing
code, so it does not suffer this issue.
Best regards,
Andreas Hindborg
On Mon Jun 30, 2025 at 1:18 PM CEST, Andreas Hindborg wrote:
> "Benno Lossin" <lossin@kernel.org> writes:
>> On Fri Jun 27, 2025 at 9:57 AM CEST, Andreas Hindborg wrote:
>>> Andreas Hindborg <a.hindborg@kernel.org> writes:
>>>> "Benno Lossin" <lossin@kernel.org> writes:
>>>>> 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...
>>>>
>>>> Ah, that is unfortunate. The init function will not run before this, so
>>>> we would need a `Once` or an atomic anyway to initialize the lock.
>>>>
>>>> I am not sure if we are allowed to sleep during this, I would have to
>>>> check. But then we could use a spin lock.
>>>>
>>>> We will need the locking anyway, when we want to enable sysfs write
>>>> access to the parameters.
>>>>
>>>>>
>>>>> Maybe we can just like you said use an atomic bool?
>>>>
>>>> Sigh, I will have to check how far that series has come.
>>>>
>>>
>>> I think I am going to build some kind of `Once` feature on top of
>>> Boqun's atomic series [1], so that we can initialize a lock in these
>>> statics. We can't use `global_lock!`, because that depends on module
>>> init to initialize the lock before first use.
>>
>> Sounds good, though we probably don't want to name it `Once`. Since it
>> is something that will be populated in the future, but not by some
>> random accessor, but rather a specific populator.
>>
>> So maybe:
>>
>> pub struct Delayed<T> {
>> dummy: T,
>> real: Opaque<T>,
>> populated: Atomic<bool>, // or Atomic<Flag>
>> writing: Atomic<bool>, // or Atomic<Flag>
>> }
>>
>> impl<T> Delayed<T> {
>> pub fn new(dummy: T) -> Self {
>> Self {
>> dummy,
>> real: Opaque::uninit(),
>> populated: Atomic::new(false),
>> writing: Atomic::new(false),
>> }
>> }
>>
>> pub fn get(&self) -> &T {
>> if self.populated.load(Acquire) {
>> unsafe { &*self.real.get() }
>> } else {
>> // maybe print a warning here?
>> // or maybe let the user configure this in `new()`?
>> &self.dummy
>> }
>> }
>>
>> pub fn populate(&self, value: T) {
>> if self.writing.cmpxchg(false, true, Release) {
>> unsafe { *self.real.get() = value };
>> self.populated.store(true, Release);
>> } else {
>> pr_warn!("`Delayed<{}>` written to twice!\n", core::any::type_name::<T>());
>> }
>> }
>> }
>>
>> (no idea if the orderings are correct, I always have to think way to
>> much about that... especially since our atomics seem to only take one
>> ordering in compare_exchange?)
>>
>>> As far as I can tell, atomics may not land in v6.17, so this series
>>> will probably not be ready for merge until v6.18 at the earliest.
>>
>> Yeah, sorry about that :(
>
> Actually, perhaps we could aim at merging this code without this
> synchronization?
I won't remember this issue in a few weeks and I fear that it will just
get buried. In fact, I already had to re-read now what the actual issue
was...
> The lack of synchronization is only a problem if we
> support custom parsing. This patch set does not allow custom parsing
> code, so it does not suffer this issue.
... In doing that, I saw my original example of UB:
module! {
// ...
params: {
my_param: i64 {
default: 0,
description: "",
},
},
}
static BAD: &'static i64 = module_parameters::my_param.get();
That can happen without custom parsing, so it's still a problem...
---
Cheers,
Benno
"Benno Lossin" <lossin@kernel.org> writes:
> On Mon Jun 30, 2025 at 1:18 PM CEST, Andreas Hindborg wrote:
>> "Benno Lossin" <lossin@kernel.org> writes:
>>> On Fri Jun 27, 2025 at 9:57 AM CEST, Andreas Hindborg wrote:
>>>> Andreas Hindborg <a.hindborg@kernel.org> writes:
>>>>> "Benno Lossin" <lossin@kernel.org> writes:
>>>>>> 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...
>>>>>
>>>>> Ah, that is unfortunate. The init function will not run before this, so
>>>>> we would need a `Once` or an atomic anyway to initialize the lock.
>>>>>
>>>>> I am not sure if we are allowed to sleep during this, I would have to
>>>>> check. But then we could use a spin lock.
>>>>>
>>>>> We will need the locking anyway, when we want to enable sysfs write
>>>>> access to the parameters.
>>>>>
>>>>>>
>>>>>> Maybe we can just like you said use an atomic bool?
>>>>>
>>>>> Sigh, I will have to check how far that series has come.
>>>>>
>>>>
>>>> I think I am going to build some kind of `Once` feature on top of
>>>> Boqun's atomic series [1], so that we can initialize a lock in these
>>>> statics. We can't use `global_lock!`, because that depends on module
>>>> init to initialize the lock before first use.
>>>
>>> Sounds good, though we probably don't want to name it `Once`. Since it
>>> is something that will be populated in the future, but not by some
>>> random accessor, but rather a specific populator.
>>>
>>> So maybe:
>>>
>>> pub struct Delayed<T> {
>>> dummy: T,
>>> real: Opaque<T>,
>>> populated: Atomic<bool>, // or Atomic<Flag>
>>> writing: Atomic<bool>, // or Atomic<Flag>
>>> }
>>>
>>> impl<T> Delayed<T> {
>>> pub fn new(dummy: T) -> Self {
>>> Self {
>>> dummy,
>>> real: Opaque::uninit(),
>>> populated: Atomic::new(false),
>>> writing: Atomic::new(false),
>>> }
>>> }
>>>
>>> pub fn get(&self) -> &T {
>>> if self.populated.load(Acquire) {
>>> unsafe { &*self.real.get() }
>>> } else {
>>> // maybe print a warning here?
>>> // or maybe let the user configure this in `new()`?
>>> &self.dummy
>>> }
>>> }
>>>
>>> pub fn populate(&self, value: T) {
>>> if self.writing.cmpxchg(false, true, Release) {
>>> unsafe { *self.real.get() = value };
>>> self.populated.store(true, Release);
>>> } else {
>>> pr_warn!("`Delayed<{}>` written to twice!\n", core::any::type_name::<T>());
>>> }
>>> }
>>> }
>>>
>>> (no idea if the orderings are correct, I always have to think way to
>>> much about that... especially since our atomics seem to only take one
>>> ordering in compare_exchange?)
>>>
>>>> As far as I can tell, atomics may not land in v6.17, so this series
>>>> will probably not be ready for merge until v6.18 at the earliest.
>>>
>>> Yeah, sorry about that :(
>>
>> Actually, perhaps we could aim at merging this code without this
>> synchronization?
>
> I won't remember this issue in a few weeks and I fear that it will just
> get buried. In fact, I already had to re-read now what the actual issue
> was...
>
>> The lack of synchronization is only a problem if we
>> support custom parsing. This patch set does not allow custom parsing
>> code, so it does not suffer this issue.
>
> ... In doing that, I saw my original example of UB:
>
> module! {
> // ...
> params: {
> my_param: i64 {
> default: 0,
> description: "",
> },
> },
> }
>
> static BAD: &'static i64 = module_parameters::my_param.get();
>
> That can happen without custom parsing, so it's still a problem...
Ah, got it. Thanks.
Best regards,
Andreas Hindborg
On Mon Jun 30, 2025 at 3:15 PM CEST, Andreas Hindborg wrote:
> "Benno Lossin" <lossin@kernel.org> writes:
>> On Mon Jun 30, 2025 at 1:18 PM CEST, Andreas Hindborg wrote:
>>> "Benno Lossin" <lossin@kernel.org> writes:
>>>> (no idea if the orderings are correct, I always have to think way to
>>>> much about that... especially since our atomics seem to only take one
>>>> ordering in compare_exchange?)
>>>>
>>>>> As far as I can tell, atomics may not land in v6.17, so this series
>>>>> will probably not be ready for merge until v6.18 at the earliest.
>>>>
>>>> Yeah, sorry about that :(
>>>
>>> Actually, perhaps we could aim at merging this code without this
>>> synchronization?
>>
>> I won't remember this issue in a few weeks and I fear that it will just
>> get buried. In fact, I already had to re-read now what the actual issue
>> was...
>>
>>> The lack of synchronization is only a problem if we
>>> support custom parsing. This patch set does not allow custom parsing
>>> code, so it does not suffer this issue.
>>
>> ... In doing that, I saw my original example of UB:
>>
>> module! {
>> // ...
>> params: {
>> my_param: i64 {
>> default: 0,
>> description: "",
>> },
>> },
>> }
>>
>> static BAD: &'static i64 = module_parameters::my_param.get();
>>
>> That can happen without custom parsing, so it's still a problem...
>
> Ah, got it. Thanks.
On second thought, we *could* just make the accessor function `unsafe`.
Of course with a pinky promise to make the implementation safe once
atomics land. But I think if it helps you get your driver faster along,
then we should do it.
---
Cheers,
Benno
"Benno Lossin" <lossin@kernel.org> writes:
> On Mon Jun 30, 2025 at 3:15 PM CEST, Andreas Hindborg wrote:
>> "Benno Lossin" <lossin@kernel.org> writes:
>>> On Mon Jun 30, 2025 at 1:18 PM CEST, Andreas Hindborg wrote:
>>>> "Benno Lossin" <lossin@kernel.org> writes:
>>>>> (no idea if the orderings are correct, I always have to think way to
>>>>> much about that... especially since our atomics seem to only take one
>>>>> ordering in compare_exchange?)
>>>>>
>>>>>> As far as I can tell, atomics may not land in v6.17, so this series
>>>>>> will probably not be ready for merge until v6.18 at the earliest.
>>>>>
>>>>> Yeah, sorry about that :(
>>>>
>>>> Actually, perhaps we could aim at merging this code without this
>>>> synchronization?
>>>
>>> I won't remember this issue in a few weeks and I fear that it will just
>>> get buried. In fact, I already had to re-read now what the actual issue
>>> was...
>>>
>>>> The lack of synchronization is only a problem if we
>>>> support custom parsing. This patch set does not allow custom parsing
>>>> code, so it does not suffer this issue.
>>>
>>> ... In doing that, I saw my original example of UB:
>>>
>>> module! {
>>> // ...
>>> params: {
>>> my_param: i64 {
>>> default: 0,
>>> description: "",
>>> },
>>> },
>>> }
>>>
>>> static BAD: &'static i64 = module_parameters::my_param.get();
>>>
>>> That can happen without custom parsing, so it's still a problem...
>>
>> Ah, got it. Thanks.
>
> On second thought, we *could* just make the accessor function `unsafe`.
> Of course with a pinky promise to make the implementation safe once
> atomics land. But I think if it helps you get your driver faster along,
> then we should do it.
No, I am OK for now with configfs.
But, progress is still great. How about if we add a copy accessor
instead for now, I think you proposed that a few million emails ago:
pub fn get(&self) -> T;
or maybe rename:
pub fn copy(&self) -> T;
Then we are fine safety wise for now, right? It is even sensible for
these `T: Copy` types.
Best regards,
Andreas Hindborg
On Tue Jul 1, 2025 at 10:43 AM CEST, Andreas Hindborg wrote:
> "Benno Lossin" <lossin@kernel.org> writes:
>> On Mon Jun 30, 2025 at 3:15 PM CEST, Andreas Hindborg wrote:
>>> "Benno Lossin" <lossin@kernel.org> writes:
>>>> On Mon Jun 30, 2025 at 1:18 PM CEST, Andreas Hindborg wrote:
>>>>> "Benno Lossin" <lossin@kernel.org> writes:
>>>>>> (no idea if the orderings are correct, I always have to think way to
>>>>>> much about that... especially since our atomics seem to only take one
>>>>>> ordering in compare_exchange?)
>>>>>>
>>>>>>> As far as I can tell, atomics may not land in v6.17, so this series
>>>>>>> will probably not be ready for merge until v6.18 at the earliest.
>>>>>>
>>>>>> Yeah, sorry about that :(
>>>>>
>>>>> Actually, perhaps we could aim at merging this code without this
>>>>> synchronization?
>>>>
>>>> I won't remember this issue in a few weeks and I fear that it will just
>>>> get buried. In fact, I already had to re-read now what the actual issue
>>>> was...
>>>>
>>>>> The lack of synchronization is only a problem if we
>>>>> support custom parsing. This patch set does not allow custom parsing
>>>>> code, so it does not suffer this issue.
>>>>
>>>> ... In doing that, I saw my original example of UB:
>>>>
>>>> module! {
>>>> // ...
>>>> params: {
>>>> my_param: i64 {
>>>> default: 0,
>>>> description: "",
>>>> },
>>>> },
>>>> }
>>>>
>>>> static BAD: &'static i64 = module_parameters::my_param.get();
>>>>
>>>> That can happen without custom parsing, so it's still a problem...
>>>
>>> Ah, got it. Thanks.
>>
>> On second thought, we *could* just make the accessor function `unsafe`.
>> Of course with a pinky promise to make the implementation safe once
>> atomics land. But I think if it helps you get your driver faster along,
>> then we should do it.
>
> No, I am OK for now with configfs.
>
> But, progress is still great. How about if we add a copy accessor
> instead for now, I think you proposed that a few million emails ago:
>
> pub fn get(&self) -> T;
>
> or maybe rename:
>
> pub fn copy(&self) -> T;
>
> Then we are fine safety wise for now, right? It is even sensible for
> these `T: Copy` types.
That is better than getting a reference, but still someone could read at
the same time that a write is happening (though we need some new
abstractions AFAIK?). But I fear that we forget about this issue,
because it'll be some time until we land parameters that are `!Copy` (if
at all...)
---
Cheers,
Benno
"Benno Lossin" <lossin@kernel.org> writes:
> On Tue Jul 1, 2025 at 10:43 AM CEST, Andreas Hindborg wrote:
>> "Benno Lossin" <lossin@kernel.org> writes:
>>> On Mon Jun 30, 2025 at 3:15 PM CEST, Andreas Hindborg wrote:
>>>> "Benno Lossin" <lossin@kernel.org> writes:
>>>>> On Mon Jun 30, 2025 at 1:18 PM CEST, Andreas Hindborg wrote:
>>>>>> "Benno Lossin" <lossin@kernel.org> writes:
>>>>>>> (no idea if the orderings are correct, I always have to think way to
>>>>>>> much about that... especially since our atomics seem to only take one
>>>>>>> ordering in compare_exchange?)
>>>>>>>
>>>>>>>> As far as I can tell, atomics may not land in v6.17, so this series
>>>>>>>> will probably not be ready for merge until v6.18 at the earliest.
>>>>>>>
>>>>>>> Yeah, sorry about that :(
>>>>>>
>>>>>> Actually, perhaps we could aim at merging this code without this
>>>>>> synchronization?
>>>>>
>>>>> I won't remember this issue in a few weeks and I fear that it will just
>>>>> get buried. In fact, I already had to re-read now what the actual issue
>>>>> was...
>>>>>
>>>>>> The lack of synchronization is only a problem if we
>>>>>> support custom parsing. This patch set does not allow custom parsing
>>>>>> code, so it does not suffer this issue.
>>>>>
>>>>> ... In doing that, I saw my original example of UB:
>>>>>
>>>>> module! {
>>>>> // ...
>>>>> params: {
>>>>> my_param: i64 {
>>>>> default: 0,
>>>>> description: "",
>>>>> },
>>>>> },
>>>>> }
>>>>>
>>>>> static BAD: &'static i64 = module_parameters::my_param.get();
>>>>>
>>>>> That can happen without custom parsing, so it's still a problem...
>>>>
>>>> Ah, got it. Thanks.
>>>
>>> On second thought, we *could* just make the accessor function `unsafe`.
>>> Of course with a pinky promise to make the implementation safe once
>>> atomics land. But I think if it helps you get your driver faster along,
>>> then we should do it.
>>
>> No, I am OK for now with configfs.
>>
>> But, progress is still great. How about if we add a copy accessor
>> instead for now, I think you proposed that a few million emails ago:
>>
>> pub fn get(&self) -> T;
>>
>> or maybe rename:
>>
>> pub fn copy(&self) -> T;
>>
>> Then we are fine safety wise for now, right? It is even sensible for
>> these `T: Copy` types.
>
> That is better than getting a reference, but still someone could read at
> the same time that a write is happening (though we need some new
> abstractions AFAIK?). But I fear that we forget about this issue,
> because it'll be some time until we land parameters that are `!Copy` (if
> at all...)
No, that could not happen when we are not allowing custom parsing or
sysfs access. Regarding forgetting, I already added a `NOTE` on `!Copy`,
and I would add one on this issue as well.
Best regards,
Andreas Hindborg
On Tue Jul 1, 2025 at 4:14 PM CEST, Andreas Hindborg wrote: > "Benno Lossin" <lossin@kernel.org> writes: >> On Tue Jul 1, 2025 at 10:43 AM CEST, Andreas Hindborg wrote: >>> No, I am OK for now with configfs. >>> >>> But, progress is still great. How about if we add a copy accessor >>> instead for now, I think you proposed that a few million emails ago: >>> >>> pub fn get(&self) -> T; >>> >>> or maybe rename: >>> >>> pub fn copy(&self) -> T; >>> >>> Then we are fine safety wise for now, right? It is even sensible for >>> these `T: Copy` types. >> >> That is better than getting a reference, but still someone could read at >> the same time that a write is happening (though we need some new >> abstractions AFAIK?). But I fear that we forget about this issue, >> because it'll be some time until we land parameters that are `!Copy` (if >> at all...) > > No, that could not happen when we are not allowing custom parsing or > sysfs access. Regarding forgetting, I already added a `NOTE` on `!Copy`, > and I would add one on this issue as well. Ultimately this is something for Miguel to decide. I would support an unsafe accessor (we should also make it `-> T`), since there it "can't go wrong", any UB is the fault of the user of the API. It also serves as a good reminder, since a `NOTE` comment shouldn't be something guaranteeing safety (we do have some of these global invariants, but I feel like this one is too tribal and doesn't usually come up, so I feel like it's more dangerous). I think we should also move this patchset along, we could also opt for no accessor at all :) Then it isn't really useful without the downstream accessor function, but that can come after. --- Cheers, Benno
"Benno Lossin" <lossin@kernel.org> writes:
> On Tue Jul 1, 2025 at 4:14 PM CEST, Andreas Hindborg wrote:
>> "Benno Lossin" <lossin@kernel.org> writes:
>>> On Tue Jul 1, 2025 at 10:43 AM CEST, Andreas Hindborg wrote:
>>>> No, I am OK for now with configfs.
>>>>
>>>> But, progress is still great. How about if we add a copy accessor
>>>> instead for now, I think you proposed that a few million emails ago:
>>>>
>>>> pub fn get(&self) -> T;
>>>>
>>>> or maybe rename:
>>>>
>>>> pub fn copy(&self) -> T;
>>>>
>>>> Then we are fine safety wise for now, right? It is even sensible for
>>>> these `T: Copy` types.
>>>
>>> That is better than getting a reference, but still someone could read at
>>> the same time that a write is happening (though we need some new
>>> abstractions AFAIK?). But I fear that we forget about this issue,
>>> because it'll be some time until we land parameters that are `!Copy` (if
>>> at all...)
>>
>> No, that could not happen when we are not allowing custom parsing or
>> sysfs access. Regarding forgetting, I already added a `NOTE` on `!Copy`,
>> and I would add one on this issue as well.
>
> Ultimately this is something for Miguel to decide. I would support an
> unsafe accessor (we should also make it `-> T`), since there it "can't
> go wrong", any UB is the fault of the user of the API. It also serves as
> a good reminder, since a `NOTE` comment shouldn't be something
> guaranteeing safety (we do have some of these global invariants, but I
> feel like this one is too tribal and doesn't usually come up, so I feel
> like it's more dangerous).
I see no reason for making it unsafe when it is safe?
/// Get a copy of the parameter value.
///
/// # Note
///
/// When this method is called in `const` context, the default
/// parameter value will be returned. During module initialization, the
/// kernel will populate the parameter with the value supplied at module
/// load time or kernel boot time.
// NOTE: When sysfs access to parameters are enabled, we have to pass in a
// held lock guard here.
//
// NOTE: When we begin supporting custom parameter parsing with user
// supplied code, this method must be synchronized.
pub const fn copy(&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. The kernel will update the parameters serially, so we
// will not race with other accesses.
unsafe { *self.data.get() }
}
>
> I think we should also move this patchset along, we could also opt for
> no accessor at all :) Then it isn't really useful without the downstream
> accessor function, but that can come after.
I would rather not merge it without an access method. Then it is just
dead code, and we will not notice if we break it.
Best regards,
Andreas Hindborg
On Tue, Jul 1, 2025 at 5:43 PM Benno Lossin <lossin@kernel.org> wrote:
>
> Ultimately this is something for Miguel to decide.
Only if you all cannot get to an agreement ;)
If Andreas wants to have it already added, then I would say just mark
it `unsafe` as Benno recommends (possibly with an overbearing
precondition), given it has proven subtle/forgettable enough and that,
if I understand correctly, it would actually become unsafe if someone
"just" added "reasonably-looking code" elsewhere.
That way we have an incentive to make it safe later on and, more
importantly, to think again about it when such a patch lands,
justifying it properly. And it could plausibly protect out-of-tree
users, too.
This is all assuming that we will not have many users of this added
right away (in a cycle or two), i.e. assuming it will be easy to
change callers later on (if only to remove the `unsafe {}`).
Cheers,
Miguel
"Miguel Ojeda" <miguel.ojeda.sandonis@gmail.com> writes:
> On Tue, Jul 1, 2025 at 5:43 PM Benno Lossin <lossin@kernel.org> wrote:
>>
>> Ultimately this is something for Miguel to decide.
>
> Only if you all cannot get to an agreement ;)
>
> If Andreas wants to have it already added, then I would say just mark
> it `unsafe` as Benno recommends (possibly with an overbearing
> precondition), given it has proven subtle/forgettable enough and that,
> if I understand correctly, it would actually become unsafe if someone
> "just" added "reasonably-looking code" elsewhere.
You are right that if someone added code to the API, the API could
become unsound. But that is the deal with all our APIs and I don't agree
that the details are very subtle here. Someone would need to add sysfs
support or user provided parameter parsing to cause the unsoundness we
are talking about.
Anyone attempting such a task should have proper understanding of the
code first, and given the ample amount of `NOTE` comments I have added,
it should be clear that the concurrent accesses that this addition would
introduce, needs to be accounted for, to avoid data races.
I will add myself as a reviewer for the rust module parameter parsing
code if that is OK with module maintainers.
> That way we have an incentive to make it safe later on and, more
> importantly, to think again about it when such a patch lands,
> justifying it properly. And it could plausibly protect out-of-tree
> users, too.
Again, I do not think it is reasonable to mark this function unsafe.
> This is all assuming that we will not have many users of this added
> right away (in a cycle or two), i.e. assuming it will be easy to
> change callers later on (if only to remove the `unsafe {}`).
rnull will use this the cycle after it is merged.
Best regards,
Andreas Hindborg
On Wed Jul 2, 2025 at 10:26 AM CEST, Andreas Hindborg wrote: > "Miguel Ojeda" <miguel.ojeda.sandonis@gmail.com> writes: > >> On Tue, Jul 1, 2025 at 5:43 PM Benno Lossin <lossin@kernel.org> wrote: >>> >>> Ultimately this is something for Miguel to decide. >> >> Only if you all cannot get to an agreement ;) > >> >> If Andreas wants to have it already added, then I would say just mark >> it `unsafe` as Benno recommends (possibly with an overbearing >> precondition), given it has proven subtle/forgettable enough and that, >> if I understand correctly, it would actually become unsafe if someone >> "just" added "reasonably-looking code" elsewhere. > > You are right that if someone added code to the API, the API could > become unsound. But that is the deal with all our APIs and I don't agree > that the details are very subtle here. Someone would need to add sysfs > support or user provided parameter parsing to cause the unsoundness we > are talking about. Normally the safety requirements & invariants are *local*. We do have some global ones, but this one would be another one. And it's not easy to control IMO (no code is allowed to run before parameter parsing finished!). > Anyone attempting such a task should have proper understanding of the > code first, and given the ample amount of `NOTE` comments I have added, > it should be clear that the concurrent accesses that this addition would > introduce, needs to be accounted for, to avoid data races. Well if I add a way to add a task to a work queue before parameter parsing, I don't need to touch this file or even know about it. > I will add myself as a reviewer for the rust module parameter parsing > code if that is OK with module maintainers. I think it's a good idea, but it is orthogonal and doesn't address the issue, since any tree can merge code that breaks the invariant above. >> That way we have an incentive to make it safe later on and, more >> importantly, to think again about it when such a patch lands, >> justifying it properly. And it could plausibly protect out-of-tree >> users, too. > > Again, I do not think it is reasonable to mark this function unsafe. We mark it `unsafe` only until atomics are merged and then we make it safe. You proposed to do it the other way and make it safe, though possibly unsound when someone adds code breaking the invariant and making it fully sound later. I don't think we should have global invariants that are temporary. --- Cheers, Benno
On Tue Jul 1, 2025 at 6:27 PM CEST, Miguel Ojeda wrote:
> On Tue, Jul 1, 2025 at 5:43 PM Benno Lossin <lossin@kernel.org> wrote:
>>
>> Ultimately this is something for Miguel to decide.
>
> Only if you all cannot get to an agreement ;)
:)
> If Andreas wants to have it already added, then I would say just mark
> it `unsafe` as Benno recommends (possibly with an overbearing
> precondition), given it has proven subtle/forgettable enough and that,
> if I understand correctly, it would actually become unsafe if someone
> "just" added "reasonably-looking code" elsewhere.
Yeah, if we added code that ran at the same time as the parameter
parsing (such as custom parameter parsing or a way to start a "thread"
before the parsing is completed) it would be a problem.
> That way we have an incentive to make it safe later on and, more
> importantly, to think again about it when such a patch lands,
> justifying it properly. And it could plausibly protect out-of-tree
> users, too.
>
> This is all assuming that we will not have many users of this added
> right away (in a cycle or two), i.e. assuming it will be easy to
> change callers later on (if only to remove the `unsafe {}`).
Yeah we would add internal synchronization and could keep the API the
same (except removing unsafe of course).
---
Cheers,
Benno
"Benno Lossin" <lossin@kernel.org> writes:
> On Tue Jul 1, 2025 at 6:27 PM CEST, Miguel Ojeda wrote:
>> On Tue, Jul 1, 2025 at 5:43 PM Benno Lossin <lossin@kernel.org> wrote:
>>>
>>> Ultimately this is something for Miguel to decide.
>>
>> Only if you all cannot get to an agreement ;)
>
> :)
>
>> If Andreas wants to have it already added, then I would say just mark
>> it `unsafe` as Benno recommends (possibly with an overbearing
>> precondition), given it has proven subtle/forgettable enough and that,
>> if I understand correctly, it would actually become unsafe if someone
>> "just" added "reasonably-looking code" elsewhere.
>
> Yeah, if we added code that ran at the same time as the parameter
> parsing (such as custom parameter parsing or a way to start a "thread"
> before the parsing is completed) it would be a problem.
Guys, we are not going to accidentally add this. I do not think this is
a valid concern.
>
>> That way we have an incentive to make it safe later on and, more
>> importantly, to think again about it when such a patch lands,
>> justifying it properly. And it could plausibly protect out-of-tree
>> users, too.
>>
>> This is all assuming that we will not have many users of this added
>> right away (in a cycle or two), i.e. assuming it will be easy to
>> change callers later on (if only to remove the `unsafe {}`).
>
> Yeah we would add internal synchronization and could keep the API the
> same (except removing unsafe of course).
That is true. But I am not going to add an unsafe block to a driver just
to read module parameters. If we cannot reach agreement on merging this
with the `copy` access method, I would rather wait on a locking version.
Best regards,
Andreas Hindborg
On Mon, Jun 23, 2025 at 1:48 PM Benno Lossin <lossin@kernel.org> wrote:
>
> 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.
There are `DO_ONCE{,_SLEEPABLE,_LITE}`.
Cheers,
Miguel
On Mon Jun 23, 2025 at 2:37 PM CEST, Miguel Ojeda wrote:
> On Mon, Jun 23, 2025 at 1:48 PM Benno Lossin <lossin@kernel.org> wrote:
>>
>> 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.
>
> There are `DO_ONCE{,_SLEEPABLE,_LITE}`.
Thanks for the pointer, it is pretty much exactly what the `Once` type
in Rust is.
It's not exactly what I'm thinking of in this case, but since it is
implemented with static key, maybe we can do something similar with a
static key? So switch the return value of the `ModuleParamAccess::read`
function depending on weather the module parameters have been written
and while they haven't just return a dummy value and WARN.
Thoughts @Andreas?
---
Cheers,
Benno
Andreas Hindborg <a.hindborg@kernel.org> writes:
> "Benno Lossin" <lossin@kernel.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.
>
>> 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).
>
>>
>> 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?
It seems the compiler builds a dependency graph to check:
https://play.rust-lang.org/?version=nightly&mode=debug&edition=2024&gist=7a4d129a3fd2ae39a0aab9df3878a3d3
Best regards,
Andreas Hindborg
On Thu Jun 12, 2025 at 3:40 PM CEST, Andreas Hindborg wrote:
> Add types and traits for interfacing the C moduleparam API.
>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
> rust/kernel/lib.rs | 1 +
> rust/kernel/module_param.rs | 201 ++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 202 insertions(+)
>
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 6b4774b2b1c3..2b439ea06185 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -87,6 +87,7 @@
> pub mod list;
> pub mod miscdevice;
> pub mod mm;
> +pub mod module_param;
> #[cfg(CONFIG_NET)]
> pub mod net;
> pub mod of;
> diff --git a/rust/kernel/module_param.rs b/rust/kernel/module_param.rs
> new file mode 100644
> index 000000000000..fd167df8e53d
> --- /dev/null
> +++ b/rust/kernel/module_param.rs
> @@ -0,0 +1,201 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Support for module parameters.
> +//!
> +//! C header: [`include/linux/moduleparam.h`](srctree/include/linux/moduleparam.h)
> +
> +use crate::prelude::*;
> +use crate::str::BStr;
> +
> +/// Newtype to make `bindings::kernel_param` [`Sync`].
> +#[repr(transparent)]
> +#[doc(hidden)]
> +pub struct RacyKernelParam(pub ::kernel::bindings::kernel_param);
s/::kernel:://
The field shouldn't be public, since then people can access it. Can
just have a `pub fn new` instead?
> +
> +// SAFETY: C kernel handles serializing access to this type. We never access it
> +// from Rust module.
> +unsafe impl Sync for RacyKernelParam {}
> +
> +/// Types that can be used for module parameters.
> +pub trait ModuleParam: Sized + Copy {
Why the `Copy` bound?
> + /// The [`ModuleParam`] will be used by the kernel module through this type.
> + ///
> + /// This may differ from `Self` if, for example, `Self` needs to track
> + /// ownership without exposing it or allocate extra space for other possible
> + /// parameter values.
> + // This is required to support string parameters in the future.
> + type Value: ?Sized;
> +
> + /// Parse a parameter argument into the parameter value.
> + ///
> + /// `Err(_)` should be returned when parsing of the argument fails.
I don't think we need to explicitly mention this.
> + ///
> + /// Parameters passed at boot time will be set before [`kmalloc`] is
> + /// available (even if the module is loaded at a later time). However, in
I think we should make a section out of this like `# No allocations` (or
something better). Let's also mention it on the trait itself, since
that's where implementers will most likely look.
> + /// this case, the argument buffer will be valid for the entire lifetime of
> + /// the kernel. So implementations of this method which need to allocate
> + /// should first check that the allocator is available (with
> + /// [`crate::bindings::slab_is_available`]) and when it is not available
We probably shouldn't recommend directly using `bindings`.
> + /// provide an alternative implementation which doesn't allocate. In cases
> + /// where the allocator is not available it is safe to save references to
> + /// `arg` in `Self`, but in other cases a copy should be made.
I don't understand this convention, but it also doesn't seem to
relevant (so feel free to leave it as is, but it would be nice if you
could explain it).
> + ///
> + /// [`kmalloc`]: srctree/include/linux/slab.h
> + fn try_from_param_arg(arg: &'static BStr) -> Result<Self>;
> +}
> +
> +/// Set the module parameter from a string.
> +///
> +/// Used to set the parameter value at kernel initialization, when loading
> +/// the module or when set through `sysfs`.
> +///
> +/// See `struct kernel_param_ops.set`.
> +///
> +/// # Safety
> +///
> +/// - If `val` is non-null then it must point to a valid null-terminated string that must be valid
> +/// for reads for the duration of the call.
> +/// - `parm` must be a pointer to a `bindings::kernel_param` that is valid for reads for the
s/parm/param/
> +/// duration of the call.
> +/// - `param.arg` must be a pointer to an initialized `T` that is valid for writes for the duration
> +/// of the function.
> +///
> +/// # Note
> +///
> +/// - The safety requirements are satisfied by C API contract when this function is invoked by the
> +/// module subsystem C code.
> +/// - Currently, we only support read-only parameters that are not readable from `sysfs`. Thus, this
> +/// function is only called at kernel initialization time, or at module load time, and we have
> +/// exclusive access to the parameter for the duration of the function.
> +///
> +/// [`module!`]: macros::module
> +unsafe extern "C" fn set_param<T>(
> + val: *const c_char,
> + param: *const crate::bindings::kernel_param,
> +) -> c_int
> +where
> + T: ModuleParam,
> +{
> + // NOTE: If we start supporting arguments without values, val _is_ allowed
> + // to be null here.
> + if val.is_null() {
> + // TODO: Use pr_warn_once available.
> + crate::pr_warn!("Null pointer passed to `module_param::set_param`");
> + return EINVAL.to_errno();
> + }
> +
> + // SAFETY: By function safety requirement, val is non-null, null-terminated
> + // and valid for reads for the duration of this function.
> + let arg = unsafe { CStr::from_char_ptr(val) };
`arg` has the type `&'static CStr`, which is not justified by the safety
comment :(
Why does `ModuleParam::try_from_param_arg` take a `&'static BStr` and
not a `&BStr`? I guess it is related to the "make copies if allocator is
available" question I had above.
> +
> + crate::error::from_result(|| {
> + let new_value = T::try_from_param_arg(arg)?;
> +
> + // SAFETY: By function safety requirements `param` is be valid for reads.
> + let old_value = unsafe { (*param).__bindgen_anon_1.arg as *mut T };
> +
> + // SAFETY: By function safety requirements, the target of `old_value` is valid for writes
> + // and is initialized.
> + unsafe { *old_value = new_value };
So if we keep the `ModuleParam: Copy` bound from above, then we don't
need to drop the type here (as `Copy` implies `!Drop`). So we could also
remove the requirement for initialized memory and use `ptr::write` here
instead. Thoughts?
> + Ok(0)
> + })
> +}
> +
> +macro_rules! impl_int_module_param {
> + ($ty:ident) => {
> + impl ModuleParam for $ty {
> + type Value = $ty;
> +
> + fn try_from_param_arg(arg: &'static BStr) -> Result<Self> {
> + <$ty as crate::str::parse_int::ParseInt>::from_str(arg)
> + }
> + }
> + };
> +}
> +
> +impl_int_module_param!(i8);
> +impl_int_module_param!(u8);
> +impl_int_module_param!(i16);
> +impl_int_module_param!(u16);
> +impl_int_module_param!(i32);
> +impl_int_module_param!(u32);
> +impl_int_module_param!(i64);
> +impl_int_module_param!(u64);
> +impl_int_module_param!(isize);
> +impl_int_module_param!(usize);
> +
> +/// 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>,
> +}
We should just re-create the `SyncUnsafeCell` [1] from upstream...
Feel free to keep this until we have it though.
[1]: https://doc.rust-lang.org/nightly/std/cell/struct.SyncUnsafeCell.html
> +
> +// 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.
> + unsafe { &*self.data.get() }
> + }
> +
> + /// Get a mutable pointer to the parameter value.
> + pub const fn as_mut_ptr(&self) -> *mut T {
> + self.data.get()
> + }
> +}
> +
> +#[doc(hidden)]
> +#[macro_export]
Why export this?
---
Cheers,
Benno
> +/// Generate a static [`kernel_param_ops`](srctree/include/linux/moduleparam.h) struct.
> +///
> +/// # Examples
> +///
> +/// ```ignore
> +/// make_param_ops!(
> +/// /// Documentation for new param ops.
> +/// PARAM_OPS_MYTYPE, // Name for the static.
> +/// MyType // A type which implements [`ModuleParam`].
> +/// );
> +/// ```
> +macro_rules! make_param_ops {
> + ($ops:ident, $ty:ty) => {
> + #[doc(hidden)]
> + pub static $ops: $crate::bindings::kernel_param_ops = $crate::bindings::kernel_param_ops {
> + flags: 0,
> + set: Some(set_param::<$ty>),
> + get: None,
> + free: None,
> + };
> + };
> +}
> +
> +make_param_ops!(PARAM_OPS_I8, i8);
> +make_param_ops!(PARAM_OPS_U8, u8);
> +make_param_ops!(PARAM_OPS_I16, i16);
> +make_param_ops!(PARAM_OPS_U16, u16);
> +make_param_ops!(PARAM_OPS_I32, i32);
> +make_param_ops!(PARAM_OPS_U32, u32);
> +make_param_ops!(PARAM_OPS_I64, i64);
> +make_param_ops!(PARAM_OPS_U64, u64);
> +make_param_ops!(PARAM_OPS_ISIZE, isize);
> +make_param_ops!(PARAM_OPS_USIZE, usize);
"Benno Lossin" <lossin@kernel.org> writes:
> On Thu Jun 12, 2025 at 3:40 PM CEST, Andreas Hindborg wrote:
>> Add types and traits for interfacing the C moduleparam API.
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>> ---
>> rust/kernel/lib.rs | 1 +
>> rust/kernel/module_param.rs | 201 ++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 202 insertions(+)
>>
>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
>> index 6b4774b2b1c3..2b439ea06185 100644
>> --- a/rust/kernel/lib.rs
>> +++ b/rust/kernel/lib.rs
>> @@ -87,6 +87,7 @@
>> pub mod list;
>> pub mod miscdevice;
>> pub mod mm;
>> +pub mod module_param;
>> #[cfg(CONFIG_NET)]
>> pub mod net;
>> pub mod of;
>> diff --git a/rust/kernel/module_param.rs b/rust/kernel/module_param.rs
>> new file mode 100644
>> index 000000000000..fd167df8e53d
>> --- /dev/null
>> +++ b/rust/kernel/module_param.rs
>> @@ -0,0 +1,201 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! Support for module parameters.
>> +//!
>> +//! C header: [`include/linux/moduleparam.h`](srctree/include/linux/moduleparam.h)
>> +
>> +use crate::prelude::*;
>> +use crate::str::BStr;
>> +
>> +/// Newtype to make `bindings::kernel_param` [`Sync`].
>> +#[repr(transparent)]
>> +#[doc(hidden)]
>> +pub struct RacyKernelParam(pub ::kernel::bindings::kernel_param);
>
> s/::kernel:://
>
> The field shouldn't be public, since then people can access it. Can
> just have a `pub fn new` instead?
OK.
>
>> +
>> +// SAFETY: C kernel handles serializing access to this type. We never access it
>> +// from Rust module.
>> +unsafe impl Sync for RacyKernelParam {}
>> +
>> +/// Types that can be used for module parameters.
>> +pub trait ModuleParam: Sized + Copy {
>
> Why the `Copy` bound?
Because of potential unsoundness due to drop [1]. I should document
this. It is noted in the change log for the series under the obscure
entry "Assign through pointer rather than using `core::ptr::replace`."
[1] https://lore.kernel.org/all/878qnbxtyi.fsf@kernel.org
>
>> + /// The [`ModuleParam`] will be used by the kernel module through this type.
>> + ///
>> + /// This may differ from `Self` if, for example, `Self` needs to track
>> + /// ownership without exposing it or allocate extra space for other possible
>> + /// parameter values.
>> + // This is required to support string parameters in the future.
>> + type Value: ?Sized;
>> +
>> + /// Parse a parameter argument into the parameter value.
>> + ///
>> + /// `Err(_)` should be returned when parsing of the argument fails.
>
> I don't think we need to explicitly mention this.
I'll remove the line.
>
>> + ///
>> + /// Parameters passed at boot time will be set before [`kmalloc`] is
>> + /// available (even if the module is loaded at a later time). However, in
>
> I think we should make a section out of this like `# No allocations` (or
> something better). Let's also mention it on the trait itself, since
> that's where implementers will most likely look.
Since this series only support `Copy` types that are passed by value, I
think we can remove this comment for now. I will also restrict the
lifetime of the string to he duration of the call. Putting static here
would be lying.
>
>> + /// this case, the argument buffer will be valid for the entire lifetime of
>> + /// the kernel. So implementations of this method which need to allocate
>> + /// should first check that the allocator is available (with
>> + /// [`crate::bindings::slab_is_available`]) and when it is not available
>
> We probably shouldn't recommend directly using `bindings`.
>
>> + /// provide an alternative implementation which doesn't allocate. In cases
>> + /// where the allocator is not available it is safe to save references to
>> + /// `arg` in `Self`, but in other cases a copy should be made.
>
> I don't understand this convention, but it also doesn't seem to
> relevant (so feel free to leave it as is, but it would be nice if you
> could explain it).
It has become irrelevant as the series evolved. When we supported
`!Copy` types we would use the reference if we knew it would be valid
for the lifetime of the kernel, otherwise we would allocate [1].
However, when the reference is passed at module load time, it is still
guaranteed to be live for the lifetime of the module, and hence it can
still be considered `'static`. But, if the reference were to find it's
way across the module boundary, it can cause UAF issues as the reference
is not truely `'static`, it is actually `'module`. This ties into the
difficulty we have around safety of unloading modules. Module unload
should be marked unsafe.
At any rate, I will remove the `'static` lifetime from the reference and
we are all good for now.
[1] https://github.com/Rust-for-Linux/linux/blob/18b7491480025420896e0c8b73c98475c3806c6f/rust/kernel/module_param.rs#L476
>
>> + ///
>> + /// [`kmalloc`]: srctree/include/linux/slab.h
>> + fn try_from_param_arg(arg: &'static BStr) -> Result<Self>;
>> +}
>> +
>> +/// Set the module parameter from a string.
>> +///
>> +/// Used to set the parameter value at kernel initialization, when loading
>> +/// the module or when set through `sysfs`.
>> +///
>> +/// See `struct kernel_param_ops.set`.
>> +///
>> +/// # Safety
>> +///
>> +/// - If `val` is non-null then it must point to a valid null-terminated string that must be valid
>> +/// for reads for the duration of the call.
>> +/// - `parm` must be a pointer to a `bindings::kernel_param` that is valid for reads for the
>
> s/parm/param/
Yea, I get contaminated with spellings used elsewhere in the kernel.
>
>> +/// duration of the call.
>> +/// - `param.arg` must be a pointer to an initialized `T` that is valid for writes for the duration
>> +/// of the function.
>> +///
>> +/// # Note
>> +///
>> +/// - The safety requirements are satisfied by C API contract when this function is invoked by the
>> +/// module subsystem C code.
>> +/// - Currently, we only support read-only parameters that are not readable from `sysfs`. Thus, this
>> +/// function is only called at kernel initialization time, or at module load time, and we have
>> +/// exclusive access to the parameter for the duration of the function.
>> +///
>> +/// [`module!`]: macros::module
>> +unsafe extern "C" fn set_param<T>(
>> + val: *const c_char,
>> + param: *const crate::bindings::kernel_param,
>> +) -> c_int
>> +where
>> + T: ModuleParam,
>> +{
>> + // NOTE: If we start supporting arguments without values, val _is_ allowed
>> + // to be null here.
>> + if val.is_null() {
>> + // TODO: Use pr_warn_once available.
>> + crate::pr_warn!("Null pointer passed to `module_param::set_param`");
>> + return EINVAL.to_errno();
>> + }
>> +
>> + // SAFETY: By function safety requirement, val is non-null, null-terminated
>> + // and valid for reads for the duration of this function.
>> + let arg = unsafe { CStr::from_char_ptr(val) };
>
> `arg` has the type `&'static CStr`, which is not justified by the safety
> comment :(
Not any longer, as outlined above. Thanks for catching this.
> Why does `ModuleParam::try_from_param_arg` take a `&'static BStr` and
> not a `&BStr`? I guess it is related to the "make copies if allocator is
> available" question I had above.
Yep.
>
>> +
>> + crate::error::from_result(|| {
>> + let new_value = T::try_from_param_arg(arg)?;
>> +
>> + // SAFETY: By function safety requirements `param` is be valid for reads.
>> + let old_value = unsafe { (*param).__bindgen_anon_1.arg as *mut T };
>> +
>> + // SAFETY: By function safety requirements, the target of `old_value` is valid for writes
>> + // and is initialized.
>> + unsafe { *old_value = new_value };
>
> So if we keep the `ModuleParam: Copy` bound from above, then we don't
> need to drop the type here (as `Copy` implies `!Drop`). So we could also
> remove the requirement for initialized memory and use `ptr::write` here
> instead. Thoughts?
Yes, that is the rationale for the `Copy` bound. What would be the
benefit of using `ptr::write`? They should be equivalent for `Copy`
types, right.
I was using `ptr::replace`, but Alice suggested the pace expression
assignment instead, since I was not using the old value.
>
>> + Ok(0)
>> + })
>> +}
>> +
>> +macro_rules! impl_int_module_param {
>> + ($ty:ident) => {
>> + impl ModuleParam for $ty {
>> + type Value = $ty;
>> +
>> + fn try_from_param_arg(arg: &'static BStr) -> Result<Self> {
>> + <$ty as crate::str::parse_int::ParseInt>::from_str(arg)
>> + }
>> + }
>> + };
>> +}
>> +
>> +impl_int_module_param!(i8);
>> +impl_int_module_param!(u8);
>> +impl_int_module_param!(i16);
>> +impl_int_module_param!(u16);
>> +impl_int_module_param!(i32);
>> +impl_int_module_param!(u32);
>> +impl_int_module_param!(i64);
>> +impl_int_module_param!(u64);
>> +impl_int_module_param!(isize);
>> +impl_int_module_param!(usize);
>> +
>> +/// 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>,
>> +}
>
> We should just re-create the `SyncUnsafeCell` [1] from upstream...
I will add a // TODO: Use `SyncUnsafeCell` when available.
>
> Feel free to keep this until we have it though.
>
> [1]: https://doc.rust-lang.org/nightly/std/cell/struct.SyncUnsafeCell.html
>
>> +
>> +// 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.
>> + unsafe { &*self.data.get() }
>> + }
>> +
>> + /// Get a mutable pointer to the parameter value.
>> + pub const fn as_mut_ptr(&self) -> *mut T {
>> + self.data.get()
>> + }
>> +}
>> +
>> +#[doc(hidden)]
>> +#[macro_export]
>
> Why export this?
Legacy debt. I'll remove it.
Best regards,
Andreas Hindborg
On Thu Jun 19, 2025 at 2:20 PM CEST, Andreas Hindborg wrote:
> "Benno Lossin" <lossin@kernel.org> writes:
>> On Thu Jun 12, 2025 at 3:40 PM CEST, Andreas Hindborg wrote:
>>> +
>>> +// SAFETY: C kernel handles serializing access to this type. We never access it
>>> +// from Rust module.
>>> +unsafe impl Sync for RacyKernelParam {}
>>> +
>>> +/// Types that can be used for module parameters.
>>> +pub trait ModuleParam: Sized + Copy {
>>
>> Why the `Copy` bound?
>
> Because of potential unsoundness due to drop [1]. I should document
> this. It is noted in the change log for the series under the obscure
> entry "Assign through pointer rather than using `core::ptr::replace`."
>
> [1] https://lore.kernel.org/all/878qnbxtyi.fsf@kernel.org
Ah thanks for the pointer, yeah please mention this in a comment
somewhere.
>>> + ///
>>> + /// Parameters passed at boot time will be set before [`kmalloc`] is
>>> + /// available (even if the module is loaded at a later time). However, in
>>
>> I think we should make a section out of this like `# No allocations` (or
>> something better). Let's also mention it on the trait itself, since
>> that's where implementers will most likely look.
>
> Since this series only support `Copy` types that are passed by value, I
> think we can remove this comment for now. I will also restrict the
> lifetime of the string to he duration of the call. Putting static here
> would be lying.
>
>>
>>> + /// this case, the argument buffer will be valid for the entire lifetime of
>>> + /// the kernel. So implementations of this method which need to allocate
>>> + /// should first check that the allocator is available (with
>>> + /// [`crate::bindings::slab_is_available`]) and when it is not available
>>
>> We probably shouldn't recommend directly using `bindings`.
>>
>>> + /// provide an alternative implementation which doesn't allocate. In cases
>>> + /// where the allocator is not available it is safe to save references to
>>> + /// `arg` in `Self`, but in other cases a copy should be made.
>>
>> I don't understand this convention, but it also doesn't seem to
>> relevant (so feel free to leave it as is, but it would be nice if you
>> could explain it).
>
> It has become irrelevant as the series evolved. When we supported
> `!Copy` types we would use the reference if we knew it would be valid
> for the lifetime of the kernel, otherwise we would allocate [1].
>
> However, when the reference is passed at module load time, it is still
> guaranteed to be live for the lifetime of the module, and hence it can
> still be considered `'static`. But, if the reference were to find it's
> way across the module boundary, it can cause UAF issues as the reference
> is not truely `'static`, it is actually `'module`. This ties into the
> difficulty we have around safety of unloading modules. Module unload
> should be marked unsafe.
Ah so the argument should rather be an enum that is either
`Static(&'static str)` or `WithAlloc(&'short str)` with the (non-safety)
guarantee that `WithAlloc` is only passed when the allocator is
available.
> At any rate, I will remove the `'static` lifetime from the reference and
> we are all good for now.
Sounds simplest for now.
>>> + crate::error::from_result(|| {
>>> + let new_value = T::try_from_param_arg(arg)?;
>>> +
>>> + // SAFETY: By function safety requirements `param` is be valid for reads.
>>> + let old_value = unsafe { (*param).__bindgen_anon_1.arg as *mut T };
>>> +
>>> + // SAFETY: By function safety requirements, the target of `old_value` is valid for writes
>>> + // and is initialized.
>>> + unsafe { *old_value = new_value };
>>
>> So if we keep the `ModuleParam: Copy` bound from above, then we don't
>> need to drop the type here (as `Copy` implies `!Drop`). So we could also
>> remove the requirement for initialized memory and use `ptr::write` here
>> instead. Thoughts?
>
> Yes, that is the rationale for the `Copy` bound. What would be the
> benefit of using `ptr::write`? They should be equivalent for `Copy`
> types, right.
They should be equivalent, but if we drop the requirement that the value
is initialized to begin with, then removing `Copy` will result in UB
here.
> I was using `ptr::replace`, but Alice suggested the pace expression
> assignment instead, since I was not using the old value.
That makes sense, but if we also remove the initialized requirement,
then I would prefer `ptr::write`.
---
Cheers,
Benno
"Benno Lossin" <lossin@kernel.org> writes:
> On Thu Jun 19, 2025 at 2:20 PM CEST, Andreas Hindborg wrote:
>> "Benno Lossin" <lossin@kernel.org> writes:
>>> On Thu Jun 12, 2025 at 3:40 PM CEST, Andreas Hindborg wrote:
[...]
>>>> + crate::error::from_result(|| {
>>>> + let new_value = T::try_from_param_arg(arg)?;
>>>> +
>>>> + // SAFETY: By function safety requirements `param` is be valid for reads.
>>>> + let old_value = unsafe { (*param).__bindgen_anon_1.arg as *mut T };
>>>> +
>>>> + // SAFETY: By function safety requirements, the target of `old_value` is valid for writes
>>>> + // and is initialized.
>>>> + unsafe { *old_value = new_value };
>>>
>>> So if we keep the `ModuleParam: Copy` bound from above, then we don't
>>> need to drop the type here (as `Copy` implies `!Drop`). So we could also
>>> remove the requirement for initialized memory and use `ptr::write` here
>>> instead. Thoughts?
>>
>> Yes, that is the rationale for the `Copy` bound. What would be the
>> benefit of using `ptr::write`? They should be equivalent for `Copy`
>> types, right.
>
> They should be equivalent, but if we drop the requirement that the value
> is initialized to begin with, then removing `Copy` will result in UB
> here.
>
>> I was using `ptr::replace`, but Alice suggested the pace expression
>> assignment instead, since I was not using the old value.
>
> That makes sense, but if we also remove the initialized requirement,
> then I would prefer `ptr::write`.
OK, we can do that.
Best regards,
Andreas Hindborg
© 2016 - 2025 Red Hat, Inc.