[PATCH v13 0/6] rust: extend `module!` macro with integer parameter support

[PATCH v13 0/6] rust: extend `module!` macro with integer parameter support

[Andreas Hindborg]

Extend the `module!` macro with support module parameters. Also add some string
to integer parsing functions and updates `BStr` with a method to strip a string
prefix.

Based on code by Adam Bratschi-Kaye lifted from the original `rust` branch [1].

Link: https://github.com/Rust-for-Linux/linux/tree/bc22545f38d74473cfef3e9fd65432733435b79f [1]
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
Changes in v13:
- remove absolute path for `ffi` types.
- Split patch 2 into 4 separate patches.
- Overhaul safety framework for `set_param`.
- Remove generated docs for `kernel_param_ops`.
- Move `parse_int` to separate file.
- Rebase on v6.16-rc1
- Link to v12: https://lore.kernel.org/r/20250506-module-params-v3-v12-0-c04d80c8a2b1@kernel.org

Changes in v12:
- Assign through pointer rather than using `core::ptr::replace`.
- Prevent a potential use-after-free during module teardown.
- Link to v11: https://lore.kernel.org/r/20250502-module-params-v3-v11-0-6096875a2b78@kernel.org

Changes in v11:
- Apply a few nits from Miguel.
- Link to v10: https://lore.kernel.org/r/20250501-module-params-v3-v10-0-4da485d343d5@kernel.org

Changes in v10:
- Apply fixups from Miguel:
  - Add integer type suffixes to `assert!` in tests.
  - Fix links to docs.kernel.org.
  - Applyy markdown and intra-doc links where possible.
  - Change to `///` for `mod` docs.
  - Slightly reword a comment.
  - Pluralize "Examples" section name.
  - Hide `use`s in example.
  - Removed `#[expect]` for the `rusttest` target.
- Link to v9: https://lore.kernel.org/r/20250321-module-params-v3-v9-0-28b905f2e345@kernel.org

Changes in v9:
- Remove UB when parsing the minimum integer values.
- Make `FromStr` trait unsafe, since wrong implementations can cause UB.
- Drop patches that were applied to rust-next.
- Link to v8: https://lore.kernel.org/r/20250227-module-params-v3-v8-0-ceeee85d9347@kernel.org

Changes in v8:
- Change print statement in sample to better communicate parameter name.
- Use imperative mode in commit messages.
- Remove prefix path from `EINVAL`.
- Change `try_from_param_arg` to accept `&BStr` rather than `&[u8]`.
- Parse integers without 128 bit integer types.
- Seal trait `FromStrRadix`.
- Strengthen safety requirement of `set_param`.
- Remove comment about Display and `PAGE_SIZE`.
- Add note describing why `ModuleParamAccess` is pub.
- Typo and grammar fixes for documentation.
- Update MAINTAINERS with rust module files.
- Link to v7: https://lore.kernel.org/r/20250218-module-params-v3-v7-0-5e1afabcac1b@kernel.org

Changes in v7:
- Remove dependency on `pr_warn_once` patches, replace with TODO.
- Rework `ParseInt::from_str` to avoid allocating.
- Add a comment explaining how we parse "0".
- Change trait bound on `Index` impl for `BStr` to match std library approach.
- Link to v6: https://lore.kernel.org/r/20250211-module-params-v3-v6-0-24b297ddc43d@kernel.org

Changes in v6:
- Fix a bug that prevented parsing of negative default values for
  parameters in the `module!` macro.
- Fix a bug that prevented parsing zero in `strip_radix`. Also add a
  test case for this.
- Add `AsRef<BStr>` for `[u8]` and `BStr`.
- Use `impl AsRef<BStr>` as type of prefix in `BStr::strip_prefix`.
- Link to v5: https://lore.kernel.org/r/20250204-module-params-v3-v5-0-bf5ec2041625@kernel.org

Changes in v5:
- Fix a typo in a safety comment in `set_param`.
- Use a match statement in `parse_int::strip_radix`.
- Add an implementation of `Index` for `BStr`.
- Fix a logic inversion bug where parameters would not be parsed.
- Use `kernel::ffi::c_char` in `set_param` rather than the one in `core`.
- Use `kernel::c_str!` rather than `c"..."` literal in module macro.
- Rebase on v6.14-rc1.
- Link to v4: https://lore.kernel.org/r/20250109-module-params-v3-v4-0-c208bcfbe11f@kernel.org

Changes in v4:
- Add module maintainers to Cc list (sorry)
- Add a few missing [`doc_links`]
- Add panic section to `expect_string_field`
- Fix a typo in safety requirement of `module_params::free`
- Change `assert!` to `pr_warn_once!` in `module_params::set_param`
- Remove `module_params::get_param` and install null pointer instead
- Remove use of the unstable feature `sync_unsafe_cell`
- Link to v3: https://lore.kernel.org/r/20241213-module-params-v3-v3-0-485a015ac2cf@kernel.org

Changes in v3:
- use `SyncUnsafeCell` rather than `static mut` and simplify parameter access
- remove `Display` bound from `ModuleParam`
- automatically generate documentation for `PARAM_OPS_.*`
- remove `as *const _ as *mut_` phrasing
- inline parameter name in struct instantiation in  `emit_params`
- move `RacyKernelParam` out of macro template
- use C string literals rather than byte string literals with explicit null
- template out `__{name}_{param_name}` in `emit_param`
- indent template in `emit_params`
- use let-else expression in `emit_params` to get rid of an indentation level
- document `expect_string_field`
- move invication of `impl_int_module_param` to be closer to macro def
- move attributes after docs in `make_param_ops`
- rename `impl_module_param` to impl_int_module_param`
- use `ty` instead of `ident` in `impl_parse_int`
- use `BStr` instead of `&str` for string manipulation
- move string parsing functions to seperate patch and add examples, fix bugs
- degrade comment about future support from doc comment to regular comment
- remove std lib path from `Sized` marker
- update documentation for `trait ModuleParam`
- Link to v2: https://lore.kernel.org/all/20240819133345.3438739-1-nmi@metaspace.dk/

Changes in v2:
- Remove support for params without values (`NOARG_ALLOWED`).
- Improve documentation for `try_from_param_arg`.
- Use prelude import.
- Refactor `try_from_param_arg` to return `Result`.
- Refactor `ParseInt::from_str` to return `Result`.
- Move C callable functions out of `ModuleParam` trait.
- Rename literal string field parser to `expect_string_field`.
- Move parameter parsing from generation to parsing stage.
- Use absolute type paths in macro code.
- Inline `kparam`and `read_func` values.
- Resolve TODO regarding alignment attributes.
- Remove unnecessary unsafe blocks in macro code.
- Improve error message for unrecognized parameter types.
- Do not use `self` receiver when reading parameter value.
- Add parameter documentation to `module!` macro.
- Use empty `enum` for parameter type.
- Use `addr_of_mut` to get address of parameter value variable.
- Enabled building of docs for for `module_param` module.
- Link to v1: https://lore.kernel.org/rust-for-linux/20240705111455.142790-1-nmi@metaspace.dk/

---
Andreas Hindborg (6):
      rust: str: add radix prefixed integer parsing functions
      rust: introduce module_param module
      rust: module: use a reference in macros::module::module
      rust: module: update the module macro with module parameter support
      rust: samples: add a module parameter to the rust_minimal sample
      modules: add rust modules files to MAINTAINERS

 MAINTAINERS                  |   2 +
 rust/kernel/lib.rs           |   1 +
 rust/kernel/module_param.rs  | 201 +++++++++++++++++++++++++++++++++++++++++++
 rust/kernel/str.rs           |   2 +
 rust/kernel/str/parse_int.rs | 171 ++++++++++++++++++++++++++++++++++++
 rust/macros/helpers.rs       |  25 ++++++
 rust/macros/lib.rs           |  31 +++++++
 rust/macros/module.rs        | 195 ++++++++++++++++++++++++++++++++++++-----
 samples/rust/rust_minimal.rs |  10 +++
 9 files changed, 618 insertions(+), 20 deletions(-)
---
base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
change-id: 20241211-module-params-v3-ae7e5c8d8b5a

Best regards,
-- 
Andreas Hindborg <a.hindborg@kernel.org>

[PATCH v13 1/6] rust: str: add radix prefixed integer parsing functions

[Andreas Hindborg]

Add the trait `ParseInt` for parsing string representations of integers
where the string representations are optionally prefixed by a radix
specifier. Implement the trait for the primitive integer types.

Tested-by: Daniel Gomez <da.gomez@samsung.com>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/str.rs           |   2 +
 rust/kernel/str/parse_int.rs | 171 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 173 insertions(+)

diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
index a927db8e079c..2b6c8b4a0ae4 100644
--- a/rust/kernel/str.rs
+++ b/rust/kernel/str.rs
@@ -8,6 +8,8 @@
 
 use crate::prelude::*;
 
+pub mod parse_int;
+
 /// Byte string without UTF-8 validity guarantee.
 #[repr(transparent)]
 pub struct BStr([u8]);
diff --git a/rust/kernel/str/parse_int.rs b/rust/kernel/str/parse_int.rs
new file mode 100644
index 000000000000..0754490aec4b
--- /dev/null
+++ b/rust/kernel/str/parse_int.rs
@@ -0,0 +1,171 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Integer parsing functions.
+//!
+//! Integer parsing functions for parsing signed and unsigned integers
+//! potentially prefixed with `0x`, `0o`, or `0b`.
+
+use crate::prelude::*;
+use crate::str::BStr;
+use core::ops::Deref;
+
+// Make `FromStrRadix` a public type with a private name. This seals
+// `ParseInt`, that is, prevents downstream users from implementing the
+// trait.
+mod private {
+    use crate::str::BStr;
+
+    /// Trait that allows parsing a [`&BStr`] to an integer with a radix.
+    ///
+    /// # Safety
+    ///
+    /// The member functions of this trait must be implemented according to
+    /// their documentation.
+    ///
+    /// [`&BStr`]: kernel::str::BStr
+    // This is required because the `from_str_radix` function on the primitive
+    // integer types is not part of any trait.
+    pub unsafe trait FromStrRadix: Sized {
+        /// The minimum value this integer type can assume.
+        const MIN: Self;
+
+        /// Parse `src` to [`Self`] using radix `radix`.
+        fn from_str_radix(src: &BStr, radix: u32) -> Result<Self, crate::error::Error>;
+
+        /// Return the absolute value of [`Self::MIN`].
+        fn abs_min() -> u64;
+
+        /// Perform bitwise 2's complement on `self`.
+        ///
+        /// Note: This function does not make sense for unsigned integers.
+        fn complement(self) -> Self;
+    }
+}
+
+/// Extract the radix from an integer literal optionally prefixed with
+/// one of `0x`, `0X`, `0o`, `0O`, `0b`, `0B`, `0`.
+fn strip_radix(src: &BStr) -> (u32, &BStr) {
+    match src.deref() {
+        [b'0', b'x' | b'X', rest @ ..] => (16, rest.as_ref()),
+        [b'0', b'o' | b'O', rest @ ..] => (8, rest.as_ref()),
+        [b'0', b'b' | b'B', rest @ ..] => (2, rest.as_ref()),
+        // NOTE: We are including the leading zero to be able to parse
+        // literal `0` here. If we removed it as a radix prefix, we would
+        // not be able to parse `0`.
+        [b'0', ..] => (8, src),
+        _ => (10, src),
+    }
+}
+
+/// Trait for parsing string representations of integers.
+///
+/// Strings beginning with `0x`, `0o`, or `0b` are parsed as hex, octal, or
+/// binary respectively. Strings beginning with `0` otherwise are parsed as
+/// octal. Anything else is parsed as decimal. A leading `+` or `-` is also
+/// permitted. Any string parsed by [`kstrtol()`] or [`kstrtoul()`] will be
+/// successfully parsed.
+///
+/// [`kstrtol()`]: https://docs.kernel.org/core-api/kernel-api.html#c.kstrtol
+/// [`kstrtoul()`]: https://docs.kernel.org/core-api/kernel-api.html#c.kstrtoul
+///
+/// # Examples
+///
+/// ```
+/// # use kernel::str::parse_int::ParseInt;
+/// # use kernel::b_str;
+///
+/// assert_eq!(Ok(0u8), u8::from_str(b_str!("0")));
+///
+/// assert_eq!(Ok(0xa2u8), u8::from_str(b_str!("0xa2")));
+/// assert_eq!(Ok(-0xa2i32), i32::from_str(b_str!("-0xa2")));
+///
+/// assert_eq!(Ok(-0o57i8), i8::from_str(b_str!("-0o57")));
+/// assert_eq!(Ok(0o57i8), i8::from_str(b_str!("057")));
+///
+/// assert_eq!(Ok(0b1001i16), i16::from_str(b_str!("0b1001")));
+/// assert_eq!(Ok(-0b1001i16), i16::from_str(b_str!("-0b1001")));
+///
+/// assert_eq!(Ok(127i8), i8::from_str(b_str!("127")));
+/// assert!(i8::from_str(b_str!("128")).is_err());
+/// assert_eq!(Ok(-128i8), i8::from_str(b_str!("-128")));
+/// assert!(i8::from_str(b_str!("-129")).is_err());
+/// assert_eq!(Ok(255u8), u8::from_str(b_str!("255")));
+/// assert!(u8::from_str(b_str!("256")).is_err());
+/// ```
+pub trait ParseInt: private::FromStrRadix + TryFrom<u64> {
+    /// Parse a string according to the description in [`Self`].
+    fn from_str(src: &BStr) -> Result<Self> {
+        match src.deref() {
+            [b'-', rest @ ..] => {
+                let (radix, digits) = strip_radix(rest.as_ref());
+                // 2's complement values range from -2^(b-1) to 2^(b-1)-1.
+                // So if we want to parse negative numbers as positive and
+                // later multiply by -1, we have to parse into a larger
+                // integer. We choose `u64` as sufficiently large.
+                //
+                // NOTE: 128 bit integers are not available on all
+                // platforms, hence the choice of 64 bits.
+                let val =
+                    u64::from_str_radix(core::str::from_utf8(digits).map_err(|_| EINVAL)?, radix)
+                        .map_err(|_| EINVAL)?;
+
+                if val > Self::abs_min() {
+                    return Err(EINVAL);
+                }
+
+                if val == Self::abs_min() {
+                    return Ok(Self::MIN);
+                }
+
+                // SAFETY: We checked that `val` will fit in `Self` above.
+                let val: Self = unsafe { val.try_into().unwrap_unchecked() };
+
+                Ok(val.complement())
+            }
+            _ => {
+                let (radix, digits) = strip_radix(src);
+                Self::from_str_radix(digits, radix).map_err(|_| EINVAL)
+            }
+        }
+    }
+}
+
+macro_rules! impl_parse_int {
+    ($ty:ty) => {
+        // SAFETY: We implement the trait according to the documentation.
+        unsafe impl private::FromStrRadix for $ty {
+            const MIN: Self = <$ty>::MIN;
+
+            fn from_str_radix(src: &BStr, radix: u32) -> Result<Self, crate::error::Error> {
+                <$ty>::from_str_radix(core::str::from_utf8(src).map_err(|_| EINVAL)?, radix)
+                    .map_err(|_| EINVAL)
+            }
+
+            fn abs_min() -> u64 {
+                #[allow(unused_comparisons)]
+                if Self::MIN < 0 {
+                    1u64 << (Self::BITS - 1)
+                } else {
+                    0
+                }
+            }
+
+            fn complement(self) -> Self {
+                (!self).wrapping_add((1 as $ty))
+            }
+        }
+
+        impl ParseInt for $ty {}
+    };
+}
+
+impl_parse_int!(i8);
+impl_parse_int!(u8);
+impl_parse_int!(i16);
+impl_parse_int!(u16);
+impl_parse_int!(i32);
+impl_parse_int!(u32);
+impl_parse_int!(i64);
+impl_parse_int!(u64);
+impl_parse_int!(isize);
+impl_parse_int!(usize);

-- 
2.47.2

[PATCH v13 2/6] rust: introduce module_param module

[Andreas Hindborg]

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

[PATCH v13 3/6] rust: module: use a reference in macros::module::module

[Andreas Hindborg]

When we add parameter support to the module macro, we want to be able to
pass a reference to `ModuleInfo` to a helper function. That is not possible
when we move out of the local `modinfo`. So change the function to access
the local via reference rather than value.

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/macros/module.rs | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/rust/macros/module.rs b/rust/macros/module.rs
index 2ddd2eeb2852..1a867a1e787e 100644
--- a/rust/macros/module.rs
+++ b/rust/macros/module.rs
@@ -179,26 +179,26 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
     // Rust does not allow hyphens in identifiers, use underscore instead.
     let ident = info.name.replace('-', "_");
     let mut modinfo = ModInfoBuilder::new(ident.as_ref());
-    if let Some(author) = info.author {
-        modinfo.emit("author", &author);
+    if let Some(author) = &info.author {
+        modinfo.emit("author", author);
     }
-    if let Some(authors) = info.authors {
+    if let Some(authors) = &info.authors {
         for author in authors {
-            modinfo.emit("author", &author);
+            modinfo.emit("author", author);
         }
     }
-    if let Some(description) = info.description {
-        modinfo.emit("description", &description);
+    if let Some(description) = &info.description {
+        modinfo.emit("description", description);
     }
     modinfo.emit("license", &info.license);
-    if let Some(aliases) = info.alias {
+    if let Some(aliases) = &info.alias {
         for alias in aliases {
-            modinfo.emit("alias", &alias);
+            modinfo.emit("alias", alias);
         }
     }
-    if let Some(firmware) = info.firmware {
+    if let Some(firmware) = &info.firmware {
         for fw in firmware {
-            modinfo.emit("firmware", &fw);
+            modinfo.emit("firmware", fw);
         }
     }
 

-- 
2.47.2

[PATCH v13 4/6] rust: module: update the module macro with module parameter support

[Andreas Hindborg]

Allow module parameters to be declared in the rust `module!` macro.

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/macros/helpers.rs |  25 +++++++
 rust/macros/lib.rs     |  31 +++++++++
 rust/macros/module.rs  | 175 ++++++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 221 insertions(+), 10 deletions(-)

diff --git a/rust/macros/helpers.rs b/rust/macros/helpers.rs
index e2602be402c1..365d7eb499c0 100644
--- a/rust/macros/helpers.rs
+++ b/rust/macros/helpers.rs
@@ -10,6 +10,17 @@ pub(crate) fn try_ident(it: &mut token_stream::IntoIter) -> Option<String> {
     }
 }
 
+pub(crate) fn try_sign(it: &mut token_stream::IntoIter) -> Option<char> {
+    let peek = it.clone().next();
+    match peek {
+        Some(TokenTree::Punct(punct)) if punct.as_char() == '-' => {
+            let _ = it.next();
+            Some(punct.as_char())
+        }
+        _ => None,
+    }
+}
+
 pub(crate) fn try_literal(it: &mut token_stream::IntoIter) -> Option<String> {
     if let Some(TokenTree::Literal(literal)) = it.next() {
         Some(literal.to_string())
@@ -103,3 +114,17 @@ pub(crate) fn file() -> String {
         proc_macro::Span::call_site().file()
     }
 }
+
+/// Parse a token stream of the form `expected_name: "value",` and return the
+/// string in the position of "value".
+///
+/// # Panics
+///
+/// - On parse error.
+pub(crate) fn expect_string_field(it: &mut token_stream::IntoIter, expected_name: &str) -> String {
+    assert_eq!(expect_ident(it), expected_name);
+    assert_eq!(expect_punct(it), ':');
+    let string = expect_string(it);
+    assert_eq!(expect_punct(it), ',');
+    string
+}
diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs
index fa847cf3a9b5..2fb520dc930a 100644
--- a/rust/macros/lib.rs
+++ b/rust/macros/lib.rs
@@ -28,6 +28,30 @@
 /// The `type` argument should be a type which implements the [`Module`]
 /// trait. Also accepts various forms of kernel metadata.
 ///
+/// The `params` field describe module parameters. Each entry has the form
+///
+/// ```ignore
+/// parameter_name: type {
+///     default: default_value,
+///     description: "Description",
+/// }
+/// ```
+///
+/// `type` may be one of
+///
+/// - [`i8`]
+/// - [`u8`]
+/// - [`i8`]
+/// - [`u8`]
+/// - [`i16`]
+/// - [`u16`]
+/// - [`i32`]
+/// - [`u32`]
+/// - [`i64`]
+/// - [`u64`]
+/// - [`isize`]
+/// - [`usize`]
+///
 /// C header: [`include/linux/moduleparam.h`](srctree/include/linux/moduleparam.h)
 ///
 /// [`Module`]: ../kernel/trait.Module.html
@@ -44,6 +68,12 @@
 ///     description: "My very own kernel module!",
 ///     license: "GPL",
 ///     alias: ["alternate_module_name"],
+///     params: {
+///         my_parameter: i64 {
+///             default: 1,
+///             description: "This parameter has a default of 1",
+///         },
+///     },
 /// }
 ///
 /// struct MyModule(i32);
@@ -52,6 +82,7 @@
 ///     fn init(_module: &'static ThisModule) -> Result<Self> {
 ///         let foo: i32 = 42;
 ///         pr_info!("I contain:  {}\n", foo);
+///         pr_info!("i32 param is:  {}\n", module_parameters::my_parameter.read());
 ///         Ok(Self(foo))
 ///     }
 /// }
diff --git a/rust/macros/module.rs b/rust/macros/module.rs
index 1a867a1e787e..b2605482822a 100644
--- a/rust/macros/module.rs
+++ b/rust/macros/module.rs
@@ -26,6 +26,7 @@ struct ModInfoBuilder<'a> {
     module: &'a str,
     counter: usize,
     buffer: String,
+    param_buffer: String,
 }
 
 impl<'a> ModInfoBuilder<'a> {
@@ -34,10 +35,11 @@ fn new(module: &'a str) -> Self {
             module,
             counter: 0,
             buffer: String::new(),
+            param_buffer: String::new(),
         }
     }
 
-    fn emit_base(&mut self, field: &str, content: &str, builtin: bool) {
+    fn emit_base(&mut self, field: &str, content: &str, builtin: bool, param: bool) {
         let string = if builtin {
             // Built-in modules prefix their modinfo strings by `module.`.
             format!(
@@ -51,8 +53,14 @@ fn emit_base(&mut self, field: &str, content: &str, builtin: bool) {
             format!("{field}={content}\0")
         };
 
+        let buffer = if param {
+            &mut self.param_buffer
+        } else {
+            &mut self.buffer
+        };
+
         write!(
-            &mut self.buffer,
+            buffer,
             "
                 {cfg}
                 #[doc(hidden)]
@@ -75,20 +83,116 @@ fn emit_base(&mut self, field: &str, content: &str, builtin: bool) {
         self.counter += 1;
     }
 
-    fn emit_only_builtin(&mut self, field: &str, content: &str) {
-        self.emit_base(field, content, true)
+    fn emit_only_builtin(&mut self, field: &str, content: &str, param: bool) {
+        self.emit_base(field, content, true, param)
     }
 
-    fn emit_only_loadable(&mut self, field: &str, content: &str) {
-        self.emit_base(field, content, false)
+    fn emit_only_loadable(&mut self, field: &str, content: &str, param: bool) {
+        self.emit_base(field, content, false, param)
     }
 
     fn emit(&mut self, field: &str, content: &str) {
-        self.emit_only_builtin(field, content);
-        self.emit_only_loadable(field, content);
+        self.emit_internal(field, content, false);
+    }
+
+    fn emit_internal(&mut self, field: &str, content: &str, param: bool) {
+        self.emit_only_builtin(field, content, param);
+        self.emit_only_loadable(field, content, param);
+    }
+
+    fn emit_param(&mut self, field: &str, param: &str, content: &str) {
+        let content = format!("{param}:{content}", param = param, content = content);
+        self.emit_internal(field, &content, true);
+    }
+
+    fn emit_params(&mut self, info: &ModuleInfo) {
+        let Some(params) = &info.params else {
+            return;
+        };
+
+        for param in params {
+            let ops = param_ops_path(&param.ptype);
+
+            // Note: The spelling of these fields is dictated by the user space
+            // tool `modinfo`.
+            self.emit_param("parmtype", &param.name, &param.ptype);
+            self.emit_param("parm", &param.name, &param.description);
+
+            write!(
+                self.param_buffer,
+                "
+                    pub(crate) static {param_name}:
+                        ::kernel::module_param::ModuleParamAccess<{param_type}> =
+                            ::kernel::module_param::ModuleParamAccess::new({param_default});
+
+                    #[link_section = \"__param\"]
+                    #[used]
+                    static __{module_name}_{param_name}_struct:
+                        ::kernel::module_param::RacyKernelParam =
+                        ::kernel::module_param::RacyKernelParam(::kernel::bindings::kernel_param {{
+                            name: if cfg!(MODULE) {{
+                                ::kernel::c_str!(\"{param_name}\").as_bytes_with_nul()
+                            }} else {{
+                                ::kernel::c_str!(\"{module_name}.{param_name}\").as_bytes_with_nul()
+                            }}.as_ptr(),
+                            // SAFETY: `__this_module` is constructed by the kernel at load time
+                            // and will not be freed until the module is unloaded.
+                            #[cfg(MODULE)]
+                            mod_: unsafe {{
+                                (&::kernel::bindings::__this_module
+                                    as *const ::kernel::bindings::module)
+                                    .cast_mut()
+                            }},
+                            #[cfg(not(MODULE))]
+                            mod_: ::core::ptr::null_mut(),
+                            ops: &{ops} as *const ::kernel::bindings::kernel_param_ops,
+                            perm: 0, // Will not appear in sysfs
+                            level: -1,
+                            flags: 0,
+                            __bindgen_anon_1:
+                                ::kernel::bindings::kernel_param__bindgen_ty_1 {{
+                                    arg: {param_name}.as_mut_ptr().cast()
+                                }},
+                        }});
+                ",
+                module_name = info.name,
+                param_type = param.ptype,
+                param_default = param.default,
+                param_name = param.name,
+                ops = ops,
+            )
+            .unwrap();
+        }
+    }
+}
+
+fn param_ops_path(param_type: &str) -> &'static str {
+    match param_type {
+        "i8" => "::kernel::module_param::PARAM_OPS_I8",
+        "u8" => "::kernel::module_param::PARAM_OPS_U8",
+        "i16" => "::kernel::module_param::PARAM_OPS_I16",
+        "u16" => "::kernel::module_param::PARAM_OPS_U16",
+        "i32" => "::kernel::module_param::PARAM_OPS_I32",
+        "u32" => "::kernel::module_param::PARAM_OPS_U32",
+        "i64" => "::kernel::module_param::PARAM_OPS_I64",
+        "u64" => "::kernel::module_param::PARAM_OPS_U64",
+        "isize" => "::kernel::module_param::PARAM_OPS_ISIZE",
+        "usize" => "::kernel::module_param::PARAM_OPS_USIZE",
+        t => panic!("Unsupported parameter type {}", t),
     }
 }
 
+fn expect_param_default(param_it: &mut token_stream::IntoIter) -> String {
+    assert_eq!(expect_ident(param_it), "default");
+    assert_eq!(expect_punct(param_it), ':');
+    let sign = try_sign(param_it);
+    let default = try_literal(param_it).expect("Expected default param value");
+    assert_eq!(expect_punct(param_it), ',');
+    let mut value = sign.map(String::from).unwrap_or_default();
+    value.push_str(&default);
+    value
+}
+
 #[derive(Debug, Default)]
 struct ModuleInfo {
     type_: String,
@@ -99,6 +203,50 @@ struct ModuleInfo {
     description: Option<String>,
     alias: Option<Vec<String>>,
     firmware: Option<Vec<String>>,
+    params: Option<Vec<Parameter>>,
+}
+
+#[derive(Debug)]
+struct Parameter {
+    name: String,
+    ptype: String,
+    default: String,
+    description: String,
+}
+
+fn expect_params(it: &mut token_stream::IntoIter) -> Vec<Parameter> {
+    let params = expect_group(it);
+    assert_eq!(params.delimiter(), Delimiter::Brace);
+    let mut it = params.stream().into_iter();
+    let mut parsed = Vec::new();
+
+    loop {
+        let param_name = match it.next() {
+            Some(TokenTree::Ident(ident)) => ident.to_string(),
+            Some(_) => panic!("Expected Ident or end"),
+            None => break,
+        };
+
+        assert_eq!(expect_punct(&mut it), ':');
+        let param_type = expect_ident(&mut it);
+        let group = expect_group(&mut it);
+        assert_eq!(group.delimiter(), Delimiter::Brace);
+        assert_eq!(expect_punct(&mut it), ',');
+
+        let mut param_it = group.stream().into_iter();
+        let param_default = expect_param_default(&mut param_it);
+        let param_description = expect_string_field(&mut param_it, "description");
+        expect_end(&mut param_it);
+
+        parsed.push(Parameter {
+            name: param_name,
+            ptype: param_type,
+            default: param_default,
+            description: param_description,
+        })
+    }
+
+    parsed
 }
 
 impl ModuleInfo {
@@ -114,6 +262,7 @@ fn parse(it: &mut token_stream::IntoIter) -> Self {
             "license",
             "alias",
             "firmware",
+            "params",
         ];
         const REQUIRED_KEYS: &[&str] = &["type", "name", "license"];
         let mut seen_keys = Vec::new();
@@ -140,6 +289,7 @@ fn parse(it: &mut token_stream::IntoIter) -> Self {
                 "license" => info.license = expect_string_ascii(it),
                 "alias" => info.alias = Some(expect_string_array(it)),
                 "firmware" => info.firmware = Some(expect_string_array(it)),
+                "params" => info.params = Some(expect_params(it)),
                 _ => panic!("Unknown key \"{key}\". Valid keys are: {EXPECTED_KEYS:?}."),
             }
 
@@ -205,7 +355,9 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
     // Built-in modules also export the `file` modinfo string.
     let file =
         std::env::var("RUST_MODFILE").expect("Unable to fetch RUST_MODFILE environmental variable");
-    modinfo.emit_only_builtin("file", &file);
+    modinfo.emit_only_builtin("file", &file, false);
+
+    modinfo.emit_params(&info);
 
     format!(
         "
@@ -369,15 +521,18 @@ unsafe fn __exit() {{
                             __MOD.assume_init_drop();
                         }}
                     }}
-
                     {modinfo}
                 }}
             }}
+            mod module_parameters {{
+                {params}
+            }}
         ",
         type_ = info.type_,
         name = info.name,
         ident = ident,
         modinfo = modinfo.buffer,
+        params = modinfo.param_buffer,
         initcall_section = ".initcall6.init"
     )
     .parse()

-- 
2.47.2

[PATCH v13 5/6] rust: samples: add a module parameter to the rust_minimal sample

[Andreas Hindborg]

Showcase the rust module parameter support by adding a module parameter to
the `rust_minimal` sample.

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 samples/rust/rust_minimal.rs | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/samples/rust/rust_minimal.rs b/samples/rust/rust_minimal.rs
index 1fc7a1be6b6d..c04cc07b3249 100644
--- a/samples/rust/rust_minimal.rs
+++ b/samples/rust/rust_minimal.rs
@@ -10,6 +10,12 @@
     authors: ["Rust for Linux Contributors"],
     description: "Rust minimal sample",
     license: "GPL",
+    params: {
+        test_parameter: i64 {
+            default: 1,
+            description: "This parameter has a default of 1",
+        },
+    },
 }
 
 struct RustMinimal {
@@ -20,6 +26,10 @@ impl kernel::Module for RustMinimal {
     fn init(_module: &'static ThisModule) -> Result<Self> {
         pr_info!("Rust minimal sample (init)\n");
         pr_info!("Am I built-in? {}\n", !cfg!(MODULE));
+        pr_info!(
+            "test_parameter: {}\n",
+            *module_parameters::test_parameter.get()
+        );
 
         let mut numbers = KVec::new();
         numbers.push(72, GFP_KERNEL)?;

-- 
2.47.2

[PATCH v13 6/6] modules: add rust modules files to MAINTAINERS

[Andreas Hindborg]

The module subsystem people agreed to maintain rust support for modules
[1]. Thus, add entries for relevant files to modules entry in MAINTAINERS.

Link: https://lore.kernel.org/all/0d9e596a-5316-4e00-862b-fd77552ae4b5@suse.com/ [1]

Acked-by: Daniel Gomez <da.gomez@samsung.com>
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index a92290fffa16..e3f43583c9c6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16794,6 +16794,8 @@ F:	include/linux/module*.h
 F:	kernel/module/
 F:	lib/test_kmod.c
 F:	lib/tests/module/
+F:	rust/kernel/module_param.rs
+F:	rust/macros/module.rs
 F:	scripts/module*
 F:	tools/testing/selftests/kmod/
 F:	tools/testing/selftests/module/

-- 
2.47.2

Re: [PATCH v13 5/6] rust: samples: add a module parameter to the rust_minimal sample

[Benno Lossin]

On Thu Jun 12, 2025 at 3:40 PM CEST, Andreas Hindborg wrote:
> Showcase the rust module parameter support by adding a module parameter to
> the `rust_minimal` sample.
>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>

Reviewed-by: Benno Lossin <lossin@kernel.org>

---
Cheers,
Benno

> ---
>  samples/rust/rust_minimal.rs | 10 ++++++++++
>  1 file changed, 10 insertions(+)

Re: [PATCH v13 3/6] rust: module: use a reference in macros::module::module

[Benno Lossin]

On Thu Jun 12, 2025 at 3:40 PM CEST, Andreas Hindborg wrote:
> When we add parameter support to the module macro, we want to be able to
> pass a reference to `ModuleInfo` to a helper function. That is not possible
> when we move out of the local `modinfo`. So change the function to access
> the local via reference rather than value.
>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>

Reviewed-by: Benno Lossin <lossin@kernel.org>

---
Cheers,
Benno

> ---
>  rust/macros/module.rs | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)

Re: [PATCH v13 1/6] rust: str: add radix prefixed integer parsing functions

[Benno Lossin]

On Thu Jun 12, 2025 at 3:40 PM CEST, Andreas Hindborg wrote:
> +pub trait ParseInt: private::FromStrRadix + TryFrom<u64> {
> +    /// Parse a string according to the description in [`Self`].
> +    fn from_str(src: &BStr) -> Result<Self> {
> +        match src.deref() {
> +            [b'-', rest @ ..] => {
> +                let (radix, digits) = strip_radix(rest.as_ref());
> +                // 2's complement values range from -2^(b-1) to 2^(b-1)-1.
> +                // So if we want to parse negative numbers as positive and
> +                // later multiply by -1, we have to parse into a larger
> +                // integer. We choose `u64` as sufficiently large.
> +                //
> +                // NOTE: 128 bit integers are not available on all
> +                // platforms, hence the choice of 64 bits.
> +                let val =
> +                    u64::from_str_radix(core::str::from_utf8(digits).map_err(|_| EINVAL)?, radix)
> +                        .map_err(|_| EINVAL)?;
> +
> +                if val > Self::abs_min() {
> +                    return Err(EINVAL);
> +                }
> +
> +                if val == Self::abs_min() {
> +                    return Ok(Self::MIN);
> +                }
> +
> +                // SAFETY: We checked that `val` will fit in `Self` above.

Sorry that it took me this long to realize, but this seems pretty weird.
I guess this is why the `FromStrRadix` is `unsafe`.

Can we just move this part of the code to `FromStrRadix` and make that
trait safe?

So essentially have:

    fn from_u64(value: u64) -> Result<Self>;

in `FromStrRadix` and remove `MIN`, `abs_min` and `complement`. Then
implement it like this in the macro below:

    const ABS_MIN = /* existing abs_min impl */;
    if value > ABS_MIN {
        return Err(EINVAL);
    }
    if val == ABS_MIN {
        return Ok(<$ty>::MIN);
    }
    // SAFETY: We checked that `val` will fit in `Self` above.
    let val: $ty = unsafe { val.try_into().unwrap_unchecked() };
    (!val).wrapping_add(1)

The reason that this is fine and the above is "weird" is the following:
The current version only has `Self: FromStrRadix` which gives it access
to the following guarantee from the `unsafe` trait:

    /// The member functions of this trait must be implemented according to
    /// their documentation.
    ///
    /// [`&BStr`]: kernel::str::BStr

This doesn't mention `TryFrom<u64>` and thus the comment "We checked
that `val` will fit in `Self` above" doesn't really apply: how does
checking with the bounds given in `FromStrRadix` make `TryFrom` return
`Ok`?

If we move this code into the implementation of `FromStrRadix`, then we
are locally in a context where we *know* the concrete type of `Self` and
can thus rely on "checking" being the correct thing for `TryFrom`.

With this adjustment, I can give my RB, but please let me take a look
before you send it again :)

---
Cheers,
Benno

> +                let val: Self = unsafe { val.try_into().unwrap_unchecked() };
> +
> +                Ok(val.complement())
> +            }
> +            _ => {
> +                let (radix, digits) = strip_radix(src);
> +                Self::from_str_radix(digits, radix).map_err(|_| EINVAL)
> +            }
> +        }
> +    }
> +}

Re: [PATCH v13 2/6] rust: introduce module_param module

[Benno Lossin]

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

Re: [PATCH v13 4/6] rust: module: update the module macro with module parameter support

[Benno Lossin]

On Thu Jun 12, 2025 at 3:40 PM CEST, Andreas Hindborg wrote:
> +
> +    fn emit_params(&mut self, info: &ModuleInfo) {
> +        let Some(params) = &info.params else {
> +            return;
> +        };
> +
> +        for param in params {
> +            let ops = param_ops_path(&param.ptype);
> +
> +            // Note: The spelling of these fields is dictated by the user space
> +            // tool `modinfo`.
> +            self.emit_param("parmtype", &param.name, &param.ptype);
> +            self.emit_param("parm", &param.name, &param.description);

I just read this part again and I want to voice my dissatisfaction with
these key names. (not that you can do anything about that :)

> +
> +            write!(
> +                self.param_buffer,
> +                "
> +                    pub(crate) static {param_name}:

Does this need to be accessed by anything else except the static below?
If no, then can we move it inside of that static? So

    #[link_section = \"__param\"]
    #[used]
    static __{module_name}_{param_name}_struct: ::kernel::module_param::RacyKernelParam = {
        static {param_name}:
            ::kernel::module_param::ModuleParamAccess<{param_type}> =
                ::kernel::module_param::ModuleParamAccess::new({param_default});
        // ...
    };

---
Cheers,
Benno

> +                        ::kernel::module_param::ModuleParamAccess<{param_type}> =
> +                            ::kernel::module_param::ModuleParamAccess::new({param_default});
> +
> +                    #[link_section = \"__param\"]
> +                    #[used]
> +                    static __{module_name}_{param_name}_struct:
> +                        ::kernel::module_param::RacyKernelParam =
> +                        ::kernel::module_param::RacyKernelParam(::kernel::bindings::kernel_param {{
> +                            name: if cfg!(MODULE) {{
> +                                ::kernel::c_str!(\"{param_name}\").as_bytes_with_nul()
> +                            }} else {{
> +                                ::kernel::c_str!(\"{module_name}.{param_name}\").as_bytes_with_nul()
> +                            }}.as_ptr(),
> +                            // SAFETY: `__this_module` is constructed by the kernel at load time
> +                            // and will not be freed until the module is unloaded.
> +                            #[cfg(MODULE)]
> +                            mod_: unsafe {{
> +                                (&::kernel::bindings::__this_module
> +                                    as *const ::kernel::bindings::module)
> +                                    .cast_mut()
> +                            }},
> +                            #[cfg(not(MODULE))]
> +                            mod_: ::core::ptr::null_mut(),
> +                            ops: &{ops} as *const ::kernel::bindings::kernel_param_ops,
> +                            perm: 0, // Will not appear in sysfs
> +                            level: -1,
> +                            flags: 0,
> +                            __bindgen_anon_1:
> +                                ::kernel::bindings::kernel_param__bindgen_ty_1 {{
> +                                    arg: {param_name}.as_mut_ptr().cast()
> +                                }},
> +                        }});
> +                ",
> +                module_name = info.name,
> +                param_type = param.ptype,
> +                param_default = param.default,
> +                param_name = param.name,
> +                ops = ops,
> +            )
> +            .unwrap();
> +        }
> +    }
> +}

Re: [PATCH v13 1/6] rust: str: add radix prefixed integer parsing functions

[Andreas Hindborg]

"Benno Lossin" <lossin@kernel.org> writes:

> On Thu Jun 12, 2025 at 3:40 PM CEST, Andreas Hindborg wrote:
>> +pub trait ParseInt: private::FromStrRadix + TryFrom<u64> {
>> +    /// Parse a string according to the description in [`Self`].
>> +    fn from_str(src: &BStr) -> Result<Self> {
>> +        match src.deref() {
>> +            [b'-', rest @ ..] => {
>> +                let (radix, digits) = strip_radix(rest.as_ref());
>> +                // 2's complement values range from -2^(b-1) to 2^(b-1)-1.
>> +                // So if we want to parse negative numbers as positive and
>> +                // later multiply by -1, we have to parse into a larger
>> +                // integer. We choose `u64` as sufficiently large.
>> +                //
>> +                // NOTE: 128 bit integers are not available on all
>> +                // platforms, hence the choice of 64 bits.
>> +                let val =
>> +                    u64::from_str_radix(core::str::from_utf8(digits).map_err(|_| EINVAL)?, radix)
>> +                        .map_err(|_| EINVAL)?;
>> +
>> +                if val > Self::abs_min() {
>> +                    return Err(EINVAL);
>> +                }
>> +
>> +                if val == Self::abs_min() {
>> +                    return Ok(Self::MIN);
>> +                }
>> +
>> +                // SAFETY: We checked that `val` will fit in `Self` above.
>
> Sorry that it took me this long to realize, but this seems pretty weird.
> I guess this is why the `FromStrRadix` is `unsafe`.
>
> Can we just move this part of the code to `FromStrRadix` and make that
> trait safe?
>
> So essentially have:
>
>     fn from_u64(value: u64) -> Result<Self>;
>
> in `FromStrRadix` and remove `MIN`, `abs_min` and `complement`. Then
> implement it like this in the macro below:
>
>     const ABS_MIN = /* existing abs_min impl */;
>     if value > ABS_MIN {
>         return Err(EINVAL);
>     }
>     if val == ABS_MIN {
>         return Ok(<$ty>::MIN);
>     }
>     // SAFETY: We checked that `val` will fit in `Self` above.
>     let val: $ty = unsafe { val.try_into().unwrap_unchecked() };
>     (!val).wrapping_add(1)
>
> The reason that this is fine and the above is "weird" is the following:
> The current version only has `Self: FromStrRadix` which gives it access
> to the following guarantee from the `unsafe` trait:
>
>     /// The member functions of this trait must be implemented according to
>     /// their documentation.
>     ///
>     /// [`&BStr`]: kernel::str::BStr
>
> This doesn't mention `TryFrom<u64>` and thus the comment "We checked
> that `val` will fit in `Self` above" doesn't really apply: how does
> checking with the bounds given in `FromStrRadix` make `TryFrom` return
> `Ok`?

I'm having a difficult time parsing. Are you suggesting that we guard
against implementations of `TryInto<u64>` that misbehave?


Best regards,
Andreas Hindborg

Re: [PATCH v13 1/6] rust: str: add radix prefixed integer parsing functions

[Benno Lossin]

On Thu Jun 19, 2025 at 1:12 PM CEST, Andreas Hindborg wrote:
> I'm having a difficult time parsing. Are you suggesting that we guard
> against implementations of `TryInto<u64>` that misbehave?

Let me try a different explanation:

The safety requirement for implementing the `FromStrRadix`:

    /// The member functions of this trait must be implemented according to
    /// their documentation.

Together with the functions of the trait:

    /// Parse `src` to [`Self`] using radix `radix`.
    fn from_str_radix(src: &BStr, radix: u32) -> Result<Self, crate::error::Error>;
    
    /// Return the absolute value of [`Self::MIN`].
    fn abs_min() -> u64;
    
    /// Perform bitwise 2's complement on `self`.
    ///
    /// Note: This function does not make sense for unsigned integers.
    fn complement(self) -> Self;

Doesn't make sense. What does it mean to return the "absolute value of
[`Self::MIN`]"? We don't have "absolute value" defined for an arbitrary
type. Similarly for `complement` and `from_str_radix`, what does "Parse
`src` to [`Self`] using radex `radix`" mean? It's not well-defined.

You use this safety requirement in the parsing branch for negative
numbers (the `unsafe` call at the bottom):

    [b'-', rest @ ..] => {
        let (radix, digits) = strip_radix(rest.as_ref());
        // 2's complement values range from -2^(b-1) to 2^(b-1)-1.
        // So if we want to parse negative numbers as positive and
        // later multiply by -1, we have to parse into a larger
        // integer. We choose `u64` as sufficiently large.
        //
        // NOTE: 128 bit integers are not available on all
        // platforms, hence the choice of 64 bits.
        let val =
            u64::from_str_radix(core::str::from_utf8(digits).map_err(|_| EINVAL)?, radix)
                .map_err(|_| EINVAL)?;
    
        if val > Self::abs_min() {
            return Err(EINVAL);
        }
    
        if val == Self::abs_min() {
            return Ok(Self::MIN);
        }
    
        // SAFETY: We checked that `val` will fit in `Self` above.
        let val: Self = unsafe { val.try_into().unwrap_unchecked() };
    
        Ok(val.complement())
    }

But you don't mention that the check is valid due to the safety
requirements of implementing `FromStrRadix`. But even if you did, that
wouldn't mean anything as I explained above.

So let's instead move all of this negation & u64 conversion logic into
the `FromStrRadix` trait. Then it can be safe & the `ParseInt::from_str`
function doesn't use `unsafe` (there still will be `unsafe` in the
macro, but that is fine, as it's more local and knows the concrete
types).

---
Cheers,
Benno

Here is what I have in mind:

diff --git a/rust/kernel/str/parse_int.rs b/rust/kernel/str/parse_int.rs
index 0754490aec4b..9d6e146c5ea7 100644
--- a/rust/kernel/str/parse_int.rs
+++ b/rust/kernel/str/parse_int.rs
@@ -13,32 +13,16 @@
 // `ParseInt`, that is, prevents downstream users from implementing the
 // trait.
 mod private {
+    use crate::prelude::*;
     use crate::str::BStr;
 
     /// Trait that allows parsing a [`&BStr`] to an integer with a radix.
-    ///
-    /// # Safety
-    ///
-    /// The member functions of this trait must be implemented according to
-    /// their documentation.
-    ///
-    /// [`&BStr`]: kernel::str::BStr
-    // This is required because the `from_str_radix` function on the primitive
-    // integer types is not part of any trait.
-    pub unsafe trait FromStrRadix: Sized {
-        /// The minimum value this integer type can assume.
-        const MIN: Self;
-
+    pub trait FromStrRadix: Sized {
         /// Parse `src` to [`Self`] using radix `radix`.
-        fn from_str_radix(src: &BStr, radix: u32) -> Result<Self, crate::error::Error>;
-
-        /// Return the absolute value of [`Self::MIN`].
-        fn abs_min() -> u64;
+        fn from_str_radix(src: &BStr, radix: u32) -> Result<Self>;
 
-        /// Perform bitwise 2's complement on `self`.
-        ///
-        /// Note: This function does not make sense for unsigned integers.
-        fn complement(self) -> Self;
+        /// Tries to convert `value` into [`Self`] and negates the resulting value.
+        fn from_u64_negated(value: u64) -> Result<Self>;
     }
 }
 
@@ -108,19 +92,7 @@ fn from_str(src: &BStr) -> Result<Self> {
                 let val =
                     u64::from_str_radix(core::str::from_utf8(digits).map_err(|_| EINVAL)?, radix)
                         .map_err(|_| EINVAL)?;
-
-                if val > Self::abs_min() {
-                    return Err(EINVAL);
-                }
-
-                if val == Self::abs_min() {
-                    return Ok(Self::MIN);
-                }
-
-                // SAFETY: We checked that `val` will fit in `Self` above.
-                let val: Self = unsafe { val.try_into().unwrap_unchecked() };
-
-                Ok(val.complement())
+                Self::from_u64_negated(val)
             }
             _ => {
                 let (radix, digits) = strip_radix(src);
@@ -131,41 +103,49 @@ fn from_str(src: &BStr) -> Result<Self> {
 }
 
 macro_rules! impl_parse_int {
-    ($ty:ty) => {
-        // SAFETY: We implement the trait according to the documentation.
-        unsafe impl private::FromStrRadix for $ty {
-            const MIN: Self = <$ty>::MIN;
-
-            fn from_str_radix(src: &BStr, radix: u32) -> Result<Self, crate::error::Error> {
-                <$ty>::from_str_radix(core::str::from_utf8(src).map_err(|_| EINVAL)?, radix)
-                    .map_err(|_| EINVAL)
-            }
-
-            fn abs_min() -> u64 {
-                #[allow(unused_comparisons)]
-                if Self::MIN < 0 {
-                    1u64 << (Self::BITS - 1)
-                } else {
-                    0
+    ($($ty:ty),*) => {
+        $(
+            impl private::FromStrRadix for $ty {
+                fn from_str_radix(src: &BStr, radix: u32) -> Result<Self> {
+                    <$ty>::from_str_radix(core::str::from_utf8(src).map_err(|_| EINVAL)?, radix)
+                        .map_err(|_| EINVAL)
                 }
-            }
 
-            fn complement(self) -> Self {
-                (!self).wrapping_add((1 as $ty))
+                fn from_u64_negated(value: u64) -> Result<Self> {
+                    const ABS_MIN: u64 = {
+                        #[allow(unused_comparisons)]
+                        if <$ty>::MIN < 0 {
+                            1u64 << (Self::BITS - 1)
+                        } else {
+                            0
+                        }
+                    };
+
+                    fn complement(self) -> Self {
+                        (!self).wrapping_add((1 as $ty))
+                    }
+                    if val > ABS_MIN {
+                        return Err(EINVAL);
+                    }
+
+                    if val == ABS_MIN {
+                        return Ok(<$ty>::MIN);
+                    }
+
+                    // SAFETY: The above checks guarantee that `val` fits into `Self`:
+                    // - if `Self` is unsigned, then `ABS_MIN == 0` and thus we have returned above
+                    //   (either `EINVAL` or `MIN`).
+                    // - if `Self` is signed, then we have that `0 <= val < ABS_MIN`. And since
+                    //   `ABS_MIN - 1` fits into `Self` by construction, `val` also does.
+                    let val: Self = unsafe { val.try_into().unwrap_unchecked() };
+
+                    Ok((!val).wrapping_add(1))
+                }
             }
-        }
 
-        impl ParseInt for $ty {}
+            impl ParseInt for $ty {}
+        )*
     };
 }
 
-impl_parse_int!(i8);
-impl_parse_int!(u8);
-impl_parse_int!(i16);
-impl_parse_int!(u16);
-impl_parse_int!(i32);
-impl_parse_int!(u32);
-impl_parse_int!(i64);
-impl_parse_int!(u64);
-impl_parse_int!(isize);
-impl_parse_int!(usize);
+impl_parse_int![i8, u8, i16, u16, i32, u32, i64, u64, isize, usize];

Re: [PATCH v13 2/6] rust: introduce module_param module

[Andreas Hindborg]

"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

Re: [PATCH v13 4/6] rust: module: update the module macro with module parameter support

[Andreas Hindborg]

"Benno Lossin" <lossin@kernel.org> writes:

> On Thu Jun 12, 2025 at 3:40 PM CEST, Andreas Hindborg wrote:
>> +
>> +    fn emit_params(&mut self, info: &ModuleInfo) {
>> +        let Some(params) = &info.params else {
>> +            return;
>> +        };
>> +
>> +        for param in params {
>> +            let ops = param_ops_path(&param.ptype);
>> +
>> +            // Note: The spelling of these fields is dictated by the user space
>> +            // tool `modinfo`.
>> +            self.emit_param("parmtype", &param.name, &param.ptype);
>> +            self.emit_param("parm", &param.name, &param.description);
>
> I just read this part again and I want to voice my dissatisfaction with
> these key names. (not that you can do anything about that :)
>
>> +
>> +            write!(
>> +                self.param_buffer,
>> +                "
>> +                    pub(crate) static {param_name}:
>
> Does this need to be accessed by anything else except the static below?
> If no, then can we move it inside of that static? So
>
>     #[link_section = \"__param\"]
>     #[used]
>     static __{module_name}_{param_name}_struct: ::kernel::module_param::RacyKernelParam = {
>         static {param_name}:
>             ::kernel::module_param::ModuleParamAccess<{param_type}> =
>                 ::kernel::module_param::ModuleParamAccess::new({param_default});
>         // ...
>     };

It is used to access the value of the parameter from the module. If you
reduce visibility you will get an error when trying to read the
parameter value:

    RUSTC     samples/rust/rust_minimal.o
  error[E0425]: cannot find value `test_parameter` in module `module_parameters`
    --> /home/aeh/src/linux-rust/module-params/samples/rust/rust_minimal.rs:31:33
    |
  31 |             *module_parameters::test_parameter.get()
    |                                 ^^^^^^^^^^^^^^ not found in `module_parameters`

  error: aborting due to 1 previous error


Best regards,
Andreas Hindborg

Re: [PATCH v13 1/6] rust: str: add radix prefixed integer parsing functions

[Andreas Hindborg]

"Benno Lossin" <lossin@kernel.org> writes:

> On Thu Jun 19, 2025 at 1:12 PM CEST, Andreas Hindborg wrote:
>> I'm having a difficult time parsing. Are you suggesting that we guard
>> against implementations of `TryInto<u64>` that misbehave?
>
> Let me try a different explanation:
>
> The safety requirement for implementing the `FromStrRadix`:
>
>     /// The member functions of this trait must be implemented according to
>     /// their documentation.
>
> Together with the functions of the trait:
>
>     /// Parse `src` to [`Self`] using radix `radix`.
>     fn from_str_radix(src: &BStr, radix: u32) -> Result<Self, crate::error::Error>;
>
>     /// Return the absolute value of [`Self::MIN`].
>     fn abs_min() -> u64;
>
>     /// Perform bitwise 2's complement on `self`.
>     ///
>     /// Note: This function does not make sense for unsigned integers.
>     fn complement(self) -> Self;
>
> Doesn't make sense. What does it mean to return the "absolute value of
> [`Self::MIN`]"? We don't have "absolute value" defined for an arbitrary
> type. Similarly for `complement` and `from_str_radix`, what does "Parse
> `src` to [`Self`] using radex `radix`" mean? It's not well-defined.
>
> You use this safety requirement in the parsing branch for negative
> numbers (the `unsafe` call at the bottom):
>
>     [b'-', rest @ ..] => {
>         let (radix, digits) = strip_radix(rest.as_ref());
>         // 2's complement values range from -2^(b-1) to 2^(b-1)-1.
>         // So if we want to parse negative numbers as positive and
>         // later multiply by -1, we have to parse into a larger
>         // integer. We choose `u64` as sufficiently large.
>         //
>         // NOTE: 128 bit integers are not available on all
>         // platforms, hence the choice of 64 bits.
>         let val =
>             u64::from_str_radix(core::str::from_utf8(digits).map_err(|_| EINVAL)?, radix)
>                 .map_err(|_| EINVAL)?;
>
>         if val > Self::abs_min() {
>             return Err(EINVAL);
>         }
>
>         if val == Self::abs_min() {
>             return Ok(Self::MIN);
>         }
>
>         // SAFETY: We checked that `val` will fit in `Self` above.
>         let val: Self = unsafe { val.try_into().unwrap_unchecked() };
>
>         Ok(val.complement())
>     }
>
> But you don't mention that the check is valid due to the safety
> requirements of implementing `FromStrRadix`. But even if you did, that
> wouldn't mean anything as I explained above.
>
> So let's instead move all of this negation & u64 conversion logic into
> the `FromStrRadix` trait. Then it can be safe & the `ParseInt::from_str`
> function doesn't use `unsafe` (there still will be `unsafe` in the
> macro, but that is fine, as it's more local and knows the concrete
> types).
>

Alright. I guess my safety comments are slightly hand-wavy. Thanks for
the suggestion, I'll apply that for next spin.

Best regards,
Andreas Hindborg

Re: [PATCH v13 2/6] rust: introduce module_param module

[Benno Lossin]

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

Re: [PATCH v13 2/6] rust: introduce module_param module

[Benno Lossin]

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? 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()
> +    }
> +}

Re: [PATCH v13 2/6] rust: introduce module_param module

[Andreas Hindborg]

"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

Re: [PATCH v13 2/6] rust: introduce module_param module

[Andreas Hindborg]

"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

Re: [PATCH v13 2/6] rust: introduce module_param module

[Andreas Hindborg]

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

Re: [PATCH v13 2/6] rust: introduce module_param module

[Benno Lossin]

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

Re: [PATCH v13 2/6] rust: introduce module_param module

[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

Re: [PATCH v13 2/6] rust: introduce module_param module

[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`)?

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

Re: [PATCH v13 2/6] rust: introduce module_param module

[Benno Lossin]

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

Re: [PATCH v13 2/6] rust: introduce module_param module

[Miguel Ojeda]

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

Re: [PATCH v13 2/6] rust: introduce module_param module

[Benno Lossin]

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

Re: [PATCH v13 2/6] rust: introduce module_param module

[Andreas Hindborg]

"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

Re: [PATCH v13 2/6] rust: introduce module_param module

[Benno Lossin]

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

Re: [PATCH v13 2/6] rust: introduce module_param module

[Andreas Hindborg]

"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

Re: [PATCH v13 2/6] rust: introduce module_param module

[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

Re: [PATCH v13 2/6] rust: introduce module_param module

[Benno Lossin]

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

Re: [PATCH v13 2/6] rust: introduce module_param module

[Andreas Hindborg]

"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

Re: [PATCH v13 5/6] rust: samples: add a module parameter to the rust_minimal sample

[Danilo Krummrich]

(Sorry for being late on this one, just a minor nit below.)

On 6/12/25 3:40 PM, Andreas Hindborg wrote:
>   struct RustMinimal {
> @@ -20,6 +26,10 @@ impl kernel::Module for RustMinimal {
>       fn init(_module: &'static ThisModule) -> Result<Self> {
>           pr_info!("Rust minimal sample (init)\n");
>           pr_info!("Am I built-in? {}\n", !cfg!(MODULE));
> +        pr_info!(
> +            "test_parameter: {}\n",
> +            *module_parameters::test_parameter.get()

Can we please call it something else than get(), maybe obtain(), access() or
just ref()?

> +        );
>   
>           let mut numbers = KVec::new();
>           numbers.push(72, GFP_KERNEL)?;
> 

Re: [PATCH v13 5/6] rust: samples: add a module parameter to the rust_minimal sample

[Andreas Hindborg]

"Danilo Krummrich" <dakr@kernel.org> writes:

> (Sorry for being late on this one, just a minor nit below.)
>
> On 6/12/25 3:40 PM, Andreas Hindborg wrote:
>>   struct RustMinimal {
>> @@ -20,6 +26,10 @@ impl kernel::Module for RustMinimal {
>>       fn init(_module: &'static ThisModule) -> Result<Self> {
>>           pr_info!("Rust minimal sample (init)\n");
>>           pr_info!("Am I built-in? {}\n", !cfg!(MODULE));
>> +        pr_info!(
>> +            "test_parameter: {}\n",
>> +            *module_parameters::test_parameter.get()
>
> Can we please call it something else than get(), maybe obtain(), access() or
> just ref()?

Probably `ref` is the most precise of the options you propose. I would
go with that one. Or, should it be `as_ref`?


Best regards,
Andreas Hindborg

Re: [PATCH v13 5/6] rust: samples: add a module parameter to the rust_minimal sample

[Danilo Krummrich]

On 6/30/25 2:12 PM, Andreas Hindborg wrote:
> "Danilo Krummrich" <dakr@kernel.org> writes:
> 
>> (Sorry for being late on this one, just a minor nit below.)
>>
>> On 6/12/25 3:40 PM, Andreas Hindborg wrote:
>>>    struct RustMinimal {
>>> @@ -20,6 +26,10 @@ impl kernel::Module for RustMinimal {
>>>        fn init(_module: &'static ThisModule) -> Result<Self> {
>>>            pr_info!("Rust minimal sample (init)\n");
>>>            pr_info!("Am I built-in? {}\n", !cfg!(MODULE));
>>> +        pr_info!(
>>> +            "test_parameter: {}\n",
>>> +            *module_parameters::test_parameter.get()
>>
>> Can we please call it something else than get(), maybe obtain(), access() or
>> just ref()?
> 
> Probably `ref` is the most precise of the options you propose. I would
> go with that one. Or, should it be `as_ref`?

Guess that works as well.

One question additional question: Can't we just impl Deref for
ModuleParamAccess<T>?

Re: [PATCH v13 5/6] rust: samples: add a module parameter to the rust_minimal sample

[Danilo Krummrich]

On 6/30/25 2:18 PM, Danilo Krummrich wrote:
> On 6/30/25 2:12 PM, Andreas Hindborg wrote:
>> "Danilo Krummrich" <dakr@kernel.org> writes:
>>
>>> (Sorry for being late on this one, just a minor nit below.)
>>>
>>> On 6/12/25 3:40 PM, Andreas Hindborg wrote:
>>>>    struct RustMinimal {
>>>> @@ -20,6 +26,10 @@ impl kernel::Module for RustMinimal {
>>>>        fn init(_module: &'static ThisModule) -> Result<Self> {
>>>>            pr_info!("Rust minimal sample (init)\n");
>>>>            pr_info!("Am I built-in? {}\n", !cfg!(MODULE));
>>>> +        pr_info!(
>>>> +            "test_parameter: {}\n",
>>>> +            *module_parameters::test_parameter.get()
>>>
>>> Can we please call it something else than get(), maybe obtain(), access() or
>>> just ref()?
>>
>> Probably `ref` is the most precise of the options you propose. I would
>> go with that one. Or, should it be `as_ref`?
> 
> Guess that works as well.
> 
> One question additional question: Can't we just impl Deref for
> ModuleParamAccess<T>?

Just notice that it would work for now, but not in the future, because this will
apparently require some lock guard? Then maybe access() describes it best?

Re: [PATCH v13 2/6] rust: introduce module_param module

[Benno Lossin]

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

Re: [PATCH v13 5/6] rust: samples: add a module parameter to the rust_minimal sample

[Benno Lossin]

On Mon Jun 30, 2025 at 2:23 PM CEST, Danilo Krummrich wrote:
> On 6/30/25 2:18 PM, Danilo Krummrich wrote:
>> On 6/30/25 2:12 PM, Andreas Hindborg wrote:
>>> "Danilo Krummrich" <dakr@kernel.org> writes:
>>>
>>>> (Sorry for being late on this one, just a minor nit below.)
>>>>
>>>> On 6/12/25 3:40 PM, Andreas Hindborg wrote:
>>>>>    struct RustMinimal {
>>>>> @@ -20,6 +26,10 @@ impl kernel::Module for RustMinimal {
>>>>>        fn init(_module: &'static ThisModule) -> Result<Self> {
>>>>>            pr_info!("Rust minimal sample (init)\n");
>>>>>            pr_info!("Am I built-in? {}\n", !cfg!(MODULE));
>>>>> +        pr_info!(
>>>>> +            "test_parameter: {}\n",
>>>>> +            *module_parameters::test_parameter.get()
>>>>
>>>> Can we please call it something else than get(), maybe obtain(), access() or
>>>> just ref()?
>>>
>>> Probably `ref` is the most precise of the options you propose. I would
>>> go with that one. Or, should it be `as_ref`?
>> 
>> Guess that works as well.
>> 
>> One question additional question: Can't we just impl Deref for
>> ModuleParamAccess<T>?
>
> Just notice that it would work for now, but not in the future, because this will
> apparently require some lock guard? Then maybe access() describes it best?

What about `value()`?

---
Cheers,
Benno

Re: [PATCH v13 2/6] rust: introduce module_param module

[Andreas Hindborg]

"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

Re: [PATCH v13 2/6] rust: introduce module_param module

[Benno Lossin]

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

Re: [PATCH v13 2/6] rust: introduce module_param module

[Andreas Hindborg]

"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

Re: [PATCH v13 2/6] rust: introduce module_param module

[Benno Lossin]

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

Re: [PATCH v13 2/6] rust: introduce module_param module

[Andreas Hindborg]

"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

Re: [PATCH v13 2/6] rust: introduce module_param module

[Benno Lossin]

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

Re: [PATCH v13 2/6] rust: introduce module_param module

[Miguel Ojeda]

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

Re: [PATCH v13 2/6] rust: introduce module_param module

[Benno Lossin]

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

Re: [PATCH v13 2/6] rust: introduce module_param module

[Andreas Hindborg]

"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

Re: [PATCH v13 2/6] rust: introduce module_param module

[Andreas Hindborg]

"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

Re: [PATCH v13 2/6] rust: introduce module_param module

[Andreas Hindborg]

"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

Re: [PATCH v13 2/6] rust: introduce module_param module

[Benno Lossin]

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