* Re: [PATCH 1/3] module: move 'struct module_use' to internal.h
From: Daniel Gomez @ 2025-06-19 16:27 UTC (permalink / raw)
To: Thomas Weißschuh, Luis Chamberlain, Petr Pavlu,
Sami Tolvanen, Daniel Gomez, Brendan Higgins, David Gow, Rae Moar
Cc: linux-modules, linux-kernel, linux-kselftest, kunit-dev
In-Reply-To: <20250612-kunit-ifdef-modules-v1-1-fdccd42dcff8@linutronix.de>
On 12/06/2025 16.53, Thomas WeiÃschuh wrote:
> The struct was moved to the public header file in
> commit c8e21ced08b3 ("module: fix kdb's illicit use of struct module_use.").
> Back then the structure was used outside of the module core.
> Nowadays this is not true anymore, so the structure can be made internal.
>
> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
Reviewed-by: Daniel Gomez <da.gomez@samsung.com>
^ permalink raw reply
* Re: [PATCH 3/3] kunit: test: Drop CONFIG_MODULE ifdeffery
From: David Gow @ 2025-06-20 9:51 UTC (permalink / raw)
To: Thomas Weißschuh
Cc: Luis Chamberlain, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Brendan Higgins, Rae Moar, linux-modules, linux-kernel,
linux-kselftest, kunit-dev
In-Reply-To: <20250612-kunit-ifdef-modules-v1-3-fdccd42dcff8@linutronix.de>
[-- Attachment #1: Type: text/plain, Size: 1888 bytes --]
On Thu, 12 Jun 2025 at 22:54, Thomas Weißschuh
<thomas.weissschuh@linutronix.de> 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>
> ---
Acked-by: David Gow <davidgow@google.com>
Cheers,
-- David
> 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.49.0
>
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5281 bytes --]
^ permalink raw reply
* Re: [PATCH v13 2/6] rust: introduce module_param module
From: Andreas Hindborg @ 2025-06-20 11:29 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: <DAQJCUE1C2JE.204A8IS7LBIVZ@kernel.org>
"Benno Lossin" <lossin@kernel.org> writes:
> On Thu Jun 12, 2025 at 3:40 PM CEST, Andreas Hindborg wrote:
>> +/// A wrapper for kernel parameters.
>> +///
>> +/// This type is instantiated by the [`module!`] macro when module parameters are
>> +/// defined. You should never need to instantiate this type directly.
>> +///
>> +/// Note: This type is `pub` because it is used by module crates to access
>> +/// parameter values.
>> +#[repr(transparent)]
>> +pub struct ModuleParamAccess<T> {
>> + data: core::cell::UnsafeCell<T>,
>> +}
>> +
>> +// SAFETY: We only create shared references to the contents of this container,
>> +// so if `T` is `Sync`, so is `ModuleParamAccess`.
>> +unsafe impl<T: Sync> Sync for ModuleParamAccess<T> {}
>> +
>> +impl<T> ModuleParamAccess<T> {
>> + #[doc(hidden)]
>> + pub const fn new(value: T) -> Self {
>> + Self {
>> + data: core::cell::UnsafeCell::new(value),
>> + }
>> + }
>> +
>> + /// Get a shared reference to the parameter value.
>> + // Note: When sysfs access to parameters are enabled, we have to pass in a
>> + // held lock guard here.
>> + pub fn get(&self) -> &T {
>> + // SAFETY: As we only support read only parameters with no sysfs
>> + // exposure, the kernel will not touch the parameter data after module
>> + // initialization.
>
> This should be a type invariant. But I'm having difficulty defining one
> that's actually correct: after parsing the parameter, this is written
> to, but when is that actually?
For built-in modules it is during kernel initialization. For loadable
modules, it during module load. No code from the module will execute
before parameters are set.
> Would we eventually execute other Rust
> code during that time? (for example when we allow custom parameter
> parsing)
I don't think we will need to synchronize because of custom parameter
parsing. Parameters are initialized sequentially. It is not a problem if
the custom parameter parsing code name other parameters, because they
are all initialized to valid values (as they are statics).
>
> This function also must never be `const` because of the following:
>
> module! {
> // ...
> params: {
> my_param: i64 {
> default: 0,
> description: "",
> },
> },
> }
>
> static BAD: &'static i64 = module_parameters::my_param.get();
>
> AFAIK, this static will be executed before loading module parameters and
> thus it makes writing to the parameter UB.
As I understand, the static will be initialized by a constant expression
evaluated at compile time. I am not sure what happens when this is
evaluated in const context:
pub fn get(&self) -> &T {
// SAFETY: As we only support read only parameters with no sysfs
// exposure, the kernel will not touch the parameter data after module
// initialization.
unsafe { &*self.data.get() }
}
Why would that not be OK? I would assume the compiler builds a dependency graph
when initializing statics?
Best regards,
Andreas Hindborg
^ permalink raw reply
* Re: [PATCH v13 2/6] rust: introduce module_param module
From: Andreas Hindborg @ 2025-06-20 10:31 UTC (permalink / raw)
To: Benno Lossin
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Nicolas Schier, Trevor Gross, Adam Bratschi-Kaye, rust-for-linux,
linux-kernel, linux-kbuild, Petr Pavlu, Sami Tolvanen,
Daniel Gomez, Simona Vetter, Greg KH, Fiona Behrens,
Daniel Almeida, linux-modules
In-Reply-To: <DAQIXKJ9VMS6.2044WT0FQQCVC@kernel.org>
"Benno Lossin" <lossin@kernel.org> writes:
> On Thu Jun 19, 2025 at 2:20 PM CEST, Andreas Hindborg wrote:
>> "Benno Lossin" <lossin@kernel.org> writes:
>>> On Thu Jun 12, 2025 at 3:40 PM CEST, Andreas Hindborg wrote:
[...]
>>>> + crate::error::from_result(|| {
>>>> + let new_value = T::try_from_param_arg(arg)?;
>>>> +
>>>> + // SAFETY: By function safety requirements `param` is be valid for reads.
>>>> + let old_value = unsafe { (*param).__bindgen_anon_1.arg as *mut T };
>>>> +
>>>> + // SAFETY: By function safety requirements, the target of `old_value` is valid for writes
>>>> + // and is initialized.
>>>> + unsafe { *old_value = new_value };
>>>
>>> So if we keep the `ModuleParam: Copy` bound from above, then we don't
>>> need to drop the type here (as `Copy` implies `!Drop`). So we could also
>>> remove the requirement for initialized memory and use `ptr::write` here
>>> instead. Thoughts?
>>
>> Yes, that is the rationale for the `Copy` bound. What would be the
>> benefit of using `ptr::write`? They should be equivalent for `Copy`
>> types, right.
>
> They should be equivalent, but if we drop the requirement that the value
> is initialized to begin with, then removing `Copy` will result in UB
> here.
>
>> I was using `ptr::replace`, but Alice suggested the pace expression
>> assignment instead, since I was not using the old value.
>
> That makes sense, but if we also remove the initialized requirement,
> then I would prefer `ptr::write`.
OK, we can do that.
Best regards,
Andreas Hindborg
^ permalink raw reply
* Re: [PATCH v13 2/6] rust: introduce module_param module
From: Andreas Hindborg @ 2025-06-20 11:52 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: <87ikkq648o.fsf@kernel.org>
Andreas Hindborg <a.hindborg@kernel.org> writes:
> "Benno Lossin" <lossin@kernel.org> writes:
>
>> On Thu Jun 12, 2025 at 3:40 PM CEST, Andreas Hindborg wrote:
>>> +/// A wrapper for kernel parameters.
>>> +///
>>> +/// This type is instantiated by the [`module!`] macro when module parameters are
>>> +/// defined. You should never need to instantiate this type directly.
>>> +///
>>> +/// Note: This type is `pub` because it is used by module crates to access
>>> +/// parameter values.
>>> +#[repr(transparent)]
>>> +pub struct ModuleParamAccess<T> {
>>> + data: core::cell::UnsafeCell<T>,
>>> +}
>>> +
>>> +// SAFETY: We only create shared references to the contents of this container,
>>> +// so if `T` is `Sync`, so is `ModuleParamAccess`.
>>> +unsafe impl<T: Sync> Sync for ModuleParamAccess<T> {}
>>> +
>>> +impl<T> ModuleParamAccess<T> {
>>> + #[doc(hidden)]
>>> + pub const fn new(value: T) -> Self {
>>> + Self {
>>> + data: core::cell::UnsafeCell::new(value),
>>> + }
>>> + }
>>> +
>>> + /// Get a shared reference to the parameter value.
>>> + // Note: When sysfs access to parameters are enabled, we have to pass in a
>>> + // held lock guard here.
>>> + pub fn get(&self) -> &T {
>>> + // SAFETY: As we only support read only parameters with no sysfs
>>> + // exposure, the kernel will not touch the parameter data after module
>>> + // initialization.
>>
>> This should be a type invariant. But I'm having difficulty defining one
>> that's actually correct: after parsing the parameter, this is written
>> to, but when is that actually?
>
> For built-in modules it is during kernel initialization. For loadable
> modules, it during module load. No code from the module will execute
> before parameters are set.
>
>> Would we eventually execute other Rust
>> code during that time? (for example when we allow custom parameter
>> parsing)
>
> I don't think we will need to synchronize because of custom parameter
> parsing. Parameters are initialized sequentially. It is not a problem if
> the custom parameter parsing code name other parameters, because they
> are all initialized to valid values (as they are statics).
>
>>
>> This function also must never be `const` because of the following:
>>
>> module! {
>> // ...
>> params: {
>> my_param: i64 {
>> default: 0,
>> description: "",
>> },
>> },
>> }
>>
>> static BAD: &'static i64 = module_parameters::my_param.get();
>>
>> AFAIK, this static will be executed before loading module parameters and
>> thus it makes writing to the parameter UB.
>
> As I understand, the static will be initialized by a constant expression
> evaluated at compile time. I am not sure what happens when this is
> evaluated in const context:
>
> pub fn get(&self) -> &T {
> // SAFETY: As we only support read only parameters with no sysfs
> // exposure, the kernel will not touch the parameter data after module
> // initialization.
> unsafe { &*self.data.get() }
> }
>
> Why would that not be OK? I would assume the compiler builds a dependency graph
> when initializing statics?
It seems the compiler builds a dependency graph to check:
https://play.rust-lang.org/?version=nightly&mode=debug&edition=2024&gist=7a4d129a3fd2ae39a0aab9df3878a3d3
Best regards,
Andreas Hindborg
^ permalink raw reply
* Re: [PATCH v13 2/6] rust: introduce module_param module
From: Benno Lossin @ 2025-06-20 12:28 UTC (permalink / raw)
To: Andreas Hindborg
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Nicolas Schier, Trevor Gross, Adam Bratschi-Kaye, rust-for-linux,
linux-kernel, linux-kbuild, Petr Pavlu, Sami Tolvanen,
Daniel Gomez, Simona Vetter, Greg KH, Fiona Behrens,
Daniel Almeida, linux-modules
In-Reply-To: <87ikkq648o.fsf@kernel.org>
On Fri Jun 20, 2025 at 1:29 PM CEST, Andreas Hindborg wrote:
> "Benno Lossin" <lossin@kernel.org> writes:
>> On Thu Jun 12, 2025 at 3:40 PM CEST, Andreas Hindborg wrote:
>>> +/// A wrapper for kernel parameters.
>>> +///
>>> +/// This type is instantiated by the [`module!`] macro when module parameters are
>>> +/// defined. You should never need to instantiate this type directly.
>>> +///
>>> +/// Note: This type is `pub` because it is used by module crates to access
>>> +/// parameter values.
>>> +#[repr(transparent)]
>>> +pub struct ModuleParamAccess<T> {
>>> + data: core::cell::UnsafeCell<T>,
>>> +}
>>> +
>>> +// SAFETY: We only create shared references to the contents of this container,
>>> +// so if `T` is `Sync`, so is `ModuleParamAccess`.
>>> +unsafe impl<T: Sync> Sync for ModuleParamAccess<T> {}
>>> +
>>> +impl<T> ModuleParamAccess<T> {
>>> + #[doc(hidden)]
>>> + pub const fn new(value: T) -> Self {
>>> + Self {
>>> + data: core::cell::UnsafeCell::new(value),
>>> + }
>>> + }
>>> +
>>> + /// Get a shared reference to the parameter value.
>>> + // Note: When sysfs access to parameters are enabled, we have to pass in a
>>> + // held lock guard here.
>>> + pub fn get(&self) -> &T {
>>> + // SAFETY: As we only support read only parameters with no sysfs
>>> + // exposure, the kernel will not touch the parameter data after module
>>> + // initialization.
>>
>> This should be a type invariant. But I'm having difficulty defining one
>> that's actually correct: after parsing the parameter, this is written
>> to, but when is that actually?
>
> For built-in modules it is during kernel initialization. For loadable
> modules, it during module load. No code from the module will execute
> before parameters are set.
Gotcha and there never ever will be custom code that is executed
before/during parameter setting (so code aside from code in `kernel`)?
>> Would we eventually execute other Rust
>> code during that time? (for example when we allow custom parameter
>> parsing)
>
> I don't think we will need to synchronize because of custom parameter
> parsing. Parameters are initialized sequentially. It is not a problem if
> the custom parameter parsing code name other parameters, because they
> are all initialized to valid values (as they are statics).
If you have `&'static i64`, then the value at that reference is never
allowed to change.
>> This function also must never be `const` because of the following:
>>
>> module! {
>> // ...
>> params: {
>> my_param: i64 {
>> default: 0,
>> description: "",
>> },
>> },
>> }
>>
>> static BAD: &'static i64 = module_parameters::my_param.get();
>>
>> AFAIK, this static will be executed before loading module parameters and
>> thus it makes writing to the parameter UB.
>
> As I understand, the static will be initialized by a constant expression
> evaluated at compile time. I am not sure what happens when this is
> evaluated in const context:
>
> pub fn get(&self) -> &T {
> // SAFETY: As we only support read only parameters with no sysfs
> // exposure, the kernel will not touch the parameter data after module
> // initialization.
> unsafe { &*self.data.get() }
> }
>
> Why would that not be OK? I would assume the compiler builds a dependency graph
> when initializing statics?
Yes it builds a dependency graph, but that is irrelevant? The problem is
that I can create a `'static` reference to the inner value *before* the
parameter is written-to (as the static is initialized before the
parameters).
---
Cheers,
Benno
^ permalink raw reply
* [PATCH v4 0/7] Add generated modalias to modules.builtin.modinfo
From: Alexey Gladkov @ 2025-06-21 13:57 UTC (permalink / raw)
To: Masahiro Yamada, Petr Pavlu, Luis Chamberlain, Sami Tolvanen,
Daniel Gomez, Nathan Chancellor, Nicolas Schier
Cc: linux-kernel, linux-modules, linux-kbuild, linux-scsi,
Alexey Gladkov
The modules.builtin.modinfo file is used by userspace (kmod to be specific) to
get information about builtin modules. Among other information about the module,
information about module aliases is stored. This is very important to determine
that a particular modalias will be handled by a module that is inside the
kernel.
There are several mechanisms for creating modalias for modules:
The first is to explicitly specify the MODULE_ALIAS of the macro. In this case,
the aliases go into the '.modinfo' section of the module if it is compiled
separately or into vmlinux.o if it is builtin into the kernel.
The second is the use of MODULE_DEVICE_TABLE followed by the use of the
modpost utility. In this case, vmlinux.o no longer has this information and
does not get it into modules.builtin.modinfo.
For example:
$ modinfo pci:v00008086d0000A36Dsv00001043sd00008694bc0Csc03i30
modinfo: ERROR: Module pci:v00008086d0000A36Dsv00001043sd00008694bc0Csc03i30 not found.
$ modinfo xhci_pci
name: xhci_pci
filename: (builtin)
license: GPL
file: drivers/usb/host/xhci-pci
description: xHCI PCI Host Controller Driver
The builtin module is missing alias "pci:v*d*sv*sd*bc0Csc03i30*" which will be
generated by modpost if the module is built separately.
To fix this it is necessary to add the generated by modpost modalias to
modules.builtin.modinfo.
Fortunately modpost already generates .vmlinux.export.c for exported symbols. It
is possible to use this file to create a '.modinfo' section for builtin modules.
The modules.builtin.modinfo file becomes a composite file. One part is extracted
from vmlinux.o, the other part from .vmlinux.export.o.
Notes:
- v4:
* Rework the patchset based on top of Masahiro Yamada's patches.
* Add removal of unnecessary __mod_device_table__* symbols to avoid symbol
table growth in vmlinux.
* rust code takes into account changes in __mod_device_table__*.
* v3: https://lore.kernel.org/all/cover.1748335606.git.legion@kernel.org/
- v3:
* Add `Reviewed-by` tag to patches from Petr Pavlu.
* Rebase to v6.15.
* v2: https://lore.kernel.org/all/20250509164237.2886508-1-legion@kernel.org/
- v2:
* Drop patch for mfd because it was already applied and is in linux-next.
* The generation of aliases for builtin modules has been redone as
suggested by Masahiro Yamada.
* Rebase to v6.15-rc5-136-g9c69f8884904
* v1: https://lore.kernel.org/all/cover.1745591072.git.legion@kernel.org/
Alexey Gladkov (3):
scsi: Always define blogic_pci_tbl structure
modpost: Add modname to mod_device_table alias
modpost: Create modalias for builtin modules
Masahiro Yamada (4):
module: remove meaningless 'name' parameter from __MODULE_INFO()
kbuild: always create intermediate vmlinux.unstripped
kbuild: keep .modinfo section in vmlinux.unstripped
kbuild: extract modules.builtin.modinfo from vmlinux.unstripped
drivers/scsi/BusLogic.c | 2 -
include/asm-generic/vmlinux.lds.h | 2 +-
include/crypto/algapi.h | 4 +-
include/linux/module.h | 21 ++++-----
include/linux/moduleparam.h | 9 ++--
include/net/tcp.h | 4 +-
rust/kernel/device_id.rs | 8 ++--
scripts/Makefile.vmlinux | 74 +++++++++++++++++++++----------
scripts/Makefile.vmlinux_o | 26 +----------
scripts/mksysmap | 6 +++
scripts/mod/file2alias.c | 34 ++++++++++++--
scripts/mod/modpost.c | 17 ++++++-
scripts/mod/modpost.h | 2 +
13 files changed, 131 insertions(+), 78 deletions(-)
--
2.49.0
^ permalink raw reply
* [PATCH v4 1/7] module: remove meaningless 'name' parameter from __MODULE_INFO()
From: Alexey Gladkov @ 2025-06-21 13:57 UTC (permalink / raw)
To: Masahiro Yamada, Petr Pavlu, Luis Chamberlain, Sami Tolvanen,
Daniel Gomez, Nathan Chancellor, Nicolas Schier
Cc: linux-kernel, linux-modules, linux-kbuild, linux-scsi, Herbert Xu,
David S. Miller
In-Reply-To: <cover.1750511018.git.legion@kernel.org>
From: Masahiro Yamada <masahiroy@kernel.org>
The symbol names in the .modinfo section are never used and already
randomized by the __UNIQUE_ID() macro.
Therefore, the second parameter of __MODULE_INFO() is meaningless
and can be removed to simplify the code.
With this change, the symbol names in the .modinfo section will be
prefixed with __UNIQUE_ID_modinfo, making it clearer that they
originate from MODULE_INFO().
[Before]
$ objcopy -j .modinfo vmlinux.o modinfo.o
$ nm -n modinfo.o | head -n10
0000000000000000 r __UNIQUE_ID_license560
0000000000000011 r __UNIQUE_ID_file559
0000000000000030 r __UNIQUE_ID_description558
0000000000000074 r __UNIQUE_ID_license580
000000000000008e r __UNIQUE_ID_file579
00000000000000bd r __UNIQUE_ID_description578
00000000000000e6 r __UNIQUE_ID_license581
00000000000000ff r __UNIQUE_ID_file580
0000000000000134 r __UNIQUE_ID_description579
0000000000000179 r __UNIQUE_ID_uncore_no_discover578
[After]
$ objcopy -j .modinfo vmlinux.o modinfo.o
$ nm -n modinfo.o | head -n10
0000000000000000 r __UNIQUE_ID_modinfo560
0000000000000011 r __UNIQUE_ID_modinfo559
0000000000000030 r __UNIQUE_ID_modinfo558
0000000000000074 r __UNIQUE_ID_modinfo580
000000000000008e r __UNIQUE_ID_modinfo579
00000000000000bd r __UNIQUE_ID_modinfo578
00000000000000e6 r __UNIQUE_ID_modinfo581
00000000000000ff r __UNIQUE_ID_modinfo580
0000000000000134 r __UNIQUE_ID_modinfo579
0000000000000179 r __UNIQUE_ID_modinfo578
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: "David S. Miller" <davem@davemloft.net>
Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---
include/crypto/algapi.h | 4 ++--
include/linux/module.h | 3 ---
include/linux/moduleparam.h | 9 +++++----
include/net/tcp.h | 4 ++--
4 files changed, 9 insertions(+), 11 deletions(-)
diff --git a/include/crypto/algapi.h b/include/crypto/algapi.h
index 188eface0a11..fc4574940636 100644
--- a/include/crypto/algapi.h
+++ b/include/crypto/algapi.h
@@ -43,8 +43,8 @@
* alias.
*/
#define MODULE_ALIAS_CRYPTO(name) \
- __MODULE_INFO(alias, alias_userspace, name); \
- __MODULE_INFO(alias, alias_crypto, "crypto-" name)
+ MODULE_INFO(alias, name); \
+ MODULE_INFO(alias, "crypto-" name)
struct crypto_aead;
struct crypto_instance;
diff --git a/include/linux/module.h b/include/linux/module.h
index 8050f77c3b64..24fe6b865e9c 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -164,9 +164,6 @@ extern void cleanup_module(void);
struct module_kobject *lookup_or_create_module_kobject(const char *name);
-/* Generic info of form tag = "info" */
-#define MODULE_INFO(tag, info) __MODULE_INFO(tag, tag, info)
-
/* For userspace: you can also call me... */
#define MODULE_ALIAS(_alias) MODULE_INFO(alias, _alias)
diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index bfb85fd13e1f..00166f747e27 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -20,18 +20,19 @@
/* Chosen so that structs with an unsigned long line up. */
#define MAX_PARAM_PREFIX_LEN (64 - sizeof(unsigned long))
-#define __MODULE_INFO(tag, name, info) \
- static const char __UNIQUE_ID(name)[] \
+/* Generic info of form tag = "info" */
+#define MODULE_INFO(tag, info) \
+ static const char __UNIQUE_ID(modinfo)[] \
__used __section(".modinfo") __aligned(1) \
= __MODULE_INFO_PREFIX __stringify(tag) "=" info
#define __MODULE_PARM_TYPE(name, _type) \
- __MODULE_INFO(parmtype, name##type, #name ":" _type)
+ MODULE_INFO(parmtype, #name ":" _type)
/* One for each parameter, describing how to use it. Some files do
multiple of these per line, so can't just use MODULE_INFO. */
#define MODULE_PARM_DESC(_parm, desc) \
- __MODULE_INFO(parm, _parm, #_parm ":" desc)
+ MODULE_INFO(parm, #_parm ":" desc)
struct kernel_param;
diff --git a/include/net/tcp.h b/include/net/tcp.h
index 5078ad868fee..9b39ef630c92 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -2662,8 +2662,8 @@ void tcp_update_ulp(struct sock *sk, struct proto *p,
void (*write_space)(struct sock *sk));
#define MODULE_ALIAS_TCP_ULP(name) \
- __MODULE_INFO(alias, alias_userspace, name); \
- __MODULE_INFO(alias, alias_tcp_ulp, "tcp-ulp-" name)
+ MODULE_INFO(alias, name); \
+ MODULE_INFO(alias, "tcp-ulp-" name)
#ifdef CONFIG_NET_SOCK_MSG
struct sk_msg;
--
2.49.0
^ permalink raw reply related
* [PATCH v4 2/7] kbuild: always create intermediate vmlinux.unstripped
From: Alexey Gladkov @ 2025-06-21 13:57 UTC (permalink / raw)
To: Masahiro Yamada, Petr Pavlu, Luis Chamberlain, Sami Tolvanen,
Daniel Gomez, Nathan Chancellor, Nicolas Schier
Cc: linux-kernel, linux-modules, linux-kbuild, linux-scsi
In-Reply-To: <cover.1750511018.git.legion@kernel.org>
From: Masahiro Yamada <masahiroy@kernel.org>
Generate the intermediate vmlinux.unstripped regardless of
CONFIG_ARCH_VMLINUX_NEEDS_RELOCS.
If CONFIG_ARCH_VMLINUX_NEEDS_RELOCS is unset, vmlinux.unstripped and
vmlinux are identiacal.
This simplifies the build rule, and allows to strip more sections
by adding them to remove-section-y.
Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---
scripts/Makefile.vmlinux | 45 ++++++++++++++++++++--------------------
1 file changed, 22 insertions(+), 23 deletions(-)
diff --git a/scripts/Makefile.vmlinux b/scripts/Makefile.vmlinux
index b64862dc6f08..4f2d4c3fb737 100644
--- a/scripts/Makefile.vmlinux
+++ b/scripts/Makefile.vmlinux
@@ -9,20 +9,6 @@ include $(srctree)/scripts/Makefile.lib
targets :=
-ifdef CONFIG_ARCH_VMLINUX_NEEDS_RELOCS
-vmlinux-final := vmlinux.unstripped
-
-quiet_cmd_strip_relocs = RSTRIP $@
- cmd_strip_relocs = $(OBJCOPY) --remove-section='.rel*' --remove-section=!'.rel*.dyn' $< $@
-
-vmlinux: $(vmlinux-final) FORCE
- $(call if_changed,strip_relocs)
-
-targets += vmlinux
-else
-vmlinux-final := vmlinux
-endif
-
%.o: %.c FORCE
$(call if_changed_rule,cc_o_c)
@@ -61,19 +47,19 @@ targets += .builtin-dtbs-list
ifdef CONFIG_GENERIC_BUILTIN_DTB
targets += .builtin-dtbs.S .builtin-dtbs.o
-$(vmlinux-final): .builtin-dtbs.o
+vmlinux.unstripped: .builtin-dtbs.o
endif
-# vmlinux
+# vmlinux.unstripped
# ---------------------------------------------------------------------------
ifdef CONFIG_MODULES
targets += .vmlinux.export.o
-$(vmlinux-final): .vmlinux.export.o
+vmlinux.unstripped: .vmlinux.export.o
endif
ifdef CONFIG_ARCH_WANTS_PRE_LINK_VMLINUX
-$(vmlinux-final): arch/$(SRCARCH)/tools/vmlinux.arch.o
+vmlinux.unstripped: arch/$(SRCARCH)/tools/vmlinux.arch.o
arch/$(SRCARCH)/tools/vmlinux.arch.o: vmlinux.o FORCE
$(Q)$(MAKE) $(build)=arch/$(SRCARCH)/tools $@
@@ -86,17 +72,30 @@ cmd_link_vmlinux = \
$< "$(LD)" "$(KBUILD_LDFLAGS)" "$(LDFLAGS_vmlinux)" "$@"; \
$(if $(ARCH_POSTLINK), $(MAKE) -f $(ARCH_POSTLINK) $@, true)
-targets += $(vmlinux-final)
-$(vmlinux-final): scripts/link-vmlinux.sh vmlinux.o $(KBUILD_LDS) FORCE
+targets += vmlinux.unstripped
+vmlinux.unstripped: scripts/link-vmlinux.sh vmlinux.o $(KBUILD_LDS) FORCE
+$(call if_changed_dep,link_vmlinux)
ifdef CONFIG_DEBUG_INFO_BTF
-$(vmlinux-final): $(RESOLVE_BTFIDS)
+vmlinux.unstripped: $(RESOLVE_BTFIDS)
endif
ifdef CONFIG_BUILDTIME_TABLE_SORT
-$(vmlinux-final): scripts/sorttable
+vmlinux.unstripped: scripts/sorttable
endif
+# vmlinux
+# ---------------------------------------------------------------------------
+
+remove-section-y :=
+remove-section-$(CONFIG_ARCH_VMLINUX_NEEDS_RELOCS) += '.rel*'
+
+quiet_cmd_strip_relocs = OBJCOPY $@
+ cmd_strip_relocs = $(OBJCOPY) $(addprefix --remove-section=,$(remove-section-y)) $< $@
+
+targets += vmlinux
+vmlinux: vmlinux.unstripped FORCE
+ $(call if_changed,strip_relocs)
+
# modules.builtin.ranges
# ---------------------------------------------------------------------------
ifdef CONFIG_BUILTIN_MODULE_RANGES
@@ -110,7 +109,7 @@ modules.builtin.ranges: $(srctree)/scripts/generate_builtin_ranges.awk \
modules.builtin vmlinux.map vmlinux.o.map FORCE
$(call if_changed,modules_builtin_ranges)
-vmlinux.map: $(vmlinux-final)
+vmlinux.map: vmlinux.unstripped
@:
endif
--
2.49.0
^ permalink raw reply related
* [PATCH v4 3/7] kbuild: keep .modinfo section in vmlinux.unstripped
From: Alexey Gladkov @ 2025-06-21 13:57 UTC (permalink / raw)
To: Masahiro Yamada, Petr Pavlu, Luis Chamberlain, Sami Tolvanen,
Daniel Gomez, Nathan Chancellor, Nicolas Schier
Cc: linux-kernel, linux-modules, linux-kbuild, linux-scsi
In-Reply-To: <cover.1750511018.git.legion@kernel.org>
From: Masahiro Yamada <masahiroy@kernel.org>
Keep the .modinfo section during linking, but strip it from the final
vmlinux.
Adjust scripts/mksysmap to exclude modinfo symbols from kallsyms.
This change will allow the next commit to extract the .modinfo section
from the vmlinux.unstripped intermediate.
Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---
include/asm-generic/vmlinux.lds.h | 2 +-
scripts/Makefile.vmlinux | 2 +-
scripts/mksysmap | 3 +++
3 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index fa5f19b8d53a..1791665006f9 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -831,6 +831,7 @@ defined(CONFIG_AUTOFDO_CLANG) || defined(CONFIG_PROPELLER_CLANG)
/* Required sections not related to debugging. */
#define ELF_DETAILS \
+ .modinfo : { *(.modinfo) } \
.comment 0 : { *(.comment) } \
.symtab 0 : { *(.symtab) } \
.strtab 0 : { *(.strtab) } \
@@ -1044,7 +1045,6 @@ defined(CONFIG_AUTOFDO_CLANG) || defined(CONFIG_PROPELLER_CLANG)
*(.discard.*) \
*(.export_symbol) \
*(.no_trim_symbol) \
- *(.modinfo) \
/* ld.bfd warns about .gnu.version* even when not emitted */ \
*(.gnu.version*) \
diff --git a/scripts/Makefile.vmlinux b/scripts/Makefile.vmlinux
index 4f2d4c3fb737..e2ceeb9e168d 100644
--- a/scripts/Makefile.vmlinux
+++ b/scripts/Makefile.vmlinux
@@ -86,7 +86,7 @@ endif
# vmlinux
# ---------------------------------------------------------------------------
-remove-section-y :=
+remove-section-y := .modinfo
remove-section-$(CONFIG_ARCH_VMLINUX_NEEDS_RELOCS) += '.rel*'
quiet_cmd_strip_relocs = OBJCOPY $@
diff --git a/scripts/mksysmap b/scripts/mksysmap
index 3accbdb269ac..a607a0059d11 100755
--- a/scripts/mksysmap
+++ b/scripts/mksysmap
@@ -79,6 +79,9 @@
/ _SDA_BASE_$/d
/ _SDA2_BASE_$/d
+# MODULE_INFO()
+/ __UNIQUE_ID_modinfo[0-9]*$/d
+
# ---------------------------------------------------------------------------
# Ignored patterns
# (symbols that contain the pattern are ignored)
--
2.49.0
^ permalink raw reply related
* [PATCH v4 4/7] kbuild: extract modules.builtin.modinfo from vmlinux.unstripped
From: Alexey Gladkov @ 2025-06-21 13:57 UTC (permalink / raw)
To: Masahiro Yamada, Petr Pavlu, Luis Chamberlain, Sami Tolvanen,
Daniel Gomez, Nathan Chancellor, Nicolas Schier
Cc: linux-kernel, linux-modules, linux-kbuild, linux-scsi,
Alexey Gladkov
In-Reply-To: <cover.1750511018.git.legion@kernel.org>
From: Masahiro Yamada <masahiroy@kernel.org>
Currently, we assume all the data for modules.builtin.modinfo are
available in vmlinux.o.
This makes it impossible for modpost, which is invoked after vmlinux.o,
to add additional module info.
This commit moves the modules.builtin.modinfo rule after modpost.
Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
Signed-off-by: Alexey Gladkov <legion@kernel.org>
---
scripts/Makefile.vmlinux | 26 ++++++++++++++++++++++++++
scripts/Makefile.vmlinux_o | 26 +-------------------------
2 files changed, 27 insertions(+), 25 deletions(-)
diff --git a/scripts/Makefile.vmlinux b/scripts/Makefile.vmlinux
index e2ceeb9e168d..fdab5aa90215 100644
--- a/scripts/Makefile.vmlinux
+++ b/scripts/Makefile.vmlinux
@@ -96,6 +96,32 @@ targets += vmlinux
vmlinux: vmlinux.unstripped FORCE
$(call if_changed,strip_relocs)
+# modules.builtin.modinfo
+# ---------------------------------------------------------------------------
+
+OBJCOPYFLAGS_modules.builtin.modinfo := -j .modinfo -O binary
+
+targets += modules.builtin.modinfo
+modules.builtin.modinfo: vmlinux.unstripped FORCE
+ $(call if_changed,objcopy)
+
+# modules.builtin
+# ---------------------------------------------------------------------------
+
+__default: modules.builtin
+
+# The second line aids cases where multiple modules share the same object.
+
+quiet_cmd_modules_builtin = GEN $@
+ cmd_modules_builtin = \
+ tr '\0' '\n' < $< | \
+ sed -n 's/^[[:alnum:]:_]*\.file=//p' | \
+ tr ' ' '\n' | uniq | sed -e 's:^:kernel/:' -e 's/$$/.ko/' > $@
+
+targets += modules.builtin
+modules.builtin: modules.builtin.modinfo FORCE
+ $(call if_changed,modules_builtin)
+
# modules.builtin.ranges
# ---------------------------------------------------------------------------
ifdef CONFIG_BUILTIN_MODULE_RANGES
diff --git a/scripts/Makefile.vmlinux_o b/scripts/Makefile.vmlinux_o
index b024ffb3e201..23c8751285d7 100644
--- a/scripts/Makefile.vmlinux_o
+++ b/scripts/Makefile.vmlinux_o
@@ -1,7 +1,7 @@
# SPDX-License-Identifier: GPL-2.0-only
PHONY := __default
-__default: vmlinux.o modules.builtin.modinfo modules.builtin
+__default: vmlinux.o
include include/config/auto.conf
include $(srctree)/scripts/Kbuild.include
@@ -73,30 +73,6 @@ vmlinux.o: $(initcalls-lds) vmlinux.a $(KBUILD_VMLINUX_LIBS) FORCE
targets += vmlinux.o
-# modules.builtin.modinfo
-# ---------------------------------------------------------------------------
-
-OBJCOPYFLAGS_modules.builtin.modinfo := -j .modinfo -O binary
-
-targets += modules.builtin.modinfo
-modules.builtin.modinfo: vmlinux.o FORCE
- $(call if_changed,objcopy)
-
-# modules.builtin
-# ---------------------------------------------------------------------------
-
-# The second line aids cases where multiple modules share the same object.
-
-quiet_cmd_modules_builtin = GEN $@
- cmd_modules_builtin = \
- tr '\0' '\n' < $< | \
- sed -n 's/^[[:alnum:]:_]*\.file=//p' | \
- tr ' ' '\n' | uniq | sed -e 's:^:kernel/:' -e 's/$$/.ko/' > $@
-
-targets += modules.builtin
-modules.builtin: modules.builtin.modinfo FORCE
- $(call if_changed,modules_builtin)
-
# Add FORCE to the prerequisites of a target to force it to be always rebuilt.
# ---------------------------------------------------------------------------
--
2.49.0
^ permalink raw reply related
* [PATCH v4 5/7] scsi: Always define blogic_pci_tbl structure
From: Alexey Gladkov @ 2025-06-21 13:57 UTC (permalink / raw)
To: Masahiro Yamada, Petr Pavlu, Luis Chamberlain, Sami Tolvanen,
Daniel Gomez, Nathan Chancellor, Nicolas Schier
Cc: linux-kernel, linux-modules, linux-kbuild, linux-scsi,
Alexey Gladkov, Khalid Aziz, Martin K. Petersen, James Bottomley
In-Reply-To: <cover.1750511018.git.legion@kernel.org>
The blogic_pci_tbl structure is used by the MODULE_DEVICE_TABLE macro.
There is no longer a need to protect it with the MODULE condition, since
this no longer causes the compiler to warn about an unused variable.
Cc: Khalid Aziz <khalid@gonehiking.org>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Suggested-by: James Bottomley <James.Bottomley@HansenPartnership.com>
Signed-off-by: Alexey Gladkov <legion@kernel.org>
---
drivers/scsi/BusLogic.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/scsi/BusLogic.c b/drivers/scsi/BusLogic.c
index 1f100270cd38..08e12a3d6703 100644
--- a/drivers/scsi/BusLogic.c
+++ b/drivers/scsi/BusLogic.c
@@ -3715,7 +3715,6 @@ static void __exit blogic_exit(void)
__setup("BusLogic=", blogic_setup);
-#ifdef MODULE
/*static const struct pci_device_id blogic_pci_tbl[] = {
{ PCI_VENDOR_ID_BUSLOGIC, PCI_DEVICE_ID_BUSLOGIC_MULTIMASTER,
PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0},
@@ -3731,7 +3730,6 @@ static const struct pci_device_id blogic_pci_tbl[] = {
{PCI_DEVICE(PCI_VENDOR_ID_BUSLOGIC, PCI_DEVICE_ID_BUSLOGIC_FLASHPOINT)},
{0, },
};
-#endif
MODULE_DEVICE_TABLE(pci, blogic_pci_tbl);
module_init(blogic_init);
--
2.49.0
^ permalink raw reply related
* [PATCH v4 6/7] modpost: Add modname to mod_device_table alias
From: Alexey Gladkov @ 2025-06-21 13:57 UTC (permalink / raw)
To: Masahiro Yamada, Petr Pavlu, Luis Chamberlain, Sami Tolvanen,
Daniel Gomez, Nathan Chancellor, Nicolas Schier
Cc: linux-kernel, linux-modules, linux-kbuild, linux-scsi,
Alexey Gladkov, Miguel Ojeda, Alex Gaynor
In-Reply-To: <cover.1750511018.git.legion@kernel.org>
At this point, if a symbol is compiled as part of the kernel,
information about which module the symbol belongs to is lost.
To save this it is possible to add the module name to the alias name.
It's not very pretty, but it's possible for now.
Cc: Miguel Ojeda <ojeda@kernel.org>
Cc: Alex Gaynor <alex.gaynor@gmail.com>
Signed-off-by: Alexey Gladkov <legion@kernel.org>
---
include/linux/module.h | 14 +++++++++++---
rust/kernel/device_id.rs | 8 ++++----
scripts/mod/file2alias.c | 18 ++++++++++++++----
3 files changed, 29 insertions(+), 11 deletions(-)
diff --git a/include/linux/module.h b/include/linux/module.h
index 24fe6b865e9c..e0f826fab2ac 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -243,11 +243,19 @@ struct module_kobject *lookup_or_create_module_kobject(const char *name);
/* What your module does. */
#define MODULE_DESCRIPTION(_description) MODULE_INFO(description, _description)
+/* Format: __mod_device_table__kmod_<modname>__<type>__<name> */
+#define __mod_device_table(type, name) \
+ __PASTE(__mod_device_table__, \
+ __PASTE(__KBUILD_MODNAME, \
+ __PASTE(__, \
+ __PASTE(type, \
+ __PASTE(__, name)))))
+
#ifdef MODULE
/* Creates an alias so file2alias.c can find device table. */
-#define MODULE_DEVICE_TABLE(type, name) \
-extern typeof(name) __mod_device_table__##type##__##name \
- __attribute__ ((unused, alias(__stringify(name))))
+#define MODULE_DEVICE_TABLE(type, name) \
+static typeof(name) __mod_device_table(type, name) \
+ __attribute__ ((used, alias(__stringify(name))))
#else /* !MODULE */
#define MODULE_DEVICE_TABLE(type, name)
#endif
diff --git a/rust/kernel/device_id.rs b/rust/kernel/device_id.rs
index 0a4eb56d98f2..365d8f544844 100644
--- a/rust/kernel/device_id.rs
+++ b/rust/kernel/device_id.rs
@@ -154,10 +154,10 @@ macro_rules! module_device_table {
($table_type: literal, $module_table_name:ident, $table_name:ident) => {
#[rustfmt::skip]
#[export_name =
- concat!("__mod_device_table__", $table_type,
- "__", module_path!(),
- "_", line!(),
- "_", stringify!($table_name))
+ concat!("__mod_device_table__", line!(),
+ "__kmod_", module_path!(),
+ "__", $table_type,
+ "__", stringify!($table_name))
]
static $module_table_name: [::core::mem::MaybeUninit<u8>; $table_name.raw_ids().size()] =
unsafe { ::core::mem::transmute_copy($table_name.raw_ids()) };
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index 00586119a25b..13021266a18f 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -1476,8 +1476,8 @@ void handle_moddevtable(struct module *mod, struct elf_info *info,
{
void *symval;
char *zeros = NULL;
- const char *type, *name;
- size_t typelen;
+ const char *type, *name, *modname;
+ size_t typelen, modnamelen;
static const char *prefix = "__mod_device_table__";
/* We're looking for a section relative symbol */
@@ -1488,10 +1488,20 @@ void handle_moddevtable(struct module *mod, struct elf_info *info,
if (ELF_ST_TYPE(sym->st_info) != STT_OBJECT)
return;
- /* All our symbols are of form __mod_device_table__<type>__<name>. */
+ /* All our symbols are of form __mod_device_table__kmod_<modname>__<type>__<name>. */
if (!strstarts(symname, prefix))
return;
- type = symname + strlen(prefix);
+
+ modname = strstr(symname, "__kmod_");
+ if (!modname)
+ return;
+ modname += strlen("__kmod_");
+
+ type = strstr(modname, "__");
+ if (!type)
+ return;
+ modnamelen = type - modname;
+ type += strlen("__");
name = strstr(type, "__");
if (!name)
--
2.49.0
^ permalink raw reply related
* [PATCH v4 7/7] modpost: Create modalias for builtin modules
From: Alexey Gladkov @ 2025-06-21 13:57 UTC (permalink / raw)
To: Masahiro Yamada, Petr Pavlu, Luis Chamberlain, Sami Tolvanen,
Daniel Gomez, Nathan Chancellor, Nicolas Schier
Cc: linux-kernel, linux-modules, linux-kbuild, linux-scsi,
Alexey Gladkov
In-Reply-To: <cover.1750511018.git.legion@kernel.org>
For some modules, modalias is generated using the modpost utility and
the section is added to the module file.
When a module is added inside vmlinux, modpost does not generate
modalias for such modules and the information is lost.
As a result kmod (which uses modules.builtin.modinfo in userspace)
cannot determine that modalias is handled by a builtin kernel module.
$ cat /sys/devices/pci0000:00/0000:00:14.0/modalias
pci:v00008086d0000A36Dsv00001043sd00008694bc0Csc03i30
$ modinfo xhci_pci
name: xhci_pci
filename: (builtin)
license: GPL
file: drivers/usb/host/xhci-pci
description: xHCI PCI Host Controller Driver
Missing modalias "pci:v*d*sv*sd*bc0Csc03i30*" which will be generated by
modpost if the module is built separately.
To fix this it is necessary to generate the same modalias for vmlinux as
for the individual modules. Fortunately '.vmlinux.export.o' is already
generated from which '.modinfo' can be extracted in the same way as for
vmlinux.o.
Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
Signed-off-by: Alexey Gladkov <legion@kernel.org>
---
include/linux/module.h | 4 ----
scripts/Makefile.vmlinux | 5 ++++-
scripts/mksysmap | 3 +++
scripts/mod/file2alias.c | 16 ++++++++++++++++
scripts/mod/modpost.c | 17 ++++++++++++++++-
scripts/mod/modpost.h | 2 ++
6 files changed, 41 insertions(+), 6 deletions(-)
diff --git a/include/linux/module.h b/include/linux/module.h
index e0f826fab2ac..3aeee5d0a77c 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -251,14 +251,10 @@ struct module_kobject *lookup_or_create_module_kobject(const char *name);
__PASTE(type, \
__PASTE(__, name)))))
-#ifdef MODULE
/* Creates an alias so file2alias.c can find device table. */
#define MODULE_DEVICE_TABLE(type, name) \
static typeof(name) __mod_device_table(type, name) \
__attribute__ ((used, alias(__stringify(name))))
-#else /* !MODULE */
-#define MODULE_DEVICE_TABLE(type, name)
-#endif
/* Version of form [<epoch>:]<version>[-<extra-version>].
* Or for CVS/RCS ID version, everything but the number is stripped.
diff --git a/scripts/Makefile.vmlinux b/scripts/Makefile.vmlinux
index fdab5aa90215..fcc188d26ead 100644
--- a/scripts/Makefile.vmlinux
+++ b/scripts/Makefile.vmlinux
@@ -89,8 +89,11 @@ endif
remove-section-y := .modinfo
remove-section-$(CONFIG_ARCH_VMLINUX_NEEDS_RELOCS) += '.rel*'
+remove-symbols := -w --strip-symbol='__mod_device_table__*'
+
quiet_cmd_strip_relocs = OBJCOPY $@
- cmd_strip_relocs = $(OBJCOPY) $(addprefix --remove-section=,$(remove-section-y)) $< $@
+ cmd_strip_relocs = $(OBJCOPY) $(addprefix --remove-section=,$(remove-section-y)) \
+ $(remove-symbols) $< $@
targets += vmlinux
vmlinux: vmlinux.unstripped FORCE
diff --git a/scripts/mksysmap b/scripts/mksysmap
index a607a0059d11..c4531eacde20 100755
--- a/scripts/mksysmap
+++ b/scripts/mksysmap
@@ -59,6 +59,9 @@
# EXPORT_SYMBOL (namespace)
/ __kstrtabns_/d
+# MODULE_DEVICE_TABLE (symbol name)
+/ __mod_device_table__/d
+
# ---------------------------------------------------------------------------
# Ignored suffixes
# (do not forget '$' after each pattern)
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index 13021266a18f..7da9735e7ab3 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -1527,5 +1527,21 @@ void handle_moddevtable(struct module *mod, struct elf_info *info,
}
}
+ if (mod->is_vmlinux) {
+ struct module_alias *alias;
+
+ /*
+ * If this is vmlinux, record the name of the builtin module.
+ * Traverse the linked list in the reverse order, and set the
+ * builtin_modname unless it has already been set in the
+ * previous call.
+ */
+ list_for_each_entry_reverse(alias, &mod->aliases, node) {
+ if (alias->builtin_modname)
+ break;
+ alias->builtin_modname = xstrndup(modname, modnamelen);
+ }
+ }
+
free(zeros);
}
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index be89921d60b6..67668b159444 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -2021,11 +2021,26 @@ static void write_if_changed(struct buffer *b, const char *fname)
static void write_vmlinux_export_c_file(struct module *mod)
{
struct buffer buf = { };
+ struct module_alias *alias, *next;
buf_printf(&buf,
- "#include <linux/export-internal.h>\n");
+ "#include <linux/export-internal.h>\n"
+ "#include <linux/module.h>\n");
add_exported_symbols(&buf, mod);
+
+ buf_printf(&buf,
+ "#undef __MODULE_INFO_PREFIX\n"
+ "#define __MODULE_INFO_PREFIX\n");
+
+ list_for_each_entry_safe(alias, next, &mod->aliases, node) {
+ buf_printf(&buf, "MODULE_INFO(%s.alias, \"%s\");\n",
+ alias->builtin_modname, alias->str);
+ list_del(&alias->node);
+ free(alias->builtin_modname);
+ free(alias);
+ }
+
write_if_changed(&buf, ".vmlinux.export.c");
free(buf.p);
}
diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
index 9133e4c3803f..2aecb8f25c87 100644
--- a/scripts/mod/modpost.h
+++ b/scripts/mod/modpost.h
@@ -99,10 +99,12 @@ buf_write(struct buffer *buf, const char *s, int len);
* struct module_alias - auto-generated MODULE_ALIAS()
*
* @node: linked to module::aliases
+ * @modname: name of the builtin module (only for vmlinux)
* @str: a string for MODULE_ALIAS()
*/
struct module_alias {
struct list_head node;
+ char *builtin_modname;
char str[];
};
--
2.49.0
^ permalink raw reply related
* Re: [PATCH v4 6/7] modpost: Add modname to mod_device_table alias
From: Miguel Ojeda @ 2025-06-21 14:48 UTC (permalink / raw)
To: Alexey Gladkov
Cc: Masahiro Yamada, Petr Pavlu, Luis Chamberlain, Sami Tolvanen,
Daniel Gomez, Nathan Chancellor, Nicolas Schier, linux-kernel,
linux-modules, linux-kbuild, linux-scsi, Miguel Ojeda,
Alex Gaynor, Greg Kroah-Hartman, Rafael J. Wysocki,
Danilo Krummrich, rust-for-linux
In-Reply-To: <6e2f70b07a710e761eb68d089d96cee7b27bb2d5.1750511018.git.legion@kernel.org>
On Sat, Jun 21, 2025 at 3:57 PM Alexey Gladkov <legion@kernel.org> wrote:
>
> rust/kernel/device_id.rs | 8 ++++----
Cc'ing maintainers and list.
Cheers,
Miguel
^ permalink raw reply
* Re: [PATCH v4 6/7] modpost: Add modname to mod_device_table alias
From: Miguel Ojeda @ 2025-06-21 15:20 UTC (permalink / raw)
To: Alexey Gladkov, FUJITA Tomonori
Cc: Masahiro Yamada, Petr Pavlu, Luis Chamberlain, Sami Tolvanen,
Daniel Gomez, Nathan Chancellor, Nicolas Schier, linux-kernel,
linux-modules, linux-kbuild, linux-scsi, Miguel Ojeda,
Alex Gaynor, Greg KH, Rafael J. Wysocki, Danilo Krummrich
In-Reply-To: <6e2f70b07a710e761eb68d089d96cee7b27bb2d5.1750511018.git.legion@kernel.org>
On Sat, Jun 21, 2025 at 3:57 PM Alexey Gladkov <legion@kernel.org> wrote:
>
> +/* Format: __mod_device_table__kmod_<modname>__<type>__<name> */
Should we mention that `__kmod_` and `__` will be the search strings,
or otherwise the Rust formatting (i.e. that is carries a line etc.)?
Cc'ing Tomo: do we need an update on `rust/kernel/net/phy.rs`? Should
we factor out the formatting?
Thanks!
Cheers,
Miguel
^ permalink raw reply
* Re: [PATCH v4 6/7] modpost: Add modname to mod_device_table alias
From: Alexey Gladkov @ 2025-06-21 16:10 UTC (permalink / raw)
To: Miguel Ojeda
Cc: FUJITA Tomonori, Masahiro Yamada, Petr Pavlu, Luis Chamberlain,
Sami Tolvanen, Daniel Gomez, Nathan Chancellor, Nicolas Schier,
linux-kernel, linux-modules, linux-kbuild, linux-scsi,
Miguel Ojeda, Alex Gaynor, Greg KH, Rafael J. Wysocki,
Danilo Krummrich
In-Reply-To: <CANiq72k+ojA3=JfwhvjZ_=+uGBG-WmhLOigzPUXdoY8VQXbf=A@mail.gmail.com>
On Sat, Jun 21, 2025 at 05:20:49PM +0200, Miguel Ojeda wrote:
> On Sat, Jun 21, 2025 at 3:57 PM Alexey Gladkov <legion@kernel.org> wrote:
> >
> > +/* Format: __mod_device_table__kmod_<modname>__<type>__<name> */
>
> Should we mention that `__kmod_` and `__` will be the search strings,
> or otherwise the Rust formatting (i.e. that is carries a line etc.)?
Ok. Make sense.
Basically there's a comment for the MODULE_DEVICE_TABLE macro that tells
where and how that symbol is parsed.
> Cc'ing Tomo: do we need an update on `rust/kernel/net/phy.rs`? Should
> we factor out the formatting?
>
> Thanks!
>
> Cheers,
> Miguel
>
--
Rgrds, legion
^ permalink raw reply
* Re: [PATCH v4 6/7] modpost: Add modname to mod_device_table alias
From: FUJITA Tomonori @ 2025-06-23 6:15 UTC (permalink / raw)
To: miguel.ojeda.sandonis
Cc: legion, fujita.tomonori, masahiroy, petr.pavlu, mcgrof,
samitolvanen, da.gomez, nathan, nicolas.schier, linux-kernel,
linux-modules, linux-kbuild, linux-scsi, ojeda, alex.gaynor,
gregkh, rafael, dakr, rust-for-linux
In-Reply-To: <CANiq72k+ojA3=JfwhvjZ_=+uGBG-WmhLOigzPUXdoY8VQXbf=A@mail.gmail.com>
On Sat, 21 Jun 2025 17:20:49 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> On Sat, Jun 21, 2025 at 3:57 PM Alexey Gladkov <legion@kernel.org> wrote:
>>
>> +/* Format: __mod_device_table__kmod_<modname>__<type>__<name> */
>
> Should we mention that `__kmod_` and `__` will be the search strings,
> or otherwise the Rust formatting (i.e. that is carries a line etc.)?
>
> Cc'ing Tomo: do we need an update on `rust/kernel/net/phy.rs`? Should
> we factor out the formatting?
Yeah, looks like the update is necessary. Thanks for the heads-up.
I've just send a patchset to convert the PHY abstractions to use
device_id crate:
https://lore.kernel.org/lkml/20250623060951.118564-1-fujita.tomonori@gmail.com/
^ permalink raw reply
* Re: [PATCH v13 2/6] rust: introduce module_param module
From: Andreas Hindborg @ 2025-06-23 9:44 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: <DARCZYNPIJVZ.3JJSZ6PSAEMEC@kernel.org>
"Benno Lossin" <lossin@kernel.org> writes:
> On Fri Jun 20, 2025 at 1:29 PM CEST, Andreas Hindborg wrote:
>> "Benno Lossin" <lossin@kernel.org> writes:
>>> On Thu Jun 12, 2025 at 3:40 PM CEST, Andreas Hindborg wrote:
>>>> +/// A wrapper for kernel parameters.
>>>> +///
>>>> +/// This type is instantiated by the [`module!`] macro when module parameters are
>>>> +/// defined. You should never need to instantiate this type directly.
>>>> +///
>>>> +/// Note: This type is `pub` because it is used by module crates to access
>>>> +/// parameter values.
>>>> +#[repr(transparent)]
>>>> +pub struct ModuleParamAccess<T> {
>>>> + data: core::cell::UnsafeCell<T>,
>>>> +}
>>>> +
>>>> +// SAFETY: We only create shared references to the contents of this container,
>>>> +// so if `T` is `Sync`, so is `ModuleParamAccess`.
>>>> +unsafe impl<T: Sync> Sync for ModuleParamAccess<T> {}
>>>> +
>>>> +impl<T> ModuleParamAccess<T> {
>>>> + #[doc(hidden)]
>>>> + pub const fn new(value: T) -> Self {
>>>> + Self {
>>>> + data: core::cell::UnsafeCell::new(value),
>>>> + }
>>>> + }
>>>> +
>>>> + /// Get a shared reference to the parameter value.
>>>> + // Note: When sysfs access to parameters are enabled, we have to pass in a
>>>> + // held lock guard here.
>>>> + pub fn get(&self) -> &T {
>>>> + // SAFETY: As we only support read only parameters with no sysfs
>>>> + // exposure, the kernel will not touch the parameter data after module
>>>> + // initialization.
>>>
>>> This should be a type invariant. But I'm having difficulty defining one
>>> that's actually correct: after parsing the parameter, this is written
>>> to, but when is that actually?
>>
>> For built-in modules it is during kernel initialization. For loadable
>> modules, it during module load. No code from the module will execute
>> before parameters are set.
>
> Gotcha and there never ever will be custom code that is executed
> before/during parameter setting (so code aside from code in `kernel`)?
>
>>> Would we eventually execute other Rust
>>> code during that time? (for example when we allow custom parameter
>>> parsing)
>>
>> I don't think we will need to synchronize because of custom parameter
>> parsing. Parameters are initialized sequentially. It is not a problem if
>> the custom parameter parsing code name other parameters, because they
>> are all initialized to valid values (as they are statics).
>
> If you have `&'static i64`, then the value at that reference is never
> allowed to change.
>
>>> This function also must never be `const` because of the following:
>>>
>>> module! {
>>> // ...
>>> params: {
>>> my_param: i64 {
>>> default: 0,
>>> description: "",
>>> },
>>> },
>>> }
>>>
>>> static BAD: &'static i64 = module_parameters::my_param.get();
>>>
>>> AFAIK, this static will be executed before loading module parameters and
>>> thus it makes writing to the parameter UB.
>>
>> As I understand, the static will be initialized by a constant expression
>> evaluated at compile time. I am not sure what happens when this is
>> evaluated in const context:
>>
>> pub fn get(&self) -> &T {
>> // SAFETY: As we only support read only parameters with no sysfs
>> // exposure, the kernel will not touch the parameter data after module
>> // initialization.
>> unsafe { &*self.data.get() }
>> }
>>
>> Why would that not be OK? I would assume the compiler builds a dependency graph
>> when initializing statics?
>
> Yes it builds a dependency graph, but that is irrelevant? The problem is
> that I can create a `'static` reference to the inner value *before* the
> parameter is written-to (as the static is initialized before the
> parameters).
I see, I did not consider this situation. Thanks for pointing this out.
Could we get around this without a lock maybe? If we change
`ModuleParamAccess::get` to take a closure instead:
/// Call `func` with a reference to the parameter value stored in `Self`.
pub fn read(&self, func: impl FnOnce(&T)) {
// SAFETY: As we only support read only parameters with no sysfs
// exposure, the kernel will not touch the parameter data after module
// initialization.
let data = unsafe { &*self.data.get() };
func(data)
}
I think this would bound the lifetime of the reference passed to the
closure to the duration of the call, right?
Best regards,
Andreas Hindborg
^ permalink raw reply
* Re: [PATCH v13 2/6] rust: introduce module_param module
From: Andreas Hindborg @ 2025-06-23 9:47 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: <DARCZYNPIJVZ.3JJSZ6PSAEMEC@kernel.org>
"Benno Lossin" <lossin@kernel.org> writes:
> On Fri Jun 20, 2025 at 1:29 PM CEST, Andreas Hindborg wrote:
>> "Benno Lossin" <lossin@kernel.org> writes:
>>> On Thu Jun 12, 2025 at 3:40 PM CEST, Andreas Hindborg wrote:
>>>> +/// A wrapper for kernel parameters.
>>>> +///
>>>> +/// This type is instantiated by the [`module!`] macro when module parameters are
>>>> +/// defined. You should never need to instantiate this type directly.
>>>> +///
>>>> +/// Note: This type is `pub` because it is used by module crates to access
>>>> +/// parameter values.
>>>> +#[repr(transparent)]
>>>> +pub struct ModuleParamAccess<T> {
>>>> + data: core::cell::UnsafeCell<T>,
>>>> +}
>>>> +
>>>> +// SAFETY: We only create shared references to the contents of this container,
>>>> +// so if `T` is `Sync`, so is `ModuleParamAccess`.
>>>> +unsafe impl<T: Sync> Sync for ModuleParamAccess<T> {}
>>>> +
>>>> +impl<T> ModuleParamAccess<T> {
>>>> + #[doc(hidden)]
>>>> + pub const fn new(value: T) -> Self {
>>>> + Self {
>>>> + data: core::cell::UnsafeCell::new(value),
>>>> + }
>>>> + }
>>>> +
>>>> + /// Get a shared reference to the parameter value.
>>>> + // Note: When sysfs access to parameters are enabled, we have to pass in a
>>>> + // held lock guard here.
>>>> + pub fn get(&self) -> &T {
>>>> + // SAFETY: As we only support read only parameters with no sysfs
>>>> + // exposure, the kernel will not touch the parameter data after module
>>>> + // initialization.
>>>
>>> This should be a type invariant. But I'm having difficulty defining one
>>> that's actually correct: after parsing the parameter, this is written
>>> to, but when is that actually?
>>
>> For built-in modules it is during kernel initialization. For loadable
>> modules, it during module load. No code from the module will execute
>> before parameters are set.
>
> Gotcha and there never ever will be custom code that is executed
> before/during parameter setting (so code aside from code in `kernel`)?
Not with the parameter parsers we provide now. In the case of custom
parsing code, I suppose there is nothing preventing the parsing code
from spinning up a thread that could do stuff while more parameters are
initialized by the kernel.
Best regards,
Andreas Hindborg
^ permalink raw reply
* Re: [PATCH v13 2/6] rust: introduce module_param module
From: Benno Lossin @ 2025-06-23 11:48 UTC (permalink / raw)
To: Andreas Hindborg
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Nicolas Schier, Trevor Gross, Adam Bratschi-Kaye, rust-for-linux,
linux-kernel, linux-kbuild, Petr Pavlu, Sami Tolvanen,
Daniel Gomez, Simona Vetter, Greg KH, Fiona Behrens,
Daniel Almeida, linux-modules
In-Reply-To: <877c126bce.fsf@kernel.org>
On Mon Jun 23, 2025 at 11:44 AM CEST, Andreas Hindborg wrote:
> "Benno Lossin" <lossin@kernel.org> writes:
>
>> On Fri Jun 20, 2025 at 1:29 PM CEST, Andreas Hindborg wrote:
>>> "Benno Lossin" <lossin@kernel.org> writes:
>>>> On Thu Jun 12, 2025 at 3:40 PM CEST, Andreas Hindborg wrote:
>>>>> +/// A wrapper for kernel parameters.
>>>>> +///
>>>>> +/// This type is instantiated by the [`module!`] macro when module parameters are
>>>>> +/// defined. You should never need to instantiate this type directly.
>>>>> +///
>>>>> +/// Note: This type is `pub` because it is used by module crates to access
>>>>> +/// parameter values.
>>>>> +#[repr(transparent)]
>>>>> +pub struct ModuleParamAccess<T> {
>>>>> + data: core::cell::UnsafeCell<T>,
>>>>> +}
>>>>> +
>>>>> +// SAFETY: We only create shared references to the contents of this container,
>>>>> +// so if `T` is `Sync`, so is `ModuleParamAccess`.
>>>>> +unsafe impl<T: Sync> Sync for ModuleParamAccess<T> {}
>>>>> +
>>>>> +impl<T> ModuleParamAccess<T> {
>>>>> + #[doc(hidden)]
>>>>> + pub const fn new(value: T) -> Self {
>>>>> + Self {
>>>>> + data: core::cell::UnsafeCell::new(value),
>>>>> + }
>>>>> + }
>>>>> +
>>>>> + /// Get a shared reference to the parameter value.
>>>>> + // Note: When sysfs access to parameters are enabled, we have to pass in a
>>>>> + // held lock guard here.
>>>>> + pub fn get(&self) -> &T {
>>>>> + // SAFETY: As we only support read only parameters with no sysfs
>>>>> + // exposure, the kernel will not touch the parameter data after module
>>>>> + // initialization.
>>>>
>>>> This should be a type invariant. But I'm having difficulty defining one
>>>> that's actually correct: after parsing the parameter, this is written
>>>> to, but when is that actually?
>>>
>>> For built-in modules it is during kernel initialization. For loadable
>>> modules, it during module load. No code from the module will execute
>>> before parameters are set.
>>
>> Gotcha and there never ever will be custom code that is executed
>> before/during parameter setting (so code aside from code in `kernel`)?
>>
>>>> Would we eventually execute other Rust
>>>> code during that time? (for example when we allow custom parameter
>>>> parsing)
>>>
>>> I don't think we will need to synchronize because of custom parameter
>>> parsing. Parameters are initialized sequentially. It is not a problem if
>>> the custom parameter parsing code name other parameters, because they
>>> are all initialized to valid values (as they are statics).
>>
>> If you have `&'static i64`, then the value at that reference is never
>> allowed to change.
>>
>>>> This function also must never be `const` because of the following:
>>>>
>>>> module! {
>>>> // ...
>>>> params: {
>>>> my_param: i64 {
>>>> default: 0,
>>>> description: "",
>>>> },
>>>> },
>>>> }
>>>>
>>>> static BAD: &'static i64 = module_parameters::my_param.get();
>>>>
>>>> AFAIK, this static will be executed before loading module parameters and
>>>> thus it makes writing to the parameter UB.
>>>
>>> As I understand, the static will be initialized by a constant expression
>>> evaluated at compile time. I am not sure what happens when this is
>>> evaluated in const context:
>>>
>>> pub fn get(&self) -> &T {
>>> // SAFETY: As we only support read only parameters with no sysfs
>>> // exposure, the kernel will not touch the parameter data after module
>>> // initialization.
>>> unsafe { &*self.data.get() }
>>> }
>>>
>>> Why would that not be OK? I would assume the compiler builds a dependency graph
>>> when initializing statics?
>>
>> Yes it builds a dependency graph, but that is irrelevant? The problem is
>> that I can create a `'static` reference to the inner value *before* the
>> parameter is written-to (as the static is initialized before the
>> parameters).
>
> I see, I did not consider this situation. Thanks for pointing this out.
>
> Could we get around this without a lock maybe? If we change
> `ModuleParamAccess::get` to take a closure instead:
>
> /// Call `func` with a reference to the parameter value stored in `Self`.
> pub fn read(&self, func: impl FnOnce(&T)) {
> // SAFETY: As we only support read only parameters with no sysfs
> // exposure, the kernel will not touch the parameter data after module
> // initialization.
> let data = unsafe { &*self.data.get() };
>
> func(data)
> }
>
> I think this would bound the lifetime of the reference passed to the
> closure to the duration of the call, right?
Yes that is correct. Now you can't assign the reference to a static.
However, this API is probably very clunky to use, since you always have
to create a closure etc.
Since you mentioned in the other reply that one could spin up a thread
and do something simultaneously, I don't think this is enough. You could
have a loop spin over the new `read` function and read the value and
then the write happens.
One way to fix this issue would be to use atomics to read the value and
to not create a reference to it. So essentially have
pub fn read(&self) -> T {
unsafe { atomic_read_unsafe_cell(&self.data) }
}
Another way would be to use a `Once`-like type (does that exist on the C
side?) so a type that can be initialized once and then never changes.
While it doesn't have a value set, we return some default value for the
param and print a warning, when it's set, we just return the value. But
this probably also requires atomics...
Is parameter accessing used that often in hot paths? Can't you just copy
the value into your `Module` struct?
---
Cheers,
Benno
^ permalink raw reply
* Re: [PATCH v13 2/6] rust: introduce module_param module
From: Miguel Ojeda @ 2025-06-23 12:37 UTC (permalink / raw)
To: Benno Lossin
Cc: 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, 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: <DATW0XWNN45X.1L2WMZ41JJ5O8@kernel.org>
On Mon, Jun 23, 2025 at 1:48 PM Benno Lossin <lossin@kernel.org> wrote:
>
> Another way would be to use a `Once`-like type (does that exist on the C
> side?) so a type that can be initialized once and then never changes.
There are `DO_ONCE{,_SLEEPABLE,_LITE}`.
Cheers,
Miguel
^ permalink raw reply
* Re: [PATCH v13 2/6] rust: introduce module_param module
From: Benno Lossin @ 2025-06-23 13:55 UTC (permalink / raw)
To: Miguel Ojeda
Cc: 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, 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: <CANiq72nV3QzpRZTyDTv7Qx-c83jeayUY3nnvhpbtOYXf47EgJA@mail.gmail.com>
On Mon Jun 23, 2025 at 2:37 PM CEST, Miguel Ojeda wrote:
> On Mon, Jun 23, 2025 at 1:48 PM Benno Lossin <lossin@kernel.org> wrote:
>>
>> Another way would be to use a `Once`-like type (does that exist on the C
>> side?) so a type that can be initialized once and then never changes.
>
> There are `DO_ONCE{,_SLEEPABLE,_LITE}`.
Thanks for the pointer, it is pretty much exactly what the `Once` type
in Rust is.
It's not exactly what I'm thinking of in this case, but since it is
implemented with static key, maybe we can do something similar with a
static key? So switch the return value of the `ModuleParamAccess::read`
function depending on weather the module parameters have been written
and while they haven't just return a dummy value and WARN.
Thoughts @Andreas?
---
Cheers,
Benno
^ permalink raw reply
* Re: [PATCH v13 2/6] rust: introduce module_param module
From: Andreas Hindborg @ 2025-06-23 14:31 UTC (permalink / raw)
To: Benno Lossin
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Nicolas Schier, Trevor Gross, Adam Bratschi-Kaye, rust-for-linux,
linux-kernel, linux-kbuild, Petr Pavlu, Sami Tolvanen,
Daniel Gomez, Simona Vetter, Greg KH, Fiona Behrens,
Daniel Almeida, linux-modules
In-Reply-To: <DATW0XWNN45X.1L2WMZ41JJ5O8@kernel.org>
"Benno Lossin" <lossin@kernel.org> writes:
> On Mon Jun 23, 2025 at 11:44 AM CEST, Andreas Hindborg wrote:
>> "Benno Lossin" <lossin@kernel.org> writes:
>>
>>> On Fri Jun 20, 2025 at 1:29 PM CEST, Andreas Hindborg wrote:
>>>> "Benno Lossin" <lossin@kernel.org> writes:
>>>>> On Thu Jun 12, 2025 at 3:40 PM CEST, Andreas Hindborg wrote:
>>>>>> +/// A wrapper for kernel parameters.
>>>>>> +///
>>>>>> +/// This type is instantiated by the [`module!`] macro when module parameters are
>>>>>> +/// defined. You should never need to instantiate this type directly.
>>>>>> +///
>>>>>> +/// Note: This type is `pub` because it is used by module crates to access
>>>>>> +/// parameter values.
>>>>>> +#[repr(transparent)]
>>>>>> +pub struct ModuleParamAccess<T> {
>>>>>> + data: core::cell::UnsafeCell<T>,
>>>>>> +}
>>>>>> +
>>>>>> +// SAFETY: We only create shared references to the contents of this container,
>>>>>> +// so if `T` is `Sync`, so is `ModuleParamAccess`.
>>>>>> +unsafe impl<T: Sync> Sync for ModuleParamAccess<T> {}
>>>>>> +
>>>>>> +impl<T> ModuleParamAccess<T> {
>>>>>> + #[doc(hidden)]
>>>>>> + pub const fn new(value: T) -> Self {
>>>>>> + Self {
>>>>>> + data: core::cell::UnsafeCell::new(value),
>>>>>> + }
>>>>>> + }
>>>>>> +
>>>>>> + /// Get a shared reference to the parameter value.
>>>>>> + // Note: When sysfs access to parameters are enabled, we have to pass in a
>>>>>> + // held lock guard here.
>>>>>> + pub fn get(&self) -> &T {
>>>>>> + // SAFETY: As we only support read only parameters with no sysfs
>>>>>> + // exposure, the kernel will not touch the parameter data after module
>>>>>> + // initialization.
>>>>>
>>>>> This should be a type invariant. But I'm having difficulty defining one
>>>>> that's actually correct: after parsing the parameter, this is written
>>>>> to, but when is that actually?
>>>>
>>>> For built-in modules it is during kernel initialization. For loadable
>>>> modules, it during module load. No code from the module will execute
>>>> before parameters are set.
>>>
>>> Gotcha and there never ever will be custom code that is executed
>>> before/during parameter setting (so code aside from code in `kernel`)?
>>>
>>>>> Would we eventually execute other Rust
>>>>> code during that time? (for example when we allow custom parameter
>>>>> parsing)
>>>>
>>>> I don't think we will need to synchronize because of custom parameter
>>>> parsing. Parameters are initialized sequentially. It is not a problem if
>>>> the custom parameter parsing code name other parameters, because they
>>>> are all initialized to valid values (as they are statics).
>>>
>>> If you have `&'static i64`, then the value at that reference is never
>>> allowed to change.
>>>
>>>>> This function also must never be `const` because of the following:
>>>>>
>>>>> module! {
>>>>> // ...
>>>>> params: {
>>>>> my_param: i64 {
>>>>> default: 0,
>>>>> description: "",
>>>>> },
>>>>> },
>>>>> }
>>>>>
>>>>> static BAD: &'static i64 = module_parameters::my_param.get();
>>>>>
>>>>> AFAIK, this static will be executed before loading module parameters and
>>>>> thus it makes writing to the parameter UB.
>>>>
>>>> As I understand, the static will be initialized by a constant expression
>>>> evaluated at compile time. I am not sure what happens when this is
>>>> evaluated in const context:
>>>>
>>>> pub fn get(&self) -> &T {
>>>> // SAFETY: As we only support read only parameters with no sysfs
>>>> // exposure, the kernel will not touch the parameter data after module
>>>> // initialization.
>>>> unsafe { &*self.data.get() }
>>>> }
>>>>
>>>> Why would that not be OK? I would assume the compiler builds a dependency graph
>>>> when initializing statics?
>>>
>>> Yes it builds a dependency graph, but that is irrelevant? The problem is
>>> that I can create a `'static` reference to the inner value *before* the
>>> parameter is written-to (as the static is initialized before the
>>> parameters).
>>
>> I see, I did not consider this situation. Thanks for pointing this out.
>>
>> Could we get around this without a lock maybe? If we change
>> `ModuleParamAccess::get` to take a closure instead:
>>
>> /// Call `func` with a reference to the parameter value stored in `Self`.
>> pub fn read(&self, func: impl FnOnce(&T)) {
>> // SAFETY: As we only support read only parameters with no sysfs
>> // exposure, the kernel will not touch the parameter data after module
>> // initialization.
>> let data = unsafe { &*self.data.get() };
>>
>> func(data)
>> }
>>
>> I think this would bound the lifetime of the reference passed to the
>> closure to the duration of the call, right?
>
> Yes that is correct. Now you can't assign the reference to a static.
> However, this API is probably very clunky to use, since you always have
> to create a closure etc.
>
> Since you mentioned in the other reply that one could spin up a thread
> and do something simultaneously, I don't think this is enough. You could
> have a loop spin over the new `read` function and read the value and
> then the write happens.
Yes you are right, we have to treat it as if it could be written at any
point in time.
> One way to fix this issue would be to use atomics to read the value and
> to not create a reference to it. So essentially have
>
> pub fn read(&self) -> T {
> unsafe { atomic_read_unsafe_cell(&self.data) }
> }
That could work.
> Another way would be to use a `Once`-like type (does that exist on the C
> side?) so a type that can be initialized once and then never changes.
> While it doesn't have a value set, we return some default value for the
> param and print a warning, when it's set, we just return the value. But
> this probably also requires atomics...
I think atomic bool is not that far away. Either that, or we can lock.
> Is parameter accessing used that often in hot paths? Can't you just copy
> the value into your `Module` struct?
I don't imagine this being read in a hot path. If so, the user could
make a copy.
Best regards,
Andreas Hindborg
^ permalink raw reply
* Re: [PATCH v13 2/6] rust: introduce module_param module
From: Benno Lossin @ 2025-06-23 15:20 UTC (permalink / raw)
To: Andreas Hindborg
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Nicolas Schier, Trevor Gross, Adam Bratschi-Kaye, rust-for-linux,
linux-kernel, linux-kbuild, Petr Pavlu, Sami Tolvanen,
Daniel Gomez, Simona Vetter, Greg KH, Fiona Behrens,
Daniel Almeida, linux-modules
In-Reply-To: <87v7om4jhq.fsf@kernel.org>
On Mon Jun 23, 2025 at 4:31 PM CEST, Andreas Hindborg wrote:
> "Benno Lossin" <lossin@kernel.org> writes:
>
>> On Mon Jun 23, 2025 at 11:44 AM CEST, Andreas Hindborg wrote:
>>> "Benno Lossin" <lossin@kernel.org> writes:
>>>
>>>> On Fri Jun 20, 2025 at 1:29 PM CEST, Andreas Hindborg wrote:
>>>>> "Benno Lossin" <lossin@kernel.org> writes:
>>>>>> On Thu Jun 12, 2025 at 3:40 PM CEST, Andreas Hindborg wrote:
>>>>>>> +/// A wrapper for kernel parameters.
>>>>>>> +///
>>>>>>> +/// This type is instantiated by the [`module!`] macro when module parameters are
>>>>>>> +/// defined. You should never need to instantiate this type directly.
>>>>>>> +///
>>>>>>> +/// Note: This type is `pub` because it is used by module crates to access
>>>>>>> +/// parameter values.
>>>>>>> +#[repr(transparent)]
>>>>>>> +pub struct ModuleParamAccess<T> {
>>>>>>> + data: core::cell::UnsafeCell<T>,
>>>>>>> +}
>>>>>>> +
>>>>>>> +// SAFETY: We only create shared references to the contents of this container,
>>>>>>> +// so if `T` is `Sync`, so is `ModuleParamAccess`.
>>>>>>> +unsafe impl<T: Sync> Sync for ModuleParamAccess<T> {}
>>>>>>> +
>>>>>>> +impl<T> ModuleParamAccess<T> {
>>>>>>> + #[doc(hidden)]
>>>>>>> + pub const fn new(value: T) -> Self {
>>>>>>> + Self {
>>>>>>> + data: core::cell::UnsafeCell::new(value),
>>>>>>> + }
>>>>>>> + }
>>>>>>> +
>>>>>>> + /// Get a shared reference to the parameter value.
>>>>>>> + // Note: When sysfs access to parameters are enabled, we have to pass in a
>>>>>>> + // held lock guard here.
>>>>>>> + pub fn get(&self) -> &T {
>>>>>>> + // SAFETY: As we only support read only parameters with no sysfs
>>>>>>> + // exposure, the kernel will not touch the parameter data after module
>>>>>>> + // initialization.
>>>>>>
>>>>>> This should be a type invariant. But I'm having difficulty defining one
>>>>>> that's actually correct: after parsing the parameter, this is written
>>>>>> to, but when is that actually?
>>>>>
>>>>> For built-in modules it is during kernel initialization. For loadable
>>>>> modules, it during module load. No code from the module will execute
>>>>> before parameters are set.
>>>>
>>>> Gotcha and there never ever will be custom code that is executed
>>>> before/during parameter setting (so code aside from code in `kernel`)?
>>>>
>>>>>> Would we eventually execute other Rust
>>>>>> code during that time? (for example when we allow custom parameter
>>>>>> parsing)
>>>>>
>>>>> I don't think we will need to synchronize because of custom parameter
>>>>> parsing. Parameters are initialized sequentially. It is not a problem if
>>>>> the custom parameter parsing code name other parameters, because they
>>>>> are all initialized to valid values (as they are statics).
>>>>
>>>> If you have `&'static i64`, then the value at that reference is never
>>>> allowed to change.
>>>>
>>>>>> This function also must never be `const` because of the following:
>>>>>>
>>>>>> module! {
>>>>>> // ...
>>>>>> params: {
>>>>>> my_param: i64 {
>>>>>> default: 0,
>>>>>> description: "",
>>>>>> },
>>>>>> },
>>>>>> }
>>>>>>
>>>>>> static BAD: &'static i64 = module_parameters::my_param.get();
>>>>>>
>>>>>> AFAIK, this static will be executed before loading module parameters and
>>>>>> thus it makes writing to the parameter UB.
>>>>>
>>>>> As I understand, the static will be initialized by a constant expression
>>>>> evaluated at compile time. I am not sure what happens when this is
>>>>> evaluated in const context:
>>>>>
>>>>> pub fn get(&self) -> &T {
>>>>> // SAFETY: As we only support read only parameters with no sysfs
>>>>> // exposure, the kernel will not touch the parameter data after module
>>>>> // initialization.
>>>>> unsafe { &*self.data.get() }
>>>>> }
>>>>>
>>>>> Why would that not be OK? I would assume the compiler builds a dependency graph
>>>>> when initializing statics?
>>>>
>>>> Yes it builds a dependency graph, but that is irrelevant? The problem is
>>>> that I can create a `'static` reference to the inner value *before* the
>>>> parameter is written-to (as the static is initialized before the
>>>> parameters).
>>>
>>> I see, I did not consider this situation. Thanks for pointing this out.
>>>
>>> Could we get around this without a lock maybe? If we change
>>> `ModuleParamAccess::get` to take a closure instead:
>>>
>>> /// Call `func` with a reference to the parameter value stored in `Self`.
>>> pub fn read(&self, func: impl FnOnce(&T)) {
>>> // SAFETY: As we only support read only parameters with no sysfs
>>> // exposure, the kernel will not touch the parameter data after module
>>> // initialization.
>>> let data = unsafe { &*self.data.get() };
>>>
>>> func(data)
>>> }
>>>
>>> I think this would bound the lifetime of the reference passed to the
>>> closure to the duration of the call, right?
>>
>> Yes that is correct. Now you can't assign the reference to a static.
>> However, this API is probably very clunky to use, since you always have
>> to create a closure etc.
>>
>> Since you mentioned in the other reply that one could spin up a thread
>> and do something simultaneously, I don't think this is enough. You could
>> have a loop spin over the new `read` function and read the value and
>> then the write happens.
>
> Yes you are right, we have to treat it as if it could be written at any
> point in time.
>
>> One way to fix this issue would be to use atomics to read the value and
>> to not create a reference to it. So essentially have
>>
>> pub fn read(&self) -> T {
>> unsafe { atomic_read_unsafe_cell(&self.data) }
>> }
>
> That could work.
>
>> Another way would be to use a `Once`-like type (does that exist on the C
>> side?) so a type that can be initialized once and then never changes.
>> While it doesn't have a value set, we return some default value for the
>> param and print a warning, when it's set, we just return the value. But
>> this probably also requires atomics...
>
> I think atomic bool is not that far away. Either that, or we can lock.
>
>> Is parameter accessing used that often in hot paths? Can't you just copy
>> the value into your `Module` struct?
>
> I don't imagine this being read in a hot path. If so, the user could
> make a copy.
That's good to know, then let's try to go for something simple.
I don't think that we can just use a `Mutex<T>`, because we don't have a
way to create it at const time... I guess we could have
impl<T> Mutex<T>
/// # Safety
///
/// The returned value needs to be pinned and then `init` needs
/// to be called before any other methods are called on this.
pub unsafe const fn const_new() -> Self;
pub unsafe fn init(&self);
}
But that seems like a bad idea, because where would we call the `init`
function? That also needs to be synchronized...
Maybe we can just like you said use an atomic bool?
---
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