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

[Petr Pavlu <petr.pavlu@suse.com>]

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.

-- 
Thanks,
Petr