linux-kernel - Re: [PATCH v3 4/4] rust: add parameter support to the `module!` macro

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: <CANiq72kb2ocNuE6n32vr4xCkZhZN0uPuCN3SFA1+Q5L+Ma4ByQ@mail.gmail.com>
Date: Fri, 13 Dec 2024 13:54:14 +0100
From: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To: Andreas Hindborg <a.hindborg@...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>, 
	Benno Lossin <benno.lossin@...ton.me>, Alice Ryhl <aliceryhl@...gle.com>, 
	Masahiro Yamada <masahiroy@...nel.org>, Nathan Chancellor <nathan@...nel.org>, 
	Nicolas Schier <nicolas@...sle.eu>, 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
Subject: Re: [PATCH v3 4/4] rust: add parameter support to the `module!` macro

On Fri, Dec 13, 2024 at 12:33 PM Andreas Hindborg <a.hindborg@...nel.org> wrote:
>
> +#![feature(sync_unsafe_cell)]

Please mention this in the commit message, the status of the feature
and justify the addition.

> +//! C header: [`include/linux/moduleparam.h`](../../../include/linux/moduleparam.h)

Please use `srctree`.

> +/// Newtype to make `bindings::kernel_param` `Sync`.

Please add intra-doc links where applicable, e.g. `Sync` here.

> +unsafe extern "C" fn set_param<T>(
> +    val: *const core::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.
> +    assert!(!val.is_null());

Should this return an error instead?

> +/// Write a string representation of the current parameter value to `buf`.
> +///
> +/// # Safety
> +///
> +/// Must not be called.
> +///
> +/// # Note
> +///
> +/// This should not be called as we declare all parameters as read only.
> +#[allow(clippy::extra_unused_type_parameters)]
> +unsafe extern "C" fn get_param<T>(
> +    _buf: *mut core::ffi::c_char,
> +    _param: *const crate::bindings::kernel_param,
> +) -> core::ffi::c_int
> +where
> +    T: ModuleParam,
> +{
> +    unreachable!("Parameters are not readable");
> +}

Do we need this? Can't the `ops` callback be `null`?

> +/// The `arg` field of `param` must be an initialized instance of `Self`.

`Self`?

> +/// Generate a static [`kernel_param_ops`](../../../include/linux/moduleparam.h) struct.

`srctree`.

> +/// Parse a token stream of the form `expected_name: "value",` and return the
> +/// string in the position of "value". Panics on parse error.

`# Panics` section.

> +/// `type` may be one of
> +///
> +/// - `i8`
> +/// - `u8`
> +/// - `i8`
> +/// - `u8`
> +/// - `i16`
> +/// - `u16`
> +/// - `i32`
> +/// - `u32`
> +/// - `i64`
> +/// - `u64`
> +/// - `isize`
> +/// - `usize`

Can these be intra-doc links?

Thanks!

Cheers,
Miguel