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: <87y10jd8o0.fsf@kernel.org>
Date: Fri, 13 Dec 2024 14:17:19 +0100
From: Andreas Hindborg <a.hindborg@...nel.org>
To: "Miguel Ojeda" <miguel.ojeda.sandonis@...il.com>
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

Hi Miguel,

"Miguel Ojeda" <miguel.ojeda.sandonis@...il.com> writes:

> 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.

I forgot, thanks for pointing that out. Following the discussion of v2
of this series I can understand that `mut static` is discouraged and
scheduled for removal. Interior mutability via `SyncUnsafeCell` provides
the same functionality and it is my understanding that this feature is
on track to be stabilized.

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

Ok.

>
>> +/// Newtype to make `bindings::kernel_param` `Sync`.
>
> Please add intra-doc links where applicable, e.g. `Sync` here.

Will do.

>
>> +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?

Not sure. `val` being null not supposed to happen in the current
configuration. It should be an unreachable state. So BUG is the right thing?

>
>> +/// 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`?

Not in the current configuration. The parameters can only be declared
"read only". It should be impossible for anyone to call this function.

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

That whole line is wrong, thanks for spotting. It should read "`arg` must
be an initialized instance of `T`. This function takes ownership of
the `T` pointed to by `arg`.".

>
>> +/// 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.

Ok.

>
>> +/// `type` may be one of
>> +///
>> +/// - `i8`
>> +/// - `u8`
>> +/// - `i8`
>> +/// - `u8`
>> +/// - `i16`
>> +/// - `u16`
>> +/// - `i32`
>> +/// - `u32`
>> +/// - `i64`
>> +/// - `u64`
>> +/// - `isize`
>> +/// - `usize`
>
> Can these be intra-doc links?

Yes!


Thanks for the comments!



Best regards,
Andreas Hindborg