[PATCH v3 0/4] module: logging and code improvements

[PATCH v3 0/4] module: logging and code improvements

[Jinchao Wang]

This series of patches cleans up and refactors the kernel's module loading
code. The goal is to make the subsystem's logging clearer and its internal
logic more straightforward for developers to understand.

The patches in this series: 

- module: signing: Use pr_err for signature rejection
  Makes module signature rejection messages more visible.
- module: show why force load fails 
  Adds a reason to the error message when force loading is disabled.
- module: centralize no-versions force load check
  Refactors the code to centralize the "no versions" force load check.
- module: separate vermagic and livepatch checks
  Improves code organization by separating vermagic and livepatch checks.

---
Changes from v2:
- show mod->name in try_to_force_load
- fix a introduced bug in patch 3

Changes from v1:
- A patch was dropped because it was based on a misunderstanding
  of the ignore versioning flag's original intent.

v2:
https://lore.kernel.org/all/20250825091545.18607-1-wangjinchao600@gmail.com/

v1 :
https://lore.kernel.org/all/20250822125454.1287066-1-wangjinchao600@gmail.com

Jinchao Wang (4):
  module: signing: Use pr_err for signature rejection
  module: show why force load fails
  module: centralize no-versions force load check
  module: separate vermagic and livepatch checks

 kernel/module/main.c    | 14 ++++++++------
 kernel/module/signing.c |  2 +-
 kernel/module/version.c | 10 ++++++++--
 3 files changed, 17 insertions(+), 9 deletions(-)

-- 
2.43.0

[PATCH v3 1/4] module: signing: Use pr_err for signature rejection

[Jinchao Wang]

Make module signature rejection messages more visible by using pr_err
instead of pr_notice.

Signed-off-by: Jinchao Wang <wangjinchao600@gmail.com>
---
 kernel/module/signing.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/module/signing.c b/kernel/module/signing.c
index a2ff4242e623..557cb795fa31 100644
--- a/kernel/module/signing.c
+++ b/kernel/module/signing.c
@@ -117,7 +117,7 @@ int module_sig_check(struct load_info *info, int flags)
 	}
 
 	if (is_module_sig_enforced()) {
-		pr_notice("Loading of %s is rejected\n", reason);
+		pr_err("Loading of %s is rejected\n", reason);
 		return -EKEYREJECTED;
 	}
 
-- 
2.43.0

Re: [PATCH v3 1/4] module: signing: Use pr_err for signature rejection

[Petr Pavlu]

On 8/29/25 10:49 AM, Jinchao Wang wrote:
> Make module signature rejection messages more visible by using pr_err
> instead of pr_notice.
> 
> Signed-off-by: Jinchao Wang <wangjinchao600@gmail.com>

Reviewed-by: Petr Pavlu <petr.pavlu@suse.com>

-- 
Thanks,
Petr

Re: [PATCH v3 1/4] module: signing: Use pr_err for signature rejection

[Daniel Gomez]

On 29/08/2025 10.49, Jinchao Wang wrote:
> Make module signature rejection messages more visible by using pr_err
> instead of pr_notice.

Can you elaborate a bit more? Why is this needed?

IMO, I don't think making it more visible is enough rational to increase the
level.

Re: [PATCH v3 1/4] module: signing: Use pr_err for signature rejection

[Jinchao Wang]

On 9/2/25 02:18, Daniel Gomez wrote:
> On 29/08/2025 10.49, Jinchao Wang wrote:
>> Make module signature rejection messages more visible by using pr_err
>> instead of pr_notice.
> 
> Can you elaborate a bit more? Why is this needed?
> 
> IMO, I don't think making it more visible is enough rational to increase the
> level.

Thank you for the feedback.

When using dmesg, pr_err is displayed in red, pr_warn in yellow, and
pr_notice/pr_info in the default color. This makes pr_err more visible
to users.

In the kernel tree, there are around 4161 pr_err calls across 20000
files, compared to 276 pr_notice calls across 827 files. From reviewing
them, pr_notice is typically used in default or informational branches,
while pr_err is mostly used in error paths.

Since this rejection path returns -EKEYREJECTED and prevents the
operation from proceeding, it aligns more closely with other uses of
pr_err than with pr_notice. For these reasons, I believe pr_err is the
appropriate choice here.

-- 
Best regards,
Jinchao

[PATCH v3 2/4] module: show why force load fails

[Jinchao Wang]

Include reason in error message when force loading is disabled.

Signed-off-by: Jinchao Wang <wangjinchao600@gmail.com>
---
 kernel/module/main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/module/main.c b/kernel/module/main.c
index c66b26184936..e7484c6ce6e3 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -1083,6 +1083,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, force load is not supported\n", mod->name, reason);
 	return -ENOEXEC;
 #endif
 }
-- 
2.43.0

Re: [PATCH v3 2/4] module: show why force load fails

[Petr Pavlu]

On 8/29/25 10:49 AM, Jinchao Wang wrote:
> Include reason in error message when force loading is disabled.
> 
> Signed-off-by: Jinchao Wang <wangjinchao600@gmail.com>

Reviewed-by: Petr Pavlu <petr.pavlu@suse.com>

-- 
Thanks,
Petr

Re: [PATCH v3 2/4] module: show why force load fails

[Daniel Gomez]

On 29/08/2025 10.49, Jinchao Wang wrote:
> Include reason in error message when force loading is disabled.
> 
> Signed-off-by: Jinchao Wang <wangjinchao600@gmail.com>
> ---
>  kernel/module/main.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index c66b26184936..e7484c6ce6e3 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -1083,6 +1083,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, force load is not supported\n", mod->name, reason);
>  	return -ENOEXEC;
>  #endif
>  }

I don't think is good to inform via kernel log buffer what the kernel supports
or what not. And definitely, not as an error.

Re: [PATCH v3 2/4] module: show why force load fails

[Jinchao Wang]

On 9/2/25 02:29, Daniel Gomez wrote:
> On 29/08/2025 10.49, Jinchao Wang wrote:
>> Include reason in error message when force loading is disabled.
>>
>> Signed-off-by: Jinchao Wang<wangjinchao600@gmail.com>
>> ---
>>   kernel/module/main.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/kernel/module/main.c b/kernel/module/main.c
>> index c66b26184936..e7484c6ce6e3 100644
>> --- a/kernel/module/main.c
>> +++ b/kernel/module/main.c
>> @@ -1083,6 +1083,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, force load is not supported\n", mod->name, reason);
>>   	return -ENOEXEC;
>>   #endif
>>   }
> I don't think is good to inform via kernel log buffer what the kernel supports
> or what not. And definitely, not as an error.
> 

Thank you for the feedback.

When debugging syzkaller, I noticed that insmod only reports a generic
failure. To understand the exact reason, I needed to trace the kernel.
This patch was meant to make it more convenient to see the precise
cause directly.

In my view, if the caller cannot perform the requested operation, that
represents an error, and the kernel log buffer is the natural place to
report the reason. This makes debugging and testing easier without
requiring additional tracing.

-- 
Best regards,
Jinchao

[PATCH v3 3/4] module: centralize no-versions force load check

[Jinchao Wang]

Move try_to_force_load() call from check_version() to
check_modstruct_version() to handle "no versions" case only once before
other version checks.

Suggested-by: Petr Pavlu <petr.pavlu@suse.com>
Signed-off-by: Jinchao Wang <wangjinchao600@gmail.com>
---
 kernel/module/version.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/kernel/module/version.c b/kernel/module/version.c
index 2beefeba82d9..7a458c681049 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
@@ -80,6 +80,7 @@ int check_modstruct_version(const struct load_info *info,
 		.gplok	= true,
 	};
 	bool have_symbol;
+	char *reason;
 
 	/*
 	 * Since this should be found in kernel (which can't be removed), no
@@ -90,6 +91,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) {
+		reason = "no versions for imported symbols";
+		return try_to_force_load(mod, reason) == 0;
+	}
 	return check_version(info, "module_layout", mod, fsa.crc);
 }
 
-- 
2.43.0

Re: [PATCH v3 3/4] module: centralize no-versions force load check

[Petr Pavlu]

On 8/29/25 10:49 AM, Jinchao Wang wrote:
> Move try_to_force_load() call from check_version() to
> check_modstruct_version() to handle "no versions" case only once before
> other version checks.
> 
> Suggested-by: Petr Pavlu <petr.pavlu@suse.com>
> Signed-off-by: Jinchao Wang <wangjinchao600@gmail.com>
> ---
>  kernel/module/version.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/module/version.c b/kernel/module/version.c
> index 2beefeba82d9..7a458c681049 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
> @@ -80,6 +80,7 @@ int check_modstruct_version(const struct load_info *info,
>  		.gplok	= true,
>  	};
>  	bool have_symbol;
> +	char *reason;
>  
>  	/*
>  	 * Since this should be found in kernel (which can't be removed), no
> @@ -90,6 +91,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) {
> +		reason = "no versions for imported symbols";
> +		return try_to_force_load(mod, reason) == 0;
> +	}
>  	return check_version(info, "module_layout", mod, fsa.crc);
>  }
>  

Nit: I would prefer this to be formatted as:

+	/* 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;
+

Nonetheless, it looks ok to me functionality-wise.

Reviewed-by: Petr Pavlu <petr.pavlu@suse.com>

-- 
Thanks,
Petr

[PATCH v3 4/4] module: separate vermagic and livepatch checks

[Jinchao Wang]

Rename check_modinfo() to check_modinfo_vermagic() to clarify it only
checks vermagic compatibility.

Move livepatch check to happen after vermagic check in early_mod_check(),
creating proper separation of concerns.
No functional changes, just clearer code organization.

Signed-off-by: Jinchao Wang <wangjinchao600@gmail.com>
---
 kernel/module/main.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/kernel/module/main.c b/kernel/module/main.c
index e7484c6ce6e3..98a678a18300 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2571,7 +2571,8 @@ static void module_augment_kernel_taints(struct module *mod, struct load_info *i
 
 }
 
-static int check_modinfo(struct module *mod, struct load_info *info, int flags)
+static int check_modinfo_vermagic(struct module *mod, struct load_info *info,
+				  int flags)
 {
 	const char *modmagic = get_modinfo(info, "vermagic");
 	int err;
@@ -2590,10 +2591,6 @@ static int check_modinfo(struct module *mod, struct load_info *info, int flags)
 		return -ENOEXEC;
 	}
 
-	err = check_modinfo_livepatch(mod, info);
-	if (err)
-		return err;
-
 	return 0;
 }
 
@@ -3334,7 +3331,11 @@ static int early_mod_check(struct load_info *info, int flags)
 	if (!check_modstruct_version(info, info->mod))
 		return -ENOEXEC;
 
-	err = check_modinfo(info->mod, info, flags);
+	err = check_modinfo_vermagic(info->mod, info, flags);
+	if (err)
+		return err;
+
+	err = check_modinfo_livepatch(info->mod, info);
 	if (err)
 		return err;
 
-- 
2.43.0

Re: [PATCH v3 4/4] module: separate vermagic and livepatch checks

[Petr Pavlu]

On 8/29/25 10:49 AM, Jinchao Wang wrote:
> Rename check_modinfo() to check_modinfo_vermagic() to clarify it only
> checks vermagic compatibility.
> 
> Move livepatch check to happen after vermagic check in early_mod_check(),
> creating proper separation of concerns.
> No functional changes, just clearer code organization.
> 
> Signed-off-by: Jinchao Wang <wangjinchao600@gmail.com>

Reviewed-by: Petr Pavlu <petr.pavlu@suse.com>

-- 
Thanks,
Petr