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

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

"Alice Ryhl" <aliceryhl@google.com> writes:

> On Fri, May 02, 2025 at 02:16:35PM +0200, 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>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>
>> +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.
>> +    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`.
>> +        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.
>> +        unsafe { core::ptr::replace(old_value, new_value) };
>
> You don't use the return value of this, so this is equivalent to
> unsafe { *old_value = new_value };

Thanks.

>
>> +macro_rules! make_param_ops {
>> +    ($ops:ident, $ty:ty) => {
>> +        ///
>> +        /// Static [`kernel_param_ops`](srctree/include/linux/moduleparam.h)
>> +        /// struct generated by `make_param_ops`
>> +        #[doc = concat!("for [`", stringify!($ty), "`].")]
>> +        pub static $ops: $crate::bindings::kernel_param_ops = $crate::bindings::kernel_param_ops {
>> +            flags: 0,
>> +            set: Some(set_param::<$ty>),
>> +            get: None,
>> +            free: Some(free::<$ty>),
>
> You could potentially only include `free` if
> `core::mem::needs_drop::<T>()` as an optimization.

Right, nice 👍

>
>> +    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});
>
> Is this global accessible to the user?

Yes.

> It would be a use-after-free to
> access it during module teardown. For example, what if I access this
> static during its own destructor? Or during the destructor of another
> module parameter?

Yes, that is a problem.

We can get around it for now by just not calling `free` for now. We only
support simple types that do not need drop. I think we would have to
seal the `ModuleParam` trait for this.

For a proper solution, we could
 - Require a token to read the parameter.
 - Synchronize on a module private field and return an option from the
   parameter getter. This would require module exit to run before param
   free. I think this is the case, but I did not check.
 - Use a `Revocable` and revoke the parameter in `free`.

Any other ideas or comments on the outlined solutions?


Best regards,
Andreas Hindborg