Re: [PATCH] module: pr_debug when there is no version info

[Jinchao Wang <wangjinchao600@gmail.com>]

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