Re: [PATCH v12 2/3] rust: add parameter support to the `module!` macro

Andreas Hindborg [off-list ref] writes:

"Benno Lossin" [off-list ref] writes:

quoted

On Tue May 6, 2025 at 3:02 PM CEST, Andreas Hindborg wrote:

quoted

Add support for module parameters to the `module!` macro. Implement read
only support for integer types without `sysfs` support.

Acked-by: Petr Pavlu <petr.pavlu@suse.com> # from modules perspective
Tested-by: Daniel Gomez <redacted>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/lib.rs           |   1 +
 rust/kernel/module_param.rs  | 204 +++++++++++++++++++++++++++++++++++++++++++
 rust/macros/helpers.rs       |  25 ++++++
 rust/macros/lib.rs           |  31 +++++++
 rust/macros/module.rs        | 195 ++++++++++++++++++++++++++++++++++++-----
 samples/rust/rust_minimal.rs |  10 +++

I know this is already the 12th version, but I think this patch should
be split into the module_param module introduction, proc-macro
modifications and the sample changes.

OK.

quoted

[...]

quoted

+/// 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.
+///   The `arg` field of `param` must be an instance of `T`.

This sentence is conveying the same (or at least similar) requirement as
the bullet point below.

Yes, you are right. At any rate the wording is wrong, a pointer cannot
"be an instance", it can point to one.

quoted

quoted

+/// - `param.arg` must be a pointer to valid `*mut T` as set up by the
+///   [`module!`] macro.
+///
+/// # Invariants
+///
+/// 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.

Why is this an Invariants section?

Looks like a mistake, I'll change it to "# Note".

quoted

quoted

+///
+/// [`module!`]: macros::module
+unsafe extern "C" fn set_param<T>(
+    val: *const kernel::ffi::c_char,
+    param: *const crate::bindings::kernel_param,
+) -> core::ffi::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 and
+    // null-terminated. By C API contract, `val` is live and valid for reads
+    // for the duration of this function.

We shouldn't mention "C API contract" here and move the liveness
requirement to the safety requirements of the function. Or change the
safety requirements for this function to only be called from some
specific C API.

OK.

quoted

quoted

+    let arg = unsafe { CStr::from_char_ptr(val) };
+
+    crate::error::from_result(|| {
+        let new_value = T::try_from_param_arg(arg)?;
+
+        // SAFETY: `param` is guaranteed to be valid by C API contract
+        // and `arg` is guaranteed to point to an instance of `T`.

Ditto.

OK.

quoted

quoted

+        let old_value = unsafe { (*param).__bindgen_anon_1.arg as *mut T };
+
+        // SAFETY: `old_value` is valid for writes, as we have exclusive
+        // access. `old_value` is pointing to an initialized static, and
+        // so it is properly initialized.

Why do we have exclusive access? Should be in the safety requirements.

Will add this.

quoted

quoted

+        unsafe { *old_value = new_value };
+        Ok(0)
+    })
+}

[...]

quoted

+#[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) => {
+        ///

Spurious newline?

Will remove.

quoted

quoted

+        /// Static [`kernel_param_ops`](srctree/include/linux/moduleparam.h)
+        /// struct generated by `make_param_ops`

Is it useful for this fact to be in the docs? I'd remove it.

OK.

Clippy thinks it is useful:

    error: missing documentation for a static
      --> /home/aeh/src/linux-rust/module-params/rust/kernel/module_param.rs:182:9
        |
    182 |         pub static $ops: $crate::bindings::kernel_param_ops = $crate::bindings::kernel_param_ops {
        |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    ...
    191 | make_param_ops!(PARAM_OPS_I8, i8);
        | --------------------------------- in this macro invocation
        |

So either we need to `#[allow(missing_docs)]` or just add the line back. What do you prefer?


Best regards,
Andreas Hindborg