* [PATCH 0/5] module: Fix minor problems related to MODULE_NAME_LEN
@ 2025-06-30 14:32 Petr Pavlu
2025-06-30 14:32 ` [PATCH 1/5] module: Prevent silent truncation of module name in delete_module(2) Petr Pavlu
` (5 more replies)
0 siblings, 6 replies; 15+ messages in thread
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
Fix a few minor problems related to MODULE_NAME_LEN and
MAX_PARAM_PREFIX_LEN, and clean up their usage.
Petr Pavlu (5):
module: Prevent silent truncation of module name in delete_module(2)
module: Remove unnecessary +1 from last_unloaded_module::name size
module: Restore the moduleparam prefix length check
tracing: Replace MAX_PARAM_PREFIX_LEN with MODULE_NAME_LEN
module: Rename MAX_PARAM_PREFIX_LEN to __MODULE_NAME_LEN
include/linux/module.h | 2 +-
include/linux/moduleparam.h | 15 +++++++++------
kernel/module/main.c | 12 +++++++-----
kernel/trace/trace.c | 2 +-
4 files changed, 18 insertions(+), 13 deletions(-)
base-commit: d0b3b7b22dfa1f4b515fd3a295b3fd958f9e81af
--
2.49.0
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/5] module: Prevent silent truncation of module name in delete_module(2)
2025-06-30 14:32 [PATCH 0/5] module: Fix minor problems related to MODULE_NAME_LEN Petr Pavlu
@ 2025-06-30 14:32 ` Petr Pavlu
2025-07-16 19:47 ` Daniel Gomez
2025-06-30 14:32 ` [PATCH 2/5] module: Remove unnecessary +1 from last_unloaded_module::name size Petr Pavlu
` (4 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
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
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 [flat|nested] 15+ messages in thread
* [PATCH 2/5] module: Remove unnecessary +1 from last_unloaded_module::name size
2025-06-30 14:32 [PATCH 0/5] module: Fix minor problems related to MODULE_NAME_LEN Petr Pavlu
2025-06-30 14:32 ` [PATCH 1/5] module: Prevent silent truncation of module name in delete_module(2) Petr Pavlu
@ 2025-06-30 14:32 ` Petr Pavlu
2025-07-16 19:49 ` Daniel Gomez
2025-06-30 14:32 ` [PATCH 3/5] module: Restore the moduleparam prefix length check Petr Pavlu
` (3 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
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
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 [flat|nested] 15+ messages in thread
* [PATCH 3/5] module: Restore the moduleparam prefix length check
2025-06-30 14:32 [PATCH 0/5] module: Fix minor problems related to MODULE_NAME_LEN Petr Pavlu
2025-06-30 14:32 ` [PATCH 1/5] module: Prevent silent truncation of module name in delete_module(2) Petr Pavlu
2025-06-30 14:32 ` [PATCH 2/5] module: Remove unnecessary +1 from last_unloaded_module::name size Petr Pavlu
@ 2025-06-30 14:32 ` Petr Pavlu
2025-07-17 19:23 ` Daniel Gomez
2025-06-30 14:32 ` [PATCH 4/5] tracing: Replace MAX_PARAM_PREFIX_LEN with MODULE_NAME_LEN Petr Pavlu
` (2 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
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
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 [flat|nested] 15+ messages in thread
* [PATCH 4/5] tracing: Replace MAX_PARAM_PREFIX_LEN with MODULE_NAME_LEN
2025-06-30 14:32 [PATCH 0/5] module: Fix minor problems related to MODULE_NAME_LEN Petr Pavlu
` (2 preceding siblings ...)
2025-06-30 14:32 ` [PATCH 3/5] module: Restore the moduleparam prefix length check Petr Pavlu
@ 2025-06-30 14:32 ` Petr Pavlu
2025-07-28 6:48 ` Daniel Gomez
2025-06-30 14:32 ` [PATCH 5/5] module: Rename MAX_PARAM_PREFIX_LEN to __MODULE_NAME_LEN Petr Pavlu
2025-07-31 12:03 ` [PATCH 0/5] module: Fix minor problems related to MODULE_NAME_LEN Daniel Gomez
5 siblings, 1 reply; 15+ messages in thread
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
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 [flat|nested] 15+ messages in thread
* [PATCH 5/5] module: Rename MAX_PARAM_PREFIX_LEN to __MODULE_NAME_LEN
2025-06-30 14:32 [PATCH 0/5] module: Fix minor problems related to MODULE_NAME_LEN Petr Pavlu
` (3 preceding siblings ...)
2025-06-30 14:32 ` [PATCH 4/5] tracing: Replace MAX_PARAM_PREFIX_LEN with MODULE_NAME_LEN Petr Pavlu
@ 2025-06-30 14:32 ` Petr Pavlu
2025-07-28 6:48 ` Daniel Gomez
2025-07-31 12:03 ` [PATCH 0/5] module: Fix minor problems related to MODULE_NAME_LEN Daniel Gomez
5 siblings, 1 reply; 15+ messages in thread
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
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 [flat|nested] 15+ messages in thread
* Re: [PATCH 1/5] module: Prevent silent truncation of module name in delete_module(2)
2025-06-30 14:32 ` [PATCH 1/5] module: Prevent silent truncation of module name in delete_module(2) Petr Pavlu
@ 2025-07-16 19:47 ` Daniel Gomez
0 siblings, 0 replies; 15+ messages in thread
From: Daniel Gomez @ 2025-07-16 19:47 UTC (permalink / raw)
To: Petr Pavlu, Luis Chamberlain, Sami Tolvanen, Daniel Gomez
Cc: linux-modules, linux-kernel
On 30/06/2025 16.32, Petr Pavlu wrote:
> 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
Including the NUL byte...
> 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;
This looks correct to me. The new code not only returns the correct errors
indicated in delete_module(2) but also checks for the case user passes an
empty string and the case where NUL char is not found when copying (with len
== MODULE_NAME_LEN) as well as it's using the correct length (MODULE_NAME_LEN)
for copying.
Reviewed-by: Daniel Gomez <da.gomez@samsung.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/5] module: Remove unnecessary +1 from last_unloaded_module::name size
2025-06-30 14:32 ` [PATCH 2/5] module: Remove unnecessary +1 from last_unloaded_module::name size Petr Pavlu
@ 2025-07-16 19:49 ` Daniel Gomez
0 siblings, 0 replies; 15+ messages in thread
From: Daniel Gomez @ 2025-07-16 19:49 UTC (permalink / raw)
To: Petr Pavlu, Luis Chamberlain, Sami Tolvanen, Daniel Gomez
Cc: linux-modules, linux-kernel
On 30/06/2025 16.32, Petr Pavlu wrote:
> 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;
>
LGTM,
Reviewed-by: Daniel Gomez <da.gomez@samsung.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/5] module: Restore the moduleparam prefix length check
2025-06-30 14:32 ` [PATCH 3/5] module: Restore the moduleparam prefix length check Petr Pavlu
@ 2025-07-17 19:23 ` Daniel Gomez
2025-07-21 9:21 ` Petr Pavlu
0 siblings, 1 reply; 15+ messages in thread
From: Daniel Gomez @ 2025-07-17 19:23 UTC (permalink / raw)
To: Petr Pavlu, Luis Chamberlain, Sami Tolvanen, Daniel Gomez
Cc: linux-modules, linux-kernel
On 30/06/2025 16.32, Petr Pavlu wrote:
> 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); \
Can you clarify if -1 to remove the dot from prefix?
Final code
static_assert(sizeof(""prefix) - 1 <= __MODULE_NAME_LEN); \
with __MODULE_NAME_LEN being:
#define __MODULE_NAME_LEN (64 - sizeof(unsigned long))
> static const char __param_str_##name[] = prefix #name; \
> static struct kernel_param __moduleparam_const __param_##name \
> __used __section("__param") \
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/5] module: Restore the moduleparam prefix length check
2025-07-17 19:23 ` Daniel Gomez
@ 2025-07-21 9:21 ` Petr Pavlu
2025-07-28 6:43 ` Daniel Gomez
0 siblings, 1 reply; 15+ messages in thread
From: Petr Pavlu @ 2025-07-21 9:21 UTC (permalink / raw)
To: Daniel Gomez
Cc: Luis Chamberlain, Sami Tolvanen, Daniel Gomez, linux-modules,
linux-kernel
On 7/17/25 9:23 PM, Daniel Gomez wrote:
> On 30/06/2025 16.32, Petr Pavlu wrote:
>> 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); \
>
> Can you clarify if -1 to remove the dot from prefix?
>
> Final code
> static_assert(sizeof(""prefix) - 1 <= __MODULE_NAME_LEN); \
>
> with __MODULE_NAME_LEN being:
>
> #define __MODULE_NAME_LEN (64 - sizeof(unsigned long))
Correct, -1 is to account for the dot at the end of the prefix.
I actually also wanted to assert that the prefix ends with a dot, but
unfortunately prefix[sizeof(prefix)-2] (with prefix being a string
literal) is not a constant expression in C.
--
Thanks,
Petr
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/5] module: Restore the moduleparam prefix length check
2025-07-21 9:21 ` Petr Pavlu
@ 2025-07-28 6:43 ` Daniel Gomez
0 siblings, 0 replies; 15+ messages in thread
From: Daniel Gomez @ 2025-07-28 6:43 UTC (permalink / raw)
To: Petr Pavlu
Cc: Luis Chamberlain, Sami Tolvanen, Daniel Gomez, linux-modules,
linux-kernel
On 21/07/2025 11.21, Petr Pavlu wrote:
> On 7/17/25 9:23 PM, Daniel Gomez wrote:
>> On 30/06/2025 16.32, Petr Pavlu wrote:
>>> 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); \
>>
>> Can you clarify if -1 to remove the dot from prefix?
>>
>> Final code
>> static_assert(sizeof(""prefix) - 1 <= __MODULE_NAME_LEN); \
>>
>> with __MODULE_NAME_LEN being:
>>
>> #define __MODULE_NAME_LEN (64 - sizeof(unsigned long))
>
> Correct, -1 is to account for the dot at the end of the prefix.
LGTM,
Reviewed-by: Daniel Gomez <da.gomez@samsung.com>
>
> I actually also wanted to assert that the prefix ends with a dot, but
> unfortunately prefix[sizeof(prefix)-2] (with prefix being a string
> literal) is not a constant expression in C.
>
But even if that would be possible, there are some calls that do not have
a prefix with dot. For example,
#define core_param(name, var, type, perm) \
param_check_##type(name, &(var)); \
__module_param_call("", name, ¶m_ops_##type, &var, perm, -1, 0)
So, you'd have to handle both cases. I mean, where __module_param_call(<prefix>
is used with either MODULE_PARAM_PREFIX or an empty string "".
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 4/5] tracing: Replace MAX_PARAM_PREFIX_LEN with MODULE_NAME_LEN
2025-06-30 14:32 ` [PATCH 4/5] tracing: Replace MAX_PARAM_PREFIX_LEN with MODULE_NAME_LEN Petr Pavlu
@ 2025-07-28 6:48 ` Daniel Gomez
2025-07-28 14:34 ` Steven Rostedt
0 siblings, 1 reply; 15+ messages in thread
From: Daniel Gomez @ 2025-07-28 6:48 UTC (permalink / raw)
To: Petr Pavlu, Luis Chamberlain, Sami Tolvanen, Daniel Gomez,
Steven Rostedt, Masami Hiramatsu
Cc: linux-modules, linux-kernel
On 30/06/2025 16.32, Petr Pavlu wrote:
> 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>
Steven, Masami,
I'm planning to merge these series into modules-next. I think and Ack/Review
would be great from you. Otherwise, let me know if you'd rather take this patch
through tracing instead (in case it looks good from your side).
> ---
>
> 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;
>
LGTM,
Reviewed-by: Daniel Gomez <da.gomez@samsung.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 5/5] module: Rename MAX_PARAM_PREFIX_LEN to __MODULE_NAME_LEN
2025-06-30 14:32 ` [PATCH 5/5] module: Rename MAX_PARAM_PREFIX_LEN to __MODULE_NAME_LEN Petr Pavlu
@ 2025-07-28 6:48 ` Daniel Gomez
0 siblings, 0 replies; 15+ messages in thread
From: Daniel Gomez @ 2025-07-28 6:48 UTC (permalink / raw)
To: Petr Pavlu, Luis Chamberlain, Sami Tolvanen, Daniel Gomez
Cc: linux-modules, linux-kernel
On 30/06/2025 16.32, Petr Pavlu wrote:
> 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") \
LGTM,
Reviewed-by: Daniel Gomez <da.gomez@samsung.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 4/5] tracing: Replace MAX_PARAM_PREFIX_LEN with MODULE_NAME_LEN
2025-07-28 6:48 ` Daniel Gomez
@ 2025-07-28 14:34 ` Steven Rostedt
0 siblings, 0 replies; 15+ messages in thread
From: Steven Rostedt @ 2025-07-28 14:34 UTC (permalink / raw)
To: Daniel Gomez
Cc: Petr Pavlu, Luis Chamberlain, Sami Tolvanen, Daniel Gomez,
Masami Hiramatsu, linux-modules, linux-kernel
On Mon, 28 Jul 2025 08:48:01 +0200
Daniel Gomez <da.gomez@kernel.org> wrote:
> On 30/06/2025 16.32, Petr Pavlu wrote:
> > 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>
>
> Steven, Masami,
>
> I'm planning to merge these series into modules-next. I think and Ack/Review
> would be great from you. Otherwise, let me know if you'd rather take this patch
> through tracing instead (in case it looks good from your side).
Acked-by: Steven Rostedt (Google) <rostedt@goodmis.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.
Yeah, that is probably something that should be done too.
Thanks,
-- Steve
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/5] module: Fix minor problems related to MODULE_NAME_LEN
2025-06-30 14:32 [PATCH 0/5] module: Fix minor problems related to MODULE_NAME_LEN Petr Pavlu
` (4 preceding siblings ...)
2025-06-30 14:32 ` [PATCH 5/5] module: Rename MAX_PARAM_PREFIX_LEN to __MODULE_NAME_LEN Petr Pavlu
@ 2025-07-31 12:03 ` Daniel Gomez
5 siblings, 0 replies; 15+ messages in thread
From: Daniel Gomez @ 2025-07-31 12:03 UTC (permalink / raw)
To: Luis Chamberlain, Sami Tolvanen, Petr Pavlu; +Cc: linux-modules, linux-kernel
On Mon, 30 Jun 2025 16:32:31 +0200, Petr Pavlu wrote:
> Fix a few minor problems related to MODULE_NAME_LEN and
> MAX_PARAM_PREFIX_LEN, and clean up their usage.
>
> Petr Pavlu (5):
> module: Prevent silent truncation of module name in delete_module(2)
> module: Remove unnecessary +1 from last_unloaded_module::name size
> module: Restore the moduleparam prefix length check
> tracing: Replace MAX_PARAM_PREFIX_LEN with MODULE_NAME_LEN
> module: Rename MAX_PARAM_PREFIX_LEN to __MODULE_NAME_LEN
>
> [...]
Applied, thanks!
[1/5] module: Prevent silent truncation of module name in delete_module(2)
commit: a6323bd4e611567913e23df5b58f2d4e4da06789
[2/5] module: Remove unnecessary +1 from last_unloaded_module::name size
commit: 6c171b2ccfe677ca97fc5334f853807959f26589
[3/5] module: Restore the moduleparam prefix length check
commit: bdc877ba6b7ff1b6d2ebeff11e63da4a50a54854
[4/5] tracing: Replace MAX_PARAM_PREFIX_LEN with MODULE_NAME_LEN
commit: a7c54b2b41dd1f6ec780e7fbfb13f70c64c9731d
[5/5] module: Rename MAX_PARAM_PREFIX_LEN to __MODULE_NAME_LEN
commit: 40a826bd6c82ae45cfd3a19cd2a60a10f56b74c0
Best regards,
--
Daniel Gomez <da.gomez@samsung.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2025-07-31 12:03 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-30 14:32 [PATCH 0/5] module: Fix minor problems related to MODULE_NAME_LEN Petr Pavlu
2025-06-30 14:32 ` [PATCH 1/5] module: Prevent silent truncation of module name in delete_module(2) Petr Pavlu
2025-07-16 19:47 ` Daniel Gomez
2025-06-30 14:32 ` [PATCH 2/5] module: Remove unnecessary +1 from last_unloaded_module::name size Petr Pavlu
2025-07-16 19:49 ` Daniel Gomez
2025-06-30 14:32 ` [PATCH 3/5] module: Restore the moduleparam prefix length check Petr Pavlu
2025-07-17 19:23 ` Daniel Gomez
2025-07-21 9:21 ` Petr Pavlu
2025-07-28 6:43 ` Daniel Gomez
2025-06-30 14:32 ` [PATCH 4/5] tracing: Replace MAX_PARAM_PREFIX_LEN with MODULE_NAME_LEN Petr Pavlu
2025-07-28 6:48 ` Daniel Gomez
2025-07-28 14:34 ` Steven Rostedt
2025-06-30 14:32 ` [PATCH 5/5] module: Rename MAX_PARAM_PREFIX_LEN to __MODULE_NAME_LEN Petr Pavlu
2025-07-28 6:48 ` Daniel Gomez
2025-07-31 12:03 ` [PATCH 0/5] module: Fix minor problems related to MODULE_NAME_LEN Daniel Gomez
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).