Message-ID: <87wmdeujfd.fsf@kernel.org>
Date: Tue, 25 Feb 2025 16:35:02 +0100
From: Andreas Hindborg <a.hindborg@...nel.org>
To: "Gary Guo" <gary@...yguo.net>
Cc: "Miguel Ojeda" <ojeda@...nel.org>,  "Alex Gaynor"
 <alex.gaynor@...il.com>,  "Boqun Feng" <boqun.feng@...il.com>,
  Björn Roy
 Baron <bjorn3_gh@...tonmail.com>,  "Benno Lossin"
 <benno.lossin@...ton.me>,  "Alice Ryhl" <aliceryhl@...gle.com>,  "Trevor
 Gross" <tmgross@...ch.edu>,  "Masahiro Yamada" <masahiroy@...nel.org>,
  "Nathan Chancellor" <nathan@...nel.org>,  "Nicolas Schier"
 <nicolas@...sle.eu>,  "Luis Chamberlain" <mcgrof@...nel.org>,
  <rust-for-linux@...r.kernel.org>,  <linux-kernel@...r.kernel.org>,  "Adam
 Bratschi-Kaye" <ark.email@...il.com>,  <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>,  <linux-modules@...r.kernel.org>
Subject: Re: [PATCH v7 6/6] rust: add parameter support to the `module!` macro

"Gary Guo" <gary@...yguo.net> writes:

> On Tue, 18 Feb 2025 14:00:48 +0100
> Andreas Hindborg <a.hindborg@...nel.org> wrote:
>
>> This patch includes changes required for Rust kernel modules to utilize
>> module parameters. This code implements read only support for integer
>> types without `sysfs` support.
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@...nel.org>
>> Acked-by: Petr Pavlu <petr.pavlu@...e.com> # from modules perspective
>> ---
>>  rust/kernel/lib.rs           |   1 +
>>  rust/kernel/module_param.rs  | 226 +++++++++++++++++++++++++++++++++++++++++++
>>  rust/macros/helpers.rs       |  25 +++++
>>  rust/macros/lib.rs           |  31 ++++++
>>  rust/macros/module.rs        | 191 ++++++++++++++++++++++++++++++++----
>>  samples/rust/rust_minimal.rs |  10 ++
>>  6 files changed, 466 insertions(+), 18 deletions(-)
>>
>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
>> index 496ed32b0911a..aec04df2bac9f 100644
>> --- a/rust/kernel/lib.rs
>> +++ b/rust/kernel/lib.rs
>> @@ -57,6 +57,7 @@
>>  pub mod kunit;
>>  pub mod list;
>>  pub mod miscdevice;
>> +pub mod module_param;
>>  #[cfg(CONFIG_NET)]
>>  pub mod net;
>>  pub mod of;
>> diff --git a/rust/kernel/module_param.rs b/rust/kernel/module_param.rs
>> new file mode 100644
>> index 0000000000000..0047126c917f4
>> --- /dev/null
>> +++ b/rust/kernel/module_param.rs
>> @@ -0,0 +1,226 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! Types for module parameters.
>> +//!
>> +//! C header: [`include/linux/moduleparam.h`](srctree/include/linux/moduleparam.h)
>> +
>> +use crate::prelude::*;
>> +use crate::str::BStr;
>> +
>> +/// Newtype to make `bindings::kernel_param` [`Sync`].
>> +#[repr(transparent)]
>> +#[doc(hidden)]
>> +pub struct RacyKernelParam(pub ::kernel::bindings::kernel_param);
>> +
>> +// SAFETY: C kernel handles serializing access to this type. We never access
>> +// from Rust module.
>> +unsafe impl Sync for RacyKernelParam {}
>
> I wonder if we should have a custom impl of `SyncUnsafeCell` for this
> kind of usage (so that when it is stabilized by Rust, we can just switc
> over).

We discussed this before, I don't recall what we decided. At any rate,
it's orthogonal. We can patch this if we add `SyncUnsafeCell`.

>
>> +
>> +/// Types that can be used for module parameters.
>> +///
>> +/// Note that displaying the type in `sysfs` will fail if
>> +/// [`Display`](core::fmt::Display) implementation would write more than
>> +/// [`PAGE_SIZE`] - 1 bytes.
>> +///
>> +/// [`PAGE_SIZE`]: `bindings::PAGE_SIZE`
>> +pub trait ModuleParam: Sized {
>> +    /// The [`ModuleParam`] will be used by the kernel module through this type.
>> +    ///
>> +    /// This may differ from `Self` if, for example, `Self` needs to track
>> +    /// ownership without exposing it or allocate extra space for other possible
>> +    /// parameter values.
>> +    // This is required to support string parameters in the future.
>> +    type Value: ?Sized;
>> +
>> +    /// Parse a parameter argument into the parameter value.
>> +    ///
>> +    /// `Err(_)` should be returned when parsing of the argument fails.
>> +    ///
>> +    /// Parameters passed at boot time will be set before [`kmalloc`] is
>> +    /// available (even if the module is loaded at a later time). However, in
>> +    /// this case, the argument buffer will be valid for the entire lifetime of
>> +    /// the kernel. So implementations of this method which need to allocate
>> +    /// should first check that the allocator is available (with
>> +    /// [`crate::bindings::slab_is_available`]) and when it is not available
>> +    /// provide an alternative implementation which doesn't allocate. In cases
>> +    /// where the allocator is not available it is safe to save references to
>> +    /// `arg` in `Self`, but in other cases a copy should be made.
>> +    ///
>> +    /// [`kmalloc`]: srctree/include/linux/slab.h
>> +    fn try_from_param_arg(arg: &'static [u8]) -> Result<Self>;
>> +}
>> +
>> +/// Set the module parameter from a string.
>> +///
>> +/// Used to set the parameter value at kernel initialization, when loading
>> +/// the module or when set through `sysfs`.
>> +///
>> +/// `param.arg` is a pointer to `*mut T` as set up by the [`module!`]
>> +/// macro.
>> +///
>> +/// See `struct kernel_param_ops.set`.
>> +///
>> +/// # Safety
>> +///
>> +/// If `val` is non-null then it must point to a valid null-terminated
>> +/// string. The `arg` field of `param` must be an instance of `T`.
>> +///
>> +/// # Invariants
>> +///
>> +/// Currently, we only support read-only parameters that are not readable
>> +/// from `sysfs`. Thus, this function is only called at kernel
>> +/// initialization time, or at module load time, and we have exclusive
>> +/// access to the parameter for the duration of the function.
>> +///
>> +/// [`module!`]: macros::module
>> +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 crate::error::code::EINVAL.to_errno();
>
> This is already in prelude, so you can just use `EINVAL` directly.

Thanks.

>
>> +    }
>> +
>> +    // 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).as_bytes() };
>> +
>> +    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) };
>> +        Ok(0)
>> +    })
>> +}
>> +
>> +/// Drop the parameter.
>> +///
>> +/// Called when unloading a module.
>> +///
>> +/// # Safety
>> +///
>> +/// The `arg` field of `param` must be an initialized instance of `T`.
>> +unsafe extern "C" fn free<T>(arg: *mut core::ffi::c_void)
>> +where
>> +    T: ModuleParam,
>> +{
>> +    // SAFETY: By function safety requirement, `arg` is an initialized
>> +    // instance of `T`. By C API contract, `arg` will not be used after
>> +    // this function returns.
>> +    unsafe { core::ptr::drop_in_place(arg as *mut T) };
>> +}
>> +
>> +macro_rules! impl_int_module_param {
>> +    ($ty:ident) => {
>> +        impl ModuleParam for $ty {
>> +            type Value = $ty;
>> +
>> +            fn try_from_param_arg(arg: &'static [u8]) -> Result<Self> {
>> +                let bstr = BStr::from_bytes(arg);
>
> Why isn't `arg` BStr in the first place?

I'll change it.


Best regards,
Andreas Hindborg

Powered by blists - more mailing lists