* [PATCH 2/5] module: Remove unnecessary +1 from last_unloaded_module::name size
From: Petr Pavlu @ 2025-06-30 14:32 UTC (permalink / raw)
To: Luis Chamberlain, Petr Pavlu, Sami Tolvanen, Daniel Gomez
Cc: linux-modules, linux-kernel
In-Reply-To: <20250630143535.267745-1-petr.pavlu@suse.com>
The variable last_unloaded_module::name tracks the name of the last
unloaded module. It is a string copy of module::name, which is
MODULE_NAME_LEN bytes in size and includes the NUL terminator. Therefore,
the size of last_unloaded_module::name can also be just MODULE_NAME_LEN,
without the need for an extra byte.
Fixes: e14af7eeb47e ("debug: track and print last unloaded module in the oops trace")
Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
---
kernel/module/main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/module/main.c b/kernel/module/main.c
index 933a9854cb7d..04173543639c 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -580,7 +580,7 @@ MODINFO_ATTR(version);
MODINFO_ATTR(srcversion);
static struct {
- char name[MODULE_NAME_LEN + 1];
+ char name[MODULE_NAME_LEN];
char taints[MODULE_FLAGS_BUF_SIZE];
} last_unloaded_module;
--
2.49.0
^ permalink raw reply related
* [PATCH 1/5] module: Prevent silent truncation of module name in delete_module(2)
From: Petr Pavlu @ 2025-06-30 14:32 UTC (permalink / raw)
To: Luis Chamberlain, Petr Pavlu, Sami Tolvanen, Daniel Gomez
Cc: linux-modules, linux-kernel
In-Reply-To: <20250630143535.267745-1-petr.pavlu@suse.com>
Passing a module name longer than MODULE_NAME_LEN to the delete_module
syscall results in its silent truncation. This really isn't much of
a problem in practice, but it could theoretically lead to the removal of an
incorrect module. It is more sensible to return ENAMETOOLONG or ENOENT in
such a case.
Update the syscall to return ENOENT, as documented in the delete_module(2)
man page to mean "No module by that name exists." This is appropriate
because a module with a name longer than MODULE_NAME_LEN cannot be loaded
in the first place.
Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
---
kernel/module/main.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/kernel/module/main.c b/kernel/module/main.c
index 413ac6ea3702..933a9854cb7d 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -751,14 +751,16 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
struct module *mod;
char name[MODULE_NAME_LEN];
char buf[MODULE_FLAGS_BUF_SIZE];
- int ret, forced = 0;
+ int ret, len, forced = 0;
if (!capable(CAP_SYS_MODULE) || modules_disabled)
return -EPERM;
- if (strncpy_from_user(name, name_user, MODULE_NAME_LEN-1) < 0)
- return -EFAULT;
- name[MODULE_NAME_LEN-1] = '\0';
+ len = strncpy_from_user(name, name_user, MODULE_NAME_LEN);
+ if (len == 0 || len == MODULE_NAME_LEN)
+ return -ENOENT;
+ if (len < 0)
+ return len;
audit_log_kern_module(name);
--
2.49.0
^ permalink raw reply related
* [PATCH 3/5] module: Restore the moduleparam prefix length check
From: Petr Pavlu @ 2025-06-30 14:32 UTC (permalink / raw)
To: Luis Chamberlain, Petr Pavlu, Sami Tolvanen, Daniel Gomez
Cc: linux-modules, linux-kernel
In-Reply-To: <20250630143535.267745-1-petr.pavlu@suse.com>
The moduleparam code allows modules to provide their own definition of
MODULE_PARAM_PREFIX, instead of using the default KBUILD_MODNAME ".".
Commit 730b69d22525 ("module: check kernel param length at compile time,
not runtime") added a check to ensure the prefix doesn't exceed
MODULE_NAME_LEN, as this is what param_sysfs_builtin() expects.
Later, commit 58f86cc89c33 ("VERIFY_OCTAL_PERMISSIONS: stricter checking
for sysfs perms.") removed this check, but there is no indication this was
intentional.
Since the check is still useful for param_sysfs_builtin() to function
properly, reintroduce it in __module_param_call(), but in a modernized form
using static_assert().
While here, clean up the __module_param_call() comments. In particular,
remove the comment "Default value instead of permissions?", which comes
from commit 9774a1f54f17 ("[PATCH] Compile-time check re world-writeable
module params"). This comment was related to the test variable
__param_perm_check_##name, which was removed in the previously mentioned
commit 58f86cc89c33.
Fixes: 58f86cc89c33 ("VERIFY_OCTAL_PERMISSIONS: stricter checking for sysfs perms.")
Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
---
include/linux/moduleparam.h | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index bfb85fd13e1f..110e9d09de24 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -282,10 +282,9 @@ struct kparam_array
#define __moduleparam_const const
#endif
-/* This is the fundamental function for registering boot/module
- parameters. */
+/* This is the fundamental function for registering boot/module parameters. */
#define __module_param_call(prefix, name, ops, arg, perm, level, flags) \
- /* Default value instead of permissions? */ \
+ static_assert(sizeof(""prefix) - 1 <= MAX_PARAM_PREFIX_LEN); \
static const char __param_str_##name[] = prefix #name; \
static struct kernel_param __moduleparam_const __param_##name \
__used __section("__param") \
--
2.49.0
^ permalink raw reply related
* [PATCH 4/5] tracing: Replace MAX_PARAM_PREFIX_LEN with MODULE_NAME_LEN
From: Petr Pavlu @ 2025-06-30 14:32 UTC (permalink / raw)
To: Luis Chamberlain, Petr Pavlu, Sami Tolvanen, Daniel Gomez
Cc: linux-modules, linux-kernel, Steven Rostedt, Masami Hiramatsu
In-Reply-To: <20250630143535.267745-1-petr.pavlu@suse.com>
Use the MODULE_NAME_LEN definition in module_exists() to obtain the maximum
size of a module name, instead of using MAX_PARAM_PREFIX_LEN. The values
are the same but MODULE_NAME_LEN is more appropriate in this context.
MAX_PARAM_PREFIX_LEN was added in commit 730b69d22525 ("module: check
kernel param length at compile time, not runtime") only to break a circular
dependency between module.h and moduleparam.h, and should mostly be limited
to use in moduleparam.h.
Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
---
As a side note, I suspect the function module_exists() would be better
replaced with !!find_module() + RCU locking, but that is a separate issue.
kernel/trace/trace.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 95ae7c4e5835..b9da0c4661a0 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -10373,7 +10373,7 @@ bool module_exists(const char *module)
{
/* All modules have the symbol __this_module */
static const char this_mod[] = "__this_module";
- char modname[MAX_PARAM_PREFIX_LEN + sizeof(this_mod) + 2];
+ char modname[MODULE_NAME_LEN + sizeof(this_mod) + 2];
unsigned long val;
int n;
--
2.49.0
^ permalink raw reply related
* [PATCH 5/5] module: Rename MAX_PARAM_PREFIX_LEN to __MODULE_NAME_LEN
From: Petr Pavlu @ 2025-06-30 14:32 UTC (permalink / raw)
To: Luis Chamberlain, Petr Pavlu, Sami Tolvanen, Daniel Gomez
Cc: linux-modules, linux-kernel
In-Reply-To: <20250630143535.267745-1-petr.pavlu@suse.com>
The maximum module name length (MODULE_NAME_LEN) is somewhat confusingly
defined in terms of the maximum parameter prefix length
(MAX_PARAM_PREFIX_LEN), when in fact the dependency is in the opposite
direction.
This split originates from commit 730b69d22525 ("module: check kernel param
length at compile time, not runtime"). The code needed to use
MODULE_NAME_LEN in moduleparam.h, but because module.h requires
moduleparam.h, this created a circular dependency. It was resolved by
introducing MAX_PARAM_PREFIX_LEN in moduleparam.h and defining
MODULE_NAME_LEN in module.h in terms of MAX_PARAM_PREFIX_LEN.
Rename MAX_PARAM_PREFIX_LEN to __MODULE_NAME_LEN for clarity. This matches
the similar approach of defining MODULE_INFO in module.h and __MODULE_INFO
in moduleparam.h.
Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
---
include/linux/module.h | 2 +-
include/linux/moduleparam.h | 12 ++++++++----
2 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/include/linux/module.h b/include/linux/module.h
index 5faa1fb1f4b4..0f1dde3996d9 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -32,7 +32,7 @@
#include <linux/percpu.h>
#include <asm/module.h>
-#define MODULE_NAME_LEN MAX_PARAM_PREFIX_LEN
+#define MODULE_NAME_LEN __MODULE_NAME_LEN
struct modversion_info {
unsigned long crc;
diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index 110e9d09de24..a04a2bc4f51e 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -6,6 +6,13 @@
#include <linux/stringify.h>
#include <linux/kernel.h>
+/*
+ * The maximum module name length, including the NUL byte.
+ * Chosen so that structs with an unsigned long line up, specifically
+ * modversion_info.
+ */
+#define __MODULE_NAME_LEN (64 - sizeof(unsigned long))
+
/* You can override this manually, but generally this should match the
module name. */
#ifdef MODULE
@@ -17,9 +24,6 @@
#define __MODULE_INFO_PREFIX KBUILD_MODNAME "."
#endif
-/* 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)[] \
__used __section(".modinfo") __aligned(1) \
@@ -284,7 +288,7 @@ struct kparam_array
/* This is the fundamental function for registering boot/module parameters. */
#define __module_param_call(prefix, name, ops, arg, perm, level, flags) \
- static_assert(sizeof(""prefix) - 1 <= MAX_PARAM_PREFIX_LEN); \
+ static_assert(sizeof(""prefix) - 1 <= __MODULE_NAME_LEN); \
static const char __param_str_##name[] = prefix #name; \
static struct kernel_param __moduleparam_const __param_##name \
__used __section("__param") \
--
2.49.0
^ permalink raw reply related
* Re: [PATCH] gendwarfksyms: order -T symtypes output by name
From: Masahiro Yamada @ 2025-06-30 15:40 UTC (permalink / raw)
To: Giuliano Procida
Cc: Sami Tolvanen, Greg Kroah-Hartman, linux-modules, linux-kbuild,
linux-kernel
In-Reply-To: <CAGvU0HkKacQKB1q9NWcqChLGoMB+1vu9UdqYc+tBRbTTc3++GQ@mail.gmail.com>
On Mon, Jun 30, 2025 at 10:46 PM Giuliano Procida <gprocida@google.com> wrote:
>
> On Mon, 30 Jun 2025 at 14:24, Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > On Mon, Jun 30, 2025 at 7:05 PM Giuliano Procida <gprocida@google.com> wrote:
> > >
> > > Hi.
> > >
> > > On Sun, 29 Jun 2025 at 18:51, Masahiro Yamada <masahiroy@kernel.org> wrote:
> > > >
> > > > On Wed, Jun 25, 2025 at 6:52 PM Giuliano Procida <gprocida@google.com> wrote:
> > > > >
> > > > > When writing symtypes information, we iterate through the entire hash
> > > > > table containing type expansions. The key order varies unpredictably
> > > > > as new entries are added, making it harder to compare symtypes between
> > > > > builds.
> > > > >
> > > > > Resolve this by sorting the type expansions by name before output.
> > > > >
> > > > > Signed-off-by: Giuliano Procida <gprocida@google.com>
> > > > > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > Reviewed-by: Sami Tolvanen <samitolvanen@google.com>
> > > > > ---
> > > > > scripts/gendwarfksyms/types.c | 29 ++++++++++++++++++++++++++---
> > > > > 1 file changed, 26 insertions(+), 3 deletions(-)
> > > > >
> > > > > [Adjusted the first line of the description. Added reviewer tags.
> > > > > Added missing CC to linux-modules.]
> > > > >
> > > > > diff --git a/scripts/gendwarfksyms/types.c b/scripts/gendwarfksyms/types.c
> > > > > index 7bd459ea6c59..51c1471e8684 100644
> > > > > --- a/scripts/gendwarfksyms/types.c
> > > > > +++ b/scripts/gendwarfksyms/types.c
> > > > > @@ -6,6 +6,8 @@
> > > > > #define _GNU_SOURCE
> > > > > #include <inttypes.h>
> > > > > #include <stdio.h>
> > > > > +#include <stdlib.h>
> > > > > +#include <string.h>
> > > > > #include <zlib.h>
> > > > >
> > > > > #include "gendwarfksyms.h"
> > > > > @@ -179,20 +181,41 @@ static int type_map_get(const char *name, struct type_expansion **res)
> > > > > return -1;
> > > > > }
> > > > >
> > > > > +static int cmp_expansion_name(const void *p1, const void *p2)
> > > > > +{
> > > > > + struct type_expansion *const *e1 = p1;
> > > > > + struct type_expansion *const *e2 = p2;
> > > > > +
> > > > > + return strcmp((*e1)->name, (*e2)->name);
> > > > > +}
> > > > > +
> > > > > static void type_map_write(FILE *file)
> > > > > {
> > > > > struct type_expansion *e;
> > > > > struct hlist_node *tmp;
> > > > > + struct type_expansion **es;
> > > > > + size_t count = 0;
> > > > > + size_t i = 0;
> > > > >
> > > > > if (!file)
> > > > > return;
> > > > >
> > > > > - hash_for_each_safe(type_map, e, tmp, hash) {
> > > > > - checkp(fputs(e->name, file));
> > > > > + hash_for_each_safe(type_map, e, tmp, hash)
> > > > > + ++count;
> > > > > + es = xmalloc(count * sizeof(struct type_expansion *));
> > > >
> > > > Just a nit:
> > > >
> > > > es = xmalloc(count * sizeof(*es));
> > > >
> > > > is better?
> > > >
> > > > > + hash_for_each_safe(type_map, e, tmp, hash)
> > > > > + es[i++] = e;
> > > > > +
> > > > > + qsort(es, count, sizeof(struct type_expansion *), cmp_expansion_name);
> > > >
> > > > qsort(es, count, sizeof(*es), cmp_expansion_name);
> > > >
> > >
> > > That's a fair point.
> > >
> > > However, in the gendwarfksyms code, all but one of the sizeofs uses an
> > > explicit type name. The exception is sizeof(stats) where stats is an array.
> > >
> > > I'll leave Sami's code as it is.
> >
> >
> > This rule is clearly documented with rationale.
> >
> > See this:
> > https://github.com/torvalds/linux/blob/v6.15/Documentation/process/coding-style.rst?plain=1#L941
> >
> >
>
> I can follow up with a change that adjusts all occurrences. That
> shouldn't take long at all.
I expected a new patch version (I do not know whether it is v2 or v3 since
you do not add such a prefix),
instead of breaking the style, and fixing it in a follow-up patch.
--
Best Regards
Masahiro Yamada
^ permalink raw reply
* Re: [PATCH v13 2/6] rust: introduce module_param module
From: Benno Lossin @ 2025-06-30 19:02 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: <87h5zxxtdw.fsf@kernel.org>
On Mon Jun 30, 2025 at 3:15 PM CEST, Andreas Hindborg wrote:
> "Benno Lossin" <lossin@kernel.org> writes:
>> On Mon Jun 30, 2025 at 1:18 PM CEST, Andreas Hindborg wrote:
>>> "Benno Lossin" <lossin@kernel.org> writes:
>>>> (no idea if the orderings are correct, I always have to think way to
>>>> much about that... especially since our atomics seem to only take one
>>>> ordering in compare_exchange?)
>>>>
>>>>> As far as I can tell, atomics may not land in v6.17, so this series
>>>>> will probably not be ready for merge until v6.18 at the earliest.
>>>>
>>>> Yeah, sorry about that :(
>>>
>>> Actually, perhaps we could aim at merging this code without this
>>> synchronization?
>>
>> I won't remember this issue in a few weeks and I fear that it will just
>> get buried. In fact, I already had to re-read now what the actual issue
>> was...
>>
>>> The lack of synchronization is only a problem if we
>>> support custom parsing. This patch set does not allow custom parsing
>>> code, so it does not suffer this issue.
>>
>> ... In doing that, I saw my original example of UB:
>>
>> module! {
>> // ...
>> params: {
>> my_param: i64 {
>> default: 0,
>> description: "",
>> },
>> },
>> }
>>
>> static BAD: &'static i64 = module_parameters::my_param.get();
>>
>> That can happen without custom parsing, so it's still a problem...
>
> Ah, got it. Thanks.
On second thought, we *could* just make the accessor function `unsafe`.
Of course with a pinky promise to make the implementation safe once
atomics land. But I think if it helps you get your driver faster along,
then we should do it.
---
Cheers,
Benno
^ permalink raw reply
* Re: [PATCH v13 2/6] rust: introduce module_param module
From: Andreas Hindborg @ 2025-07-01 8:43 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: <DB03MZI2FCOW.2JBFL3TY38FK@kernel.org>
"Benno Lossin" <lossin@kernel.org> writes:
> On Mon Jun 30, 2025 at 3:15 PM CEST, Andreas Hindborg wrote:
>> "Benno Lossin" <lossin@kernel.org> writes:
>>> On Mon Jun 30, 2025 at 1:18 PM CEST, Andreas Hindborg wrote:
>>>> "Benno Lossin" <lossin@kernel.org> writes:
>>>>> (no idea if the orderings are correct, I always have to think way to
>>>>> much about that... especially since our atomics seem to only take one
>>>>> ordering in compare_exchange?)
>>>>>
>>>>>> As far as I can tell, atomics may not land in v6.17, so this series
>>>>>> will probably not be ready for merge until v6.18 at the earliest.
>>>>>
>>>>> Yeah, sorry about that :(
>>>>
>>>> Actually, perhaps we could aim at merging this code without this
>>>> synchronization?
>>>
>>> I won't remember this issue in a few weeks and I fear that it will just
>>> get buried. In fact, I already had to re-read now what the actual issue
>>> was...
>>>
>>>> The lack of synchronization is only a problem if we
>>>> support custom parsing. This patch set does not allow custom parsing
>>>> code, so it does not suffer this issue.
>>>
>>> ... In doing that, I saw my original example of UB:
>>>
>>> module! {
>>> // ...
>>> params: {
>>> my_param: i64 {
>>> default: 0,
>>> description: "",
>>> },
>>> },
>>> }
>>>
>>> static BAD: &'static i64 = module_parameters::my_param.get();
>>>
>>> That can happen without custom parsing, so it's still a problem...
>>
>> Ah, got it. Thanks.
>
> On second thought, we *could* just make the accessor function `unsafe`.
> Of course with a pinky promise to make the implementation safe once
> atomics land. But I think if it helps you get your driver faster along,
> then we should do it.
No, I am OK for now with configfs.
But, progress is still great. How about if we add a copy accessor
instead for now, I think you proposed that a few million emails ago:
pub fn get(&self) -> T;
or maybe rename:
pub fn copy(&self) -> T;
Then we are fine safety wise for now, right? It is even sensible for
these `T: Copy` types.
Best regards,
Andreas Hindborg
^ permalink raw reply
* Re: [PATCH v13 2/6] rust: introduce module_param module
From: Benno Lossin @ 2025-07-01 9:05 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: <87bjq4xpv7.fsf@kernel.org>
On Tue Jul 1, 2025 at 10:43 AM CEST, Andreas Hindborg wrote:
> "Benno Lossin" <lossin@kernel.org> writes:
>> On Mon Jun 30, 2025 at 3:15 PM CEST, Andreas Hindborg wrote:
>>> "Benno Lossin" <lossin@kernel.org> writes:
>>>> On Mon Jun 30, 2025 at 1:18 PM CEST, Andreas Hindborg wrote:
>>>>> "Benno Lossin" <lossin@kernel.org> writes:
>>>>>> (no idea if the orderings are correct, I always have to think way to
>>>>>> much about that... especially since our atomics seem to only take one
>>>>>> ordering in compare_exchange?)
>>>>>>
>>>>>>> As far as I can tell, atomics may not land in v6.17, so this series
>>>>>>> will probably not be ready for merge until v6.18 at the earliest.
>>>>>>
>>>>>> Yeah, sorry about that :(
>>>>>
>>>>> Actually, perhaps we could aim at merging this code without this
>>>>> synchronization?
>>>>
>>>> I won't remember this issue in a few weeks and I fear that it will just
>>>> get buried. In fact, I already had to re-read now what the actual issue
>>>> was...
>>>>
>>>>> The lack of synchronization is only a problem if we
>>>>> support custom parsing. This patch set does not allow custom parsing
>>>>> code, so it does not suffer this issue.
>>>>
>>>> ... In doing that, I saw my original example of UB:
>>>>
>>>> module! {
>>>> // ...
>>>> params: {
>>>> my_param: i64 {
>>>> default: 0,
>>>> description: "",
>>>> },
>>>> },
>>>> }
>>>>
>>>> static BAD: &'static i64 = module_parameters::my_param.get();
>>>>
>>>> That can happen without custom parsing, so it's still a problem...
>>>
>>> Ah, got it. Thanks.
>>
>> On second thought, we *could* just make the accessor function `unsafe`.
>> Of course with a pinky promise to make the implementation safe once
>> atomics land. But I think if it helps you get your driver faster along,
>> then we should do it.
>
> No, I am OK for now with configfs.
>
> But, progress is still great. How about if we add a copy accessor
> instead for now, I think you proposed that a few million emails ago:
>
> pub fn get(&self) -> T;
>
> or maybe rename:
>
> pub fn copy(&self) -> T;
>
> Then we are fine safety wise for now, right? It is even sensible for
> these `T: Copy` types.
That is better than getting a reference, but still someone could read at
the same time that a write is happening (though we need some new
abstractions AFAIK?). But I fear that we forget about this issue,
because it'll be some time until we land parameters that are `!Copy` (if
at all...)
---
Cheers,
Benno
^ permalink raw reply
* Re: [PATCH v3] module: Make sure relocations are applied to the per-CPU section
From: Daniel Gomez @ 2025-07-01 13:40 UTC (permalink / raw)
To: linux-modules, Sebastian Andrzej Siewior
Cc: Petr Pavlu, Luis Chamberlain, Sami Tolvanen, Peter Zijlstra,
Thomas Gleixner
In-Reply-To: <20250610163328.URcsSUC1@linutronix.de>
On Tue, 10 Jun 2025 18:33:28 +0200, Sebastian Andrzej Siewior wrote:
> The per-CPU data section is handled differently than the other sections.
> The memory allocations requires a special __percpu pointer and then the
> section is copied into the view of each CPU. Therefore the SHF_ALLOC
> flag is removed to ensure move_module() skips it.
>
> Later, relocations are applied and apply_relocations() skips sections
> without SHF_ALLOC because they have not been copied. This also skips the
> per-CPU data section.
> The missing relocations result in a NULL pointer on x86-64 and very
> small values on x86-32. This results in a crash because it is not
> skipped like NULL pointer would and can't be dereferenced.
>
> [...]
Applied, thanks!
[1/1] module: Make sure relocations are applied to the per-CPU section
commit: 6c7ceed3f375bf9e2b093623af76df3094c88871
Best regards,
--
Daniel Gomez <da.gomez@samsung.com>
^ permalink raw reply
* Re: [PATCH v13 2/6] rust: introduce module_param module
From: Andreas Hindborg @ 2025-07-01 14: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: <DB0LKI8BO3HZ.3FF03JN4364RM@kernel.org>
"Benno Lossin" <lossin@kernel.org> writes:
> On Tue Jul 1, 2025 at 10:43 AM CEST, Andreas Hindborg wrote:
>> "Benno Lossin" <lossin@kernel.org> writes:
>>> On Mon Jun 30, 2025 at 3:15 PM CEST, Andreas Hindborg wrote:
>>>> "Benno Lossin" <lossin@kernel.org> writes:
>>>>> On Mon Jun 30, 2025 at 1:18 PM CEST, Andreas Hindborg wrote:
>>>>>> "Benno Lossin" <lossin@kernel.org> writes:
>>>>>>> (no idea if the orderings are correct, I always have to think way to
>>>>>>> much about that... especially since our atomics seem to only take one
>>>>>>> ordering in compare_exchange?)
>>>>>>>
>>>>>>>> As far as I can tell, atomics may not land in v6.17, so this series
>>>>>>>> will probably not be ready for merge until v6.18 at the earliest.
>>>>>>>
>>>>>>> Yeah, sorry about that :(
>>>>>>
>>>>>> Actually, perhaps we could aim at merging this code without this
>>>>>> synchronization?
>>>>>
>>>>> I won't remember this issue in a few weeks and I fear that it will just
>>>>> get buried. In fact, I already had to re-read now what the actual issue
>>>>> was...
>>>>>
>>>>>> The lack of synchronization is only a problem if we
>>>>>> support custom parsing. This patch set does not allow custom parsing
>>>>>> code, so it does not suffer this issue.
>>>>>
>>>>> ... In doing that, I saw my original example of UB:
>>>>>
>>>>> module! {
>>>>> // ...
>>>>> params: {
>>>>> my_param: i64 {
>>>>> default: 0,
>>>>> description: "",
>>>>> },
>>>>> },
>>>>> }
>>>>>
>>>>> static BAD: &'static i64 = module_parameters::my_param.get();
>>>>>
>>>>> That can happen without custom parsing, so it's still a problem...
>>>>
>>>> Ah, got it. Thanks.
>>>
>>> On second thought, we *could* just make the accessor function `unsafe`.
>>> Of course with a pinky promise to make the implementation safe once
>>> atomics land. But I think if it helps you get your driver faster along,
>>> then we should do it.
>>
>> No, I am OK for now with configfs.
>>
>> But, progress is still great. How about if we add a copy accessor
>> instead for now, I think you proposed that a few million emails ago:
>>
>> pub fn get(&self) -> T;
>>
>> or maybe rename:
>>
>> pub fn copy(&self) -> T;
>>
>> Then we are fine safety wise for now, right? It is even sensible for
>> these `T: Copy` types.
>
> That is better than getting a reference, but still someone could read at
> the same time that a write is happening (though we need some new
> abstractions AFAIK?). But I fear that we forget about this issue,
> because it'll be some time until we land parameters that are `!Copy` (if
> at all...)
No, that could not happen when we are not allowing custom parsing or
sysfs access. Regarding forgetting, I already added a `NOTE` on `!Copy`,
and I would add one on this issue as well.
Best regards,
Andreas Hindborg
^ permalink raw reply
* [PATCH v2 0/2] gendwarfksyms - improve symtypes output
From: Giuliano Procida @ 2025-07-01 15:19 UTC (permalink / raw)
To: Masahiro Yamada, Sami Tolvanen, Greg Kroah-Hartman
Cc: Giuliano Procida, linux-modules, linux-kbuild, linux-kernel
In-Reply-To: <CAK7LNASzE1CtRo9T4byPXJtB-HtuWsGe=OLba+8JU9fB28Chow@mail.gmail.com>
When investigating MODVERSIONS CRC changes from one build to the next,
we need to diff corresponding symtypes files. However, gendwarfksyms
did not order these files.
The first change in this series makes gendwarfksyms code conform to
the preferred style for the size parameter passed to allocation
functions.
https://github.com/torvalds/linux/blob/v6.15/Documentation/process/coding-style.rst?plain=1#L941
The second change in this series ensures symtypes are output in key
order.
The series is marked as v2 to distinguish it from earlier versions
where the changes were posted individually.
Giuliano Procida (2):
gendwarfksyms: use preferred form of sizeof for allocation
gendwarfksyms: order -T symtypes output by name
scripts/gendwarfksyms/cache.c | 2 +-
scripts/gendwarfksyms/die.c | 4 ++--
scripts/gendwarfksyms/dwarf.c | 2 +-
scripts/gendwarfksyms/kabi.c | 2 +-
scripts/gendwarfksyms/symbols.c | 2 +-
scripts/gendwarfksyms/types.c | 33 ++++++++++++++++++++++++++++-----
6 files changed, 34 insertions(+), 11 deletions(-)
--
2.50.0.727.gbf7dc18ff4-goog
^ permalink raw reply
* [PATCH v2 1/2] gendwarfksyms: use preferred form of sizeof for allocation
From: Giuliano Procida @ 2025-07-01 15:19 UTC (permalink / raw)
To: Masahiro Yamada, Sami Tolvanen, Greg Kroah-Hartman
Cc: Giuliano Procida, linux-modules, linux-kbuild, linux-kernel
In-Reply-To: <20250701152000.2477659-1-gprocida@google.com>
The preferred form is to use the variable being allocated to, rather
than explicitly supplying a type name which might become stale.
Also do this for memset.
Suggested-by: Masahiro Yamada <masahiroy@kernel.org>
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
scripts/gendwarfksyms/cache.c | 2 +-
scripts/gendwarfksyms/die.c | 4 ++--
scripts/gendwarfksyms/dwarf.c | 2 +-
scripts/gendwarfksyms/kabi.c | 2 +-
scripts/gendwarfksyms/symbols.c | 2 +-
scripts/gendwarfksyms/types.c | 4 ++--
6 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/scripts/gendwarfksyms/cache.c b/scripts/gendwarfksyms/cache.c
index c9c19b86a686..1c640db93db3 100644
--- a/scripts/gendwarfksyms/cache.c
+++ b/scripts/gendwarfksyms/cache.c
@@ -15,7 +15,7 @@ void cache_set(struct cache *cache, unsigned long key, int value)
{
struct cache_item *ci;
- ci = xmalloc(sizeof(struct cache_item));
+ ci = xmalloc(sizeof(*ci));
ci->key = key;
ci->value = value;
hash_add(cache->cache, &ci->hash, hash_32(key));
diff --git a/scripts/gendwarfksyms/die.c b/scripts/gendwarfksyms/die.c
index 6183bbbe7b54..052f7a3f975a 100644
--- a/scripts/gendwarfksyms/die.c
+++ b/scripts/gendwarfksyms/die.c
@@ -33,7 +33,7 @@ static struct die *create_die(Dwarf_Die *die, enum die_state state)
{
struct die *cd;
- cd = xmalloc(sizeof(struct die));
+ cd = xmalloc(sizeof(*cd));
init_die(cd);
cd->addr = (uintptr_t)die->addr;
@@ -123,7 +123,7 @@ static struct die_fragment *append_item(struct die *cd)
{
struct die_fragment *df;
- df = xmalloc(sizeof(struct die_fragment));
+ df = xmalloc(sizeof(*df));
df->type = FRAGMENT_EMPTY;
list_add_tail(&df->list, &cd->fragments);
return df;
diff --git a/scripts/gendwarfksyms/dwarf.c b/scripts/gendwarfksyms/dwarf.c
index 13ea7bf1ae7d..3538a7d9cb07 100644
--- a/scripts/gendwarfksyms/dwarf.c
+++ b/scripts/gendwarfksyms/dwarf.c
@@ -634,7 +634,7 @@ static int get_union_kabi_status(Dwarf_Die *die, Dwarf_Die *placeholder,
* Note that the user of this feature is responsible for ensuring
* that the structure actually remains ABI compatible.
*/
- memset(&state.kabi, 0, sizeof(struct kabi_state));
+ memset(&state.kabi, 0, sizeof(state.kabi));
res = checkp(process_die_container(&state, NULL, die,
check_union_member_kabi_status,
diff --git a/scripts/gendwarfksyms/kabi.c b/scripts/gendwarfksyms/kabi.c
index b3ade713778f..e3c2a3ccf51a 100644
--- a/scripts/gendwarfksyms/kabi.c
+++ b/scripts/gendwarfksyms/kabi.c
@@ -228,7 +228,7 @@ void kabi_read_rules(int fd)
if (type == KABI_RULE_TYPE_UNKNOWN)
error("unsupported kABI rule type: '%s'", field);
- rule = xmalloc(sizeof(struct rule));
+ rule = xmalloc(sizeof(*rule));
rule->type = type;
rule->target = xstrdup(get_rule_field(&rule_str, &left));
diff --git a/scripts/gendwarfksyms/symbols.c b/scripts/gendwarfksyms/symbols.c
index 327f87389c34..35ed594f0749 100644
--- a/scripts/gendwarfksyms/symbols.c
+++ b/scripts/gendwarfksyms/symbols.c
@@ -146,7 +146,7 @@ void symbol_read_exports(FILE *file)
continue;
}
- sym = xcalloc(1, sizeof(struct symbol));
+ sym = xcalloc(1, sizeof(*sym));
sym->name = name;
sym->addr.section = SHN_UNDEF;
sym->state = SYMBOL_UNPROCESSED;
diff --git a/scripts/gendwarfksyms/types.c b/scripts/gendwarfksyms/types.c
index 7bd459ea6c59..5344c7b9a9ce 100644
--- a/scripts/gendwarfksyms/types.c
+++ b/scripts/gendwarfksyms/types.c
@@ -43,7 +43,7 @@ static int type_list_append(struct list_head *list, const char *s, void *owned)
if (!s)
return 0;
- entry = xmalloc(sizeof(struct type_list_entry));
+ entry = xmalloc(sizeof(*entry));
entry->str = s;
entry->owned = owned;
list_add_tail(&entry->list, list);
@@ -120,7 +120,7 @@ static struct type_expansion *type_map_add(const char *name,
struct type_expansion *e;
if (__type_map_get(name, &e)) {
- e = xmalloc(sizeof(struct type_expansion));
+ e = xmalloc(sizeof(*e));
type_expansion_init(e);
e->name = xstrdup(name);
--
2.50.0.727.gbf7dc18ff4-goog
^ permalink raw reply related
* [PATCH v2 2/2] gendwarfksyms: order -T symtypes output by name
From: Giuliano Procida @ 2025-07-01 15:19 UTC (permalink / raw)
To: Masahiro Yamada, Sami Tolvanen, Greg Kroah-Hartman
Cc: Giuliano Procida, linux-modules, linux-kbuild, linux-kernel
In-Reply-To: <20250701152000.2477659-1-gprocida@google.com>
When writing symtypes information, we iterate through the entire hash
table containing type expansions. The key order varies unpredictably
as new entries are added, making it harder to compare symtypes between
builds.
Resolve this by sorting the type expansions by name before output.
Signed-off-by: Giuliano Procida <gprocida@google.com>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Sami Tolvanen <samitolvanen@google.com>
---
scripts/gendwarfksyms/types.c | 29 ++++++++++++++++++++++++++---
1 file changed, 26 insertions(+), 3 deletions(-)
diff --git a/scripts/gendwarfksyms/types.c b/scripts/gendwarfksyms/types.c
index 5344c7b9a9ce..9c3b053bf061 100644
--- a/scripts/gendwarfksyms/types.c
+++ b/scripts/gendwarfksyms/types.c
@@ -6,6 +6,8 @@
#define _GNU_SOURCE
#include <inttypes.h>
#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
#include <zlib.h>
#include "gendwarfksyms.h"
@@ -179,20 +181,41 @@ static int type_map_get(const char *name, struct type_expansion **res)
return -1;
}
+static int cmp_expansion_name(const void *p1, const void *p2)
+{
+ struct type_expansion *const *e1 = p1;
+ struct type_expansion *const *e2 = p2;
+
+ return strcmp((*e1)->name, (*e2)->name);
+}
+
static void type_map_write(FILE *file)
{
struct type_expansion *e;
struct hlist_node *tmp;
+ struct type_expansion **es;
+ size_t count = 0;
+ size_t i = 0;
if (!file)
return;
- hash_for_each_safe(type_map, e, tmp, hash) {
- checkp(fputs(e->name, file));
+ hash_for_each_safe(type_map, e, tmp, hash)
+ ++count;
+ es = xmalloc(count * sizeof(*es));
+ hash_for_each_safe(type_map, e, tmp, hash)
+ es[i++] = e;
+
+ qsort(es, count, sizeof(*es), cmp_expansion_name);
+
+ for (i = 0; i < count; ++i) {
+ checkp(fputs(es[i]->name, file));
checkp(fputs(" ", file));
- type_list_write(&e->expanded, file);
+ type_list_write(&es[i]->expanded, file);
checkp(fputs("\n", file));
}
+
+ free(es);
}
static void type_map_free(void)
--
2.50.0.727.gbf7dc18ff4-goog
^ permalink raw reply related
* Re: [PATCH v13 2/6] rust: introduce module_param module
From: Benno Lossin @ 2025-07-01 15:43 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: <87zfdovvz4.fsf@kernel.org>
On Tue Jul 1, 2025 at 4:14 PM CEST, Andreas Hindborg wrote:
> "Benno Lossin" <lossin@kernel.org> writes:
>> On Tue Jul 1, 2025 at 10:43 AM CEST, Andreas Hindborg wrote:
>>> No, I am OK for now with configfs.
>>>
>>> But, progress is still great. How about if we add a copy accessor
>>> instead for now, I think you proposed that a few million emails ago:
>>>
>>> pub fn get(&self) -> T;
>>>
>>> or maybe rename:
>>>
>>> pub fn copy(&self) -> T;
>>>
>>> Then we are fine safety wise for now, right? It is even sensible for
>>> these `T: Copy` types.
>>
>> That is better than getting a reference, but still someone could read at
>> the same time that a write is happening (though we need some new
>> abstractions AFAIK?). But I fear that we forget about this issue,
>> because it'll be some time until we land parameters that are `!Copy` (if
>> at all...)
>
> No, that could not happen when we are not allowing custom parsing or
> sysfs access. Regarding forgetting, I already added a `NOTE` on `!Copy`,
> and I would add one on this issue as well.
Ultimately this is something for Miguel to decide. I would support an
unsafe accessor (we should also make it `-> T`), since there it "can't
go wrong", any UB is the fault of the user of the API. It also serves as
a good reminder, since a `NOTE` comment shouldn't be something
guaranteeing safety (we do have some of these global invariants, but I
feel like this one is too tribal and doesn't usually come up, so I feel
like it's more dangerous).
I think we should also move this patchset along, we could also opt for
no accessor at all :) Then it isn't really useful without the downstream
accessor function, but that can come after.
---
Cheers,
Benno
^ permalink raw reply
* Re: [PATCH v13 2/6] rust: introduce module_param module
From: Miguel Ojeda @ 2025-07-01 16:27 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: <DB0U12HAEVZ6.JKFPI2UQHDRY@kernel.org>
On Tue, Jul 1, 2025 at 5:43 PM Benno Lossin <lossin@kernel.org> wrote:
>
> Ultimately this is something for Miguel to decide.
Only if you all cannot get to an agreement ;)
If Andreas wants to have it already added, then I would say just mark
it `unsafe` as Benno recommends (possibly with an overbearing
precondition), given it has proven subtle/forgettable enough and that,
if I understand correctly, it would actually become unsafe if someone
"just" added "reasonably-looking code" elsewhere.
That way we have an incentive to make it safe later on and, more
importantly, to think again about it when such a patch lands,
justifying it properly. And it could plausibly protect out-of-tree
users, too.
This is all assuming that we will not have many users of this added
right away (in a cycle or two), i.e. assuming it will be easy to
change callers later on (if only to remove the `unsafe {}`).
Cheers,
Miguel
^ permalink raw reply
* Re: [PATCH v13 2/6] rust: introduce module_param module
From: Benno Lossin @ 2025-07-01 16:54 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: <CANiq72kFUSFgBv7Es3Mhe4HUaSPZk0EVW=JaMdaAGHsQOxYN6w@mail.gmail.com>
On Tue Jul 1, 2025 at 6:27 PM CEST, Miguel Ojeda wrote:
> On Tue, Jul 1, 2025 at 5:43 PM Benno Lossin <lossin@kernel.org> wrote:
>>
>> Ultimately this is something for Miguel to decide.
>
> Only if you all cannot get to an agreement ;)
:)
> If Andreas wants to have it already added, then I would say just mark
> it `unsafe` as Benno recommends (possibly with an overbearing
> precondition), given it has proven subtle/forgettable enough and that,
> if I understand correctly, it would actually become unsafe if someone
> "just" added "reasonably-looking code" elsewhere.
Yeah, if we added code that ran at the same time as the parameter
parsing (such as custom parameter parsing or a way to start a "thread"
before the parsing is completed) it would be a problem.
> That way we have an incentive to make it safe later on and, more
> importantly, to think again about it when such a patch lands,
> justifying it properly. And it could plausibly protect out-of-tree
> users, too.
>
> This is all assuming that we will not have many users of this added
> right away (in a cycle or two), i.e. assuming it will be easy to
> change callers later on (if only to remove the `unsafe {}`).
Yeah we would add internal synchronization and could keep the API the
same (except removing unsafe of course).
---
Cheers,
Benno
^ permalink raw reply
* Re: [PATCH v2 1/2] gendwarfksyms: use preferred form of sizeof for allocation
From: Sami Tolvanen @ 2025-07-01 17:06 UTC (permalink / raw)
To: Giuliano Procida
Cc: Masahiro Yamada, Greg Kroah-Hartman, linux-modules, linux-kbuild,
linux-kernel
In-Reply-To: <20250701152000.2477659-2-gprocida@google.com>
On Tue, Jul 01, 2025 at 04:19:10PM +0100, Giuliano Procida wrote:
> The preferred form is to use the variable being allocated to, rather
> than explicitly supplying a type name which might become stale.
>
> Also do this for memset.
>
> Suggested-by: Masahiro Yamada <masahiroy@kernel.org>
> Signed-off-by: Giuliano Procida <gprocida@google.com>
Reviewed-by: Sami Tolvanen <samitolvanen@google.com>
Sami
^ permalink raw reply
* Re: [PATCH v2 0/2] gendwarfksyms - improve symtypes output
From: Masahiro Yamada @ 2025-07-02 2:30 UTC (permalink / raw)
To: Giuliano Procida
Cc: Sami Tolvanen, Greg Kroah-Hartman, linux-modules, linux-kbuild,
linux-kernel
In-Reply-To: <20250701152000.2477659-1-gprocida@google.com>
On Wed, Jul 2, 2025 at 12:20 AM Giuliano Procida <gprocida@google.com> wrote:
>
> When investigating MODVERSIONS CRC changes from one build to the next,
> we need to diff corresponding symtypes files. However, gendwarfksyms
> did not order these files.
>
> The first change in this series makes gendwarfksyms code conform to
> the preferred style for the size parameter passed to allocation
> functions.
>
> https://github.com/torvalds/linux/blob/v6.15/Documentation/process/coding-style.rst?plain=1#L941
>
> The second change in this series ensures symtypes are output in key
> order.
>
> The series is marked as v2 to distinguish it from earlier versions
> where the changes were posted individually.
>
Both applied to linux-kbuild.
Thanks!
--
Best Regards
Masahiro Yamada
^ permalink raw reply
* Re: [PATCH v13 2/6] rust: introduce module_param module
From: Andreas Hindborg @ 2025-07-02 7:56 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: <DB0U12HAEVZ6.JKFPI2UQHDRY@kernel.org>
"Benno Lossin" <lossin@kernel.org> writes:
> On Tue Jul 1, 2025 at 4:14 PM CEST, Andreas Hindborg wrote:
>> "Benno Lossin" <lossin@kernel.org> writes:
>>> On Tue Jul 1, 2025 at 10:43 AM CEST, Andreas Hindborg wrote:
>>>> No, I am OK for now with configfs.
>>>>
>>>> But, progress is still great. How about if we add a copy accessor
>>>> instead for now, I think you proposed that a few million emails ago:
>>>>
>>>> pub fn get(&self) -> T;
>>>>
>>>> or maybe rename:
>>>>
>>>> pub fn copy(&self) -> T;
>>>>
>>>> Then we are fine safety wise for now, right? It is even sensible for
>>>> these `T: Copy` types.
>>>
>>> That is better than getting a reference, but still someone could read at
>>> the same time that a write is happening (though we need some new
>>> abstractions AFAIK?). But I fear that we forget about this issue,
>>> because it'll be some time until we land parameters that are `!Copy` (if
>>> at all...)
>>
>> No, that could not happen when we are not allowing custom parsing or
>> sysfs access. Regarding forgetting, I already added a `NOTE` on `!Copy`,
>> and I would add one on this issue as well.
>
> Ultimately this is something for Miguel to decide. I would support an
> unsafe accessor (we should also make it `-> T`), since there it "can't
> go wrong", any UB is the fault of the user of the API. It also serves as
> a good reminder, since a `NOTE` comment shouldn't be something
> guaranteeing safety (we do have some of these global invariants, but I
> feel like this one is too tribal and doesn't usually come up, so I feel
> like it's more dangerous).
I see no reason for making it unsafe when it is safe?
/// Get a copy of the parameter value.
///
/// # Note
///
/// When this method is called in `const` context, the default
/// parameter value will be returned. During module initialization, the
/// kernel will populate the parameter with the value supplied at module
/// load time or kernel boot time.
// NOTE: When sysfs access to parameters are enabled, we have to pass in a
// held lock guard here.
//
// NOTE: When we begin supporting custom parameter parsing with user
// supplied code, this method must be synchronized.
pub const fn copy(&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. The kernel will update the parameters serially, so we
// will not race with other accesses.
unsafe { *self.data.get() }
}
>
> I think we should also move this patchset along, we could also opt for
> no accessor at all :) Then it isn't really useful without the downstream
> accessor function, but that can come after.
I would rather not merge it without an access method. Then it is just
dead code, and we will not notice if we break it.
Best regards,
Andreas Hindborg
^ permalink raw reply
* Re: [PATCH v13 2/6] rust: introduce module_param module
From: Andreas Hindborg @ 2025-07-02 8:26 UTC (permalink / raw)
To: Miguel Ojeda
Cc: Benno Lossin, 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: <CANiq72kFUSFgBv7Es3Mhe4HUaSPZk0EVW=JaMdaAGHsQOxYN6w@mail.gmail.com>
"Miguel Ojeda" <miguel.ojeda.sandonis@gmail.com> writes:
> On Tue, Jul 1, 2025 at 5:43 PM Benno Lossin <lossin@kernel.org> wrote:
>>
>> Ultimately this is something for Miguel to decide.
>
> Only if you all cannot get to an agreement ;)
>
> If Andreas wants to have it already added, then I would say just mark
> it `unsafe` as Benno recommends (possibly with an overbearing
> precondition), given it has proven subtle/forgettable enough and that,
> if I understand correctly, it would actually become unsafe if someone
> "just" added "reasonably-looking code" elsewhere.
You are right that if someone added code to the API, the API could
become unsound. But that is the deal with all our APIs and I don't agree
that the details are very subtle here. Someone would need to add sysfs
support or user provided parameter parsing to cause the unsoundness we
are talking about.
Anyone attempting such a task should have proper understanding of the
code first, and given the ample amount of `NOTE` comments I have added,
it should be clear that the concurrent accesses that this addition would
introduce, needs to be accounted for, to avoid data races.
I will add myself as a reviewer for the rust module parameter parsing
code if that is OK with module maintainers.
> That way we have an incentive to make it safe later on and, more
> importantly, to think again about it when such a patch lands,
> justifying it properly. And it could plausibly protect out-of-tree
> users, too.
Again, I do not think it is reasonable to mark this function unsafe.
> This is all assuming that we will not have many users of this added
> right away (in a cycle or two), i.e. assuming it will be easy to
> change callers later on (if only to remove the `unsafe {}`).
rnull will use this the cycle after it is merged.
Best regards,
Andreas Hindborg
^ permalink raw reply
* Re: [PATCH v13 2/6] rust: introduce module_param module
From: Andreas Hindborg @ 2025-07-02 8:30 UTC (permalink / raw)
To: Benno Lossin
Cc: Miguel Ojeda, 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: <DB0VJ9HRT0VG.GT9HOT7J29EL@kernel.org>
"Benno Lossin" <lossin@kernel.org> writes:
> On Tue Jul 1, 2025 at 6:27 PM CEST, Miguel Ojeda wrote:
>> On Tue, Jul 1, 2025 at 5:43 PM Benno Lossin <lossin@kernel.org> wrote:
>>>
>>> Ultimately this is something for Miguel to decide.
>>
>> Only if you all cannot get to an agreement ;)
>
> :)
>
>> If Andreas wants to have it already added, then I would say just mark
>> it `unsafe` as Benno recommends (possibly with an overbearing
>> precondition), given it has proven subtle/forgettable enough and that,
>> if I understand correctly, it would actually become unsafe if someone
>> "just" added "reasonably-looking code" elsewhere.
>
> Yeah, if we added code that ran at the same time as the parameter
> parsing (such as custom parameter parsing or a way to start a "thread"
> before the parsing is completed) it would be a problem.
Guys, we are not going to accidentally add this. I do not think this is
a valid concern.
>
>> That way we have an incentive to make it safe later on and, more
>> importantly, to think again about it when such a patch lands,
>> justifying it properly. And it could plausibly protect out-of-tree
>> users, too.
>>
>> This is all assuming that we will not have many users of this added
>> right away (in a cycle or two), i.e. assuming it will be easy to
>> change callers later on (if only to remove the `unsafe {}`).
>
> Yeah we would add internal synchronization and could keep the API the
> same (except removing unsafe of course).
That is true. But I am not going to add an unsafe block to a driver just
to read module parameters. If we cannot reach agreement on merging this
with the `copy` access method, I would rather wait on a locking version.
Best regards,
Andreas Hindborg
^ permalink raw reply
* Re: [PATCH v13 2/6] rust: introduce module_param module
From: Benno Lossin @ 2025-07-02 10:01 UTC (permalink / raw)
To: Andreas Hindborg, Miguel Ojeda
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: <87o6u3vw04.fsf@kernel.org>
On Wed Jul 2, 2025 at 10:26 AM CEST, Andreas Hindborg wrote:
> "Miguel Ojeda" <miguel.ojeda.sandonis@gmail.com> writes:
>
>> On Tue, Jul 1, 2025 at 5:43 PM Benno Lossin <lossin@kernel.org> wrote:
>>>
>>> Ultimately this is something for Miguel to decide.
>>
>> Only if you all cannot get to an agreement ;)
>
>>
>> If Andreas wants to have it already added, then I would say just mark
>> it `unsafe` as Benno recommends (possibly with an overbearing
>> precondition), given it has proven subtle/forgettable enough and that,
>> if I understand correctly, it would actually become unsafe if someone
>> "just" added "reasonably-looking code" elsewhere.
>
> You are right that if someone added code to the API, the API could
> become unsound. But that is the deal with all our APIs and I don't agree
> that the details are very subtle here. Someone would need to add sysfs
> support or user provided parameter parsing to cause the unsoundness we
> are talking about.
Normally the safety requirements & invariants are *local*. We do have
some global ones, but this one would be another one. And it's not easy
to control IMO (no code is allowed to run before parameter parsing
finished!).
> Anyone attempting such a task should have proper understanding of the
> code first, and given the ample amount of `NOTE` comments I have added,
> it should be clear that the concurrent accesses that this addition would
> introduce, needs to be accounted for, to avoid data races.
Well if I add a way to add a task to a work queue before parameter
parsing, I don't need to touch this file or even know about it.
> I will add myself as a reviewer for the rust module parameter parsing
> code if that is OK with module maintainers.
I think it's a good idea, but it is orthogonal and doesn't address the
issue, since any tree can merge code that breaks the invariant above.
>> That way we have an incentive to make it safe later on and, more
>> importantly, to think again about it when such a patch lands,
>> justifying it properly. And it could plausibly protect out-of-tree
>> users, too.
>
> Again, I do not think it is reasonable to mark this function unsafe.
We mark it `unsafe` only until atomics are merged and then we make it
safe.
You proposed to do it the other way and make it safe, though possibly
unsound when someone adds code breaking the invariant and making it
fully sound later.
I don't think we should have global invariants that are temporary.
---
Cheers,
Benno
^ permalink raw reply
* [PATCH v14 4/7] rust: module: use a reference in macros::module::module
From: Andreas Hindborg @ 2025-07-02 13:18 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: <20250702-module-params-v3-v14-0-5b1cc32311af@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 v14 1/7] rust: sync: add `OnceLock`
From: Andreas Hindborg @ 2025-07-02 13:18 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: <20250702-module-params-v3-v14-0-5b1cc32311af@kernel.org>
Introduce the `OnceLock` 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>
---
rust/kernel/sync.rs | 1 +
rust/kernel/sync/once_lock.rs | 104 ++++++++++++++++++++++++++++++++++++++++++
2 files changed, 105 insertions(+)
diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
index c7c0e552bafe..f2ee07315091 100644
--- a/rust/kernel/sync.rs
+++ b/rust/kernel/sync.rs
@@ -15,6 +15,7 @@
mod condvar;
pub mod lock;
mod locked_by;
+pub mod once_lock;
pub mod poll;
pub mod rcu;
diff --git a/rust/kernel/sync/once_lock.rs b/rust/kernel/sync/once_lock.rs
new file mode 100644
index 000000000000..cd311bea3919
--- /dev/null
+++ b/rust/kernel/sync/once_lock.rs
@@ -0,0 +1,104 @@
+//! A container that can be initialized at most once.
+
+use super::atomic::ordering::Acquire;
+use super::atomic::ordering::Release;
+use super::atomic::Atomic;
+use kernel::types::Opaque;
+
+/// A container that can be populated at most once. Thread safe.
+///
+/// Once the a [`OnceLock`] is populated, it remains populated by the same object for the
+/// lifetime `Self`.
+///
+/// # Invariants
+///
+/// `init` tracks the state of the container:
+///
+/// - If the container is empty, `init` is `0`.
+/// - If the container is mutably accessed, `init` is `1`.
+/// - If the container is populated and ready for shared access, `init` is `2`.
+///
+/// # Example
+///
+/// ```
+/// # use kernel::sync::once_lock::OnceLock;
+/// let value = OnceLock::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 OnceLock<T> {
+ init: Atomic<u32>,
+ value: Opaque<T>,
+}
+
+impl<T> Default for OnceLock<T> {
+ fn default() -> Self {
+ Self::new()
+ }
+}
+
+impl<T> OnceLock<T> {
+ /// Create a new [`OnceLock`].
+ ///
+ /// The returned instance will be empty.
+ pub const fn new() -> Self {
+ // INVARIANT: The container is empty and we set `init` to `0`.
+ Self {
+ value: Opaque::uninit(),
+ init: Atomic::new(0),
+ }
+ }
+
+ /// Get a reference to the contained object.
+ ///
+ /// Returns [`None`] if this [`OnceLock`] is empty.
+ pub fn as_ref(&self) -> Option<&T> {
+ if self.init.load(Acquire) == 2 {
+ // SAFETY: As determined by the load above, the object is ready for shared access.
+ Some(unsafe { &*self.value.get() })
+ } else {
+ None
+ }
+ }
+
+ /// Populate the [`OnceLock`].
+ ///
+ /// Returns `true` if the [`OnceLock`] was successfully populated.
+ pub fn populate(&self, value: T) -> bool {
+ // INVARIANT: We obtain exclusive access to the contained allocation and write 1 to
+ // `init`.
+ if let Ok(0) = self.init.cmpxchg(0, 1, Acquire) {
+ // SAFETY: We obtained exclusive access to the contained object.
+ unsafe { core::ptr::write(self.value.get(), value) };
+ // INVARIANT: We release our exclusive access and transition the object to shared
+ // access.
+ self.init.store(2, Release);
+ true
+ } else {
+ false
+ }
+ }
+}
+
+impl<T: Copy> OnceLock<T> {
+ /// Get a copy of the contained object.
+ ///
+ /// Returns [`None`] if the [`OnceLock`] is empty.
+ pub fn copy(&self) -> Option<T> {
+ if self.init.load(Acquire) == 2 {
+ // SAFETY: As determined by the load above, the object is ready for shared access.
+ Some(unsafe { *self.value.get() })
+ } else {
+ None
+ }
+ }
+}
--
2.47.2
^ permalink raw reply related
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