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

[Wang Jinchao]

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 <wangjinchao...@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