linux-kernel - Re: [PATCH v14 5/7] rust: module: update the module macro with module parameter support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <875xg8rvei.fsf@kernel.org>
Date: Fri, 04 Jul 2025 14:29:57 +0200
From: Andreas Hindborg <a.hindborg@...nel.org>
To: "Benno Lossin" <lossin@...nel.org>
Cc: "Miguel Ojeda" <ojeda@...nel.org>,  "Alex Gaynor"
 <alex.gaynor@...il.com>,  "Boqun Feng" <boqun.feng@...il.com>,  "Gary Guo"
 <gary@...yguo.net>,  Björn Roy Baron
 <bjorn3_gh@...tonmail.com>,  "Alice
 Ryhl" <aliceryhl@...gle.com>,  "Masahiro Yamada" <masahiroy@...nel.org>,
  "Nathan Chancellor" <nathan@...nel.org>,  "Luis Chamberlain"
 <mcgrof@...nel.org>,  "Danilo Krummrich" <dakr@...nel.org>,  "Nicolas
 Schier" <nicolas.schier@...ux.dev>,  "Trevor Gross" <tmgross@...ch.edu>,
  "Adam Bratschi-Kaye" <ark.email@...il.com>,
  <rust-for-linux@...r.kernel.org>,  <linux-kernel@...r.kernel.org>,
  <linux-kbuild@...r.kernel.org>,  "Petr Pavlu" <petr.pavlu@...e.com>,
  "Sami Tolvanen" <samitolvanen@...gle.com>,  "Daniel Gomez"
 <da.gomez@...sung.com>,  "Simona Vetter" <simona.vetter@...ll.ch>,  "Greg
 KH" <gregkh@...uxfoundation.org>,  "Fiona Behrens" <me@...enk.dev>,
  "Daniel Almeida" <daniel.almeida@...labora.com>,
  <linux-modules@...r.kernel.org>
Subject: Re: [PATCH v14 5/7] rust: module: update the module macro with
 module parameter support

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

> On Wed Jul 2, 2025 at 3:18 PM CEST, Andreas Hindborg wrote:
>> Allow module parameters to be declared in the rust `module!` macro.
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@...nel.org>
>
> A few nits below, with those fixed
>
> Reviewed-by: Benno Lossin <lossin@...nel.org>
>
>> ---
>>  rust/macros/helpers.rs |  25 +++++++
>>  rust/macros/lib.rs     |  31 +++++++++
>>  rust/macros/module.rs  | 177 ++++++++++++++++++++++++++++++++++++++++++++++---
>>  3 files changed, 223 insertions(+), 10 deletions(-)
>
>> +    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:
>
> Does it make sense to move this static to a `const _: () = {};` block?

Yes, that makes sense.

>
>> +                        ::kernel::module_param::RacyKernelParam =
>> +                        ::kernel::module_param::RacyKernelParam::new(
>> +                          ::kernel::bindings::kernel_param {{
>> +                            name: if cfg!(MODULE) {{
>
> s/cfg/::core::cfg/

OK.

>
> :)
>
> Also there seems to only be a 2-space indentation here.

Fixed.

>
>> +                                ::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()
>> +                            }},
>
> It doesn't stop with the improvements...
>
>     https://github.com/Rust-for-Linux/linux/issues/1176
>
> Maybe we should also have one to use it here, but eh we can do that
> later (and it's not as bad to forget about :)

Applying `from_ref`.

>
>> +                            #[cfg(not(MODULE))]
>> +                            mod_: ::core::ptr::null_mut(),
>> +                            ops: &{ops} as *const ::kernel::bindings::kernel_param_ops,
>
>     ::core::ptr::from_ref(&{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_void_ptr()
>> +                                }},
>
> Formatting?
>
> +                            __bindgen_anon_1: ::kernel::bindings::kernel_param__bindgen_ty_1 {{
> +                                arg: {param_name}.as_void_ptr()
> +                            }},


That makes the line more than 100 characters after changing other
formatting things. Perhaps I should just left shift all this?


Best regards,
Andreas Hindborg