* Re: [PATCH v2 0/3] module: make structure definitions always visible
From: Daniel Gomez @ 2025-07-11 13:39 UTC (permalink / raw)
To: Luis Chamberlain, Petr Pavlu, Sami Tolvanen, Brendan Higgins,
David Gow, Rae Moar, Thomas Weißschuh
Cc: linux-modules, linux-kernel, linux-kselftest, kunit-dev
In-Reply-To: <20250711-kunit-ifdef-modules-v2-0-39443decb1f8@linutronix.de>
On Fri, 11 Jul 2025 15:31:35 +0200, Thomas Weißschuh wrote:
> Code using IS_ENABLED(CONFIG_MODULES) as a C expression may need access
> to the module structure definitions to compile.
> Make sure these structure definitions are always visible.
>
> This will conflict with commit 6bb37af62634 ("module: Move modprobe_path
> and modules_disabled ctl_tables into the module subsys") from the sysctl
> tree, but the resolution is trivial.
>
> [...]
Applied, thanks!
[1/3] module: move 'struct module_use' to internal.h
commit: bb02f22eaabc4d878577e2b8c46ed7b6be5f5459
[2/3] module: make structure definitions always visible
commit: 02281b559cd1fdfdc8f7eb05bbbe3ab7b35246f0
[3/3] kunit: test: Drop CONFIG_MODULE ifdeffery
commit: dffcba8acea3a80b3478750ac32f17bd5345b68e
Best regards,
--
Daniel Gomez <da.gomez@samsung.com>
^ permalink raw reply
* [PATCH v2 3/3] kunit: test: Drop CONFIG_MODULE ifdeffery
From: Thomas Weißschuh @ 2025-07-11 13:31 UTC (permalink / raw)
To: Luis Chamberlain, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Brendan Higgins, David Gow, Rae Moar
Cc: linux-modules, linux-kernel, linux-kselftest, kunit-dev,
Thomas Weißschuh
In-Reply-To: <20250711-kunit-ifdef-modules-v2-0-39443decb1f8@linutronix.de>
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>
Acked-by: David Gow <davidgow@google.com>
Reviewed-by: Daniel Gomez <da.gomez@samsung.com>
---
lib/kunit/test.c | 8 --------
1 file changed, 8 deletions(-)
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index 146d1b48a0965e8aaddb6162928f408bbb542645..019b2ac9c8469021542b610278f8842e100d57ad 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -759,7 +759,6 @@ void __kunit_test_suites_exit(struct kunit_suite **suites, int num_suites)
}
EXPORT_SYMBOL_GPL(__kunit_test_suites_exit);
-#ifdef CONFIG_MODULES
static void kunit_module_init(struct module *mod)
{
struct kunit_suite_set suite_set, filtered_set;
@@ -847,7 +846,6 @@ static struct notifier_block kunit_mod_nb = {
.notifier_call = kunit_module_notify,
.priority = 0,
};
-#endif
KUNIT_DEFINE_ACTION_WRAPPER(kfree_action_wrapper, kfree, const void *)
@@ -938,20 +936,14 @@ static int __init kunit_init(void)
kunit_debugfs_init();
kunit_bus_init();
-#ifdef CONFIG_MODULES
return register_module_notifier(&kunit_mod_nb);
-#else
- return 0;
-#endif
}
late_initcall(kunit_init);
static void __exit kunit_exit(void)
{
memset(&kunit_hooks, 0, sizeof(kunit_hooks));
-#ifdef CONFIG_MODULES
unregister_module_notifier(&kunit_mod_nb);
-#endif
kunit_bus_shutdown();
--
2.50.0
^ permalink raw reply related
* [PATCH v2 2/3] module: make structure definitions always visible
From: Thomas Weißschuh @ 2025-07-11 13:31 UTC (permalink / raw)
To: Luis Chamberlain, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Brendan Higgins, David Gow, Rae Moar
Cc: linux-modules, linux-kernel, linux-kselftest, kunit-dev,
Thomas Weißschuh
In-Reply-To: <20250711-kunit-ifdef-modules-v2-0-39443decb1f8@linutronix.de>
To write code that works with both CONFIG_MODULES=y and CONFIG_MODULES=n
it is convenient to use "if (IS_ENABLED(CONFIG_MODULES))" over raw #ifdef.
The code will still fully typechecked but the unreachable parts are
discarded by the compiler. This prevents accidental breakage when a certain
kconfig combination was not specifically tested by the developer.
This pattern is already supported to some extend by module.h defining
empty stub functions if CONFIG_MODULES=n.
However some users of module.h work on the structured defined by module.h.
Therefore these structure definitions need to be visible, too.
Many structure members are still gated by specific configuration settings.
The assumption for those is that the code using them will be gated behind
the same configuration setting anyways.
Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
Reviewed-by: Daniel Gomez <da.gomez@samsung.com>
---
include/linux/module.h | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/include/linux/module.h b/include/linux/module.h
index 52f7b0487a2733c56e2531a434887e56e1bf45b2..2f330e60a7420a144abeed6d357ac93c39a705e9 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -302,17 +302,6 @@ static typeof(name) __mod_device_table__##type##__##name \
struct notifier_block;
-#ifdef CONFIG_MODULES
-
-extern int modules_disabled; /* for sysctl */
-/* Get/put a kernel symbol (calls must be symmetric) */
-void *__symbol_get(const char *symbol);
-void *__symbol_get_gpl(const char *symbol);
-#define symbol_get(x) ({ \
- static const char __notrim[] \
- __used __section(".no_trim_symbol") = __stringify(x); \
- (typeof(&x))(__symbol_get(__stringify(x))); })
-
enum module_state {
MODULE_STATE_LIVE, /* Normal state. */
MODULE_STATE_COMING, /* Full formed, running module_init. */
@@ -602,6 +591,17 @@ struct module {
#define MODULE_ARCH_INIT {}
#endif
+#ifdef CONFIG_MODULES
+
+extern int modules_disabled; /* for sysctl */
+/* Get/put a kernel symbol (calls must be symmetric) */
+void *__symbol_get(const char *symbol);
+void *__symbol_get_gpl(const char *symbol);
+#define symbol_get(x) ({ \
+ static const char __notrim[] \
+ __used __section(".no_trim_symbol") = __stringify(x); \
+ (typeof(&x))(__symbol_get(__stringify(x))); })
+
#ifndef HAVE_ARCH_KALLSYMS_SYMBOL_VALUE
static inline unsigned long kallsyms_symbol_value(const Elf_Sym *sym)
{
--
2.50.0
^ permalink raw reply related
* [PATCH v2 1/3] module: move 'struct module_use' to internal.h
From: Thomas Weißschuh @ 2025-07-11 13:31 UTC (permalink / raw)
To: Luis Chamberlain, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Brendan Higgins, David Gow, Rae Moar
Cc: linux-modules, linux-kernel, linux-kselftest, kunit-dev,
Thomas Weißschuh
In-Reply-To: <20250711-kunit-ifdef-modules-v2-0-39443decb1f8@linutronix.de>
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>
Reviewed-by: Petr Pavlu <petr.pavlu@suse.com>
---
include/linux/module.h | 7 -------
kernel/module/internal.h | 7 +++++++
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/include/linux/module.h b/include/linux/module.h
index 92e1420fccdffc9de9f49da9061546cc1e0c89d1..52f7b0487a2733c56e2531a434887e56e1bf45b2 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -313,13 +313,6 @@ void *__symbol_get_gpl(const char *symbol);
__used __section(".no_trim_symbol") = __stringify(x); \
(typeof(&x))(__symbol_get(__stringify(x))); })
-/* modules using other modules: kdb wants to see this. */
-struct module_use {
- struct list_head source_list;
- struct list_head target_list;
- struct module *source, *target;
-};
-
enum module_state {
MODULE_STATE_LIVE, /* Normal state. */
MODULE_STATE_COMING, /* Full formed, running module_init. */
diff --git a/kernel/module/internal.h b/kernel/module/internal.h
index 8d74b0a21c82b5360977a29736eca78ee6b6be3e..1c2e0b0dc52e72d5ecd2f1b310ce535364b3f33b 100644
--- a/kernel/module/internal.h
+++ b/kernel/module/internal.h
@@ -109,6 +109,13 @@ struct find_symbol_arg {
enum mod_license license;
};
+/* modules using other modules */
+struct module_use {
+ struct list_head source_list;
+ struct list_head target_list;
+ struct module *source, *target;
+};
+
int mod_verify_sig(const void *mod, struct load_info *info);
int try_to_force_load(struct module *mod, const char *reason);
bool find_symbol(struct find_symbol_arg *fsa);
--
2.50.0
^ permalink raw reply related
* [PATCH v2 0/3] module: make structure definitions always visible
From: Thomas Weißschuh @ 2025-07-11 13:31 UTC (permalink / raw)
To: Luis Chamberlain, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Brendan Higgins, David Gow, Rae Moar
Cc: linux-modules, linux-kernel, linux-kselftest, kunit-dev,
Thomas Weißschuh
Code using IS_ENABLED(CONFIG_MODULES) as a C expression may need access
to the module structure definitions to compile.
Make sure these structure definitions are always visible.
This will conflict with commit 6bb37af62634 ("module: Move modprobe_path
and modules_disabled ctl_tables into the module subsys") from the sysctl
tree, but the resolution is trivial.
Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
---
Changes in v2:
- Pick up tags from v1
- Keep MODULE_ARCH_INIT and 'struct module' definitions together
- Link to v1: https://lore.kernel.org/r/20250612-kunit-ifdef-modules-v1-0-fdccd42dcff8@linutronix.de
---
Thomas Weißschuh (3):
module: move 'struct module_use' to internal.h
module: make structure definitions always visible
kunit: test: Drop CONFIG_MODULE ifdeffery
include/linux/module.h | 29 +++++++++++------------------
kernel/module/internal.h | 7 +++++++
lib/kunit/test.c | 8 --------
3 files changed, 18 insertions(+), 26 deletions(-)
---
base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
change-id: 20250611-kunit-ifdef-modules-0fefd13ae153
Best regards,
--
Thomas Weißschuh <thomas.weissschuh@linutronix.de>
^ permalink raw reply
* Re: [PATCH 3/3] kunit: test: Drop CONFIG_MODULE ifdeffery
From: Daniel Gomez @ 2025-07-11 13:25 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-3-fdccd42dcff8@linutronix.de>
On 12/06/2025 16.53, 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>
Reviewed-by: Daniel Gomez <da.gomez@samsung.com>
^ permalink raw reply
* Re: [PATCH 2/3] module: make structure definitions always visible
From: Daniel Gomez @ 2025-07-11 13:24 UTC (permalink / raw)
To: Thomas Weißschuh
Cc: Luis Chamberlain, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Brendan Higgins, David Gow, Rae Moar, linux-modules, linux-kernel,
linux-kselftest, kunit-dev
In-Reply-To: <20250711081047-ea2c1e83-1b87-4331-acad-cbbfe6be67d8@linutronix.de>
On 11/07/2025 08.29, Thomas WeiÃschuh wrote:
> On Mon, Jul 07, 2025 at 09:11:05PM +0200, Daniel Gomez wrote:
>> On 12/06/2025 16.53, Thomas WeiÃschuh wrote:
>>> To write code that works with both CONFIG_MODULES=y and CONFIG_MODULES=n
>>> it is convenient to use "if (IS_ENABLED(CONFIG_MODULES))" over raw #ifdef.
>>> The code will still fully typechecked but the unreachable parts are
>>> discarded by the compiler. This prevents accidental breakage when a certain
>>> kconfig combination was not specifically tested by the developer.
>>> This pattern is already supported to some extend by module.h defining
>>> empty stub functions if CONFIG_MODULES=n.
>>> However some users of module.h work on the structured defined by module.h.
>>>
>>> Therefore these structure definitions need to be visible, too.
>>
>> We are missing here which structures are needed. + we are making more things
>> visible than what we actually need.
>>
>>>
>>> Many structure members are still gated by specific configuration settings.
>>> The assumption for those is that the code using them will be gated behind
>>> the same configuration setting anyways.
>>
>> I think code and kconfig need to reflect the actual dependencies. For example,
>> if CONFIG_LIVEPATCH depends on CONFIG_MODULES, we need to specify that in
>> Kconfig with depends on, as well as keep the code gated by these 2 configs with
>> ifdef/IS_ENABLED.
>
> If CONFIG_LIVEPATCH depends on CONFIG_MODULES in kconfig then
> IS_ENABLED(CONFIG_LIVEPATCH) will depend on CONFIG_MODULES automatically.
> There is no need for another explicit IS_ENABLED(CONFIG_MODULES).
This makes sense to me. My assessment before to reflect in code what we have in
kconfig does not scale. Thanks.
>
>>>
>>> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
>>> ---
>>> include/linux/module.h | 23 ++++++++++++-----------
>>> 1 file changed, 12 insertions(+), 11 deletions(-)
{...}
>>
>> After the patch, mod_tree_node is not needed externally.
>
> Can you explain what you mean with "not needed externally"?
> 'struct mod_tree_node' is only ever used by core module code.
> It is only public because it is embedded in the public 'struct module'
But only when MODULES_TREE_LOOKUP is enabled. Now, all kernels (regardless of
that config) will define mod_tree_node data structure.
However, Petr already stated that is harmless to do so. I was trying here to
not be useless.
With that, changes look good to me:
Reviewed-by: Daniel Gomez <da.gomez@samsung.com>
^ permalink raw reply
* [PATCH v17 4/7] rust: module: use a reference in macros::module::module
From: Andreas Hindborg @ 2025-07-11 6:35 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Benno Lossin, Benno Lossin, 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, Andreas Hindborg
In-Reply-To: <20250711-module-params-v3-v17-0-cf9b10d4923d@kernel.org>
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.
Reviewed-by: Benno Lossin <lossin@kernel.org>
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
rust/macros/module.rs | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/rust/macros/module.rs b/rust/macros/module.rs
index 2ddd2eeb2852..1a867a1e787e 100644
--- a/rust/macros/module.rs
+++ b/rust/macros/module.rs
@@ -179,26 +179,26 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
// Rust does not allow hyphens in identifiers, use underscore instead.
let ident = info.name.replace('-', "_");
let mut modinfo = ModInfoBuilder::new(ident.as_ref());
- if let Some(author) = info.author {
- modinfo.emit("author", &author);
+ if let Some(author) = &info.author {
+ modinfo.emit("author", author);
}
- if let Some(authors) = info.authors {
+ if let Some(authors) = &info.authors {
for author in authors {
- modinfo.emit("author", &author);
+ modinfo.emit("author", author);
}
}
- if let Some(description) = info.description {
- modinfo.emit("description", &description);
+ if let Some(description) = &info.description {
+ modinfo.emit("description", description);
}
modinfo.emit("license", &info.license);
- if let Some(aliases) = info.alias {
+ if let Some(aliases) = &info.alias {
for alias in aliases {
- modinfo.emit("alias", &alias);
+ modinfo.emit("alias", alias);
}
}
- if let Some(firmware) = info.firmware {
+ if let Some(firmware) = &info.firmware {
for fw in firmware {
- modinfo.emit("firmware", &fw);
+ modinfo.emit("firmware", fw);
}
}
--
2.47.2
^ permalink raw reply related
* [PATCH v17 1/7] rust: sync: add `SetOnce`
From: Andreas Hindborg @ 2025-07-11 6:35 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Benno Lossin, Benno Lossin, 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, Andreas Hindborg
In-Reply-To: <20250711-module-params-v3-v17-0-cf9b10d4923d@kernel.org>
Introduce the `SetOnce` type, a container that can only be written once.
The container uses an internal atomic to synchronize writes to the internal
value.
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Benno Lossin <lossin@kernel.org>
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
rust/kernel/sync.rs | 2 +
rust/kernel/sync/set_once.rs | 125 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 127 insertions(+)
diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
index 81e3a806e57e..13e6bc7fa87a 100644
--- a/rust/kernel/sync.rs
+++ b/rust/kernel/sync.rs
@@ -18,6 +18,7 @@
mod locked_by;
pub mod poll;
pub mod rcu;
+mod set_once;
pub use arc::{Arc, ArcBorrow, UniqueArc};
pub use completion::Completion;
@@ -26,6 +27,7 @@
pub use lock::mutex::{new_mutex, Mutex, MutexGuard};
pub use lock::spinlock::{new_spinlock, SpinLock, SpinLockGuard};
pub use locked_by::LockedBy;
+pub use set_once::SetOnce;
/// Represents a lockdep class. It's a wrapper around C's `lock_class_key`.
#[repr(transparent)]
diff --git a/rust/kernel/sync/set_once.rs b/rust/kernel/sync/set_once.rs
new file mode 100644
index 000000000000..bdba601807d8
--- /dev/null
+++ b/rust/kernel/sync/set_once.rs
@@ -0,0 +1,125 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! A container that can be initialized at most once.
+
+use super::atomic::{
+ ordering::{Acquire, Relaxed, Release},
+ Atomic,
+};
+use core::{cell::UnsafeCell, mem::MaybeUninit};
+
+/// A container that can be populated at most once. Thread safe.
+///
+/// Once the a [`SetOnce`] is populated, it remains populated by the same object for the
+/// lifetime `Self`.
+///
+/// # Invariants
+///
+/// - `init` may only increase in value.
+/// - `init` may only assume values in the range `0..=2`.
+/// - `init == 0` if and only if `value` is uninitialized.
+/// - `init == 1` if and only if there is exactly one thread with exclusive
+/// access to `self.value`.
+/// - `init == 2` if and only if `value` is initialized and valid for shared
+/// access.
+///
+/// # Example
+///
+/// ```
+/// # use kernel::sync::SetOnce;
+/// let value = SetOnce::new();
+/// assert_eq!(None, value.as_ref());
+///
+/// let status = value.populate(42u8);
+/// assert_eq!(true, status);
+/// assert_eq!(Some(&42u8), value.as_ref());
+/// assert_eq!(Some(42u8), value.copy());
+///
+/// let status = value.populate(101u8);
+/// assert_eq!(false, status);
+/// assert_eq!(Some(&42u8), value.as_ref());
+/// assert_eq!(Some(42u8), value.copy());
+/// ```
+pub struct SetOnce<T> {
+ init: Atomic<u32>,
+ value: UnsafeCell<MaybeUninit<T>>,
+}
+
+impl<T> Default for SetOnce<T> {
+ fn default() -> Self {
+ Self::new()
+ }
+}
+
+impl<T> SetOnce<T> {
+ /// Create a new [`SetOnce`].
+ ///
+ /// The returned instance will be empty.
+ pub const fn new() -> Self {
+ // INVARIANT: The container is empty and we initialize `init` to `0`.
+ Self {
+ value: UnsafeCell::new(MaybeUninit::uninit()),
+ init: Atomic::new(0),
+ }
+ }
+
+ /// Get a reference to the contained object.
+ ///
+ /// Returns [`None`] if this [`SetOnce`] is empty.
+ pub fn as_ref(&self) -> Option<&T> {
+ if self.init.load(Acquire) == 2 {
+ // SAFETY: By the type invariants of `Self`, `self.init == 2` means that `self.value`
+ // is initialized and valid for shared access.
+ Some(unsafe { &*self.value.get().cast() })
+ } else {
+ None
+ }
+ }
+
+ /// Populate the [`SetOnce`].
+ ///
+ /// Returns `true` if the [`SetOnce`] was successfully populated.
+ pub fn populate(&self, value: T) -> bool {
+ // INVARIANT: If the swap succeeds:
+ // - We increase `init`.
+ // - We write the valid value `1` to `init`.
+ // - Only one thread can succeed in this write, so we have exclusive access after the
+ // write.
+ if let Ok(0) = self.init.cmpxchg(0, 1, Relaxed) {
+ // SAFETY: By the type invariants of `Self`, the fact that we succeeded in writing `1`
+ // to `self.init` means we obtained exclusive access to `self.value`.
+ unsafe { core::ptr::write(self.value.get().cast(), value) };
+ // INVARIANT:
+ // - We increase `init`.
+ // - We write the valid value `2` to `init`.
+ // - We release our exclusive access to `self.value` and it is now valid for shared
+ // access.
+ self.init.store(2, Release);
+ true
+ } else {
+ false
+ }
+ }
+
+ /// Get a copy of the contained object.
+ ///
+ /// Returns [`None`] if the [`SetOnce`] is empty.
+ pub fn copy(&self) -> Option<T>
+ where
+ T: Copy,
+ {
+ self.as_ref().copied()
+ }
+}
+
+impl<T> Drop for SetOnce<T> {
+ fn drop(&mut self) {
+ if *self.init.get_mut() == 2 {
+ let value = self.value.get_mut();
+ // SAFETY: By the type invariants of `Self`, `self.init == 2` means that `self.value`
+ // contains a valid value. We have exclusive access, as we hold a `mut` reference to
+ // `self`.
+ unsafe { value.assume_init_drop() };
+ }
+ }
+}
--
2.47.2
^ permalink raw reply related
* [PATCH v17 3/7] rust: introduce module_param module
From: Andreas Hindborg @ 2025-07-11 6:35 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Benno Lossin, Benno Lossin, 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, Andreas Hindborg
In-Reply-To: <20250711-module-params-v3-v17-0-cf9b10d4923d@kernel.org>
Add types and traits for interfacing the C moduleparam API.
Reviewed-by: Benno Lossin <lossin@kernel.org>
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
rust/kernel/lib.rs | 1 +
rust/kernel/module_param.rs | 181 ++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 182 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..9b187ed1d351
--- /dev/null
+++ b/rust/kernel/module_param.rs
@@ -0,0 +1,181 @@
+// 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;
+use bindings;
+use kernel::sync::SetOnce;
+
+/// Newtype to make `bindings::kernel_param` [`Sync`].
+#[repr(transparent)]
+#[doc(hidden)]
+pub struct KernelParam(bindings::kernel_param);
+
+impl KernelParam {
+ #[doc(hidden)]
+ pub const fn new(val: bindings::kernel_param) -> Self {
+ Self(val)
+ }
+}
+
+// SAFETY: C kernel handles serializing access to this type. We never access it
+// from Rust module.
+unsafe impl Sync for KernelParam {}
+
+/// Types that can be used for module parameters.
+// NOTE: This trait is `Copy` because drop could produce unsoundness during teardown.
+pub trait ModuleParam: Sized + Copy {
+ /// Parse a parameter argument into the parameter value.
+ fn try_from_param_arg(arg: &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.
+/// - `param` must be a pointer to a `bindings::kernel_param` initialized by the rust module macro.
+/// The pointee must be valid for reads for the duration of the call.
+///
+/// # 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 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) };
+
+ crate::error::from_result(|| {
+ let new_value = T::try_from_param_arg(arg)?;
+
+ // SAFETY: By function safety requirements, this access is safe.
+ let container = unsafe { &*((*param).__bindgen_anon_1.arg as *mut SetOnce<T>) };
+
+ container
+ .populate(new_value)
+ .then_some(0)
+ .ok_or(kernel::error::code::EEXIST)
+ })
+}
+
+macro_rules! impl_int_module_param {
+ ($ty:ident) => {
+ impl ModuleParam for $ty {
+ fn try_from_param_arg(arg: &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.
+pub struct ModuleParamAccess<T> {
+ value: SetOnce<T>,
+ default: 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(default: T) -> Self {
+ Self {
+ value: SetOnce::new(),
+ default,
+ }
+ }
+
+ /// 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 value(&self) -> &T {
+ self.value.as_ref().unwrap_or(&self.default)
+ }
+
+ /// Get a mutable pointer to `self`.
+ ///
+ /// NOTE: In most cases it is not safe deref the returned pointer.
+ pub const fn as_void_ptr(&self) -> *mut c_void {
+ core::ptr::from_ref(self).cast_mut().cast()
+ }
+}
+
+#[doc(hidden)]
+/// 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);
--
2.47.2
^ permalink raw reply related
* [PATCH v17 0/7] rust: extend `module!` macro with integer parameter support
From: Andreas Hindborg @ 2025-07-11 6:35 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Benno Lossin, Benno Lossin, 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, Andreas Hindborg
Extend the `module!` macro with support module parameters. Also add some
string to integer parsing functions.
Based on the original module parameter support by Miguel [1],
later extended and generalized by Adam for more types [2][3].
Originally tracked at [4].
Link: https://github.com/Rust-for-Linux/linux/pull/7 [1]
Link: https://github.com/Rust-for-Linux/linux/pull/82 [2]
Link: https://github.com/Rust-for-Linux/linux/pull/87 [3]
Link: https://github.com/Rust-for-Linux/linux/issues/11 [4]
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
Changes in v17:
- Fix drop impl of `SetOnce` so that it works with `UnsafeCell<MaybeUninit<_>>`.
- Slightly reword safety framework in `SetOnce`.
- Rebase on atomic series v6.
- Link to v16: https://lore.kernel.org/r/20250709-module-params-v3-v16-0-4f926bcccb50@kernel.org
Changes in v16:
- Normalize imports in `set_once.rs`.
- Use `UnsafeCell<MaybeUninit<T>>` rather than `Opaque<T>` for `SetOnce`.
- Use regular load in drop of `SetOnce`.
- Update attribution paragraph in cover letter with details from Miguel.
- Remove stray TODO in `set_once.rs`
- Link to v15: https://lore.kernel.org/r/20250707-module-params-v3-v15-0-c1f4269a57b9@kernel.org
Changes in v15:
- Rebase on v6.16-rc5.
- Dedent code in module macro for better formatting.
- Rename `OnceLock` to `SetOnce`.
- Use "being initialized" rather than "being mutably accessed" when
describing initialization state of `OnceLock`.
- Use `Relaxed` ordering when transitioning to exclusive access in
`OnceLock`.
- Add drop implementation for `OnceLock`.
- Re-export `OnceLock` from `kernel::sync` module.
- Improve indentation of in macro code. Prefix `cfg` to `::core::cfg` in
macro code.
- Use `core::ptr::from_ref` rather than `as` casts.
- Hide `KernelParam` instances behind `const _: ()` blocks.
- Rename `ModuleParamAccess::get` to `ModuleParamAccess::value`.
- Rename `RacyKernelParam` to `KernelParam`.
- Remove `ModuleParam::Value`.
- Move `copy` implementation of `OnceLock`.
- Update safety comments and invariants of `OnceLock`.
- Link to v14: https://lore.kernel.org/r/20250702-module-params-v3-v14-0-5b1cc32311af@kernel.org
Note: This series now depends on the atomics series [1].
[1] https://lore.kernel.org/all/20250618164934.19817-1-boqun.feng@gmail.com
Changes in v14:
- Remove unnecessary `crate::` prefix from `module_param::set_param`.
- Make `FromStrRadix` safe again by moving unsafe blocks to macro implementation (thanks Benno).
- Use `core::ptr::write` in `set_param` and drop safety requirement regarding initialization.
- Add a TODO to use `SyncUnsafeCell` for `ModuleParamAccess` when available.
- Add a NOTE regarding `Copy` bound on `ModuleParam`.
- Remove `'static` lifetime qualifier from `ModuleParam::try_from_param_arg` argument.
- Fix a typo in the safety requirements for `set_param`.
- Remove unused `#[macro_export]` attribute.
- Remove obsolete documentation for `ModuleParam::try_from_param_arg`.
- Make `RacyKernelParam` tuple field private.
- Introduce `OnceLock` and use that to synchronize population of parameter values.
- Link to v13: https://lore.kernel.org/r/20250612-module-params-v3-v13-0-bc219cd1a3f8@kernel.org
Changes in v13:
- remove absolute path for `ffi` types.
- Split patch 2 into 4 separate patches.
- Overhaul safety framework for `set_param`.
- Remove generated docs for `kernel_param_ops`.
- Move `parse_int` to separate file.
- Rebase on v6.16-rc1
- Link to v12: https://lore.kernel.org/r/20250506-module-params-v3-v12-0-c04d80c8a2b1@kernel.org
Changes in v12:
- Assign through pointer rather than using `core::ptr::replace`.
- Prevent a potential use-after-free during module teardown.
- Link to v11: https://lore.kernel.org/r/20250502-module-params-v3-v11-0-6096875a2b78@kernel.org
Changes in v11:
- Apply a few nits from Miguel.
- Link to v10: https://lore.kernel.org/r/20250501-module-params-v3-v10-0-4da485d343d5@kernel.org
Changes in v10:
- Apply fixups from Miguel:
- Add integer type suffixes to `assert!` in tests.
- Fix links to docs.kernel.org.
- Applyy markdown and intra-doc links where possible.
- Change to `///` for `mod` docs.
- Slightly reword a comment.
- Pluralize "Examples" section name.
- Hide `use`s in example.
- Removed `#[expect]` for the `rusttest` target.
- Link to v9: https://lore.kernel.org/r/20250321-module-params-v3-v9-0-28b905f2e345@kernel.org
Changes in v9:
- Remove UB when parsing the minimum integer values.
- Make `FromStr` trait unsafe, since wrong implementations can cause UB.
- Drop patches that were applied to rust-next.
- Link to v8: https://lore.kernel.org/r/20250227-module-params-v3-v8-0-ceeee85d9347@kernel.org
Changes in v8:
- Change print statement in sample to better communicate parameter name.
- Use imperative mode in commit messages.
- Remove prefix path from `EINVAL`.
- Change `try_from_param_arg` to accept `&BStr` rather than `&[u8]`.
- Parse integers without 128 bit integer types.
- Seal trait `FromStrRadix`.
- Strengthen safety requirement of `set_param`.
- Remove comment about Display and `PAGE_SIZE`.
- Add note describing why `ModuleParamAccess` is pub.
- Typo and grammar fixes for documentation.
- Update MAINTAINERS with rust module files.
- Link to v7: https://lore.kernel.org/r/20250218-module-params-v3-v7-0-5e1afabcac1b@kernel.org
Changes in v7:
- Remove dependency on `pr_warn_once` patches, replace with TODO.
- Rework `ParseInt::from_str` to avoid allocating.
- Add a comment explaining how we parse "0".
- Change trait bound on `Index` impl for `BStr` to match std library approach.
- Link to v6: https://lore.kernel.org/r/20250211-module-params-v3-v6-0-24b297ddc43d@kernel.org
Changes in v6:
- Fix a bug that prevented parsing of negative default values for
parameters in the `module!` macro.
- Fix a bug that prevented parsing zero in `strip_radix`. Also add a
test case for this.
- Add `AsRef<BStr>` for `[u8]` and `BStr`.
- Use `impl AsRef<BStr>` as type of prefix in `BStr::strip_prefix`.
- Link to v5: https://lore.kernel.org/r/20250204-module-params-v3-v5-0-bf5ec2041625@kernel.org
Changes in v5:
- Fix a typo in a safety comment in `set_param`.
- Use a match statement in `parse_int::strip_radix`.
- Add an implementation of `Index` for `BStr`.
- Fix a logic inversion bug where parameters would not be parsed.
- Use `kernel::ffi::c_char` in `set_param` rather than the one in `core`.
- Use `kernel::c_str!` rather than `c"..."` literal in module macro.
- Rebase on v6.14-rc1.
- Link to v4: https://lore.kernel.org/r/20250109-module-params-v3-v4-0-c208bcfbe11f@kernel.org
Changes in v4:
- Add module maintainers to Cc list (sorry)
- Add a few missing [`doc_links`]
- Add panic section to `expect_string_field`
- Fix a typo in safety requirement of `module_params::free`
- Change `assert!` to `pr_warn_once!` in `module_params::set_param`
- Remove `module_params::get_param` and install null pointer instead
- Remove use of the unstable feature `sync_unsafe_cell`
- Link to v3: https://lore.kernel.org/r/20241213-module-params-v3-v3-0-485a015ac2cf@kernel.org
Changes in v3:
- use `SyncUnsafeCell` rather than `static mut` and simplify parameter access
- remove `Display` bound from `ModuleParam`
- automatically generate documentation for `PARAM_OPS_.*`
- remove `as *const _ as *mut_` phrasing
- inline parameter name in struct instantiation in `emit_params`
- move `RacyKernelParam` out of macro template
- use C string literals rather than byte string literals with explicit null
- template out `__{name}_{param_name}` in `emit_param`
- indent template in `emit_params`
- use let-else expression in `emit_params` to get rid of an indentation level
- document `expect_string_field`
- move invication of `impl_int_module_param` to be closer to macro def
- move attributes after docs in `make_param_ops`
- rename `impl_module_param` to impl_int_module_param`
- use `ty` instead of `ident` in `impl_parse_int`
- use `BStr` instead of `&str` for string manipulation
- move string parsing functions to seperate patch and add examples, fix bugs
- degrade comment about future support from doc comment to regular comment
- remove std lib path from `Sized` marker
- update documentation for `trait ModuleParam`
- Link to v2: https://lore.kernel.org/all/20240819133345.3438739-1-nmi@metaspace.dk/
Changes in v2:
- Remove support for params without values (`NOARG_ALLOWED`).
- Improve documentation for `try_from_param_arg`.
- Use prelude import.
- Refactor `try_from_param_arg` to return `Result`.
- Refactor `ParseInt::from_str` to return `Result`.
- Move C callable functions out of `ModuleParam` trait.
- Rename literal string field parser to `expect_string_field`.
- Move parameter parsing from generation to parsing stage.
- Use absolute type paths in macro code.
- Inline `kparam`and `read_func` values.
- Resolve TODO regarding alignment attributes.
- Remove unnecessary unsafe blocks in macro code.
- Improve error message for unrecognized parameter types.
- Do not use `self` receiver when reading parameter value.
- Add parameter documentation to `module!` macro.
- Use empty `enum` for parameter type.
- Use `addr_of_mut` to get address of parameter value variable.
- Enabled building of docs for for `module_param` module.
- Link to v1: https://lore.kernel.org/rust-for-linux/20240705111455.142790-1-nmi@metaspace.dk/
---
Andreas Hindborg (7):
rust: sync: add `SetOnce`
rust: str: add radix prefixed integer parsing functions
rust: introduce module_param module
rust: module: use a reference in macros::module::module
rust: module: update the module macro with module parameter support
rust: samples: add a module parameter to the rust_minimal sample
modules: add rust modules files to MAINTAINERS
MAINTAINERS | 2 +
rust/kernel/lib.rs | 1 +
rust/kernel/module_param.rs | 181 +++++++++++++++++++++++++++++++++++++++
rust/kernel/str.rs | 2 +
rust/kernel/str/parse_int.rs | 148 ++++++++++++++++++++++++++++++++
rust/kernel/sync.rs | 2 +
rust/kernel/sync/set_once.rs | 125 +++++++++++++++++++++++++++
rust/macros/helpers.rs | 25 ++++++
rust/macros/lib.rs | 31 +++++++
rust/macros/module.rs | 198 ++++++++++++++++++++++++++++++++++++++-----
samples/rust/rust_minimal.rs | 10 +++
11 files changed, 705 insertions(+), 20 deletions(-)
---
base-commit: 47633099a672fc7bfe604ef454e4f116e2c954b1
change-id: 20241211-module-params-v3-ae7e5c8d8b5a
prerequisite-message-id: <20250710060052.11955-1-boqun.feng@gmail.com>
prerequisite-patch-id: 28896267479b4eb64b31c7f0369398c3d45381d5
prerequisite-patch-id: d2f889e59033a98b2939ec8b1f73b82026eeca57
prerequisite-patch-id: 820b700bc77f6ab3b7ab41056675f8e8f27ca924
prerequisite-patch-id: 8050edc4035cf1b57251d8de496b890b87918cb9
prerequisite-patch-id: ce22d95bad2cf5b701db6d62ec3dd8e5f47d8af0
prerequisite-patch-id: 5755c226b2678a53a8cfd7d09f90743c1d6f6ae4
prerequisite-patch-id: 766b6439b20cb954b3434830603cf88626a35310
prerequisite-patch-id: 98fffa4e658617923ab1c360dc40bacb9d741e3c
prerequisite-patch-id: b393394acd88dae6dfe76ac10af2e9469a08fdc4
Best regards,
--
Andreas Hindborg <a.hindborg@kernel.org>
^ permalink raw reply
* [PATCH v17 7/7] modules: add rust modules files to MAINTAINERS
From: Andreas Hindborg @ 2025-07-11 6:35 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Benno Lossin, Benno Lossin, 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, Andreas Hindborg
In-Reply-To: <20250711-module-params-v3-v17-0-cf9b10d4923d@kernel.org>
The module subsystem people agreed to maintain rust support for modules
[1]. Thus, add entries for relevant files to modules entry in MAINTAINERS.
Link: https://lore.kernel.org/all/0d9e596a-5316-4e00-862b-fd77552ae4b5@suse.com/ [1]
Acked-by: Daniel Gomez <da.gomez@samsung.com>
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
MAINTAINERS | 2 ++
1 file changed, 2 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index d431320ed3b2..afa385ecc5c4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16835,6 +16835,8 @@ F: include/linux/module*.h
F: kernel/module/
F: lib/test_kmod.c
F: lib/tests/module/
+F: rust/kernel/module_param.rs
+F: rust/macros/module.rs
F: scripts/module*
F: tools/testing/selftests/kmod/
F: tools/testing/selftests/module/
--
2.47.2
^ permalink raw reply related
* [PATCH v17 6/7] rust: samples: add a module parameter to the rust_minimal sample
From: Andreas Hindborg @ 2025-07-11 6:35 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Benno Lossin, Benno Lossin, 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, Andreas Hindborg
In-Reply-To: <20250711-module-params-v3-v17-0-cf9b10d4923d@kernel.org>
Showcase the rust module parameter support by adding a module parameter to
the `rust_minimal` sample.
Reviewed-by: Benno Lossin <lossin@kernel.org>
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
samples/rust/rust_minimal.rs | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/samples/rust/rust_minimal.rs b/samples/rust/rust_minimal.rs
index 1fc7a1be6b6d..8eb9583571d7 100644
--- a/samples/rust/rust_minimal.rs
+++ b/samples/rust/rust_minimal.rs
@@ -10,6 +10,12 @@
authors: ["Rust for Linux Contributors"],
description: "Rust minimal sample",
license: "GPL",
+ params: {
+ test_parameter: i64 {
+ default: 1,
+ description: "This parameter has a default of 1",
+ },
+ },
}
struct RustMinimal {
@@ -20,6 +26,10 @@ impl kernel::Module for RustMinimal {
fn init(_module: &'static ThisModule) -> Result<Self> {
pr_info!("Rust minimal sample (init)\n");
pr_info!("Am I built-in? {}\n", !cfg!(MODULE));
+ pr_info!(
+ "test_parameter: {}\n",
+ *module_parameters::test_parameter.value()
+ );
let mut numbers = KVec::new();
numbers.push(72, GFP_KERNEL)?;
--
2.47.2
^ permalink raw reply related
* [PATCH v17 2/7] rust: str: add radix prefixed integer parsing functions
From: Andreas Hindborg @ 2025-07-11 6:35 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Benno Lossin, Benno Lossin, 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, Andreas Hindborg
In-Reply-To: <20250711-module-params-v3-v17-0-cf9b10d4923d@kernel.org>
Add the trait `ParseInt` for parsing string representations of integers
where the string representations are optionally prefixed by a radix
specifier. Implement the trait for the primitive integer types.
Suggested-by: Benno Lossin <benno.lossin@proton.me>
Tested-by: Daniel Gomez <da.gomez@samsung.com>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Benno Lossin <lossin@kernel.org>
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
rust/kernel/str.rs | 2 +
rust/kernel/str/parse_int.rs | 148 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 150 insertions(+)
diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
index a927db8e079c..2b6c8b4a0ae4 100644
--- a/rust/kernel/str.rs
+++ b/rust/kernel/str.rs
@@ -8,6 +8,8 @@
use crate::prelude::*;
+pub mod parse_int;
+
/// Byte string without UTF-8 validity guarantee.
#[repr(transparent)]
pub struct BStr([u8]);
diff --git a/rust/kernel/str/parse_int.rs b/rust/kernel/str/parse_int.rs
new file mode 100644
index 000000000000..48eb4c202984
--- /dev/null
+++ b/rust/kernel/str/parse_int.rs
@@ -0,0 +1,148 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Integer parsing functions.
+//!
+//! Integer parsing functions for parsing signed and unsigned integers
+//! potentially prefixed with `0x`, `0o`, or `0b`.
+
+use crate::prelude::*;
+use crate::str::BStr;
+use core::ops::Deref;
+
+// Make `FromStrRadix` a public type with a private name. This seals
+// `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.
+ pub trait FromStrRadix: Sized {
+ /// Parse `src` to [`Self`] using radix `radix`.
+ fn from_str_radix(src: &BStr, radix: u32) -> Result<Self>;
+
+ /// Tries to convert `value` into [`Self`] and negates the resulting value.
+ fn from_u64_negated(value: u64) -> Result<Self>;
+ }
+}
+
+/// Extract the radix from an integer literal optionally prefixed with
+/// one of `0x`, `0X`, `0o`, `0O`, `0b`, `0B`, `0`.
+fn strip_radix(src: &BStr) -> (u32, &BStr) {
+ match src.deref() {
+ [b'0', b'x' | b'X', rest @ ..] => (16, rest.as_ref()),
+ [b'0', b'o' | b'O', rest @ ..] => (8, rest.as_ref()),
+ [b'0', b'b' | b'B', rest @ ..] => (2, rest.as_ref()),
+ // NOTE: We are including the leading zero to be able to parse
+ // literal `0` here. If we removed it as a radix prefix, we would
+ // not be able to parse `0`.
+ [b'0', ..] => (8, src),
+ _ => (10, src),
+ }
+}
+
+/// Trait for parsing string representations of integers.
+///
+/// Strings beginning with `0x`, `0o`, or `0b` are parsed as hex, octal, or
+/// binary respectively. Strings beginning with `0` otherwise are parsed as
+/// octal. Anything else is parsed as decimal. A leading `+` or `-` is also
+/// permitted. Any string parsed by [`kstrtol()`] or [`kstrtoul()`] will be
+/// successfully parsed.
+///
+/// [`kstrtol()`]: https://docs.kernel.org/core-api/kernel-api.html#c.kstrtol
+/// [`kstrtoul()`]: https://docs.kernel.org/core-api/kernel-api.html#c.kstrtoul
+///
+/// # Examples
+///
+/// ```
+/// # use kernel::str::parse_int::ParseInt;
+/// # use kernel::b_str;
+///
+/// assert_eq!(Ok(0u8), u8::from_str(b_str!("0")));
+///
+/// assert_eq!(Ok(0xa2u8), u8::from_str(b_str!("0xa2")));
+/// assert_eq!(Ok(-0xa2i32), i32::from_str(b_str!("-0xa2")));
+///
+/// assert_eq!(Ok(-0o57i8), i8::from_str(b_str!("-0o57")));
+/// assert_eq!(Ok(0o57i8), i8::from_str(b_str!("057")));
+///
+/// assert_eq!(Ok(0b1001i16), i16::from_str(b_str!("0b1001")));
+/// assert_eq!(Ok(-0b1001i16), i16::from_str(b_str!("-0b1001")));
+///
+/// assert_eq!(Ok(127i8), i8::from_str(b_str!("127")));
+/// assert!(i8::from_str(b_str!("128")).is_err());
+/// assert_eq!(Ok(-128i8), i8::from_str(b_str!("-128")));
+/// assert!(i8::from_str(b_str!("-129")).is_err());
+/// assert_eq!(Ok(255u8), u8::from_str(b_str!("255")));
+/// assert!(u8::from_str(b_str!("256")).is_err());
+/// ```
+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)?;
+ Self::from_u64_negated(val)
+ }
+ _ => {
+ let (radix, digits) = strip_radix(src);
+ Self::from_str_radix(digits, radix).map_err(|_| EINVAL)
+ }
+ }
+ }
+}
+
+macro_rules! impl_parse_int {
+ ($($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 from_u64_negated(value: u64) -> Result<Self> {
+ const ABS_MIN: u64 = {
+ #[allow(unused_comparisons)]
+ if <$ty>::MIN < 0 {
+ 1u64 << (<$ty>::BITS - 1)
+ } else {
+ 0
+ }
+ };
+
+ if value > ABS_MIN {
+ return Err(EINVAL);
+ }
+
+ if value == ABS_MIN {
+ return Ok(<$ty>::MIN);
+ }
+
+ // SAFETY: The above checks guarantee that `value` 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 <= value < ABS_MIN`. And since
+ // `ABS_MIN - 1` fits into `Self` by construction, `value` also does.
+ let value: Self = unsafe { value.try_into().unwrap_unchecked() };
+
+ Ok((!value).wrapping_add(1))
+ }
+ }
+
+ impl ParseInt for $ty {}
+ )*
+ };
+}
+
+impl_parse_int![i8, u8, i16, u16, i32, u32, i64, u64, isize, usize];
--
2.47.2
^ permalink raw reply related
* [PATCH v17 5/7] rust: module: update the module macro with module parameter support
From: Andreas Hindborg @ 2025-07-11 6:35 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Benno Lossin, Benno Lossin, 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, Andreas Hindborg
In-Reply-To: <20250711-module-params-v3-v17-0-cf9b10d4923d@kernel.org>
Allow module parameters to be declared in the rust `module!` macro.
Reviewed-by: Benno Lossin <lossin@kernel.org>
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
rust/macros/helpers.rs | 25 +++++++
rust/macros/lib.rs | 31 +++++++++
rust/macros/module.rs | 178 ++++++++++++++++++++++++++++++++++++++++++++++---
3 files changed, 224 insertions(+), 10 deletions(-)
diff --git a/rust/macros/helpers.rs b/rust/macros/helpers.rs
index e2602be402c1..365d7eb499c0 100644
--- a/rust/macros/helpers.rs
+++ b/rust/macros/helpers.rs
@@ -10,6 +10,17 @@ pub(crate) fn try_ident(it: &mut token_stream::IntoIter) -> Option<String> {
}
}
+pub(crate) fn try_sign(it: &mut token_stream::IntoIter) -> Option<char> {
+ let peek = it.clone().next();
+ match peek {
+ Some(TokenTree::Punct(punct)) if punct.as_char() == '-' => {
+ let _ = it.next();
+ Some(punct.as_char())
+ }
+ _ => None,
+ }
+}
+
pub(crate) fn try_literal(it: &mut token_stream::IntoIter) -> Option<String> {
if let Some(TokenTree::Literal(literal)) = it.next() {
Some(literal.to_string())
@@ -103,3 +114,17 @@ pub(crate) fn file() -> String {
proc_macro::Span::call_site().file()
}
}
+
+/// Parse a token stream of the form `expected_name: "value",` and return the
+/// string in the position of "value".
+///
+/// # Panics
+///
+/// - On parse error.
+pub(crate) fn expect_string_field(it: &mut token_stream::IntoIter, expected_name: &str) -> String {
+ assert_eq!(expect_ident(it), expected_name);
+ assert_eq!(expect_punct(it), ':');
+ let string = expect_string(it);
+ assert_eq!(expect_punct(it), ',');
+ string
+}
diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs
index fa847cf3a9b5..2fb520dc930a 100644
--- a/rust/macros/lib.rs
+++ b/rust/macros/lib.rs
@@ -28,6 +28,30 @@
/// The `type` argument should be a type which implements the [`Module`]
/// trait. Also accepts various forms of kernel metadata.
///
+/// The `params` field describe module parameters. Each entry has the form
+///
+/// ```ignore
+/// parameter_name: type {
+/// default: default_value,
+/// description: "Description",
+/// }
+/// ```
+///
+/// `type` may be one of
+///
+/// - [`i8`]
+/// - [`u8`]
+/// - [`i8`]
+/// - [`u8`]
+/// - [`i16`]
+/// - [`u16`]
+/// - [`i32`]
+/// - [`u32`]
+/// - [`i64`]
+/// - [`u64`]
+/// - [`isize`]
+/// - [`usize`]
+///
/// C header: [`include/linux/moduleparam.h`](srctree/include/linux/moduleparam.h)
///
/// [`Module`]: ../kernel/trait.Module.html
@@ -44,6 +68,12 @@
/// description: "My very own kernel module!",
/// license: "GPL",
/// alias: ["alternate_module_name"],
+/// params: {
+/// my_parameter: i64 {
+/// default: 1,
+/// description: "This parameter has a default of 1",
+/// },
+/// },
/// }
///
/// struct MyModule(i32);
@@ -52,6 +82,7 @@
/// fn init(_module: &'static ThisModule) -> Result<Self> {
/// let foo: i32 = 42;
/// pr_info!("I contain: {}\n", foo);
+/// pr_info!("i32 param is: {}\n", module_parameters::my_parameter.read());
/// Ok(Self(foo))
/// }
/// }
diff --git a/rust/macros/module.rs b/rust/macros/module.rs
index 1a867a1e787e..c1400597774a 100644
--- a/rust/macros/module.rs
+++ b/rust/macros/module.rs
@@ -26,6 +26,7 @@ struct ModInfoBuilder<'a> {
module: &'a str,
counter: usize,
buffer: String,
+ param_buffer: String,
}
impl<'a> ModInfoBuilder<'a> {
@@ -34,10 +35,11 @@ fn new(module: &'a str) -> Self {
module,
counter: 0,
buffer: String::new(),
+ param_buffer: String::new(),
}
}
- fn emit_base(&mut self, field: &str, content: &str, builtin: bool) {
+ fn emit_base(&mut self, field: &str, content: &str, builtin: bool, param: bool) {
let string = if builtin {
// Built-in modules prefix their modinfo strings by `module.`.
format!(
@@ -51,8 +53,14 @@ fn emit_base(&mut self, field: &str, content: &str, builtin: bool) {
format!("{field}={content}\0")
};
+ let buffer = if param {
+ &mut self.param_buffer
+ } else {
+ &mut self.buffer
+ };
+
write!(
- &mut self.buffer,
+ buffer,
"
{cfg}
#[doc(hidden)]
@@ -75,20 +83,119 @@ fn emit_base(&mut self, field: &str, content: &str, builtin: bool) {
self.counter += 1;
}
- fn emit_only_builtin(&mut self, field: &str, content: &str) {
- self.emit_base(field, content, true)
+ fn emit_only_builtin(&mut self, field: &str, content: &str, param: bool) {
+ self.emit_base(field, content, true, param)
}
- fn emit_only_loadable(&mut self, field: &str, content: &str) {
- self.emit_base(field, content, false)
+ fn emit_only_loadable(&mut self, field: &str, content: &str, param: bool) {
+ self.emit_base(field, content, false, param)
}
fn emit(&mut self, field: &str, content: &str) {
- self.emit_only_builtin(field, content);
- self.emit_only_loadable(field, content);
+ self.emit_internal(field, content, false);
+ }
+
+ fn emit_internal(&mut self, field: &str, content: &str, param: bool) {
+ self.emit_only_builtin(field, content, param);
+ self.emit_only_loadable(field, content, param);
+ }
+
+ fn emit_param(&mut self, field: &str, param: &str, content: &str) {
+ let content = format!("{param}:{content}", param = param, content = content);
+ self.emit_internal(field, &content, true);
+ }
+
+ 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);
+
+ write!(
+ self.param_buffer,
+ "
+ pub(crate) static {param_name}:
+ ::kernel::module_param::ModuleParamAccess<{param_type}> =
+ ::kernel::module_param::ModuleParamAccess::new({param_default});
+
+ const _: () = {{
+ #[link_section = \"__param\"]
+ #[used]
+ static __{module_name}_{param_name}_struct:
+ ::kernel::module_param::KernelParam =
+ ::kernel::module_param::KernelParam::new(
+ ::kernel::bindings::kernel_param {{
+ name: if ::core::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 {{
+ core::ptr::from_ref(&::kernel::bindings::__this_module)
+ .cast_mut()
+ }},
+ #[cfg(not(MODULE))]
+ mod_: ::core::ptr::null_mut(),
+ ops: core::ptr::from_ref(&{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_void_ptr()
+ }},
+ }}
+ );
+ }};
+ ",
+ module_name = info.name,
+ param_type = param.ptype,
+ param_default = param.default,
+ param_name = param.name,
+ ops = ops,
+ )
+ .unwrap();
+ }
+ }
+}
+
+fn param_ops_path(param_type: &str) -> &'static str {
+ match param_type {
+ "i8" => "::kernel::module_param::PARAM_OPS_I8",
+ "u8" => "::kernel::module_param::PARAM_OPS_U8",
+ "i16" => "::kernel::module_param::PARAM_OPS_I16",
+ "u16" => "::kernel::module_param::PARAM_OPS_U16",
+ "i32" => "::kernel::module_param::PARAM_OPS_I32",
+ "u32" => "::kernel::module_param::PARAM_OPS_U32",
+ "i64" => "::kernel::module_param::PARAM_OPS_I64",
+ "u64" => "::kernel::module_param::PARAM_OPS_U64",
+ "isize" => "::kernel::module_param::PARAM_OPS_ISIZE",
+ "usize" => "::kernel::module_param::PARAM_OPS_USIZE",
+ t => panic!("Unsupported parameter type {}", t),
}
}
+fn expect_param_default(param_it: &mut token_stream::IntoIter) -> String {
+ assert_eq!(expect_ident(param_it), "default");
+ assert_eq!(expect_punct(param_it), ':');
+ let sign = try_sign(param_it);
+ let default = try_literal(param_it).expect("Expected default param value");
+ assert_eq!(expect_punct(param_it), ',');
+ let mut value = sign.map(String::from).unwrap_or_default();
+ value.push_str(&default);
+ value
+}
+
#[derive(Debug, Default)]
struct ModuleInfo {
type_: String,
@@ -99,6 +206,50 @@ struct ModuleInfo {
description: Option<String>,
alias: Option<Vec<String>>,
firmware: Option<Vec<String>>,
+ params: Option<Vec<Parameter>>,
+}
+
+#[derive(Debug)]
+struct Parameter {
+ name: String,
+ ptype: String,
+ default: String,
+ description: String,
+}
+
+fn expect_params(it: &mut token_stream::IntoIter) -> Vec<Parameter> {
+ let params = expect_group(it);
+ assert_eq!(params.delimiter(), Delimiter::Brace);
+ let mut it = params.stream().into_iter();
+ let mut parsed = Vec::new();
+
+ loop {
+ let param_name = match it.next() {
+ Some(TokenTree::Ident(ident)) => ident.to_string(),
+ Some(_) => panic!("Expected Ident or end"),
+ None => break,
+ };
+
+ assert_eq!(expect_punct(&mut it), ':');
+ let param_type = expect_ident(&mut it);
+ let group = expect_group(&mut it);
+ assert_eq!(group.delimiter(), Delimiter::Brace);
+ assert_eq!(expect_punct(&mut it), ',');
+
+ let mut param_it = group.stream().into_iter();
+ let param_default = expect_param_default(&mut param_it);
+ let param_description = expect_string_field(&mut param_it, "description");
+ expect_end(&mut param_it);
+
+ parsed.push(Parameter {
+ name: param_name,
+ ptype: param_type,
+ default: param_default,
+ description: param_description,
+ })
+ }
+
+ parsed
}
impl ModuleInfo {
@@ -114,6 +265,7 @@ fn parse(it: &mut token_stream::IntoIter) -> Self {
"license",
"alias",
"firmware",
+ "params",
];
const REQUIRED_KEYS: &[&str] = &["type", "name", "license"];
let mut seen_keys = Vec::new();
@@ -140,6 +292,7 @@ fn parse(it: &mut token_stream::IntoIter) -> Self {
"license" => info.license = expect_string_ascii(it),
"alias" => info.alias = Some(expect_string_array(it)),
"firmware" => info.firmware = Some(expect_string_array(it)),
+ "params" => info.params = Some(expect_params(it)),
_ => panic!("Unknown key \"{key}\". Valid keys are: {EXPECTED_KEYS:?}."),
}
@@ -205,7 +358,9 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
// 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!(
"
@@ -369,15 +524,18 @@ unsafe fn __exit() {{
__MOD.assume_init_drop();
}}
}}
-
{modinfo}
}}
}}
+ mod module_parameters {{
+ {params}
+ }}
",
type_ = info.type_,
name = info.name,
ident = ident,
modinfo = modinfo.buffer,
+ params = modinfo.param_buffer,
initcall_section = ".initcall6.init"
)
.parse()
--
2.47.2
^ permalink raw reply related
* Re: [PATCH 2/3] module: make structure definitions always visible
From: Thomas Weißschuh @ 2025-07-11 6:29 UTC (permalink / raw)
To: Daniel Gomez
Cc: Luis Chamberlain, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Brendan Higgins, David Gow, Rae Moar, linux-modules, linux-kernel,
linux-kselftest, kunit-dev
In-Reply-To: <a9eade27-9b77-431f-b7c8-24c3fb891673@kernel.org>
On Mon, Jul 07, 2025 at 09:11:05PM +0200, Daniel Gomez wrote:
> On 12/06/2025 16.53, Thomas WeiÃschuh wrote:
> > To write code that works with both CONFIG_MODULES=y and CONFIG_MODULES=n
> > it is convenient to use "if (IS_ENABLED(CONFIG_MODULES))" over raw #ifdef.
> > The code will still fully typechecked but the unreachable parts are
> > discarded by the compiler. This prevents accidental breakage when a certain
> > kconfig combination was not specifically tested by the developer.
> > This pattern is already supported to some extend by module.h defining
> > empty stub functions if CONFIG_MODULES=n.
> > However some users of module.h work on the structured defined by module.h.
> >
> > Therefore these structure definitions need to be visible, too.
>
> We are missing here which structures are needed. + we are making more things
> visible than what we actually need.
>
> >
> > Many structure members are still gated by specific configuration settings.
> > The assumption for those is that the code using them will be gated behind
> > the same configuration setting anyways.
>
> I think code and kconfig need to reflect the actual dependencies. For example,
> if CONFIG_LIVEPATCH depends on CONFIG_MODULES, we need to specify that in
> Kconfig with depends on, as well as keep the code gated by these 2 configs with
> ifdef/IS_ENABLED.
If CONFIG_LIVEPATCH depends on CONFIG_MODULES in kconfig then
IS_ENABLED(CONFIG_LIVEPATCH) will depend on CONFIG_MODULES automatically.
There is no need for another explicit IS_ENABLED(CONFIG_MODULES).
> >
> > Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> > ---
> > include/linux/module.h | 23 ++++++++++++-----------
> > 1 file changed, 12 insertions(+), 11 deletions(-)
> >
> > diff --git a/include/linux/module.h b/include/linux/module.h
> > index 52f7b0487a2733c56e2531a434887e56e1bf45b2..7f783e71636542b99db3dd869a9387d14992df45 100644
> > --- a/include/linux/module.h
> > +++ b/include/linux/module.h
> > @@ -302,17 +302,6 @@ static typeof(name) __mod_device_table__##type##__##name \
> >
> > struct notifier_block;
> >
> > -#ifdef CONFIG_MODULES
> > -
> > -extern int modules_disabled; /* for sysctl */
> > -/* Get/put a kernel symbol (calls must be symmetric) */
> > -void *__symbol_get(const char *symbol);
> > -void *__symbol_get_gpl(const char *symbol);
> > -#define symbol_get(x) ({ \
> > - static const char __notrim[] \
> > - __used __section(".no_trim_symbol") = __stringify(x); \
> > - (typeof(&x))(__symbol_get(__stringify(x))); })
> > -
> > enum module_state {
> > MODULE_STATE_LIVE, /* Normal state. */
> > MODULE_STATE_COMING, /* Full formed, running module_init. */
> > @@ -598,6 +587,18 @@ struct module {
> > struct _ddebug_info dyndbg_info;
> > #endif
> > } ____cacheline_aligned __randomize_layout;
> > +
> > +#ifdef CONFIG_MODULES
> > +
> > +extern int modules_disabled; /* for sysctl */
> > +/* Get/put a kernel symbol (calls must be symmetric) */
> > +void *__symbol_get(const char *symbol);
> > +void *__symbol_get_gpl(const char *symbol);
> > +#define symbol_get(x) ({ \
> > + static const char __notrim[] \
> > + __used __section(".no_trim_symbol") = __stringify(x); \
> > + (typeof(&x))(__symbol_get(__stringify(x))); })
> > +
>
> The patch exposes data structures that are not needed. + breaks the
> config dependencies.
If we want to expose 'struct module' to !CONFIG_MODULES code, all it's
effective member types also need to be included.
With my patch these member types are actually still implictly gated behind
CONFIG_MODULES as they depend on it through kconfig.
>
> For example, before this patch:
>
> #ifdef CONFIG_MODULES
>
> {...}
>
> struct mod_tree_node {
>
> {...}
>
> struct module_memory {
> void *base;
> bool is_rox;
> unsigned int size;
>
> #ifdef CONFIG_MODULES_TREE_LOOKUP
> struct mod_tree_node mtn;
> #endif
> };
>
> {...}
> #endif /* CONFIG_MODULES */
>
> After the patch, mod_tree_node is not needed externally.
Can you explain what you mean with "not needed externally"?
'struct mod_tree_node' is only ever used by core module code.
It is only public because it is embedded in the public 'struct module'
> And the mtn field
> in module_memory is exposed only under MODULES_TREE_LOOKUP and not MODULES
> + MODULES_TREE_LOOKUP.
As mentioned above, MODULES_TREE_LOOKUP && !MODULES can never happen.
> I general, I see the issues I mentioned with LIVEPATCH, mod_tree_node, macros,
> and LOOKUP.
>
> > #define MODULE_ARCH_INIT {}
> > #endif
> >
^ permalink raw reply
* Re: [PATCH v2 2/8] execmem: introduce execmem_alloc_rw()
From: Petr Pavlu @ 2025-07-10 13:25 UTC (permalink / raw)
To: Mike Rapoport
Cc: Andrew Morton, Andy Lutomirski, Borislav Petkov, Christophe Leroy,
Daniel Gomez, Dave Hansen, Ingo Molnar, Liam R. Howlett,
Luis Chamberlain, Mark Rutland, Masami Hiramatsu, H. Peter Anvin,
Peter Zijlstra, Sami Tolvanen, Steven Rostedt, Thomas Gleixner,
Yann Ylavic, linux-kernel, linux-mm, linux-modules,
linux-trace-kernel, x86
In-Reply-To: <20250709134933.3848895-3-rppt@kernel.org>
On 7/9/25 3:49 PM, Mike Rapoport wrote:
> From: "Mike Rapoport (Microsoft)" <rppt@kernel.org>
>
> Some callers of execmem_alloc() require the memory to be temporarily
> writable even when it is allocated from ROX cache. These callers use
> execemem_make_temp_rw() right after the call to execmem_alloc().
>
> Wrap this sequence in execmem_alloc_rw() API.
>
> Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
Reviewed-by: Petr Pavlu <petr.pavlu@suse.com>
> @@ -165,6 +149,28 @@ struct execmem_info *execmem_arch_setup(void);
> */
> void *execmem_alloc(enum execmem_type type, size_t size);
>
> +/**
> + * execmem_alloc_rw - allocate writatble executable memory
Nit: writatble -> writable.
--
Thanks,
Petr
^ permalink raw reply
* Re: [PATCH v2 0/8] x86: enable EXECMEM_ROX_CACHE for ftrace and kprobes
From: Peter Zijlstra @ 2025-07-10 11:18 UTC (permalink / raw)
To: Mike Rapoport
Cc: Andrew Morton, Andy Lutomirski, Borislav Petkov, Christophe Leroy,
Daniel Gomez, Dave Hansen, Ingo Molnar, Liam R. Howlett,
Luis Chamberlain, Mark Rutland, Masami Hiramatsu, H. Peter Anvin,
Petr Pavlu, Sami Tolvanen, Steven Rostedt, Thomas Gleixner,
Yann Ylavic, linux-kernel, linux-mm, linux-modules,
linux-trace-kernel, x86
In-Reply-To: <20250709134933.3848895-1-rppt@kernel.org>
On Wed, Jul 09, 2025 at 04:49:25PM +0300, Mike Rapoport wrote:
> Mike Rapoport (Microsoft) (8):
> execmem: drop unused execmem_update_copy()
> execmem: introduce execmem_alloc_rw()
> execmem: rework execmem_cache_free()
> execmem: move execmem_force_rw() and execmem_restore_rox() before use
> execmem: add fallback for failures in vmalloc(VM_ALLOW_HUGE_VMAP)
> execmem: drop writable parameter from execmem_fill_trapping_insns()
> x86/kprobes: enable EXECMEM_ROX_CACHE for kprobes allocations
> x86/ftrace: enable EXECMEM_ROX_CACHE for ftrace allocations
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
^ permalink raw reply
* Re: [PATCH v16 1/7] rust: sync: add `SetOnce`
From: Andreas Hindborg @ 2025-07-10 10:14 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: <DB7SUDRTDJK7.2TXKP6EQJ77FL@kernel.org>
"Benno Lossin" <lossin@kernel.org> writes:
> On Wed Jul 9, 2025 at 7:52 PM CEST, Andreas Hindborg wrote:
>> Introduce the `SetOnce` type, a container that can only be written once.
>> The container uses an internal atomic to synchronize writes to the internal
>> value.
>>
>> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>
> A couple notes on safety documentation below. Also one pretty subtle
> functionality change from last version. With everything fixed:
>
> Reviewed-by: Benno Lossin <lossin@kernel.org>
>
>> ---
>> rust/kernel/sync.rs | 2 +
>> rust/kernel/sync/set_once.rs | 122 +++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 124 insertions(+)
>>
>> diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
>> index 81e3a806e57e2..13e6bc7fa87ac 100644
>> --- a/rust/kernel/sync.rs
>> +++ b/rust/kernel/sync.rs
>> @@ -18,6 +18,7 @@
>> mod locked_by;
>> pub mod poll;
>> pub mod rcu;
>> +mod set_once;
>>
>> pub use arc::{Arc, ArcBorrow, UniqueArc};
>> pub use completion::Completion;
>> @@ -26,6 +27,7 @@
>> pub use lock::mutex::{new_mutex, Mutex, MutexGuard};
>> pub use lock::spinlock::{new_spinlock, SpinLock, SpinLockGuard};
>> pub use locked_by::LockedBy;
>> +pub use set_once::SetOnce;
>>
>> /// Represents a lockdep class. It's a wrapper around C's `lock_class_key`.
>> #[repr(transparent)]
>> diff --git a/rust/kernel/sync/set_once.rs b/rust/kernel/sync/set_once.rs
>> new file mode 100644
>> index 0000000000000..73706abfe9991
>> --- /dev/null
>> +++ b/rust/kernel/sync/set_once.rs
>> @@ -0,0 +1,122 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! A container that can be initialized at most once.
>> +
>> +use super::atomic::{
>> + ordering::{Acquire, Relaxed, Release},
>> + Atomic,
>> +};
>> +use core::{cell::UnsafeCell, mem::MaybeUninit, ptr::drop_in_place};
>> +
>> +/// A container that can be populated at most once. Thread safe.
>> +///
>> +/// Once the a [`SetOnce`] is populated, it remains populated by the same object for the
>> +/// lifetime `Self`.
>> +///
>> +/// # Invariants
>> +///
>> +/// - `init` may only increase in value.
>> +/// - `init` may only assume values in the range `0..=2`.
>> +/// - `init == 0` if and only if the container is empty.
>> +/// - `init == 1` if and only if being initialized.
>> +/// - `init == 2` if and only if the container is populated and valid for shared access.
>
> I think I have a better idea for the last three invariants:
>
> - `init == 0` if and only if `value` is uninitialized.
> - `init == 1` if and only if there is exactly one thread with exclusive
> access to `self.value`.
> - `init == 2` if and only if `value` is initialized and valid for shared
> access.
Sounds good to me.
>
>> +///
>> +/// # Example
>> +///
>> +/// ```
>> +/// # use kernel::sync::SetOnce;
>> +/// let value = SetOnce::new();
>> +/// assert_eq!(None, value.as_ref());
>> +///
>> +/// let status = value.populate(42u8);
>> +/// assert_eq!(true, status);
>> +/// assert_eq!(Some(&42u8), value.as_ref());
>> +/// assert_eq!(Some(42u8), value.copy());
>> +///
>> +/// let status = value.populate(101u8);
>> +/// assert_eq!(false, status);
>> +/// assert_eq!(Some(&42u8), value.as_ref());
>> +/// assert_eq!(Some(42u8), value.copy());
>> +/// ```
>> +pub struct SetOnce<T> {
>> + init: Atomic<u32>,
>> + value: UnsafeCell<MaybeUninit<T>>,
>> +}
>> +
>> +impl<T> Default for SetOnce<T> {
>> + fn default() -> Self {
>> + Self::new()
>> + }
>> +}
>> +
>> +impl<T> SetOnce<T> {
>> + /// Create a new [`SetOnce`].
>> + ///
>> + /// The returned instance will be empty.
>> + pub const fn new() -> Self {
>> + // INVARIANT: The container is empty and we initialize `init` to `0`.
>> + Self {
>> + value: UnsafeCell::new(MaybeUninit::uninit()),
>> + init: Atomic::new(0),
>> + }
>> + }
>> +
>> + /// Get a reference to the contained object.
>> + ///
>> + /// Returns [`None`] if this [`SetOnce`] is empty.
>> + pub fn as_ref(&self) -> Option<&T> {
>> + if self.init.load(Acquire) == 2 {
>> + // SAFETY: By the type invariants of `Self`, `self.init == 2` means that `self.value`
>> + // contains a valid value.
>
> s/contains a valid value/is initialized and valid for shared access/
OK.
>
>> + Some(unsafe { &*self.value.get().cast() })
>> + } else {
>> + None
>> + }
>> + }
>> +
>> + /// Populate the [`SetOnce`].
>> + ///
>> + /// Returns `true` if the [`SetOnce`] was successfully populated.
>> + pub fn populate(&self, value: T) -> bool {
>> + // INVARIANT: If the swap succeeds:
>> + // - We increase `init`.
>> + // - We write the valid value `1` to `init`.
>> + // - Only one thread can succeed in this write, so we have exclusive access after the
>> + // write.
>> + if let Ok(0) = self.init.cmpxchg(0, 1, Relaxed) {
>> + // SAFETY: By the type invariants of `Self`, the fact that we succeeded in writing `1`
>> + // to `self.init` means we obtained exclusive access to the contained object.
>
> s/to the contained object/to `self.value`/
OK.
>
>> + unsafe { core::ptr::write(self.value.get().cast(), value) };
>> + // INVARIANT:
>> + // - We increase `init`.
>> + // - We write the valid value `2` to `init`.
>> + // - We release our exclusive access to the contained object and the object is now
>> + // valid for shared access.
>> + self.init.store(2, Release);
>> + true
>> + } else {
>> + false
>> + }
>> + }
>> +
>> + /// Get a copy of the contained object.
>> + ///
>> + /// Returns [`None`] if the [`SetOnce`] is empty.
>> + pub fn copy(&self) -> Option<T>
>> + where
>> + T: Copy,
>> + {
>> + self.as_ref().copied()
>> + }
>> +}
>> +
>> +impl<T> Drop for SetOnce<T> {
>> + fn drop(&mut self) {
>> + if *self.init.get_mut() == 2 {
>> + // SAFETY: By the type invariants of `Self`, `self.init == 2` means that `self.value`
>> + // contains a valid value. We have exclusive access, as we hold a `mut` reference to
>> + // `self`.
>> + unsafe { drop_in_place(self.value.get()) };
>
> This is sadly doing the wrong thing now since you changed the type of
> `value`: `self.value.get()` is of type `MaybeUninit<T>` and dropping
> that has (obviously) no effect. So we probably need to do
>
> let value = unsafe { &mut *self.value.get() };
> unsafe { value.assume_init_drop() };
>
> I almost overlooked this :)
Oops.
Best regards,
Andreas Hindborg
^ permalink raw reply
* Re: [PATCH v15 1/7] rust: sync: add `SetOnce`
From: Boqun Feng @ 2025-07-09 21:05 UTC (permalink / raw)
To: Benno Lossin
Cc: Andreas Hindborg, Alice Ryhl, Miguel Ojeda, Alex Gaynor, Gary Guo,
Björn Roy Baron, 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: <DB7SYPUAUBUS.30DITWX21NJ96@kernel.org>
On Wed, Jul 09, 2025 at 10:22:04PM +0200, Benno Lossin wrote:
[...]
> >> >>>>> > +impl<T> Drop for SetOnce<T> {
> >> >>>>> > + fn drop(&mut self) {
> >> >>>>> > + if self.init.load(Acquire) == 2 {
> >> >>>>> > + // SAFETY: By the type invariants of `Self`, `self.init == 2` means that `self.value`
> >> >>>>> > + // contains a valid value. We have exclusive access, as we hold a `mut` reference to
> >> >>>>> > + // `self`.
> >> >>>>> > + unsafe { drop_in_place(self.value.get()) };
> >> >>>>>
> >> >>>>> This load does not need to be Acquire. It can be a Relaxed load or
> >> >>>>> even an unsynchronized one since the access is exclusive.
> >> >>>>
> >> >>>> Right, I think we can do the similar as Revocable here:
> >> >>>>
> >> >>>> if *self.init.get_mut() == 2 { }
> >> >
> >> > Ok, now I got it. You are saying I don't need to use the atomic load
> >> > method, because I have mutable access. Sounds good.
> >> >
> >> > But I guess a relaxed load and access through a mutable reference should
> >> > result in the same code generation on most (all?) platforms?
> >>
> >> AFAIK it is not the same on arm.
> >>
> >
> > Right, when LTO=y, arm64 use acquire load to implement
> > READ_ONCE()/atomic_read().
>
> But Andreas was talking about relaxed load vs mutable reference (=
> normal unsynchronized write)?
>
No, I think it was a relaxed load (self.init.load(Relaxed)) vs a normal
unsynchronized *load* (*self.init.get_mut()). Yes, there is a mutable
reference, but we never use it for write.
Regards,
Boqun
> ---
> Cheers,
> Benno
>
^ permalink raw reply
* Re: [PATCH v15 1/7] rust: sync: add `SetOnce`
From: Benno Lossin @ 2025-07-09 20:22 UTC (permalink / raw)
To: Boqun Feng
Cc: Andreas Hindborg, Alice Ryhl, Miguel Ojeda, Alex Gaynor, Gary Guo,
Björn Roy Baron, 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: <aG7NFizFGeFBSXY-@tardis.local>
On Wed Jul 9, 2025 at 10:12 PM CEST, Boqun Feng wrote:
> On Wed, Jul 09, 2025 at 08:22:16PM +0200, Benno Lossin wrote:
>> On Wed Jul 9, 2025 at 12:34 PM CEST, Andreas Hindborg wrote:
>> > "Benno Lossin" <lossin@kernel.org> writes:
>> >> On Tue Jul 8, 2025 at 10:54 AM CEST, Andreas Hindborg wrote:
>> >>> "Boqun Feng" <boqun.feng@gmail.com> writes:
>> >>>> On Mon, Jul 07, 2025 at 03:38:58PM +0200, Alice Ryhl wrote:
>> >>>>> On Mon, Jul 7, 2025 at 3:32 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>> >>>>> >
>> >>>>> > Introduce the `SetOnce` type, a container that can only be written once.
>> >>>>> > The container uses an internal atomic to synchronize writes to the internal
>> >>>>> > value.
>> >>>>> >
>> >>>>> > Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>> >>>>>
>> >>>>> LGTM:
>> >>>>> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
>> >>>>>
>> >>>>> > +impl<T> Drop for SetOnce<T> {
>> >>>>> > + fn drop(&mut self) {
>> >>>>> > + if self.init.load(Acquire) == 2 {
>> >>>>> > + // SAFETY: By the type invariants of `Self`, `self.init == 2` means that `self.value`
>> >>>>> > + // contains a valid value. We have exclusive access, as we hold a `mut` reference to
>> >>>>> > + // `self`.
>> >>>>> > + unsafe { drop_in_place(self.value.get()) };
>> >>>>>
>> >>>>> This load does not need to be Acquire. It can be a Relaxed load or
>> >>>>> even an unsynchronized one since the access is exclusive.
>> >>>>
>> >>>> Right, I think we can do the similar as Revocable here:
>> >>>>
>> >>>> if *self.init.get_mut() == 2 { }
>> >
>> > Ok, now I got it. You are saying I don't need to use the atomic load
>> > method, because I have mutable access. Sounds good.
>> >
>> > But I guess a relaxed load and access through a mutable reference should
>> > result in the same code generation on most (all?) platforms?
>>
>> AFAIK it is not the same on arm.
>>
>
> Right, when LTO=y, arm64 use acquire load to implement
> READ_ONCE()/atomic_read().
But Andreas was talking about relaxed load vs mutable reference (=
normal unsynchronized write)?
---
Cheers,
Benno
^ permalink raw reply
* Re: [PATCH v16 1/7] rust: sync: add `SetOnce`
From: Benno Lossin @ 2025-07-09 20:16 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: <20250709-module-params-v3-v16-1-4f926bcccb50@kernel.org>
On Wed Jul 9, 2025 at 7:52 PM CEST, Andreas Hindborg wrote:
> Introduce the `SetOnce` type, a container that can only be written once.
> The container uses an internal atomic to synchronize writes to the internal
> value.
>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
A couple notes on safety documentation below. Also one pretty subtle
functionality change from last version. With everything fixed:
Reviewed-by: Benno Lossin <lossin@kernel.org>
> ---
> rust/kernel/sync.rs | 2 +
> rust/kernel/sync/set_once.rs | 122 +++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 124 insertions(+)
>
> diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
> index 81e3a806e57e2..13e6bc7fa87ac 100644
> --- a/rust/kernel/sync.rs
> +++ b/rust/kernel/sync.rs
> @@ -18,6 +18,7 @@
> mod locked_by;
> pub mod poll;
> pub mod rcu;
> +mod set_once;
>
> pub use arc::{Arc, ArcBorrow, UniqueArc};
> pub use completion::Completion;
> @@ -26,6 +27,7 @@
> pub use lock::mutex::{new_mutex, Mutex, MutexGuard};
> pub use lock::spinlock::{new_spinlock, SpinLock, SpinLockGuard};
> pub use locked_by::LockedBy;
> +pub use set_once::SetOnce;
>
> /// Represents a lockdep class. It's a wrapper around C's `lock_class_key`.
> #[repr(transparent)]
> diff --git a/rust/kernel/sync/set_once.rs b/rust/kernel/sync/set_once.rs
> new file mode 100644
> index 0000000000000..73706abfe9991
> --- /dev/null
> +++ b/rust/kernel/sync/set_once.rs
> @@ -0,0 +1,122 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! A container that can be initialized at most once.
> +
> +use super::atomic::{
> + ordering::{Acquire, Relaxed, Release},
> + Atomic,
> +};
> +use core::{cell::UnsafeCell, mem::MaybeUninit, ptr::drop_in_place};
> +
> +/// A container that can be populated at most once. Thread safe.
> +///
> +/// Once the a [`SetOnce`] is populated, it remains populated by the same object for the
> +/// lifetime `Self`.
> +///
> +/// # Invariants
> +///
> +/// - `init` may only increase in value.
> +/// - `init` may only assume values in the range `0..=2`.
> +/// - `init == 0` if and only if the container is empty.
> +/// - `init == 1` if and only if being initialized.
> +/// - `init == 2` if and only if the container is populated and valid for shared access.
I think I have a better idea for the last three invariants:
- `init == 0` if and only if `value` is uninitialized.
- `init == 1` if and only if there is exactly one thread with exclusive
access to `self.value`.
- `init == 2` if and only if `value` is initialized and valid for shared
access.
> +///
> +/// # Example
> +///
> +/// ```
> +/// # use kernel::sync::SetOnce;
> +/// let value = SetOnce::new();
> +/// assert_eq!(None, value.as_ref());
> +///
> +/// let status = value.populate(42u8);
> +/// assert_eq!(true, status);
> +/// assert_eq!(Some(&42u8), value.as_ref());
> +/// assert_eq!(Some(42u8), value.copy());
> +///
> +/// let status = value.populate(101u8);
> +/// assert_eq!(false, status);
> +/// assert_eq!(Some(&42u8), value.as_ref());
> +/// assert_eq!(Some(42u8), value.copy());
> +/// ```
> +pub struct SetOnce<T> {
> + init: Atomic<u32>,
> + value: UnsafeCell<MaybeUninit<T>>,
> +}
> +
> +impl<T> Default for SetOnce<T> {
> + fn default() -> Self {
> + Self::new()
> + }
> +}
> +
> +impl<T> SetOnce<T> {
> + /// Create a new [`SetOnce`].
> + ///
> + /// The returned instance will be empty.
> + pub const fn new() -> Self {
> + // INVARIANT: The container is empty and we initialize `init` to `0`.
> + Self {
> + value: UnsafeCell::new(MaybeUninit::uninit()),
> + init: Atomic::new(0),
> + }
> + }
> +
> + /// Get a reference to the contained object.
> + ///
> + /// Returns [`None`] if this [`SetOnce`] is empty.
> + pub fn as_ref(&self) -> Option<&T> {
> + if self.init.load(Acquire) == 2 {
> + // SAFETY: By the type invariants of `Self`, `self.init == 2` means that `self.value`
> + // contains a valid value.
s/contains a valid value/is initialized and valid for shared access/
> + Some(unsafe { &*self.value.get().cast() })
> + } else {
> + None
> + }
> + }
> +
> + /// Populate the [`SetOnce`].
> + ///
> + /// Returns `true` if the [`SetOnce`] was successfully populated.
> + pub fn populate(&self, value: T) -> bool {
> + // INVARIANT: If the swap succeeds:
> + // - We increase `init`.
> + // - We write the valid value `1` to `init`.
> + // - Only one thread can succeed in this write, so we have exclusive access after the
> + // write.
> + if let Ok(0) = self.init.cmpxchg(0, 1, Relaxed) {
> + // SAFETY: By the type invariants of `Self`, the fact that we succeeded in writing `1`
> + // to `self.init` means we obtained exclusive access to the contained object.
s/to the contained object/to `self.value`/
> + unsafe { core::ptr::write(self.value.get().cast(), value) };
> + // INVARIANT:
> + // - We increase `init`.
> + // - We write the valid value `2` to `init`.
> + // - We release our exclusive access to the contained object and the object is now
> + // valid for shared access.
> + self.init.store(2, Release);
> + true
> + } else {
> + false
> + }
> + }
> +
> + /// Get a copy of the contained object.
> + ///
> + /// Returns [`None`] if the [`SetOnce`] is empty.
> + pub fn copy(&self) -> Option<T>
> + where
> + T: Copy,
> + {
> + self.as_ref().copied()
> + }
> +}
> +
> +impl<T> Drop for SetOnce<T> {
> + fn drop(&mut self) {
> + if *self.init.get_mut() == 2 {
> + // SAFETY: By the type invariants of `Self`, `self.init == 2` means that `self.value`
> + // contains a valid value. We have exclusive access, as we hold a `mut` reference to
> + // `self`.
> + unsafe { drop_in_place(self.value.get()) };
This is sadly doing the wrong thing now since you changed the type of
`value`: `self.value.get()` is of type `MaybeUninit<T>` and dropping
that has (obviously) no effect. So we probably need to do
let value = unsafe { &mut *self.value.get() };
unsafe { value.assume_init_drop() };
I almost overlooked this :)
---
Cheers,
Benno
> + }
> + }
> +}
^ permalink raw reply
* Re: [PATCH v15 1/7] rust: sync: add `SetOnce`
From: Boqun Feng @ 2025-07-09 20:12 UTC (permalink / raw)
To: Benno Lossin
Cc: Andreas Hindborg, Alice Ryhl, Miguel Ojeda, Alex Gaynor, Gary Guo,
Björn Roy Baron, 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: <DB7QEZNTH2SB.SSRG5H7TXZZT@kernel.org>
On Wed, Jul 09, 2025 at 08:22:16PM +0200, Benno Lossin wrote:
> On Wed Jul 9, 2025 at 12:34 PM CEST, Andreas Hindborg wrote:
> > "Benno Lossin" <lossin@kernel.org> writes:
> >> On Tue Jul 8, 2025 at 10:54 AM CEST, Andreas Hindborg wrote:
> >>> "Boqun Feng" <boqun.feng@gmail.com> writes:
> >>>> On Mon, Jul 07, 2025 at 03:38:58PM +0200, Alice Ryhl wrote:
> >>>>> On Mon, Jul 7, 2025 at 3:32 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
> >>>>> >
> >>>>> > Introduce the `SetOnce` type, a container that can only be written once.
> >>>>> > The container uses an internal atomic to synchronize writes to the internal
> >>>>> > value.
> >>>>> >
> >>>>> > Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> >>>>>
> >>>>> LGTM:
> >>>>> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> >>>>>
> >>>>> > +impl<T> Drop for SetOnce<T> {
> >>>>> > + fn drop(&mut self) {
> >>>>> > + if self.init.load(Acquire) == 2 {
> >>>>> > + // SAFETY: By the type invariants of `Self`, `self.init == 2` means that `self.value`
> >>>>> > + // contains a valid value. We have exclusive access, as we hold a `mut` reference to
> >>>>> > + // `self`.
> >>>>> > + unsafe { drop_in_place(self.value.get()) };
> >>>>>
> >>>>> This load does not need to be Acquire. It can be a Relaxed load or
> >>>>> even an unsynchronized one since the access is exclusive.
> >>>>
> >>>> Right, I think we can do the similar as Revocable here:
> >>>>
> >>>> if *self.init.get_mut() == 2 { }
> >
> > Ok, now I got it. You are saying I don't need to use the atomic load
> > method, because I have mutable access. Sounds good.
> >
> > But I guess a relaxed load and access through a mutable reference should
> > result in the same code generation on most (all?) platforms?
>
> AFAIK it is not the same on arm.
>
Right, when LTO=y, arm64 use acquire load to implement
READ_ONCE()/atomic_read().
Regards,
Boqun
> ---
> Cheers,
> Benno
>
^ permalink raw reply
* Re: [GIT PULL] Modules fixes for v6.16-rc6
From: Daniel Gomez @ 2025-07-09 19:14 UTC (permalink / raw)
To: Linus Torvalds
Cc: Luis Chamberlain, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
linux-modules, Sebastian Andrzej Siewior, linux-kernel
In-Reply-To: <CAHk-=whHRv28HBL47x-L5okG+fZDJyX432uxG3AQqbqRAgkAYQ@mail.gmail.com>
On 08/07/2025 22.21, Linus Torvalds wrote:
> On Tue, 8 Jul 2025 at 12:38, Daniel Gomez <da.gomez@kernel.org> wrote:
>>
>> Petr and Sebastian went down to the archive history (before Git) and
>> found the commit that broke it b3b91325f3c7 ("Don't relocate
>> non-allocated regions in modules.").
>
> When mentioning git commits in other trees, you really do need to
> mention *which* other tree it is.
Makes sense. I thought there was only one history tree to search for this.
>
> In this case, it seems to be
>
> https://git.kernel.org/pub/scm/linux/kernel/git/mpe/linux-fullhistory.git/
>
> that has that commit, but that's actually a kind of odd tree.
>
> The typical bitkeeper conversion tree (which is where I suspect the
> full-history conversion then got it from, unless somebody did a
> separate conversion) that contains that commit is actually
>
> https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/
Thanks for the link.
>
> and there the commit has commit ID 1a6100caae ("Don't relocate
> non-allocated regions in modules")
That's it.
>
> Linus
^ permalink raw reply
* Re: [PATCH v15 1/7] rust: sync: add `SetOnce`
From: Benno Lossin @ 2025-07-09 18:22 UTC (permalink / raw)
To: Andreas Hindborg
Cc: Boqun Feng, Alice Ryhl, Miguel Ojeda, Alex Gaynor, Gary Guo,
Björn Roy Baron, 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: <87ple9lkjr.fsf@kernel.org>
On Wed Jul 9, 2025 at 12:34 PM CEST, Andreas Hindborg wrote:
> "Benno Lossin" <lossin@kernel.org> writes:
>> On Tue Jul 8, 2025 at 10:54 AM CEST, Andreas Hindborg wrote:
>>> "Boqun Feng" <boqun.feng@gmail.com> writes:
>>>> On Mon, Jul 07, 2025 at 03:38:58PM +0200, Alice Ryhl wrote:
>>>>> On Mon, Jul 7, 2025 at 3:32 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>>>> >
>>>>> > Introduce the `SetOnce` type, a container that can only be written once.
>>>>> > The container uses an internal atomic to synchronize writes to the internal
>>>>> > value.
>>>>> >
>>>>> > Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>>>>>
>>>>> LGTM:
>>>>> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
>>>>>
>>>>> > +impl<T> Drop for SetOnce<T> {
>>>>> > + fn drop(&mut self) {
>>>>> > + if self.init.load(Acquire) == 2 {
>>>>> > + // SAFETY: By the type invariants of `Self`, `self.init == 2` means that `self.value`
>>>>> > + // contains a valid value. We have exclusive access, as we hold a `mut` reference to
>>>>> > + // `self`.
>>>>> > + unsafe { drop_in_place(self.value.get()) };
>>>>>
>>>>> This load does not need to be Acquire. It can be a Relaxed load or
>>>>> even an unsynchronized one since the access is exclusive.
>>>>
>>>> Right, I think we can do the similar as Revocable here:
>>>>
>>>> if *self.init.get_mut() == 2 { }
>
> Ok, now I got it. You are saying I don't need to use the atomic load
> method, because I have mutable access. Sounds good.
>
> But I guess a relaxed load and access through a mutable reference should
> result in the same code generation on most (all?) platforms?
AFAIK it is not the same on arm.
---
Cheers,
Benno
^ 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