* Re: [PATCH 1/3] module: move 'struct module_use' to internal.h
From: Daniel Gomez @ 2025-06-19 16:27 UTC (permalink / raw)
To: Thomas Weißschuh, Luis Chamberlain, Petr Pavlu,
Sami Tolvanen, Daniel Gomez, Brendan Higgins, David Gow, Rae Moar
Cc: linux-modules, linux-kernel, linux-kselftest, kunit-dev
In-Reply-To: <20250612-kunit-ifdef-modules-v1-1-fdccd42dcff8@linutronix.de>
On 12/06/2025 16.53, Thomas WeiÃschuh wrote:
> The struct was moved to the public header file in
> commit c8e21ced08b3 ("module: fix kdb's illicit use of struct module_use.").
> Back then the structure was used outside of the module core.
> Nowadays this is not true anymore, so the structure can be made internal.
>
> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
Reviewed-by: Daniel Gomez <da.gomez@samsung.com>
^ permalink raw reply
* Re: [PATCH v13 2/6] rust: introduce module_param module
From: Benno Lossin @ 2025-06-19 13:15 UTC (permalink / raw)
To: Andreas Hindborg, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Nicolas Schier
Cc: Trevor Gross, Adam Bratschi-Kaye, rust-for-linux, linux-kernel,
linux-kbuild, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Simona Vetter, Greg KH, Fiona Behrens, Daniel Almeida,
linux-modules
In-Reply-To: <20250612-module-params-v3-v13-2-bc219cd1a3f8@kernel.org>
On Thu Jun 12, 2025 at 3:40 PM CEST, Andreas Hindborg wrote:
> +/// 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.
> +///
> +/// Note: This type is `pub` because it is used by module crates to access
> +/// parameter values.
> +#[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.
This should be a type invariant. But I'm having difficulty defining one
that's actually correct: after parsing the parameter, this is written
to, but when is that actually? Would we eventually execute other Rust
code during that time? (for example when we allow custom parameter
parsing)
This function also must never be `const` because of the following:
module! {
// ...
params: {
my_param: i64 {
default: 0,
description: "",
},
},
}
static BAD: &'static i64 = module_parameters::my_param.get();
AFAIK, this static will be executed before loading module parameters and
thus it makes writing to the parameter UB.
So maybe we should just use some sort of synchronization tool here...
---
Cheers,
Benno
> + unsafe { &*self.data.get() }
> + }
> +
> + /// Get a mutable pointer to the parameter value.
> + pub const fn as_mut_ptr(&self) -> *mut T {
> + self.data.get()
> + }
> +}
^ permalink raw reply
* Re: [PATCH v13 2/6] rust: introduce module_param module
From: Benno Lossin @ 2025-06-19 12:55 UTC (permalink / raw)
To: Andreas Hindborg
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Nicolas Schier, Trevor Gross, Adam Bratschi-Kaye, rust-for-linux,
linux-kernel, linux-kbuild, Petr Pavlu, Sami Tolvanen,
Daniel Gomez, Simona Vetter, Greg KH, Fiona Behrens,
Daniel Almeida, linux-modules
In-Reply-To: <87v7or7wiv.fsf@kernel.org>
On Thu Jun 19, 2025 at 2:20 PM CEST, Andreas Hindborg wrote:
> "Benno Lossin" <lossin@kernel.org> writes:
>> On Thu Jun 12, 2025 at 3:40 PM CEST, Andreas Hindborg wrote:
>>> +
>>> +// SAFETY: C kernel handles serializing access to this type. We never access it
>>> +// from Rust module.
>>> +unsafe impl Sync for RacyKernelParam {}
>>> +
>>> +/// Types that can be used for module parameters.
>>> +pub trait ModuleParam: Sized + Copy {
>>
>> Why the `Copy` bound?
>
> Because of potential unsoundness due to drop [1]. I should document
> this. It is noted in the change log for the series under the obscure
> entry "Assign through pointer rather than using `core::ptr::replace`."
>
> [1] https://lore.kernel.org/all/878qnbxtyi.fsf@kernel.org
Ah thanks for the pointer, yeah please mention this in a comment
somewhere.
>>> + ///
>>> + /// Parameters passed at boot time will be set before [`kmalloc`] is
>>> + /// available (even if the module is loaded at a later time). However, in
>>
>> I think we should make a section out of this like `# No allocations` (or
>> something better). Let's also mention it on the trait itself, since
>> that's where implementers will most likely look.
>
> Since this series only support `Copy` types that are passed by value, I
> think we can remove this comment for now. I will also restrict the
> lifetime of the string to he duration of the call. Putting static here
> would be lying.
>
>>
>>> + /// 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
>>
>> We probably shouldn't recommend directly using `bindings`.
>>
>>> + /// 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.
>>
>> I don't understand this convention, but it also doesn't seem to
>> relevant (so feel free to leave it as is, but it would be nice if you
>> could explain it).
>
> It has become irrelevant as the series evolved. When we supported
> `!Copy` types we would use the reference if we knew it would be valid
> for the lifetime of the kernel, otherwise we would allocate [1].
>
> However, when the reference is passed at module load time, it is still
> guaranteed to be live for the lifetime of the module, and hence it can
> still be considered `'static`. But, if the reference were to find it's
> way across the module boundary, it can cause UAF issues as the reference
> is not truely `'static`, it is actually `'module`. This ties into the
> difficulty we have around safety of unloading modules. Module unload
> should be marked unsafe.
Ah so the argument should rather be an enum that is either
`Static(&'static str)` or `WithAlloc(&'short str)` with the (non-safety)
guarantee that `WithAlloc` is only passed when the allocator is
available.
> At any rate, I will remove the `'static` lifetime from the reference and
> we are all good for now.
Sounds simplest for now.
>>> + crate::error::from_result(|| {
>>> + let new_value = T::try_from_param_arg(arg)?;
>>> +
>>> + // SAFETY: By function safety requirements `param` is be valid for reads.
>>> + let old_value = unsafe { (*param).__bindgen_anon_1.arg as *mut T };
>>> +
>>> + // SAFETY: By function safety requirements, the target of `old_value` is valid for writes
>>> + // and is initialized.
>>> + unsafe { *old_value = new_value };
>>
>> So if we keep the `ModuleParam: Copy` bound from above, then we don't
>> need to drop the type here (as `Copy` implies `!Drop`). So we could also
>> remove the requirement for initialized memory and use `ptr::write` here
>> instead. Thoughts?
>
> Yes, that is the rationale for the `Copy` bound. What would be the
> benefit of using `ptr::write`? They should be equivalent for `Copy`
> types, right.
They should be equivalent, but if we drop the requirement that the value
is initialized to begin with, then removing `Copy` will result in UB
here.
> I was using `ptr::replace`, but Alice suggested the pace expression
> assignment instead, since I was not using the old value.
That makes sense, but if we also remove the initialized requirement,
then I would prefer `ptr::write`.
---
Cheers,
Benno
^ permalink raw reply
* Re: [PATCH v13 1/6] rust: str: add radix prefixed integer parsing functions
From: Andreas Hindborg @ 2025-06-19 12:41 UTC (permalink / raw)
To: Benno Lossin
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Nicolas Schier, Trevor Gross, Adam Bratschi-Kaye, rust-for-linux,
linux-kernel, linux-kbuild, Petr Pavlu, Sami Tolvanen,
Daniel Gomez, Simona Vetter, Greg KH, Fiona Behrens,
Daniel Almeida, linux-modules
In-Reply-To: <DAQI4RPK2Y7T.3TQ1G3IMZCNK4@kernel.org>
"Benno Lossin" <lossin@kernel.org> writes:
> On Thu Jun 19, 2025 at 1:12 PM CEST, Andreas Hindborg wrote:
>> I'm having a difficult time parsing. Are you suggesting that we guard
>> against implementations of `TryInto<u64>` that misbehave?
>
> Let me try a different explanation:
>
> The safety requirement for implementing the `FromStrRadix`:
>
> /// The member functions of this trait must be implemented according to
> /// their documentation.
>
> Together with the functions of the trait:
>
> /// Parse `src` to [`Self`] using radix `radix`.
> fn from_str_radix(src: &BStr, radix: u32) -> Result<Self, crate::error::Error>;
>
> /// Return the absolute value of [`Self::MIN`].
> fn abs_min() -> u64;
>
> /// Perform bitwise 2's complement on `self`.
> ///
> /// Note: This function does not make sense for unsigned integers.
> fn complement(self) -> Self;
>
> Doesn't make sense. What does it mean to return the "absolute value of
> [`Self::MIN`]"? We don't have "absolute value" defined for an arbitrary
> type. Similarly for `complement` and `from_str_radix`, what does "Parse
> `src` to [`Self`] using radex `radix`" mean? It's not well-defined.
>
> You use this safety requirement in the parsing branch for negative
> numbers (the `unsafe` call at the bottom):
>
> [b'-', rest @ ..] => {
> let (radix, digits) = strip_radix(rest.as_ref());
> // 2's complement values range from -2^(b-1) to 2^(b-1)-1.
> // So if we want to parse negative numbers as positive and
> // later multiply by -1, we have to parse into a larger
> // integer. We choose `u64` as sufficiently large.
> //
> // NOTE: 128 bit integers are not available on all
> // platforms, hence the choice of 64 bits.
> let val =
> u64::from_str_radix(core::str::from_utf8(digits).map_err(|_| EINVAL)?, radix)
> .map_err(|_| EINVAL)?;
>
> if val > Self::abs_min() {
> return Err(EINVAL);
> }
>
> if val == Self::abs_min() {
> return Ok(Self::MIN);
> }
>
> // SAFETY: We checked that `val` will fit in `Self` above.
> let val: Self = unsafe { val.try_into().unwrap_unchecked() };
>
> Ok(val.complement())
> }
>
> But you don't mention that the check is valid due to the safety
> requirements of implementing `FromStrRadix`. But even if you did, that
> wouldn't mean anything as I explained above.
>
> So let's instead move all of this negation & u64 conversion logic into
> the `FromStrRadix` trait. Then it can be safe & the `ParseInt::from_str`
> function doesn't use `unsafe` (there still will be `unsafe` in the
> macro, but that is fine, as it's more local and knows the concrete
> types).
>
Alright. I guess my safety comments are slightly hand-wavy. Thanks for
the suggestion, I'll apply that for next spin.
Best regards,
Andreas Hindborg
^ permalink raw reply
* Re: [PATCH v13 4/6] rust: module: update the module macro with module parameter support
From: Andreas Hindborg @ 2025-06-19 12:31 UTC (permalink / raw)
To: Benno Lossin
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Nicolas Schier, Trevor Gross, Adam Bratschi-Kaye, rust-for-linux,
linux-kernel, linux-kbuild, Petr Pavlu, Sami Tolvanen,
Daniel Gomez, Simona Vetter, Greg KH, Fiona Behrens,
Daniel Almeida, linux-modules
In-Reply-To: <DAPYS2D9HNBT.1ZTN1VHCPN4XA@kernel.org>
"Benno Lossin" <lossin@kernel.org> writes:
> On Thu Jun 12, 2025 at 3:40 PM CEST, Andreas Hindborg wrote:
>> +
>> + fn emit_params(&mut self, info: &ModuleInfo) {
>> + let Some(params) = &info.params else {
>> + return;
>> + };
>> +
>> + for param in params {
>> + let ops = param_ops_path(¶m.ptype);
>> +
>> + // Note: The spelling of these fields is dictated by the user space
>> + // tool `modinfo`.
>> + self.emit_param("parmtype", ¶m.name, ¶m.ptype);
>> + self.emit_param("parm", ¶m.name, ¶m.description);
>
> I just read this part again and I want to voice my dissatisfaction with
> these key names. (not that you can do anything about that :)
>
>> +
>> + write!(
>> + self.param_buffer,
>> + "
>> + pub(crate) static {param_name}:
>
> Does this need to be accessed by anything else except the static below?
> If no, then can we move it inside of that static? So
>
> #[link_section = \"__param\"]
> #[used]
> static __{module_name}_{param_name}_struct: ::kernel::module_param::RacyKernelParam = {
> static {param_name}:
> ::kernel::module_param::ModuleParamAccess<{param_type}> =
> ::kernel::module_param::ModuleParamAccess::new({param_default});
> // ...
> };
It is used to access the value of the parameter from the module. If you
reduce visibility you will get an error when trying to read the
parameter value:
RUSTC samples/rust/rust_minimal.o
error[E0425]: cannot find value `test_parameter` in module `module_parameters`
--> /home/aeh/src/linux-rust/module-params/samples/rust/rust_minimal.rs:31:33
|
31 | *module_parameters::test_parameter.get()
| ^^^^^^^^^^^^^^ not found in `module_parameters`
error: aborting due to 1 previous error
Best regards,
Andreas Hindborg
^ permalink raw reply
* Re: [PATCH v13 2/6] rust: introduce module_param module
From: Andreas Hindborg @ 2025-06-19 12:20 UTC (permalink / raw)
To: Benno Lossin
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Nicolas Schier, Trevor Gross, Adam Bratschi-Kaye, rust-for-linux,
linux-kernel, linux-kbuild, Petr Pavlu, Sami Tolvanen,
Daniel Gomez, Simona Vetter, Greg KH, Fiona Behrens,
Daniel Almeida, linux-modules
In-Reply-To: <DAPYMAB44RUZ.7NIWDUWY1UYF@kernel.org>
"Benno Lossin" <lossin@kernel.org> writes:
> On Thu Jun 12, 2025 at 3:40 PM CEST, Andreas Hindborg wrote:
>> Add types and traits for interfacing the C moduleparam API.
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>> ---
>> rust/kernel/lib.rs | 1 +
>> rust/kernel/module_param.rs | 201 ++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 202 insertions(+)
>>
>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
>> index 6b4774b2b1c3..2b439ea06185 100644
>> --- a/rust/kernel/lib.rs
>> +++ b/rust/kernel/lib.rs
>> @@ -87,6 +87,7 @@
>> pub mod list;
>> pub mod miscdevice;
>> pub mod mm;
>> +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 000000000000..fd167df8e53d
>> --- /dev/null
>> +++ b/rust/kernel/module_param.rs
>> @@ -0,0 +1,201 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! Support 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);
>
> s/::kernel:://
>
> The field shouldn't be public, since then people can access it. Can
> just have a `pub fn new` instead?
OK.
>
>> +
>> +// SAFETY: C kernel handles serializing access to this type. We never access it
>> +// from Rust module.
>> +unsafe impl Sync for RacyKernelParam {}
>> +
>> +/// Types that can be used for module parameters.
>> +pub trait ModuleParam: Sized + Copy {
>
> Why the `Copy` bound?
Because of potential unsoundness due to drop [1]. I should document
this. It is noted in the change log for the series under the obscure
entry "Assign through pointer rather than using `core::ptr::replace`."
[1] https://lore.kernel.org/all/878qnbxtyi.fsf@kernel.org
>
>> + /// 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.
>
> I don't think we need to explicitly mention this.
I'll remove the line.
>
>> + ///
>> + /// Parameters passed at boot time will be set before [`kmalloc`] is
>> + /// available (even if the module is loaded at a later time). However, in
>
> I think we should make a section out of this like `# No allocations` (or
> something better). Let's also mention it on the trait itself, since
> that's where implementers will most likely look.
Since this series only support `Copy` types that are passed by value, I
think we can remove this comment for now. I will also restrict the
lifetime of the string to he duration of the call. Putting static here
would be lying.
>
>> + /// 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
>
> We probably shouldn't recommend directly using `bindings`.
>
>> + /// 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.
>
> I don't understand this convention, but it also doesn't seem to
> relevant (so feel free to leave it as is, but it would be nice if you
> could explain it).
It has become irrelevant as the series evolved. When we supported
`!Copy` types we would use the reference if we knew it would be valid
for the lifetime of the kernel, otherwise we would allocate [1].
However, when the reference is passed at module load time, it is still
guaranteed to be live for the lifetime of the module, and hence it can
still be considered `'static`. But, if the reference were to find it's
way across the module boundary, it can cause UAF issues as the reference
is not truely `'static`, it is actually `'module`. This ties into the
difficulty we have around safety of unloading modules. Module unload
should be marked unsafe.
At any rate, I will remove the `'static` lifetime from the reference and
we are all good for now.
[1] https://github.com/Rust-for-Linux/linux/blob/18b7491480025420896e0c8b73c98475c3806c6f/rust/kernel/module_param.rs#L476
>
>> + ///
>> + /// [`kmalloc`]: srctree/include/linux/slab.h
>> + fn try_from_param_arg(arg: &'static BStr) -> 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`.
>> +///
>> +/// See `struct kernel_param_ops.set`.
>> +///
>> +/// # Safety
>> +///
>> +/// - If `val` is non-null then it must point to a valid null-terminated string that must be valid
>> +/// for reads for the duration of the call.
>> +/// - `parm` must be a pointer to a `bindings::kernel_param` that is valid for reads for the
>
> s/parm/param/
Yea, I get contaminated with spellings used elsewhere in the kernel.
>
>> +/// duration of the call.
>> +/// - `param.arg` must be a pointer to an initialized `T` that is valid for writes for the duration
>> +/// of the function.
>> +///
>> +/// # Note
>> +///
>> +/// - The safety requirements are satisfied by C API contract when this function is invoked by the
>> +/// module subsystem C code.
>> +/// - 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 c_char,
>> + param: *const crate::bindings::kernel_param,
>> +) -> 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 EINVAL.to_errno();
>> + }
>> +
>> + // SAFETY: By function safety requirement, val is non-null, null-terminated
>> + // and valid for reads for the duration of this function.
>> + let arg = unsafe { CStr::from_char_ptr(val) };
>
> `arg` has the type `&'static CStr`, which is not justified by the safety
> comment :(
Not any longer, as outlined above. Thanks for catching this.
> Why does `ModuleParam::try_from_param_arg` take a `&'static BStr` and
> not a `&BStr`? I guess it is related to the "make copies if allocator is
> available" question I had above.
Yep.
>
>> +
>> + crate::error::from_result(|| {
>> + let new_value = T::try_from_param_arg(arg)?;
>> +
>> + // SAFETY: By function safety requirements `param` is be valid for reads.
>> + let old_value = unsafe { (*param).__bindgen_anon_1.arg as *mut T };
>> +
>> + // SAFETY: By function safety requirements, the target of `old_value` is valid for writes
>> + // and is initialized.
>> + unsafe { *old_value = new_value };
>
> So if we keep the `ModuleParam: Copy` bound from above, then we don't
> need to drop the type here (as `Copy` implies `!Drop`). So we could also
> remove the requirement for initialized memory and use `ptr::write` here
> instead. Thoughts?
Yes, that is the rationale for the `Copy` bound. What would be the
benefit of using `ptr::write`? They should be equivalent for `Copy`
types, right.
I was using `ptr::replace`, but Alice suggested the pace expression
assignment instead, since I was not using the old value.
>
>> + Ok(0)
>> + })
>> +}
>> +
>> +macro_rules! impl_int_module_param {
>> + ($ty:ident) => {
>> + impl ModuleParam for $ty {
>> + type Value = $ty;
>> +
>> + fn try_from_param_arg(arg: &'static BStr) -> Result<Self> {
>> + <$ty as crate::str::parse_int::ParseInt>::from_str(arg)
>> + }
>> + }
>> + };
>> +}
>> +
>> +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.
>> +///
>> +/// Note: This type is `pub` because it is used by module crates to access
>> +/// parameter values.
>> +#[repr(transparent)]
>> +pub struct ModuleParamAccess<T> {
>> + data: core::cell::UnsafeCell<T>,
>> +}
>
> We should just re-create the `SyncUnsafeCell` [1] from upstream...
I will add a // TODO: Use `SyncUnsafeCell` when available.
>
> Feel free to keep this until we have it though.
>
> [1]: https://doc.rust-lang.org/nightly/std/cell/struct.SyncUnsafeCell.html
>
>> +
>> +// 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()
>> + }
>> +}
>> +
>> +#[doc(hidden)]
>> +#[macro_export]
>
> Why export this?
Legacy debt. I'll remove it.
Best regards,
Andreas Hindborg
^ permalink raw reply
* Re: [PATCH v13 1/6] rust: str: add radix prefixed integer parsing functions
From: Benno Lossin @ 2025-06-19 12:17 UTC (permalink / raw)
To: Andreas Hindborg
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Nicolas Schier, Trevor Gross, Adam Bratschi-Kaye, rust-for-linux,
linux-kernel, linux-kbuild, Petr Pavlu, Sami Tolvanen,
Daniel Gomez, Simona Vetter, Greg KH, Fiona Behrens,
Daniel Almeida, linux-modules
In-Reply-To: <871prg7zoh.fsf@kernel.org>
On Thu Jun 19, 2025 at 1:12 PM CEST, Andreas Hindborg wrote:
> I'm having a difficult time parsing. Are you suggesting that we guard
> against implementations of `TryInto<u64>` that misbehave?
Let me try a different explanation:
The safety requirement for implementing the `FromStrRadix`:
/// The member functions of this trait must be implemented according to
/// their documentation.
Together with the functions of the trait:
/// Parse `src` to [`Self`] using radix `radix`.
fn from_str_radix(src: &BStr, radix: u32) -> Result<Self, crate::error::Error>;
/// Return the absolute value of [`Self::MIN`].
fn abs_min() -> u64;
/// Perform bitwise 2's complement on `self`.
///
/// Note: This function does not make sense for unsigned integers.
fn complement(self) -> Self;
Doesn't make sense. What does it mean to return the "absolute value of
[`Self::MIN`]"? We don't have "absolute value" defined for an arbitrary
type. Similarly for `complement` and `from_str_radix`, what does "Parse
`src` to [`Self`] using radex `radix`" mean? It's not well-defined.
You use this safety requirement in the parsing branch for negative
numbers (the `unsafe` call at the bottom):
[b'-', rest @ ..] => {
let (radix, digits) = strip_radix(rest.as_ref());
// 2's complement values range from -2^(b-1) to 2^(b-1)-1.
// So if we want to parse negative numbers as positive and
// later multiply by -1, we have to parse into a larger
// integer. We choose `u64` as sufficiently large.
//
// NOTE: 128 bit integers are not available on all
// platforms, hence the choice of 64 bits.
let val =
u64::from_str_radix(core::str::from_utf8(digits).map_err(|_| EINVAL)?, radix)
.map_err(|_| EINVAL)?;
if val > Self::abs_min() {
return Err(EINVAL);
}
if val == Self::abs_min() {
return Ok(Self::MIN);
}
// SAFETY: We checked that `val` will fit in `Self` above.
let val: Self = unsafe { val.try_into().unwrap_unchecked() };
Ok(val.complement())
}
But you don't mention that the check is valid due to the safety
requirements of implementing `FromStrRadix`. But even if you did, that
wouldn't mean anything as I explained above.
So let's instead move all of this negation & u64 conversion logic into
the `FromStrRadix` trait. Then it can be safe & the `ParseInt::from_str`
function doesn't use `unsafe` (there still will be `unsafe` in the
macro, but that is fine, as it's more local and knows the concrete
types).
---
Cheers,
Benno
Here is what I have in mind:
diff --git a/rust/kernel/str/parse_int.rs b/rust/kernel/str/parse_int.rs
index 0754490aec4b..9d6e146c5ea7 100644
--- a/rust/kernel/str/parse_int.rs
+++ b/rust/kernel/str/parse_int.rs
@@ -13,32 +13,16 @@
// `ParseInt`, that is, prevents downstream users from implementing the
// trait.
mod private {
+ use crate::prelude::*;
use crate::str::BStr;
/// Trait that allows parsing a [`&BStr`] to an integer with a radix.
- ///
- /// # Safety
- ///
- /// The member functions of this trait must be implemented according to
- /// their documentation.
- ///
- /// [`&BStr`]: kernel::str::BStr
- // This is required because the `from_str_radix` function on the primitive
- // integer types is not part of any trait.
- pub unsafe trait FromStrRadix: Sized {
- /// The minimum value this integer type can assume.
- const MIN: Self;
-
+ pub trait FromStrRadix: Sized {
/// Parse `src` to [`Self`] using radix `radix`.
- fn from_str_radix(src: &BStr, radix: u32) -> Result<Self, crate::error::Error>;
-
- /// Return the absolute value of [`Self::MIN`].
- fn abs_min() -> u64;
+ fn from_str_radix(src: &BStr, radix: u32) -> Result<Self>;
- /// Perform bitwise 2's complement on `self`.
- ///
- /// Note: This function does not make sense for unsigned integers.
- fn complement(self) -> Self;
+ /// Tries to convert `value` into [`Self`] and negates the resulting value.
+ fn from_u64_negated(value: u64) -> Result<Self>;
}
}
@@ -108,19 +92,7 @@ fn from_str(src: &BStr) -> Result<Self> {
let val =
u64::from_str_radix(core::str::from_utf8(digits).map_err(|_| EINVAL)?, radix)
.map_err(|_| EINVAL)?;
-
- if val > Self::abs_min() {
- return Err(EINVAL);
- }
-
- if val == Self::abs_min() {
- return Ok(Self::MIN);
- }
-
- // SAFETY: We checked that `val` will fit in `Self` above.
- let val: Self = unsafe { val.try_into().unwrap_unchecked() };
-
- Ok(val.complement())
+ Self::from_u64_negated(val)
}
_ => {
let (radix, digits) = strip_radix(src);
@@ -131,41 +103,49 @@ fn from_str(src: &BStr) -> Result<Self> {
}
macro_rules! impl_parse_int {
- ($ty:ty) => {
- // SAFETY: We implement the trait according to the documentation.
- unsafe impl private::FromStrRadix for $ty {
- const MIN: Self = <$ty>::MIN;
-
- fn from_str_radix(src: &BStr, radix: u32) -> Result<Self, crate::error::Error> {
- <$ty>::from_str_radix(core::str::from_utf8(src).map_err(|_| EINVAL)?, radix)
- .map_err(|_| EINVAL)
- }
-
- fn abs_min() -> u64 {
- #[allow(unused_comparisons)]
- if Self::MIN < 0 {
- 1u64 << (Self::BITS - 1)
- } else {
- 0
+ ($($ty:ty),*) => {
+ $(
+ impl private::FromStrRadix for $ty {
+ fn from_str_radix(src: &BStr, radix: u32) -> Result<Self> {
+ <$ty>::from_str_radix(core::str::from_utf8(src).map_err(|_| EINVAL)?, radix)
+ .map_err(|_| EINVAL)
}
- }
- fn complement(self) -> Self {
- (!self).wrapping_add((1 as $ty))
+ fn from_u64_negated(value: u64) -> Result<Self> {
+ const ABS_MIN: u64 = {
+ #[allow(unused_comparisons)]
+ if <$ty>::MIN < 0 {
+ 1u64 << (Self::BITS - 1)
+ } else {
+ 0
+ }
+ };
+
+ fn complement(self) -> Self {
+ (!self).wrapping_add((1 as $ty))
+ }
+ if val > ABS_MIN {
+ return Err(EINVAL);
+ }
+
+ if val == ABS_MIN {
+ return Ok(<$ty>::MIN);
+ }
+
+ // SAFETY: The above checks guarantee that `val` fits into `Self`:
+ // - if `Self` is unsigned, then `ABS_MIN == 0` and thus we have returned above
+ // (either `EINVAL` or `MIN`).
+ // - if `Self` is signed, then we have that `0 <= val < ABS_MIN`. And since
+ // `ABS_MIN - 1` fits into `Self` by construction, `val` also does.
+ let val: Self = unsafe { val.try_into().unwrap_unchecked() };
+
+ Ok((!val).wrapping_add(1))
+ }
}
- }
- impl ParseInt for $ty {}
+ impl ParseInt for $ty {}
+ )*
};
}
-impl_parse_int!(i8);
-impl_parse_int!(u8);
-impl_parse_int!(i16);
-impl_parse_int!(u16);
-impl_parse_int!(i32);
-impl_parse_int!(u32);
-impl_parse_int!(i64);
-impl_parse_int!(u64);
-impl_parse_int!(isize);
-impl_parse_int!(usize);
+impl_parse_int![i8, u8, i16, u16, i32, u32, i64, u64, isize, usize];
^ permalink raw reply related
* Re: [PATCH v13 1/6] rust: str: add radix prefixed integer parsing functions
From: Andreas Hindborg @ 2025-06-19 11:12 UTC (permalink / raw)
To: Benno Lossin
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Nicolas Schier, Trevor Gross, Adam Bratschi-Kaye, rust-for-linux,
linux-kernel, linux-kbuild, Petr Pavlu, Sami Tolvanen,
Daniel Gomez, Simona Vetter, Greg KH, Fiona Behrens,
Daniel Almeida, linux-modules
In-Reply-To: <DAPY5HF9HGXC.FCEKAMLPFY1H@kernel.org>
"Benno Lossin" <lossin@kernel.org> writes:
> On Thu Jun 12, 2025 at 3:40 PM CEST, Andreas Hindborg wrote:
>> +pub trait ParseInt: private::FromStrRadix + TryFrom<u64> {
>> + /// Parse a string according to the description in [`Self`].
>> + fn from_str(src: &BStr) -> Result<Self> {
>> + match src.deref() {
>> + [b'-', rest @ ..] => {
>> + let (radix, digits) = strip_radix(rest.as_ref());
>> + // 2's complement values range from -2^(b-1) to 2^(b-1)-1.
>> + // So if we want to parse negative numbers as positive and
>> + // later multiply by -1, we have to parse into a larger
>> + // integer. We choose `u64` as sufficiently large.
>> + //
>> + // NOTE: 128 bit integers are not available on all
>> + // platforms, hence the choice of 64 bits.
>> + let val =
>> + u64::from_str_radix(core::str::from_utf8(digits).map_err(|_| EINVAL)?, radix)
>> + .map_err(|_| EINVAL)?;
>> +
>> + if val > Self::abs_min() {
>> + return Err(EINVAL);
>> + }
>> +
>> + if val == Self::abs_min() {
>> + return Ok(Self::MIN);
>> + }
>> +
>> + // SAFETY: We checked that `val` will fit in `Self` above.
>
> Sorry that it took me this long to realize, but this seems pretty weird.
> I guess this is why the `FromStrRadix` is `unsafe`.
>
> Can we just move this part of the code to `FromStrRadix` and make that
> trait safe?
>
> So essentially have:
>
> fn from_u64(value: u64) -> Result<Self>;
>
> in `FromStrRadix` and remove `MIN`, `abs_min` and `complement`. Then
> implement it like this in the macro below:
>
> const ABS_MIN = /* existing abs_min impl */;
> if value > ABS_MIN {
> return Err(EINVAL);
> }
> if val == ABS_MIN {
> return Ok(<$ty>::MIN);
> }
> // SAFETY: We checked that `val` will fit in `Self` above.
> let val: $ty = unsafe { val.try_into().unwrap_unchecked() };
> (!val).wrapping_add(1)
>
> The reason that this is fine and the above is "weird" is the following:
> The current version only has `Self: FromStrRadix` which gives it access
> to the following guarantee from the `unsafe` trait:
>
> /// The member functions of this trait must be implemented according to
> /// their documentation.
> ///
> /// [`&BStr`]: kernel::str::BStr
>
> This doesn't mention `TryFrom<u64>` and thus the comment "We checked
> that `val` will fit in `Self` above" doesn't really apply: how does
> checking with the bounds given in `FromStrRadix` make `TryFrom` return
> `Ok`?
I'm having a difficult time parsing. Are you suggesting that we guard
against implementations of `TryInto<u64>` that misbehave?
Best regards,
Andreas Hindborg
^ permalink raw reply
* Re: [PATCH v13 4/6] rust: module: update the module macro with module parameter support
From: Benno Lossin @ 2025-06-18 21:07 UTC (permalink / raw)
To: Andreas Hindborg, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Nicolas Schier
Cc: Trevor Gross, Adam Bratschi-Kaye, rust-for-linux, linux-kernel,
linux-kbuild, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Simona Vetter, Greg KH, Fiona Behrens, Daniel Almeida,
linux-modules
In-Reply-To: <20250612-module-params-v3-v13-4-bc219cd1a3f8@kernel.org>
On Thu Jun 12, 2025 at 3:40 PM CEST, Andreas Hindborg wrote:
> +
> + fn emit_params(&mut self, info: &ModuleInfo) {
> + let Some(params) = &info.params else {
> + return;
> + };
> +
> + for param in params {
> + let ops = param_ops_path(¶m.ptype);
> +
> + // Note: The spelling of these fields is dictated by the user space
> + // tool `modinfo`.
> + self.emit_param("parmtype", ¶m.name, ¶m.ptype);
> + self.emit_param("parm", ¶m.name, ¶m.description);
I just read this part again and I want to voice my dissatisfaction with
these key names. (not that you can do anything about that :)
> +
> + write!(
> + self.param_buffer,
> + "
> + pub(crate) static {param_name}:
Does this need to be accessed by anything else except the static below?
If no, then can we move it inside of that static? So
#[link_section = \"__param\"]
#[used]
static __{module_name}_{param_name}_struct: ::kernel::module_param::RacyKernelParam = {
static {param_name}:
::kernel::module_param::ModuleParamAccess<{param_type}> =
::kernel::module_param::ModuleParamAccess::new({param_default});
// ...
};
---
Cheers,
Benno
> + ::kernel::module_param::ModuleParamAccess<{param_type}> =
> + ::kernel::module_param::ModuleParamAccess::new({param_default});
> +
> + #[link_section = \"__param\"]
> + #[used]
> + static __{module_name}_{param_name}_struct:
> + ::kernel::module_param::RacyKernelParam =
> + ::kernel::module_param::RacyKernelParam(::kernel::bindings::kernel_param {{
> + name: if cfg!(MODULE) {{
> + ::kernel::c_str!(\"{param_name}\").as_bytes_with_nul()
> + }} else {{
> + ::kernel::c_str!(\"{module_name}.{param_name}\").as_bytes_with_nul()
> + }}.as_ptr(),
> + // SAFETY: `__this_module` is constructed by the kernel at load time
> + // and will not be freed until the module is unloaded.
> + #[cfg(MODULE)]
> + mod_: unsafe {{
> + (&::kernel::bindings::__this_module
> + as *const ::kernel::bindings::module)
> + .cast_mut()
> + }},
> + #[cfg(not(MODULE))]
> + mod_: ::core::ptr::null_mut(),
> + ops: &{ops} as *const ::kernel::bindings::kernel_param_ops,
> + perm: 0, // Will not appear in sysfs
> + level: -1,
> + flags: 0,
> + __bindgen_anon_1:
> + ::kernel::bindings::kernel_param__bindgen_ty_1 {{
> + arg: {param_name}.as_mut_ptr().cast()
> + }},
> + }});
> + ",
> + module_name = info.name,
> + param_type = param.ptype,
> + param_default = param.default,
> + param_name = param.name,
> + ops = ops,
> + )
> + .unwrap();
> + }
> + }
> +}
^ permalink raw reply
* Re: [PATCH v13 2/6] rust: introduce module_param module
From: Benno Lossin @ 2025-06-18 20:59 UTC (permalink / raw)
To: Andreas Hindborg, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Nicolas Schier
Cc: Trevor Gross, Adam Bratschi-Kaye, rust-for-linux, linux-kernel,
linux-kbuild, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Simona Vetter, Greg KH, Fiona Behrens, Daniel Almeida,
linux-modules
In-Reply-To: <20250612-module-params-v3-v13-2-bc219cd1a3f8@kernel.org>
On Thu Jun 12, 2025 at 3:40 PM CEST, Andreas Hindborg wrote:
> Add types and traits for interfacing the C moduleparam API.
>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
> rust/kernel/lib.rs | 1 +
> rust/kernel/module_param.rs | 201 ++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 202 insertions(+)
>
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 6b4774b2b1c3..2b439ea06185 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -87,6 +87,7 @@
> pub mod list;
> pub mod miscdevice;
> pub mod mm;
> +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 000000000000..fd167df8e53d
> --- /dev/null
> +++ b/rust/kernel/module_param.rs
> @@ -0,0 +1,201 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Support 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);
s/::kernel:://
The field shouldn't be public, since then people can access it. Can
just have a `pub fn new` instead?
> +
> +// SAFETY: C kernel handles serializing access to this type. We never access it
> +// from Rust module.
> +unsafe impl Sync for RacyKernelParam {}
> +
> +/// Types that can be used for module parameters.
> +pub trait ModuleParam: Sized + Copy {
Why the `Copy` bound?
> + /// 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.
I don't think we need to explicitly mention this.
> + ///
> + /// Parameters passed at boot time will be set before [`kmalloc`] is
> + /// available (even if the module is loaded at a later time). However, in
I think we should make a section out of this like `# No allocations` (or
something better). Let's also mention it on the trait itself, since
that's where implementers will most likely look.
> + /// 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
We probably shouldn't recommend directly using `bindings`.
> + /// 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.
I don't understand this convention, but it also doesn't seem to
relevant (so feel free to leave it as is, but it would be nice if you
could explain it).
> + ///
> + /// [`kmalloc`]: srctree/include/linux/slab.h
> + fn try_from_param_arg(arg: &'static BStr) -> 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`.
> +///
> +/// See `struct kernel_param_ops.set`.
> +///
> +/// # Safety
> +///
> +/// - If `val` is non-null then it must point to a valid null-terminated string that must be valid
> +/// for reads for the duration of the call.
> +/// - `parm` must be a pointer to a `bindings::kernel_param` that is valid for reads for the
s/parm/param/
> +/// duration of the call.
> +/// - `param.arg` must be a pointer to an initialized `T` that is valid for writes for the duration
> +/// of the function.
> +///
> +/// # Note
> +///
> +/// - The safety requirements are satisfied by C API contract when this function is invoked by the
> +/// module subsystem C code.
> +/// - 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 c_char,
> + param: *const crate::bindings::kernel_param,
> +) -> 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 EINVAL.to_errno();
> + }
> +
> + // SAFETY: By function safety requirement, val is non-null, null-terminated
> + // and valid for reads for the duration of this function.
> + let arg = unsafe { CStr::from_char_ptr(val) };
`arg` has the type `&'static CStr`, which is not justified by the safety
comment :(
Why does `ModuleParam::try_from_param_arg` take a `&'static BStr` and
not a `&BStr`? I guess it is related to the "make copies if allocator is
available" question I had above.
> +
> + crate::error::from_result(|| {
> + let new_value = T::try_from_param_arg(arg)?;
> +
> + // SAFETY: By function safety requirements `param` is be valid for reads.
> + let old_value = unsafe { (*param).__bindgen_anon_1.arg as *mut T };
> +
> + // SAFETY: By function safety requirements, the target of `old_value` is valid for writes
> + // and is initialized.
> + unsafe { *old_value = new_value };
So if we keep the `ModuleParam: Copy` bound from above, then we don't
need to drop the type here (as `Copy` implies `!Drop`). So we could also
remove the requirement for initialized memory and use `ptr::write` here
instead. Thoughts?
> + Ok(0)
> + })
> +}
> +
> +macro_rules! impl_int_module_param {
> + ($ty:ident) => {
> + impl ModuleParam for $ty {
> + type Value = $ty;
> +
> + fn try_from_param_arg(arg: &'static BStr) -> Result<Self> {
> + <$ty as crate::str::parse_int::ParseInt>::from_str(arg)
> + }
> + }
> + };
> +}
> +
> +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.
> +///
> +/// Note: This type is `pub` because it is used by module crates to access
> +/// parameter values.
> +#[repr(transparent)]
> +pub struct ModuleParamAccess<T> {
> + data: core::cell::UnsafeCell<T>,
> +}
We should just re-create the `SyncUnsafeCell` [1] from upstream...
Feel free to keep this until we have it though.
[1]: https://doc.rust-lang.org/nightly/std/cell/struct.SyncUnsafeCell.html
> +
> +// 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()
> + }
> +}
> +
> +#[doc(hidden)]
> +#[macro_export]
Why export this?
---
Cheers,
Benno
> +/// Generate a static [`kernel_param_ops`](srctree/include/linux/moduleparam.h) struct.
> +///
> +/// # Examples
> +///
> +/// ```ignore
> +/// make_param_ops!(
> +/// /// Documentation for new param ops.
> +/// PARAM_OPS_MYTYPE, // Name for the static.
> +/// MyType // A type which implements [`ModuleParam`].
> +/// );
> +/// ```
> +macro_rules! make_param_ops {
> + ($ops:ident, $ty:ty) => {
> + #[doc(hidden)]
> + pub static $ops: $crate::bindings::kernel_param_ops = $crate::bindings::kernel_param_ops {
> + flags: 0,
> + set: Some(set_param::<$ty>),
> + get: None,
> + free: None,
> + };
> + };
> +}
> +
> +make_param_ops!(PARAM_OPS_I8, i8);
> +make_param_ops!(PARAM_OPS_U8, u8);
> +make_param_ops!(PARAM_OPS_I16, i16);
> +make_param_ops!(PARAM_OPS_U16, u16);
> +make_param_ops!(PARAM_OPS_I32, i32);
> +make_param_ops!(PARAM_OPS_U32, u32);
> +make_param_ops!(PARAM_OPS_I64, i64);
> +make_param_ops!(PARAM_OPS_U64, u64);
> +make_param_ops!(PARAM_OPS_ISIZE, isize);
> +make_param_ops!(PARAM_OPS_USIZE, usize);
^ permalink raw reply
* Re: [PATCH v13 1/6] rust: str: add radix prefixed integer parsing functions
From: Benno Lossin @ 2025-06-18 20:38 UTC (permalink / raw)
To: Andreas Hindborg, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Nicolas Schier
Cc: Trevor Gross, Adam Bratschi-Kaye, rust-for-linux, linux-kernel,
linux-kbuild, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Simona Vetter, Greg KH, Fiona Behrens, Daniel Almeida,
linux-modules
In-Reply-To: <20250612-module-params-v3-v13-1-bc219cd1a3f8@kernel.org>
On Thu Jun 12, 2025 at 3:40 PM CEST, Andreas Hindborg wrote:
> +pub trait ParseInt: private::FromStrRadix + TryFrom<u64> {
> + /// Parse a string according to the description in [`Self`].
> + fn from_str(src: &BStr) -> Result<Self> {
> + match src.deref() {
> + [b'-', rest @ ..] => {
> + let (radix, digits) = strip_radix(rest.as_ref());
> + // 2's complement values range from -2^(b-1) to 2^(b-1)-1.
> + // So if we want to parse negative numbers as positive and
> + // later multiply by -1, we have to parse into a larger
> + // integer. We choose `u64` as sufficiently large.
> + //
> + // NOTE: 128 bit integers are not available on all
> + // platforms, hence the choice of 64 bits.
> + let val =
> + u64::from_str_radix(core::str::from_utf8(digits).map_err(|_| EINVAL)?, radix)
> + .map_err(|_| EINVAL)?;
> +
> + if val > Self::abs_min() {
> + return Err(EINVAL);
> + }
> +
> + if val == Self::abs_min() {
> + return Ok(Self::MIN);
> + }
> +
> + // SAFETY: We checked that `val` will fit in `Self` above.
Sorry that it took me this long to realize, but this seems pretty weird.
I guess this is why the `FromStrRadix` is `unsafe`.
Can we just move this part of the code to `FromStrRadix` and make that
trait safe?
So essentially have:
fn from_u64(value: u64) -> Result<Self>;
in `FromStrRadix` and remove `MIN`, `abs_min` and `complement`. Then
implement it like this in the macro below:
const ABS_MIN = /* existing abs_min impl */;
if value > ABS_MIN {
return Err(EINVAL);
}
if val == ABS_MIN {
return Ok(<$ty>::MIN);
}
// SAFETY: We checked that `val` will fit in `Self` above.
let val: $ty = unsafe { val.try_into().unwrap_unchecked() };
(!val).wrapping_add(1)
The reason that this is fine and the above is "weird" is the following:
The current version only has `Self: FromStrRadix` which gives it access
to the following guarantee from the `unsafe` trait:
/// The member functions of this trait must be implemented according to
/// their documentation.
///
/// [`&BStr`]: kernel::str::BStr
This doesn't mention `TryFrom<u64>` and thus the comment "We checked
that `val` will fit in `Self` above" doesn't really apply: how does
checking with the bounds given in `FromStrRadix` make `TryFrom` return
`Ok`?
If we move this code into the implementation of `FromStrRadix`, then we
are locally in a context where we *know* the concrete type of `Self` and
can thus rely on "checking" being the correct thing for `TryFrom`.
With this adjustment, I can give my RB, but please let me take a look
before you send it again :)
---
Cheers,
Benno
> + let val: Self = unsafe { val.try_into().unwrap_unchecked() };
> +
> + Ok(val.complement())
> + }
> + _ => {
> + let (radix, digits) = strip_radix(src);
> + Self::from_str_radix(digits, radix).map_err(|_| EINVAL)
> + }
> + }
> + }
> +}
^ permalink raw reply
* Re: [PATCH v13 3/6] rust: module: use a reference in macros::module::module
From: Benno Lossin @ 2025-06-18 20:07 UTC (permalink / raw)
To: Andreas Hindborg, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Nicolas Schier
Cc: Trevor Gross, Adam Bratschi-Kaye, rust-for-linux, linux-kernel,
linux-kbuild, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Simona Vetter, Greg KH, Fiona Behrens, Daniel Almeida,
linux-modules
In-Reply-To: <20250612-module-params-v3-v13-3-bc219cd1a3f8@kernel.org>
On Thu Jun 12, 2025 at 3:40 PM CEST, Andreas Hindborg wrote:
> When we add parameter support to the module macro, we want to be able to
> pass a reference to `ModuleInfo` to a helper function. That is not possible
> when we move out of the local `modinfo`. So change the function to access
> the local via reference rather than value.
>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
Reviewed-by: Benno Lossin <lossin@kernel.org>
---
Cheers,
Benno
> ---
> rust/macros/module.rs | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
^ permalink raw reply
* Re: [PATCH v13 5/6] rust: samples: add a module parameter to the rust_minimal sample
From: Benno Lossin @ 2025-06-18 19:48 UTC (permalink / raw)
To: Andreas Hindborg, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Nicolas Schier
Cc: Trevor Gross, Adam Bratschi-Kaye, rust-for-linux, linux-kernel,
linux-kbuild, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Simona Vetter, Greg KH, Fiona Behrens, Daniel Almeida,
linux-modules
In-Reply-To: <20250612-module-params-v3-v13-5-bc219cd1a3f8@kernel.org>
On Thu Jun 12, 2025 at 3:40 PM CEST, Andreas Hindborg wrote:
> Showcase the rust module parameter support by adding a module parameter to
> the `rust_minimal` sample.
>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
Reviewed-by: Benno Lossin <lossin@kernel.org>
---
Cheers,
Benno
> ---
> samples/rust/rust_minimal.rs | 10 ++++++++++
> 1 file changed, 10 insertions(+)
^ permalink raw reply
* Re: [PATCH v2 1/2] module: Fix memory deallocation on error path in move_module()
From: Daniel Gomez @ 2025-06-18 14:02 UTC (permalink / raw)
To: Petr Pavlu, Luis Chamberlain, Sami Tolvanen, Daniel Gomez
Cc: linux-modules, linux-kernel
In-Reply-To: <20250618122730.51324-2-petr.pavlu@suse.com>
On 18/06/2025 14.26, Petr Pavlu wrote:
> The function move_module() uses the variable t to track how many memory
> types it has allocated and consequently how many should be freed if an
> error occurs.
>
> The variable is initially set to 0 and is updated when a call to
> module_memory_alloc() fails. However, move_module() can fail for other
> reasons as well, in which case t remains set to 0 and no memory is freed.
>
> Fix the problem by initializing t to MOD_MEM_NUM_TYPES. Additionally, make
> the deallocation loop more robust by not relying on the mod_mem_type_t enum
> having a signed integer as its underlying type.
>
> Fixes: c7ee8aebf6c0 ("module: add stop-grap sanity check on module memcpy()")
> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
> Reviewed-by: Sami Tolvanen <samitolvanen@google.com>
> ---
> kernel/module/main.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 413ac6ea3702..9ac994b2f354 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -2697,7 +2697,7 @@ static int find_module_sections(struct module *mod, struct load_info *info)
> static int move_module(struct module *mod, struct load_info *info)
> {
> int i;
> - enum mod_mem_type t = 0;
> + enum mod_mem_type t = MOD_MEM_NUM_TYPES;
> int ret = -ENOMEM;
> bool codetag_section_found = false;
>
> @@ -2776,7 +2776,7 @@ static int move_module(struct module *mod, struct load_info *info)
> return 0;
> out_err:
> module_memory_restore_rox(mod);
> - for (t--; t >= 0; t--)
> + while (t--)
> module_memory_free(mod, t);
> if (codetag_section_found)
> codetag_free_module_sections(mod);
Reviewed-by: Daniel Gomez <da.gomez@samsung.com>
^ permalink raw reply
* Re: [PATCH v2 2/2] module: Avoid unnecessary return value initialization in move_module()
From: Daniel Gomez @ 2025-06-18 14:02 UTC (permalink / raw)
To: Petr Pavlu, Luis Chamberlain, Sami Tolvanen, Daniel Gomez
Cc: linux-modules, linux-kernel
In-Reply-To: <20250618122730.51324-3-petr.pavlu@suse.com>
On 18/06/2025 14.26, Petr Pavlu wrote:
> All error conditions in move_module() set the return value by updating the
> ret variable. Therefore, it is not necessary to the initialize the variable
> when declaring it.
>
> Remove the unnecessary initialization.
>
> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
> Reviewed-by: Sami Tolvanen <samitolvanen@google.com>
> ---
> kernel/module/main.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 9ac994b2f354..7822b91fca6b 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -2696,9 +2696,8 @@ static int find_module_sections(struct module *mod, struct load_info *info)
>
> static int move_module(struct module *mod, struct load_info *info)
> {
> - int i;
> + int i, ret;
> enum mod_mem_type t = MOD_MEM_NUM_TYPES;
> - int ret = -ENOMEM;
> bool codetag_section_found = false;
>
> for_each_mod_mem_type(type) {
Reviewed-by: Daniel Gomez <da.gomez@samsung.com>
^ permalink raw reply
* [PATCH v2] codetag: Avoid unused alloc_tags sections/symbols
From: Petr Pavlu @ 2025-06-18 12:50 UTC (permalink / raw)
To: Andrew Morton
Cc: Suren Baghdasaryan, Kent Overstreet, Arnd Bergmann, Casey Chen,
linux-arch, linux-modules, linux-kernel, Petr Pavlu
With CONFIG_MEM_ALLOC_PROFILING=n, vmlinux and all modules unnecessarily
contain the symbols __start_alloc_tags and __stop_alloc_tags, which define
an empty range. In the case of modules, the presence of these symbols also
forces the linker to create an empty .codetag.alloc_tags section.
Update codetag.lds.h to make the data conditional on
CONFIG_MEM_ALLOC_PROFILING.
Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
Reviewed-by: Kent Overstreet <kent.overstreet@linux.dev>
Reviewed-by: Suren Baghdasaryan <surenb@google.com>
---
Changes since v1 [1]:
- Trivially rebased the patch on top of "alloc_tag: remove empty module tag
section" [2].
[1] https://lore.kernel.org/all/20250313143002.9118-1-petr.pavlu@suse.com/
[2] https://lore.kernel.org/all/20250610162258.324645-1-cachen@purestorage.com/
include/asm-generic/codetag.lds.h | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/include/asm-generic/codetag.lds.h b/include/asm-generic/codetag.lds.h
index a45fe3d141a1..a14f4bdafdda 100644
--- a/include/asm-generic/codetag.lds.h
+++ b/include/asm-generic/codetag.lds.h
@@ -2,6 +2,12 @@
#ifndef __ASM_GENERIC_CODETAG_LDS_H
#define __ASM_GENERIC_CODETAG_LDS_H
+#ifdef CONFIG_MEM_ALLOC_PROFILING
+#define IF_MEM_ALLOC_PROFILING(...) __VA_ARGS__
+#else
+#define IF_MEM_ALLOC_PROFILING(...)
+#endif
+
#define SECTION_WITH_BOUNDARIES(_name) \
. = ALIGN(8); \
__start_##_name = .; \
@@ -9,7 +15,7 @@
__stop_##_name = .;
#define CODETAG_SECTIONS() \
- SECTION_WITH_BOUNDARIES(alloc_tags)
+ IF_MEM_ALLOC_PROFILING(SECTION_WITH_BOUNDARIES(alloc_tags))
#define MOD_SEPARATE_CODETAG_SECTION(_name) \
.codetag.##_name : { \
@@ -22,6 +28,6 @@
* unload them individually once unused.
*/
#define MOD_SEPARATE_CODETAG_SECTIONS() \
- MOD_SEPARATE_CODETAG_SECTION(alloc_tags)
+ IF_MEM_ALLOC_PROFILING(MOD_SEPARATE_CODETAG_SECTION(alloc_tags))
#endif /* __ASM_GENERIC_CODETAG_LDS_H */
base-commit: 52da431bf03b5506203bca27fe14a97895c80faf
prerequisite-patch-id: acb6e2f6708cd75488806308bfecf682b2367dc9
--
2.49.0
^ permalink raw reply related
* [PATCH v2 1/2] module: Fix memory deallocation on error path in move_module()
From: Petr Pavlu @ 2025-06-18 12:26 UTC (permalink / raw)
To: Luis Chamberlain, Petr Pavlu, Sami Tolvanen, Daniel Gomez
Cc: linux-modules, linux-kernel
In-Reply-To: <20250618122730.51324-1-petr.pavlu@suse.com>
The function move_module() uses the variable t to track how many memory
types it has allocated and consequently how many should be freed if an
error occurs.
The variable is initially set to 0 and is updated when a call to
module_memory_alloc() fails. However, move_module() can fail for other
reasons as well, in which case t remains set to 0 and no memory is freed.
Fix the problem by initializing t to MOD_MEM_NUM_TYPES. Additionally, make
the deallocation loop more robust by not relying on the mod_mem_type_t enum
having a signed integer as its underlying type.
Fixes: c7ee8aebf6c0 ("module: add stop-grap sanity check on module memcpy()")
Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
Reviewed-by: Sami Tolvanen <samitolvanen@google.com>
---
kernel/module/main.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/kernel/module/main.c b/kernel/module/main.c
index 413ac6ea3702..9ac994b2f354 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2697,7 +2697,7 @@ static int find_module_sections(struct module *mod, struct load_info *info)
static int move_module(struct module *mod, struct load_info *info)
{
int i;
- enum mod_mem_type t = 0;
+ enum mod_mem_type t = MOD_MEM_NUM_TYPES;
int ret = -ENOMEM;
bool codetag_section_found = false;
@@ -2776,7 +2776,7 @@ static int move_module(struct module *mod, struct load_info *info)
return 0;
out_err:
module_memory_restore_rox(mod);
- for (t--; t >= 0; t--)
+ while (t--)
module_memory_free(mod, t);
if (codetag_section_found)
codetag_free_module_sections(mod);
--
2.49.0
^ permalink raw reply related
* [PATCH v2 2/2] module: Avoid unnecessary return value initialization in move_module()
From: Petr Pavlu @ 2025-06-18 12:26 UTC (permalink / raw)
To: Luis Chamberlain, Petr Pavlu, Sami Tolvanen, Daniel Gomez
Cc: linux-modules, linux-kernel
In-Reply-To: <20250618122730.51324-1-petr.pavlu@suse.com>
All error conditions in move_module() set the return value by updating the
ret variable. Therefore, it is not necessary to the initialize the variable
when declaring it.
Remove the unnecessary initialization.
Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
Reviewed-by: Sami Tolvanen <samitolvanen@google.com>
---
kernel/module/main.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/kernel/module/main.c b/kernel/module/main.c
index 9ac994b2f354..7822b91fca6b 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2696,9 +2696,8 @@ static int find_module_sections(struct module *mod, struct load_info *info)
static int move_module(struct module *mod, struct load_info *info)
{
- int i;
+ int i, ret;
enum mod_mem_type t = MOD_MEM_NUM_TYPES;
- int ret = -ENOMEM;
bool codetag_section_found = false;
for_each_mod_mem_type(type) {
--
2.49.0
^ permalink raw reply related
* [PATCH v2 0/2] module: Fix memory deallocation on error path in move_module()
From: Petr Pavlu @ 2025-06-18 12:26 UTC (permalink / raw)
To: Luis Chamberlain, Petr Pavlu, Sami Tolvanen, Daniel Gomez
Cc: linux-modules, linux-kernel
The first patch is an actual fix. The second patch is a minor related
cleanup.
Changes since v1 [1]:
- Initialize t to MOD_MEM_NUM_TYPES in move_module(), instead of assigning
the value later.
- Merge the definitions of the variables i and ret in move_module().
[1] https://lore.kernel.org/linux-modules/20250607161823.409691-1-petr.pavlu@suse.com/
Petr Pavlu (2):
module: Fix memory deallocation on error path in move_module()
module: Avoid unnecessary return value initialization in move_module()
kernel/module/main.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
base-commit: 52da431bf03b5506203bca27fe14a97895c80faf
--
2.49.0
^ permalink raw reply
* Re: [PATCH 1/2] module: Fix memory deallocation on error path in move_module()
From: Daniel Gomez @ 2025-06-17 17:17 UTC (permalink / raw)
To: Petr Pavlu
Cc: Luis Chamberlain, Sami Tolvanen, Daniel Gomez, linux-modules,
linux-kernel
In-Reply-To: <2c277bfe-3086-4007-bf04-ef229e6cbfb7@suse.com>
>> One thing though, we are "releasing" the memory even if we have skipped the
>> allocation in the first place. So, I think it would make sense to release only
>> for the types we have actually allocated. What do you think?
>
> I noticed this too, specifically because move_module() is inconsistent
> in this regard with free_mod_mem(). The latter function contains:
>
> if (mod_mem->size)
> module_memory_free(mod, type);
>
> However, my preference is actually to update free_mod_mem() and remove
> the check. The function module_memory_free() should be a no-op if
> mod->base is NULL, similarly to how calling free(NULL) is a no-op.
>
Sound good to me. Perhaps a different patch type for cleanup/refactor. The fix
here would be back-ported to stable branches. So these are 2 different things.
^ permalink raw reply
* Re: [PATCH 1/2] module: Fix memory deallocation on error path in move_module()
From: Petr Pavlu @ 2025-06-17 15:41 UTC (permalink / raw)
To: Daniel Gomez
Cc: Luis Chamberlain, Sami Tolvanen, Daniel Gomez, linux-modules,
linux-kernel
In-Reply-To: <732dedee-c7c5-4226-ad87-f4c2311b11b3@kernel.org>
On 6/17/25 11:47 AM, Daniel Gomez wrote:
>> Do you mean the following, or something else:
>>
>> static int move_module(struct module *mod, struct load_info *info)
>> {
>> int i;
>> enum mod_mem_type t = MOD_MEM_NUM_TYPES;
>> int ret;
>> bool codetag_section_found = false;
>>
>> for_each_mod_mem_type(type) {
>> if (!mod->mem[type].size) {
>> mod->mem[type].base = NULL;
>> continue;
>> }
>>
>> ret = module_memory_alloc(mod, type);
>> if (ret) {
>> t = type;
>> goto out_err;
>> }
>> }
>>
>> [...]
>> }
>>
>
> Yes, that's it. From your patch, moving MOD_MEM_NUM_TYPE assigment to the
> initialization and use the while() loop suggested later on.
Ok.
>
> One thing though, we are "releasing" the memory even if we have skipped the
> allocation in the first place. So, I think it would make sense to release only
> for the types we have actually allocated. What do you think?
I noticed this too, specifically because move_module() is inconsistent
in this regard with free_mod_mem(). The latter function contains:
if (mod_mem->size)
module_memory_free(mod, type);
However, my preference is actually to update free_mod_mem() and remove
the check. The function module_memory_free() should be a no-op if
mod->base is NULL, similarly to how calling free(NULL) is a no-op.
--
Thanks,
Petr
^ permalink raw reply
* Re: [PATCH 3/3] kunit: test: Drop CONFIG_MODULE ifdeffery
From: Petr Pavlu @ 2025-06-17 15:07 UTC (permalink / raw)
To: Thomas Weißschuh
Cc: Luis Chamberlain, Sami Tolvanen, Daniel Gomez, Brendan Higgins,
David Gow, Rae Moar, linux-modules, linux-kernel, linux-kselftest,
kunit-dev
In-Reply-To: <20250617095936-50d985a4-ea18-49cf-9d16-dfd0dd0b627f@linutronix.de>
On 6/17/25 10:39 AM, Thomas Weißschuh wrote:
> On Tue, Jun 17, 2025 at 09:44:49AM +0200, Petr Pavlu wrote:
>> On 6/12/25 4:53 PM, Thomas Weißschuh wrote:
>>> The function stubs exposed by module.h allow the code to compile properly
>>> without the ifdeffery. The generated object code stays the same, as the
>>> compiler can optimize away all the dead code.
>>> As the code is still typechecked developer errors can be detected faster.
>>>
>>> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
>>
>> I'm worried that patches #2 and #3 make the code harder to understand
>> because they hide what is compiled and when.
>>
>> Normally, '#ifdef CONFIG_XYZ' or IS_ENABLED(CONFIG_XYZ) directly
>> surrounds functionality that should be conditional. This makes it clear
>> what is used and when.
>
> #ifdef is discouraged in C files and IS_ENABLED(CONFIG_MODULES) does not work
> (here) without patch #2.
>
>> The patches obscure whether, for instance, kunit_module_notify() in
>> lib/kunit/test.c is actually used and present in the resulting binary
>> behind several steps. Understanding its usage requires tracing the code
>> from kunit_module_notify() to the definition of kunit_mod_nb, then to
>> the register_module_notifier() call, and finally depends on an ifdef in
>> another file, include/linux/module.h.
>
> I agree that it is not completely clear what will end up in the binary.
> On the other hand we can program the happy path and the compiler will take care
> of all the corner cases.
> We could add an "if (IS_ENABLED(CONFIG_MODULES))" which does not really change
> anything but would be clearer to read.
>
>> Is this really better? Are there places where this pattern is already
>> used? Does it actually help in practice, considering that CONFIG_MODULES
>> is enabled in most cases?
>
> This came up for me when refactoring some KUnit internal code.
> I used "kunit.py run" (which uses CONFIG_MODULES=n) to test my changes.
> But some callers of changed functions were not updated and this wasn't reported.
I see.
>
> The stub functions are a standard pattern and already implemented by module.h.
Stub functions are ok, I have no concerns about that pattern.
> I have not found a header which hides structure definitions.
It seems you're right. I think that makes patch #2 acceptable, it is
consistent with other kernel code.
>
> Documentation/process/coding-style.rst:
>
> 21) Conditional Compilation
> ---------------------------
>
> Wherever possible, don't use preprocessor conditionals (#if, #ifdef) in .c
> files; doing so makes code harder to read and logic harder to follow. Instead,
> use such conditionals in a header file defining functions for use in those .c
> files, providing no-op stub versions in the #else case, and then call those
> functions unconditionally from .c files. The compiler will avoid generating
> any code for the stub calls, producing identical results, but the logic will
> remain easy to follow.
>
> I should add the documentation reference to patch #2.
This part discusses stub functions. I feel patch #3 stretches the
intention of the coding style described here. As discussed, the patch
somewhat hides when the functions are actually used, which might not be
desirable, but I'll leave it to the kunit folks to decide what they
prefer.
--
Thanks,
Petr
^ permalink raw reply
* Re: [PATCH] alloc_tag: remove empty module tag section
From: Suren Baghdasaryan @ 2025-06-17 15:05 UTC (permalink / raw)
To: Petr Pavlu
Cc: Casey Chen, akpm, linux-mm, linux-kernel, linux-modules,
linux-arch, kent.overstreet, arnd, mcgrof, pasha.tatashin, yzhong
In-Reply-To: <2cd3947a-63d9-4a79-a24a-eb1ae8164169@suse.com>
On Tue, Jun 17, 2025 at 2:27 AM Petr Pavlu <petr.pavlu@suse.com> wrote:
>
> On 6/10/25 6:22 PM, Casey Chen wrote:
> > The empty MOD_CODETAG_SECTIONS() macro added an incomplete .data
> > section in module linker script, which caused symbol lookup tools
> > like gdb to misinterpret symbol addresses e.g., __ib_process_cq
> > incorrectly mapping to unrelated functions like below.
> >
> > (gdb) disas __ib_process_cq
> > Dump of assembler code for function trace_event_fields_cq_schedule:
> >
> > Removing the empty section restores proper symbol resolution and
> > layout, ensuring .data placement behaves as expected.
>
> The patch looks ok me, but I'm somewhat confused about the problem.
> I think a linker should not add an empty output section if it doesn't
> contain anything, or if .data actually contains something then the extra
> dummy definition should be also harmless?
I also assumed so but apparently this is not entirely harmless.
>
> This also reminds me of my previous related fix "codetag: Avoid unused
> alloc_tags sections/symbols" [1] which fell through the cracks. I can
> rebase it on top of this patch.
>
> [1] https://lore.kernel.org/all/20250313143002.9118-1-petr.pavlu@suse.com/
Yes please.
>
> --
> Thanks,
> Petr
^ permalink raw reply
* Re: [PATCH 1/2] module: Fix memory deallocation on error path in move_module()
From: Daniel Gomez @ 2025-06-17 9:47 UTC (permalink / raw)
To: Petr Pavlu
Cc: Luis Chamberlain, Sami Tolvanen, Daniel Gomez, linux-modules,
linux-kernel
In-Reply-To: <97f26140-bf53-4c4d-bf63-2dd353a3ec85@suse.com>
> Do you mean the following, or something else:
>
> static int move_module(struct module *mod, struct load_info *info)
> {
> int i;
> enum mod_mem_type t = MOD_MEM_NUM_TYPES;
> int ret;
> bool codetag_section_found = false;
>
> for_each_mod_mem_type(type) {
> if (!mod->mem[type].size) {
> mod->mem[type].base = NULL;
> continue;
> }
>
> ret = module_memory_alloc(mod, type);
> if (ret) {
> t = type;
> goto out_err;
> }
> }
>
> [...]
> }
>
Yes, that's it. From your patch, moving MOD_MEM_NUM_TYPE assigment to the
initialization and use the while() loop suggested later on.
One thing though, we are "releasing" the memory even if we have skipped the
allocation in the first place. So, I think it would make sense to release only
for the types we have actually allocated. What do you think?
^ permalink raw reply
* Re: [PATCH] alloc_tag: remove empty module tag section
From: Petr Pavlu @ 2025-06-17 9:27 UTC (permalink / raw)
To: Casey Chen
Cc: akpm, linux-mm, linux-kernel, linux-modules, linux-arch, surenb,
kent.overstreet, arnd, mcgrof, pasha.tatashin, yzhong
In-Reply-To: <20250610162258.324645-1-cachen@purestorage.com>
On 6/10/25 6:22 PM, Casey Chen wrote:
> The empty MOD_CODETAG_SECTIONS() macro added an incomplete .data
> section in module linker script, which caused symbol lookup tools
> like gdb to misinterpret symbol addresses e.g., __ib_process_cq
> incorrectly mapping to unrelated functions like below.
>
> (gdb) disas __ib_process_cq
> Dump of assembler code for function trace_event_fields_cq_schedule:
>
> Removing the empty section restores proper symbol resolution and
> layout, ensuring .data placement behaves as expected.
The patch looks ok me, but I'm somewhat confused about the problem.
I think a linker should not add an empty output section if it doesn't
contain anything, or if .data actually contains something then the extra
dummy definition should be also harmless?
This also reminds me of my previous related fix "codetag: Avoid unused
alloc_tags sections/symbols" [1] which fell through the cracks. I can
rebase it on top of this patch.
[1] https://lore.kernel.org/all/20250313143002.9118-1-petr.pavlu@suse.com/
--
Thanks,
Petr
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox