Message-ID: <87o6yqxxhv.fsf@kernel.org>
Date: Tue, 25 Feb 2025 09:02:52 +0100
From: Andreas Hindborg <a.hindborg@...nel.org>
To: "Daniel Almeida" <daniel.almeida@...labora.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>,
  "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

"Daniel Almeida" <daniel.almeida@...labora.com> writes:

> Hi Andreas, thanks for working on this, I can see that this patch took a lot
> of effort.

Thanks! It's not all me though, it's based on old code from the
pre-merge days.

[...]

>> 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.
>
> nit: maybe “Support for module parameters”?
>
> Or anything else other than “types”, really :)

I agree. I think originally the naming came from this being types
backing the macro implementation.

>
>> +//!
>> +//! 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
>
> nit: perhaps: “we never access *it* from *a* Rust module” ?

Right 👍

>
>> +// from Rust module.
>> +unsafe impl Sync for RacyKernelParam {}
>> +
>> +/// 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
>
> nit: perhaps `implementation writes more than`? Although it’d be great if a
> native speaker could chime in on this one.

Actually, we removed the support for displaying in sysfs from the
series, so I will just yank that note.

>
>> +/// [`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.
>
> I don’t understand what’s being said here. e.g.: what does “Self needs to track
> ownership without exposing it” mean? Can you expand on this?

For some parameter types, such as string values, the parameter may
assume a reference value or an owned value. The reference value would be
used as default (as a reference to a static string), while an owned
value would be passed in when the value is set. For that, `Value` can be
an enum.

We yanked support for anything but integer parameters from this series,
but I would like to keep this around to make adding string parameters
less churn in the near future.

If you follow link [1] in the cover letter, you can see the original
code from the pre-merge branch that this patch is based on.

>
> Also this is pub. It should perhaps also be sealed?

We might in the future allow users to implement their own parsers.

>
>
>> +    // This is required to support string parameters in the future.
>> +    type Value: ?Sized;
>
> Why? Can you also expand on this a tad further?

As explained above.

>
>> +
>> +    /// 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.
>
> Perhaps the above should also be an invariant?

Actually, I think it should be part of the safety requirements.

[...]

>> +
>> +impl<T> ModuleParamAccess<T> {
>> +    #[doc(hidden)]
>> +    pub const fn new(value: T) -> Self {
>
> I assume that this is pub so that the macro can find it? If so, can you leave a note
> outlining this?

Yes, it must be accessible from other crates (modules). Will add a note.

>
>> +        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.
>
> What lock guard, guarding what exactly?

That is yet to be determined. When we enable sysfs, we will have async
access to the parameter value when user space interacts with sysfs.
Thus, we need to apply synchronization on parameter value access. I
envision a lock being taken before this method is called and a lock
guard passed in to access the data.

The code has deviated quite a bit from the original, but you can see a
possible implementation here [1].

[1] https://github.com/Rust-for-Linux/linux/blob/bc22545f38d74473cfef3e9fd65432733435b79f/rust/macros/module.rs#L410


[...]

>> +    fn emit_params(&mut self, info: &ModuleInfo) {
>> +        let Some(params) = &info.params else {
>> +            return;
>> +        };
>
> Shouldn’t this panic? A call to emit_params() where there’s nothing to emit doesn’t
> look right at a first glance.

No, having no parameters is a valid configuration. If the "params" key
is left out in the module macro call, the option will be `None`. The
call to this function when generating code is unconditional.

[...]

>> -    if let Some(firmware) = info.firmware {
>> +    if let Some(firmware) = &info.firmware {
>>         for fw in firmware {
>> -            modinfo.emit("firmware", &fw);
>> +            modinfo.emit("firmware", fw);
>>         }
>>     }
>
> These seem a bit unrelated?

They could be split in a precursor patch, but they are required for this
change set. If you insist I will split them out, but I am also happy to
keep them as one big change.

>
>>
>>     // Built-in modules also export the `file` modinfo string.
>>     let file =
>>         std::env::var("RUST_MODFILE").expect("Unable to fetch RUST_MODFILE environmental variable");
>> -    modinfo.emit_only_builtin("file", &file);
>> +    modinfo.emit_only_builtin("file", &file, false);
>> +
>> +    modinfo.emit_params(&info);
>>
>>     format!(
>>         "
>> @@ -362,14 +514,17 @@ unsafe fn __exit() {{
>>                             __MOD.assume_init_drop();
>>                         }}
>>                     }}
>> -
>>                     {modinfo}
>>                 }}
>>             }}
>> +            mod module_parameters {{
>> +                {params}
>> +            }}
>>         ",
>>         type_ = info.type_,
>>         name = info.name,
>>         modinfo = modinfo.buffer,
>> +        params = modinfo.param_buffer,
>>         initcall_section = ".initcall6.init"
>>     )
>>     .parse()
>> diff --git a/samples/rust/rust_minimal.rs b/samples/rust/rust_minimal.rs
>> index 4aaf117bf8e3c..d999a77c6eb9a 100644
>
> I wonder if the changes to rust_minimal.rs should be a separate patch.

Either way works for me.


Thanks for the comments!


Best regards,
Andreas Hindborg

Powered by blists - more mailing lists