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: <DB1OK2PQZ790.S317HUWYJR3J@kernel.org>
Date: Wed, 02 Jul 2025 17:38:52 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Andreas Hindborg" <a.hindborg@...nel.org>, "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>
Cc: "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

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?

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

s/cfg/::core::cfg/

:)

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

> +                                ::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 :)

> +                            #[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()
+                            }},

---
Cheers,
Benno

> +                          }}
> +                        );
> +                ",
> +                module_name = info.name,
> +                param_type = param.ptype,
> +                param_default = param.default,
> +                param_name = param.name,
> +                ops = ops,
> +            )
> +            .unwrap();
> +        }
> +    }
> +}