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

[Andreas Hindborg <a.hindborg@kernel.org>]

Andreas Hindborg <a.hindborg@kernel.org> writes:

> "Benno Lossin" <lossin@kernel.org> writes:
>
>> On Tue May 6, 2025 at 3:02 PM CEST, Andreas Hindborg wrote:
>>> 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 <da.gomez@samsung.com>
>>> 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.
>
>> [...]
>>
>>> +/// 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.
>
>>
>>> +/// - `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".
>
>>
>>> +///
>>> +/// [`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.
>
>>
>>> +    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.
>
>>
>>> +        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.
>
>>
>>> +        unsafe { *old_value = new_value };
>>> +        Ok(0)
>>> +    })
>>> +}
>>
>> [...]
>>
>>> +#[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.
>
>>
>>> +        /// 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