Message-ID: <20250225151417.7805b697@eugeo>
Date: Tue, 25 Feb 2025 15:14:17 +0000
From: Gary Guo <gary@...yguo.net>
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>, 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

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

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

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

> +                <$ty as crate::str::parse_int::ParseInt>::from_str(bstr)
> +            }
> +        }
> +    };
> +}
> +
> +impl_int_module_param!(i8);
> +impl_int_module_param!(u8);
> +impl_int_module_param!(i16);
> +impl_int_module_param!(u16);
> +impl_int_module_param!(i32);
> +impl_int_module_param!(u32);
> +impl_int_module_param!(i64);
> +impl_int_module_param!(u64);
> +impl_int_module_param!(isize);
> +impl_int_module_param!(usize);
> +
> +/// A wrapper for kernel parameters.
> +///
> +/// This type is instantiated by the [`module!`] macro when module parameters are
> +/// defined. You should never need to instantiate this type directly.
> +#[repr(transparent)]
> +pub struct ModuleParamAccess<T> {
> +    data: core::cell::UnsafeCell<T>,
> +}
> +
> +// SAFETY: We only create shared references to the contents of this container,
> +// so if `T` is `Sync`, so is `ModuleParamAccess`.
> +unsafe impl<T: Sync> Sync for ModuleParamAccess<T> {}
> +
> +impl<T> ModuleParamAccess<T> {
> +    #[doc(hidden)]
> +    pub const fn new(value: T) -> Self {
> +        Self {
> +            data: core::cell::UnsafeCell::new(value),
> +        }
> +    }
> +
> +    /// Get a shared reference to the parameter value.
> +    // Note: When sysfs access to parameters are enabled, we have to pass in a
> +    // held lock guard here.
> +    pub fn get(&self) -> &T {
> +        // SAFETY: As we only support read only parameters with no sysfs
> +        // exposure, the kernel will not touch the parameter data after module
> +        // initialization.
> +        unsafe { &*self.data.get() }
> +    }
> +
> +    /// Get a mutable pointer to the parameter value.
> +    pub const fn as_mut_ptr(&self) -> *mut T {
> +        self.data.get()
> +    }
> +}
> +

Powered by blists - more mailing lists