kernel/module/version.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
When there is no version information, modprobe and insmod only
report "invalid format".
Print the actual cause to make it easier to diagnose the issue.
This helps developers quickly identify version-related module
loading failures.
Signed-off-by: Wang Jinchao <wangjinchao600@gmail.com>
---
kernel/module/version.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/kernel/module/version.c b/kernel/module/version.c
index 2beefeba82d9..bc28c697ff3a 100644
--- a/kernel/module/version.c
+++ b/kernel/module/version.c
@@ -42,8 +42,10 @@ int check_version(const struct load_info *info,
}
/* No versions at all? modprobe --force does this. */
- if (versindex == 0)
+ if (versindex == 0) {
+ pr_debug("No version info for module %s\n", info->name);
return try_to_force_load(mod, symname) == 0;
+ }
versions = (void *)sechdrs[versindex].sh_addr;
num_versions = sechdrs[versindex].sh_size
--
2.43.0On 7/21/25 6:52 AM, Wang Jinchao wrote:
> When there is no version information, modprobe and insmod only
> report "invalid format".
> Print the actual cause to make it easier to diagnose the issue.
> This helps developers quickly identify version-related module
> loading failures.
> Signed-off-by: Wang Jinchao <wangjinchao600@gmail.com>
> ---
> kernel/module/version.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/module/version.c b/kernel/module/version.c
> index 2beefeba82d9..bc28c697ff3a 100644
> --- a/kernel/module/version.c
> +++ b/kernel/module/version.c
> @@ -42,8 +42,10 @@ int check_version(const struct load_info *info,
> }
>
> /* No versions at all? modprobe --force does this. */
> - if (versindex == 0)
> + if (versindex == 0) {
> + pr_debug("No version info for module %s\n", info->name);
> return try_to_force_load(mod, symname) == 0;
> + }
>
> versions = (void *)sechdrs[versindex].sh_addr;
> num_versions = sechdrs[versindex].sh_size
I think it would be better to instead improve the behavior of
try_to_force_load(). The function should print the error reason prior to
returning -ENOEXEC. This would also help its two other callers,
check_modinfo() and check_export_symbol_versions().
Additionally, I suggest moving the check to ensure version information
is available for imported symbols earlier in the loading process.
A suitable place might be check_modstruct_version(). This way the check
is performed only once per module.
The following is a prototype patch:
diff --git a/kernel/module/main.c b/kernel/module/main.c
index c2c08007029d..c1ccd343e8c3 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -1053,6 +1053,7 @@ int try_to_force_load(struct module *mod, const char *reason)
add_taint_module(mod, TAINT_FORCED_MODULE, LOCKDEP_NOW_UNRELIABLE);
return 0;
#else
+ pr_err("%s: %s\n", mod->name, reason);
return -ENOEXEC;
#endif
}
diff --git a/kernel/module/version.c b/kernel/module/version.c
index 2beefeba82d9..4d9ebf0834de 100644
--- a/kernel/module/version.c
+++ b/kernel/module/version.c
@@ -41,9 +41,9 @@ int check_version(const struct load_info *info,
return 1;
}
- /* No versions at all? modprobe --force does this. */
+ /* No versions? Ok, already tainted in check_modstruct_version(). */
if (versindex == 0)
- return try_to_force_load(mod, symname) == 0;
+ return 1;
versions = (void *)sechdrs[versindex].sh_addr;
num_versions = sechdrs[versindex].sh_size
@@ -90,6 +90,11 @@ int check_modstruct_version(const struct load_info *info,
have_symbol = find_symbol(&fsa);
BUG_ON(!have_symbol);
+ /* No versions at all? modprobe --force does this. */
+ if (!info->index.vers && !info->index.vers_ext_crc)
+ return try_to_force_load(
+ mod, "no versions for imported symbols") == 0;
+
return check_version(info, "module_layout", mod, fsa.crc);
}
As a side note, I'm confused why with CONFIG_MODULE_FORCE_LOAD=y, the
code treats missing modversions for imported symbols as ok, even without
MODULE_INIT_IGNORE_MODVERSIONS. This is at least consistent with the
handling of missing vermagic, but it seems this behavior should be
stricter.
--
Thanks,
Petr
On 7/21/25 22:40, Petr Pavlu wrote:
> On 7/21/25 6:52 AM, Wang Jinchao wrote:
>> When there is no version information, modprobe and insmod only
>> report "invalid format".
>> Print the actual cause to make it easier to diagnose the issue.
>> This helps developers quickly identify version-related module
>> loading failures.
>> Signed-off-by: Wang Jinchao <wangjinchao600@gmail.com>
>> ---
>> kernel/module/version.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/module/version.c b/kernel/module/version.c
>> index 2beefeba82d9..bc28c697ff3a 100644
>> --- a/kernel/module/version.c
>> +++ b/kernel/module/version.c
>> @@ -42,8 +42,10 @@ int check_version(const struct load_info *info,
>> }
>>
>> /* No versions at all? modprobe --force does this. */
>> - if (versindex == 0)
>> + if (versindex == 0) {
>> + pr_debug("No version info for module %s\n", info->name);
>> return try_to_force_load(mod, symname) == 0;
>> + }
>>
>> versions = (void *)sechdrs[versindex].sh_addr;
>> num_versions = sechdrs[versindex].sh_size
>
> I think it would be better to instead improve the behavior of
> try_to_force_load(). The function should print the error reason prior to
> returning -ENOEXEC. This would also help its two other callers,
> check_modinfo() and check_export_symbol_versions().
>
> Additionally, I suggest moving the check to ensure version information
> is available for imported symbols earlier in the loading process.
> A suitable place might be check_modstruct_version(). This way the check
> is performed only once per module.
>
> The following is a prototype patch:
>
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index c2c08007029d..c1ccd343e8c3 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -1053,6 +1053,7 @@ int try_to_force_load(struct module *mod, const char *reason)
> add_taint_module(mod, TAINT_FORCED_MODULE, LOCKDEP_NOW_UNRELIABLE);
> return 0;
> #else
> + pr_err("%s: %s\n", mod->name, reason);
> return -ENOEXEC;
> #endif
> }
> diff --git a/kernel/module/version.c b/kernel/module/version.c
> index 2beefeba82d9..4d9ebf0834de 100644
> --- a/kernel/module/version.c
> +++ b/kernel/module/version.c
> @@ -41,9 +41,9 @@ int check_version(const struct load_info *info,
> return 1;
> }
>
> - /* No versions at all? modprobe --force does this. */
> + /* No versions? Ok, already tainted in check_modstruct_version(). */
> if (versindex == 0)
> - return try_to_force_load(mod, symname) == 0;
> + return 1;
>
> versions = (void *)sechdrs[versindex].sh_addr;
> num_versions = sechdrs[versindex].sh_size
> @@ -90,6 +90,11 @@ int check_modstruct_version(const struct load_info *info,
> have_symbol = find_symbol(&fsa);
> BUG_ON(!have_symbol);
>
> + /* No versions at all? modprobe --force does this. */
> + if (!info->index.vers && !info->index.vers_ext_crc)
> + return try_to_force_load(
> + mod, "no versions for imported symbols") == 0;
> +
> return check_version(info, "module_layout", mod, fsa.crc);
> }
>
>
> As a side note, I'm confused why with CONFIG_MODULE_FORCE_LOAD=y, the
> code treats missing modversions for imported symbols as ok, even without
> MODULE_INIT_IGNORE_MODVERSIONS. This is at least consistent with the
> handling of missing vermagic, but it seems this behavior should be
> stricter.
>
When debugging syzkaller, I noticed that the insmod command always
reports errors. However, to get the exact information, I need to trace
the kernel, so I casually submitted this patch.
Based on your response, I also feel that the meaning of force_load here
is somewhat unclear. It would be better to create a mask or a clear list
to indicate which fields can be forced and which cannot. Once this is
clear, we can create a function named may_force_check().
In addition, check_modstruct_version also calls check_version to handle
tainting. So there's a minor issue with the logic in your example patch.
--
Best regards,
Jinchao
On 7/22/25 5:08 AM, Wang Jinchao wrote:
> On 7/21/25 22:40, Petr Pavlu wrote:
>> On 7/21/25 6:52 AM, Wang Jinchao wrote:
>>> When there is no version information, modprobe and insmod only
>>> report "invalid format".
>>> Print the actual cause to make it easier to diagnose the issue.
>>> This helps developers quickly identify version-related module
>>> loading failures.
>>> Signed-off-by: Wang Jinchao <wangjinchao600@gmail.com>
>>> ---
>>> kernel/module/version.c | 4 +++-
>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/kernel/module/version.c b/kernel/module/version.c
>>> index 2beefeba82d9..bc28c697ff3a 100644
>>> --- a/kernel/module/version.c
>>> +++ b/kernel/module/version.c
>>> @@ -42,8 +42,10 @@ int check_version(const struct load_info *info,
>>> }
>>> /* No versions at all? modprobe --force does this. */
>>> - if (versindex == 0)
>>> + if (versindex == 0) {
>>> + pr_debug("No version info for module %s\n", info->name);
>>> return try_to_force_load(mod, symname) == 0;
>>> + }
>>> versions = (void *)sechdrs[versindex].sh_addr;
>>> num_versions = sechdrs[versindex].sh_size
>>
>> I think it would be better to instead improve the behavior of
>> try_to_force_load(). The function should print the error reason prior to
>> returning -ENOEXEC. This would also help its two other callers,
>> check_modinfo() and check_export_symbol_versions().
>>
>> Additionally, I suggest moving the check to ensure version information
>> is available for imported symbols earlier in the loading process.
>> A suitable place might be check_modstruct_version(). This way the check
>> is performed only once per module.
>>
>> The following is a prototype patch:
>>
>> diff --git a/kernel/module/main.c b/kernel/module/main.c
>> index c2c08007029d..c1ccd343e8c3 100644
>> --- a/kernel/module/main.c
>> +++ b/kernel/module/main.c
>> @@ -1053,6 +1053,7 @@ int try_to_force_load(struct module *mod, const char *reason)
>> add_taint_module(mod, TAINT_FORCED_MODULE, LOCKDEP_NOW_UNRELIABLE);
>> return 0;
>> #else
>> + pr_err("%s: %s\n", mod->name, reason);
>> return -ENOEXEC;
>> #endif
>> }
>> diff --git a/kernel/module/version.c b/kernel/module/version.c
>> index 2beefeba82d9..4d9ebf0834de 100644
>> --- a/kernel/module/version.c
>> +++ b/kernel/module/version.c
>> @@ -41,9 +41,9 @@ int check_version(const struct load_info *info,
>> return 1;
>> }
>> - /* No versions at all? modprobe --force does this. */
>> + /* No versions? Ok, already tainted in check_modstruct_version(). */
>> if (versindex == 0)
>> - return try_to_force_load(mod, symname) == 0;
>> + return 1;
>> versions = (void *)sechdrs[versindex].sh_addr;
>> num_versions = sechdrs[versindex].sh_size
>> @@ -90,6 +90,11 @@ int check_modstruct_version(const struct load_info *info,
>> have_symbol = find_symbol(&fsa);
>> BUG_ON(!have_symbol);
>> + /* No versions at all? modprobe --force does this. */
>> + if (!info->index.vers && !info->index.vers_ext_crc)
>> + return try_to_force_load(
>> + mod, "no versions for imported symbols") == 0;
>> +
>> return check_version(info, "module_layout", mod, fsa.crc);
>> }
>>
>> As a side note, I'm confused why with CONFIG_MODULE_FORCE_LOAD=y, the
>> code treats missing modversions for imported symbols as ok, even without
>> MODULE_INIT_IGNORE_MODVERSIONS. This is at least consistent with the
>> handling of missing vermagic, but it seems this behavior should be
>> stricter.
>>
> When debugging syzkaller, I noticed that the insmod command always reports errors. However, to get the exact information, I need to trace the kernel, so I casually submitted this patch.
>
> Based on your response, I also feel that the meaning of force_load here is somewhat unclear. It would be better to create a mask or a clear list to indicate which fields can be forced and which cannot. Once this is clear, we can create a function named may_force_check().
I cannot find an explicit reason in the Git history why a missing
vermagic is treated as if the module was loaded with
MODULE_INIT_IGNORE_VERMAGIC, and similarly why missing modversions data
is treated as if the module was loaded with
MODULE_INIT_IGNORE_MODVERSIONS.
I would argue that a more sensible behavior would be to consider
a missing vermagic as an error and allow loading the module only if
MODULE_INIT_IGNORE_VERMAGIC is explicitly specified. And analogously for
missing modversions and MODULE_INIT_IGNORE_MODVERSIONS.
Nonetheless, if I understand correctly, this should be mostly separate
from your issue.
>
> In addition, check_modstruct_version also calls check_version to handle tainting. So there's a minor issue with the logic in your example patch.
>
I'm not sure I follow here. My example lifts the try_to_force_load()
call from check_version() to check_modstruct_version(), and should still
result in tainting the kernel.
--
Thanks,
Petr
On 7/22/25 10:25 AM, Petr Pavlu wrote:
> On 7/22/25 5:08 AM, Wang Jinchao wrote:
>> On 7/21/25 22:40, Petr Pavlu wrote:
>>> On 7/21/25 6:52 AM, Wang Jinchao wrote:
>>>> When there is no version information, modprobe and insmod only
>>>> report "invalid format".
>>>> Print the actual cause to make it easier to diagnose the issue.
>>>> This helps developers quickly identify version-related module
>>>> loading failures.
>>>> Signed-off-by: Wang Jinchao <wangjinchao600@gmail.com>
>>>> ---
>>>> kernel/module/version.c | 4 +++-
>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/kernel/module/version.c b/kernel/module/version.c
>>>> index 2beefeba82d9..bc28c697ff3a 100644
>>>> --- a/kernel/module/version.c
>>>> +++ b/kernel/module/version.c
>>>> @@ -42,8 +42,10 @@ int check_version(const struct load_info *info,
>>>> }
>>>> /* No versions at all? modprobe --force does this. */
>>>> - if (versindex == 0)
>>>> + if (versindex == 0) {
>>>> + pr_debug("No version info for module %s\n", info->name);
>>>> return try_to_force_load(mod, symname) == 0;
>>>> + }
>>>> versions = (void *)sechdrs[versindex].sh_addr;
>>>> num_versions = sechdrs[versindex].sh_size
>>>
>>> I think it would be better to instead improve the behavior of
>>> try_to_force_load(). The function should print the error reason prior to
>>> returning -ENOEXEC. This would also help its two other callers,
>>> check_modinfo() and check_export_symbol_versions().
>>>
>>> Additionally, I suggest moving the check to ensure version information
>>> is available for imported symbols earlier in the loading process.
>>> A suitable place might be check_modstruct_version(). This way the check
>>> is performed only once per module.
>>>
>>> The following is a prototype patch:
>>>
>>> diff --git a/kernel/module/main.c b/kernel/module/main.c
>>> index c2c08007029d..c1ccd343e8c3 100644
>>> --- a/kernel/module/main.c
>>> +++ b/kernel/module/main.c
>>> @@ -1053,6 +1053,7 @@ int try_to_force_load(struct module *mod, const char *reason)
>>> add_taint_module(mod, TAINT_FORCED_MODULE, LOCKDEP_NOW_UNRELIABLE);
>>> return 0;
>>> #else
>>> + pr_err("%s: %s\n", mod->name, reason);
>>> return -ENOEXEC;
>>> #endif
>>> }
>>> diff --git a/kernel/module/version.c b/kernel/module/version.c
>>> index 2beefeba82d9..4d9ebf0834de 100644
>>> --- a/kernel/module/version.c
>>> +++ b/kernel/module/version.c
>>> @@ -41,9 +41,9 @@ int check_version(const struct load_info *info,
>>> return 1;
>>> }
>>> - /* No versions at all? modprobe --force does this. */
>>> + /* No versions? Ok, already tainted in check_modstruct_version(). */
>>> if (versindex == 0)
>>> - return try_to_force_load(mod, symname) == 0;
>>> + return 1;
>>> versions = (void *)sechdrs[versindex].sh_addr;
>>> num_versions = sechdrs[versindex].sh_size
>>> @@ -90,6 +90,11 @@ int check_modstruct_version(const struct load_info *info,
>>> have_symbol = find_symbol(&fsa);
>>> BUG_ON(!have_symbol);
>>> + /* No versions at all? modprobe --force does this. */
>>> + if (!info->index.vers && !info->index.vers_ext_crc)
>>> + return try_to_force_load(
>>> + mod, "no versions for imported symbols") == 0;
>>> +
>>> return check_version(info, "module_layout", mod, fsa.crc);
>>> }
>>>
>>> As a side note, I'm confused why with CONFIG_MODULE_FORCE_LOAD=y, the
>>> code treats missing modversions for imported symbols as ok, even without
>>> MODULE_INIT_IGNORE_MODVERSIONS. This is at least consistent with the
>>> handling of missing vermagic, but it seems this behavior should be
>>> stricter.
>>>
>> When debugging syzkaller, I noticed that the insmod command always reports errors. However, to get the exact information, I need to trace the kernel, so I casually submitted this patch.
>>
>> Based on your response, I also feel that the meaning of force_load here is somewhat unclear. It would be better to create a mask or a clear list to indicate which fields can be forced and which cannot. Once this is clear, we can create a function named may_force_check().
>
> I cannot find an explicit reason in the Git history why a missing
> vermagic is treated as if the module was loaded with
> MODULE_INIT_IGNORE_VERMAGIC, and similarly why missing modversions data
> is treated as if the module was loaded with
> MODULE_INIT_IGNORE_MODVERSIONS.
>
> I would argue that a more sensible behavior would be to consider
> a missing vermagic as an error and allow loading the module only if
> MODULE_INIT_IGNORE_VERMAGIC is explicitly specified. And analogously for
> missing modversions and MODULE_INIT_IGNORE_MODVERSIONS.
>
> Nonetheless, if I understand correctly, this should be mostly separate
> from your issue.
To answer my own confusion, the thing that I missed is that the
MODULE_INIT_IGNORE_* flags are available only for the finit_module
syscall, not for init_module. In the case of init_module, the force
logic is handled by kmod in userspace by stripping the relevant
modversions and vermagic data. This means that when using init_module,
the module loader cannot distinguish between a module that lacks this
data and one that has it deliberately removed. When finit_module was
introduced in 2012, commit 2f3238aebedb ("module: add flags arg to
sys_finit_module()") added the MODULE_INIT_IGNORE_* flags, and they were
simply implemented to mirror the kmod behavior.
-- Petr
On 8/26/25 15:20, Petr Pavlu wrote:
> On 7/22/25 10:25 AM, Petr Pavlu wrote:
>> On 7/22/25 5:08 AM, Wang Jinchao wrote:
>>> On 7/21/25 22:40, Petr Pavlu wrote:
>>>> On 7/21/25 6:52 AM, Wang Jinchao wrote:
>>>>> When there is no version information, modprobe and insmod only
>>>>> report "invalid format".
>>>>> Print the actual cause to make it easier to diagnose the issue.
>>>>> This helps developers quickly identify version-related module
>>>>> loading failures.
>>>>> Signed-off-by: Wang Jinchao <wangjinchao600@gmail.com>
>>>>> ---
>>>>> kernel/module/version.c | 4 +++-
>>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/kernel/module/version.c b/kernel/module/version.c
>>>>> index 2beefeba82d9..bc28c697ff3a 100644
>>>>> --- a/kernel/module/version.c
>>>>> +++ b/kernel/module/version.c
>>>>> @@ -42,8 +42,10 @@ int check_version(const struct load_info *info,
>>>>> }
>>>>> /* No versions at all? modprobe --force does this. */
>>>>> - if (versindex == 0)
>>>>> + if (versindex == 0) {
>>>>> + pr_debug("No version info for module %s\n", info->name);
>>>>> return try_to_force_load(mod, symname) == 0;
>>>>> + }
>>>>> versions = (void *)sechdrs[versindex].sh_addr;
>>>>> num_versions = sechdrs[versindex].sh_size
>>>>
>>>> I think it would be better to instead improve the behavior of
>>>> try_to_force_load(). The function should print the error reason prior to
>>>> returning -ENOEXEC. This would also help its two other callers,
>>>> check_modinfo() and check_export_symbol_versions().
>>>>
>>>> Additionally, I suggest moving the check to ensure version information
>>>> is available for imported symbols earlier in the loading process.
>>>> A suitable place might be check_modstruct_version(). This way the check
>>>> is performed only once per module.
>>>>
>>>> The following is a prototype patch:
>>>>
>>>> diff --git a/kernel/module/main.c b/kernel/module/main.c
>>>> index c2c08007029d..c1ccd343e8c3 100644
>>>> --- a/kernel/module/main.c
>>>> +++ b/kernel/module/main.c
>>>> @@ -1053,6 +1053,7 @@ int try_to_force_load(struct module *mod, const char *reason)
>>>> add_taint_module(mod, TAINT_FORCED_MODULE, LOCKDEP_NOW_UNRELIABLE);
>>>> return 0;
>>>> #else
>>>> + pr_err("%s: %s\n", mod->name, reason);
>>>> return -ENOEXEC;
>>>> #endif
>>>> }
>>>> diff --git a/kernel/module/version.c b/kernel/module/version.c
>>>> index 2beefeba82d9..4d9ebf0834de 100644
>>>> --- a/kernel/module/version.c
>>>> +++ b/kernel/module/version.c
>>>> @@ -41,9 +41,9 @@ int check_version(const struct load_info *info,
>>>> return 1;
>>>> }
>>>> - /* No versions at all? modprobe --force does this. */
>>>> + /* No versions? Ok, already tainted in check_modstruct_version(). */
>>>> if (versindex == 0)
>>>> - return try_to_force_load(mod, symname) == 0;
>>>> + return 1;
>>>> versions = (void *)sechdrs[versindex].sh_addr;
>>>> num_versions = sechdrs[versindex].sh_size
>>>> @@ -90,6 +90,11 @@ int check_modstruct_version(const struct load_info *info,
>>>> have_symbol = find_symbol(&fsa);
>>>> BUG_ON(!have_symbol);
>>>> + /* No versions at all? modprobe --force does this. */
>>>> + if (!info->index.vers && !info->index.vers_ext_crc)
>>>> + return try_to_force_load(
>>>> + mod, "no versions for imported symbols") == 0;
>>>> +
>>>> return check_version(info, "module_layout", mod, fsa.crc);
>>>> }
>>>>
>>>> As a side note, I'm confused why with CONFIG_MODULE_FORCE_LOAD=y, the
>>>> code treats missing modversions for imported symbols as ok, even without
>>>> MODULE_INIT_IGNORE_MODVERSIONS. This is at least consistent with the
>>>> handling of missing vermagic, but it seems this behavior should be
>>>> stricter.
>>>>
>>> When debugging syzkaller, I noticed that the insmod command always reports errors. However, to get the exact information, I need to trace the kernel, so I casually submitted this patch.
>>>
>>> Based on your response, I also feel that the meaning of force_load here is somewhat unclear. It would be better to create a mask or a clear list to indicate which fields can be forced and which cannot. Once this is clear, we can create a function named may_force_check().
>>
>> I cannot find an explicit reason in the Git history why a missing
>> vermagic is treated as if the module was loaded with
>> MODULE_INIT_IGNORE_VERMAGIC, and similarly why missing modversions data
>> is treated as if the module was loaded with
>> MODULE_INIT_IGNORE_MODVERSIONS.
>>
>> I would argue that a more sensible behavior would be to consider
>> a missing vermagic as an error and allow loading the module only if
>> MODULE_INIT_IGNORE_VERMAGIC is explicitly specified. And analogously for
>> missing modversions and MODULE_INIT_IGNORE_MODVERSIONS.
>>
>> Nonetheless, if I understand correctly, this should be mostly separate
>> from your issue.
>
> To answer my own confusion, the thing that I missed is that the
> MODULE_INIT_IGNORE_* flags are available only for the finit_module
> syscall, not for init_module. In the case of init_module, the force
> logic is handled by kmod in userspace by stripping the relevant
> modversions and vermagic data. This means that when using init_module,
> the module loader cannot distinguish between a module that lacks this
> data and one that has it deliberately removed. When finit_module was
> introduced in 2012, commit 2f3238aebedb ("module: add flags arg to
> sys_finit_module()") added the MODULE_INIT_IGNORE_* flags, and they were
> simply implemented to mirror the kmod behavior.
>
> -- Petr
The composition of 'force' and 'ignore' is confusing.
I learn much from your feedback, thank you very much.
--
Best regards,
Jinchao
On 7/22/25 16:25, Petr Pavlu wrote:
> On 7/22/25 5:08 AM, Wang Jinchao wrote:
>> On 7/21/25 22:40, Petr Pavlu wrote:
>>> On 7/21/25 6:52 AM, Wang Jinchao wrote:
>>>> When there is no version information, modprobe and insmod only
>>>> report "invalid format".
>>>> Print the actual cause to make it easier to diagnose the issue.
>>>> This helps developers quickly identify version-related module
>>>> loading failures.
>>>> Signed-off-by: Wang Jinchao <wangjinchao600@gmail.com>
>>>> ---
>>>> kernel/module/version.c | 4 +++-
>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/kernel/module/version.c b/kernel/module/version.c
>>>> index 2beefeba82d9..bc28c697ff3a 100644
>>>> --- a/kernel/module/version.c
>>>> +++ b/kernel/module/version.c
>>>> @@ -42,8 +42,10 @@ int check_version(const struct load_info *info,
>>>> }
>>>> /* No versions at all? modprobe --force does this. */
>>>> - if (versindex == 0)
>>>> + if (versindex == 0) {
>>>> + pr_debug("No version info for module %s\n", info->name);
>>>> return try_to_force_load(mod, symname) == 0;
>>>> + }
>>>> versions = (void *)sechdrs[versindex].sh_addr;
>>>> num_versions = sechdrs[versindex].sh_size
>>>
>>> I think it would be better to instead improve the behavior of
>>> try_to_force_load(). The function should print the error reason prior to
>>> returning -ENOEXEC. This would also help its two other callers,
>>> check_modinfo() and check_export_symbol_versions().
>>>
>>> Additionally, I suggest moving the check to ensure version information
>>> is available for imported symbols earlier in the loading process.
>>> A suitable place might be check_modstruct_version(). This way the check
>>> is performed only once per module.
>>>
>>> The following is a prototype patch:
>>>
>>> diff --git a/kernel/module/main.c b/kernel/module/main.c
>>> index c2c08007029d..c1ccd343e8c3 100644
>>> --- a/kernel/module/main.c
>>> +++ b/kernel/module/main.c
>>> @@ -1053,6 +1053,7 @@ int try_to_force_load(struct module *mod, const char *reason)
>>> add_taint_module(mod, TAINT_FORCED_MODULE, LOCKDEP_NOW_UNRELIABLE);
>>> return 0;
>>> #else
>>> + pr_err("%s: %s\n", mod->name, reason);
>>> return -ENOEXEC;
>>> #endif
>>> }
>>> diff --git a/kernel/module/version.c b/kernel/module/version.c
>>> index 2beefeba82d9..4d9ebf0834de 100644
>>> --- a/kernel/module/version.c
>>> +++ b/kernel/module/version.c
>>> @@ -41,9 +41,9 @@ int check_version(const struct load_info *info,
>>> return 1;
>>> }
>>> - /* No versions at all? modprobe --force does this. */
>>> + /* No versions? Ok, already tainted in check_modstruct_version(). */
>>> if (versindex == 0)
>>> - return try_to_force_load(mod, symname) == 0;
>>> + return 1;
>>> versions = (void *)sechdrs[versindex].sh_addr;
>>> num_versions = sechdrs[versindex].sh_size
>>> @@ -90,6 +90,11 @@ int check_modstruct_version(const struct load_info *info,
>>> have_symbol = find_symbol(&fsa);
>>> BUG_ON(!have_symbol);
>>> + /* No versions at all? modprobe --force does this. */
>>> + if (!info->index.vers && !info->index.vers_ext_crc)
>>> + return try_to_force_load(
>>> + mod, "no versions for imported symbols") == 0;
>>> +
>>> return check_version(info, "module_layout", mod, fsa.crc);
>>> }
>>>
>>> As a side note, I'm confused why with CONFIG_MODULE_FORCE_LOAD=y, the
>>> code treats missing modversions for imported symbols as ok, even without
>>> MODULE_INIT_IGNORE_MODVERSIONS. This is at least consistent with the
>>> handling of missing vermagic, but it seems this behavior should be
>>> stricter.
>>>
>> When debugging syzkaller, I noticed that the insmod command always reports errors. However, to get the exact information, I need to trace the kernel, so I casually submitted this patch.
>>
>> Based on your response, I also feel that the meaning of force_load here is somewhat unclear. It would be better to create a mask or a clear list to indicate which fields can be forced and which cannot. Once this is clear, we can create a function named may_force_check().
>
> I cannot find an explicit reason in the Git history why a missing
> vermagic is treated as if the module was loaded with
> MODULE_INIT_IGNORE_VERMAGIC, and similarly why missing modversions data
> is treated as if the module was loaded with
> MODULE_INIT_IGNORE_MODVERSIONS.
>
> I would argue that a more sensible behavior would be to consider
> a missing vermagic as an error and allow loading the module only if
> MODULE_INIT_IGNORE_VERMAGIC is explicitly specified. And analogously for
> missing modversions and MODULE_INIT_IGNORE_MODVERSIONS.
>
> Nonetheless, if I understand correctly, this should be mostly separate
> from your issue.
Got it, thanks for the explanation.
I agree it would be good to refactor the force-load logic to make the
behavior and options (e.g. ignoring modversions) more explicit.
I’d be happy to work on this in my spare time.
>
>>
>> In addition, check_modstruct_version also calls check_version to handle tainting. So there's a minor issue with the logic in your example patch.
>>
>
> I'm not sure I follow here. My example lifts the try_to_force_load()
> call from check_version() to check_modstruct_version(), and should still
> result in tainting the kernel.
>
You are right. I miss the botton half. :)
--
Best regards,
Jinchao
© 2016 - 2025 Red Hat, Inc.