[PATCH v2 0/2] module: Fix memory deallocation on error path in move_module()

[PATCH v2 0/2] module: Fix memory deallocation on error path in move_module()

[Petr Pavlu]

The first patch is an actual fix. The second patch is a minor related
cleanup.

Changes since v1 [1]:
- Initialize t to MOD_MEM_NUM_TYPES in move_module(), instead of assigning
  the value later.
- Merge the definitions of the variables i and ret in move_module().

[1] https://lore.kernel.org/linux-modules/20250607161823.409691-1-petr.pavlu@suse.com/

Petr Pavlu (2):
  module: Fix memory deallocation on error path in move_module()
  module: Avoid unnecessary return value initialization in move_module()

 kernel/module/main.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)


base-commit: 52da431bf03b5506203bca27fe14a97895c80faf
-- 
2.49.0

[PATCH v2 1/2] module: Fix memory deallocation on error path in move_module()

[Petr Pavlu]

The function move_module() uses the variable t to track how many memory
types it has allocated and consequently how many should be freed if an
error occurs.

The variable is initially set to 0 and is updated when a call to
module_memory_alloc() fails. However, move_module() can fail for other
reasons as well, in which case t remains set to 0 and no memory is freed.

Fix the problem by initializing t to MOD_MEM_NUM_TYPES. Additionally, make
the deallocation loop more robust by not relying on the mod_mem_type_t enum
having a signed integer as its underlying type.

Fixes: c7ee8aebf6c0 ("module: add stop-grap sanity check on module memcpy()")
Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
Reviewed-by: Sami Tolvanen <samitolvanen@google.com>
---
 kernel/module/main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/module/main.c b/kernel/module/main.c
index 413ac6ea3702..9ac994b2f354 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2697,7 +2697,7 @@ static int find_module_sections(struct module *mod, struct load_info *info)
 static int move_module(struct module *mod, struct load_info *info)
 {
 	int i;
-	enum mod_mem_type t = 0;
+	enum mod_mem_type t = MOD_MEM_NUM_TYPES;
 	int ret = -ENOMEM;
 	bool codetag_section_found = false;
 
@@ -2776,7 +2776,7 @@ static int move_module(struct module *mod, struct load_info *info)
 	return 0;
 out_err:
 	module_memory_restore_rox(mod);
-	for (t--; t >= 0; t--)
+	while (t--)
 		module_memory_free(mod, t);
 	if (codetag_section_found)
 		codetag_free_module_sections(mod);
-- 
2.49.0

Re: [PATCH v2 1/2] module: Fix memory deallocation on error path in move_module()

[Daniel Gomez]

On 18/06/2025 14.26, Petr Pavlu wrote:
> The function move_module() uses the variable t to track how many memory
> types it has allocated and consequently how many should be freed if an
> error occurs.
> 
> The variable is initially set to 0 and is updated when a call to
> module_memory_alloc() fails. However, move_module() can fail for other
> reasons as well, in which case t remains set to 0 and no memory is freed.
> 
> Fix the problem by initializing t to MOD_MEM_NUM_TYPES. Additionally, make
> the deallocation loop more robust by not relying on the mod_mem_type_t enum
> having a signed integer as its underlying type.
> 
> Fixes: c7ee8aebf6c0 ("module: add stop-grap sanity check on module memcpy()")
> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
> Reviewed-by: Sami Tolvanen <samitolvanen@google.com>
> ---
>  kernel/module/main.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 413ac6ea3702..9ac994b2f354 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -2697,7 +2697,7 @@ static int find_module_sections(struct module *mod, struct load_info *info)
>  static int move_module(struct module *mod, struct load_info *info)
>  {
>  	int i;
> -	enum mod_mem_type t = 0;
> +	enum mod_mem_type t = MOD_MEM_NUM_TYPES;
>  	int ret = -ENOMEM;
>  	bool codetag_section_found = false;
>  
> @@ -2776,7 +2776,7 @@ static int move_module(struct module *mod, struct load_info *info)
>  	return 0;
>  out_err:
>  	module_memory_restore_rox(mod);
> -	for (t--; t >= 0; t--)
> +	while (t--)
>  		module_memory_free(mod, t);
>  	if (codetag_section_found)
>  		codetag_free_module_sections(mod);

Reviewed-by: Daniel Gomez <da.gomez@samsung.com>

Re: [PATCH v2 1/2] module: Fix memory deallocation on error path in move_module()

[Daniel Gomez]

On 18/06/2025 14.26, Petr Pavlu wrote:
> The function move_module() uses the variable t to track how many memory
> types it has allocated and consequently how many should be freed if an
> error occurs.
> 
> The variable is initially set to 0 and is updated when a call to
> module_memory_alloc() fails. However, move_module() can fail for other
> reasons as well, in which case t remains set to 0 and no memory is freed.
> 
> Fix the problem by initializing t to MOD_MEM_NUM_TYPES. Additionally, make
> the deallocation loop more robust by not relying on the mod_mem_type_t enum
> having a signed integer as its underlying type.

This is a follow up based on v1 discussion [1] regarding a way to reproduce
the leak.

[1]
https://lore.kernel.org/linux-modules/ae967353-71fa-4438-a84b-8f7e2815f485@kernel.org/

I tried using eBPF and moderr [2][3] to find a way to reproduce this. I run into
some issues but I applied some workarounds. If this is useful we can check at
how to solve this properly.

[2] moderr RFC:
https://lore.kernel.org/linux-modules/20250122-modules-error-injection-v1-0-910590a04fd5@samsung.com/

[3] updated moderr branch:
https://git.kernel.org/pub/scm/linux/kernel/git/da.gomez/linux.git/?h=b4%2Fmodules-error-injection

The logs below show an A/B test memory consumption comparison when a module
is loaded and leaked (A) because of the issue described in the commit log
vs when is correctly loaded/unloaded (B). Besides observing a huge leak,
kernel logs (dmesg) show the message "execmem: unable to allocate memory".
Note that we can't use kmemleak as we skip it with kmemleak_not_leak() in
module_memory_alloc(), except for is_rox memory.

Memory lost:
A test: 1220444 kB
B test: 892 kB

Thoughts?


Workflow to reproduce the issue (before applying this patchset):

1. Run moderr to return error in codetag_needs_module_section()

sudo moderr \
  -modname=brd
  --modfunc=module_codetag_needs_module_section \
  --error=1

Note: We can also force codetag_alloc_module_section() with:

sudo moderr \
--modname=brd \
--modfunc=module_codetag_alloc_module_section \
--error=-22
 
2. Run a loop to probe a module with:

while true; do modprobe brd; done

3. Monitor memory before/after and check delta:

Note: this output it's a wrapper around the above loop.

[*] Dropping caches...
[*] Wait...
[*] Starting memory: 6758564 kB
[*] Loop 200s...
[*] Wait...
[*] Dropping caches...
[*] Wait...
[*] Ending memory: 5538120 kB
[*] Memory lost: 1220444 kB
--- meminfo_before.txt  2025-06-30 10:29:49.880661145 +0000
+++ meminfo_after.txt   2025-06-30 10:33:20.091501671 +0000
@@ -1,38 +1,38 @@
 MemTotal:        8133036 kB
-MemFree:         6858820 kB
-MemAvailable:    6758652 kB
-Buffers:            2576 kB
-Cached:            73164 kB
+MemFree:         5620596 kB
+MemAvailable:    5538204 kB
+Buffers:            2552 kB
+Cached:           107256 kB
 SwapCached:            0 kB
-Active:            25448 kB
-Inactive:          95272 kB
-Active(anon):        176 kB
-Inactive(anon):    45684 kB
-Active(file):      25272 kB
-Inactive(file):    49588 kB
+Active:            67688 kB
+Inactive:          95284 kB
+Active(anon):        184 kB
+Inactive(anon):    54028 kB
+Active(file):      67504 kB
+Inactive(file):    41256 kB
 Unevictable:           0 kB
 Mlocked:               0 kB
 SwapTotal:             0 kB
 SwapFree:              0 kB
-Dirty:               468 kB
+Dirty:              2804 kB
 Writeback:             0 kB
-AnonPages:         45192 kB
-Mapped:            72028 kB
-Shmem:               916 kB
-KReclaimable:       5816 kB
-Slab:              42336 kB
-SReclaimable:       5816 kB
-SUnreclaim:        36520 kB
-KernelStack:        1984 kB
-PageTables:         2652 kB
+AnonPages:         53556 kB
+Mapped:           105784 kB
+Shmem:               936 kB
+KReclaimable:       7468 kB
+Slab:             176080 kB
+SReclaimable:       7468 kB
+SUnreclaim:       168612 kB
+KernelStack:        2016 kB
+PageTables:         5472 kB
 SecPageTables:         0 kB
 NFS_Unstable:          0 kB
 Bounce:                0 kB
 WritebackTmp:          0 kB
 CommitLimit:     3542228 kB
-Committed_AS:      89960 kB
+Committed_AS:      92956 kB
 VmallocTotal:   34359738367 kB
-VmallocUsed:        5112 kB
+VmallocUsed:     1041252 kB
 VmallocChunk:          0 kB
 Percpu:             1712 kB
 AnonHugePages:      4096 kB
@@ -49,6 +49,6 @@
 HugePages_Surp:        0
 Hugepagesize:       2048 kB
 Hugetlb:         1048576 kB
-DirectMap4k:       30548 kB
-DirectMap2M:     3115008 kB
-DirectMap1G:     7340032 kB
+DirectMap4k:      730964 kB
+DirectMap2M:     3463168 kB
+DirectMap1G:     6291456 kB

4. Check kernel logs

[   30.099667] tee (618): drop_caches: 3
[  221.695667] execmem: unable to allocate memory
{...}
[  226.701772] execmem_vmalloc: 1110 callbacks suppressed
{...}
[  231.705976] execmem_vmalloc: 1099 callbacks suppressed
{...}
[  231.742226] execmem: unable to allocate memory
[  231.747107] execmem: unable to allocate memory
[  240.109107] tee (92935): drop_caches: 3


Now, I repeated the test without error injection:

Loop:
while true; do modprobe brd && rmmod brd; done


[*] Dropping caches...
[*] Wait...
[*] Starting memory: 6747960 kB
Loop 100s...
[*] Wait...
[*] Dropping caches...
[*] Wait...
[*] Ending memory: 6747068 kB
[*] Memory lost: 892 kB
--- meminfo_before.txt  2025-06-30 11:21:44.922597662 +0000
+++ meminfo_after.txt   2025-06-30 11:23:35.023419139 +0000
@@ -1,38 +1,38 @@
 MemTotal:        8133036 kB
-MemFree:         6853796 kB
-MemAvailable:    6747960 kB
-Buffers:            2920 kB
-Cached:            61976 kB
+MemFree:         6852956 kB
+MemAvailable:    6747244 kB
+Buffers:            2540 kB
+Cached:            62124 kB
 SwapCached:            0 kB
-Active:            25968 kB
-Inactive:          83616 kB
-Active(anon):        176 kB
-Inactive(anon):    45744 kB
-Active(file):      25792 kB
-Inactive(file):    37872 kB
+Active:            26744 kB
+Inactive:          83120 kB
+Active(anon):        172 kB
+Inactive(anon):    46048 kB
+Active(file):      26572 kB
+Inactive(file):    37072 kB
 Unevictable:           0 kB
 Mlocked:               0 kB
 SwapTotal:             0 kB
 SwapFree:              0 kB
-Dirty:               996 kB
+Dirty:               452 kB
 Writeback:             0 kB
-AnonPages:         45116 kB
-Mapped:            54632 kB
-Shmem:               928 kB
-KReclaimable:       5680 kB
-Slab:              42392 kB
-SReclaimable:       5680 kB
-SUnreclaim:        36712 kB
-KernelStack:        1888 kB
-PageTables:         2468 kB
+AnonPages:         45372 kB
+Mapped:            59120 kB
+Shmem:               992 kB
+KReclaimable:       5940 kB
+Slab:              43056 kB
+SReclaimable:       5940 kB
+SUnreclaim:        37116 kB
+KernelStack:        1904 kB
+PageTables:         2460 kB
 SecPageTables:         0 kB
 NFS_Unstable:          0 kB
 Bounce:                0 kB
 WritebackTmp:          0 kB
 CommitLimit:     3542228 kB
-Committed_AS:      81484 kB
+Committed_AS:      81680 kB
 VmallocTotal:   34359738367 kB
-VmallocUsed:        5056 kB
+VmallocUsed:        7112 kB
 VmallocChunk:          0 kB
 Percpu:             1632 kB
 AnonHugePages:      2048 kB
@@ -49,6 +49,6 @@
 HugePages_Surp:        0
 Hugepagesize:       2048 kB
 Hugetlb:         1048576 kB
-DirectMap4k:       28500 kB
-DirectMap2M:     3117056 kB
-DirectMap1G:     7340032 kB
+DirectMap4k:      274260 kB
+DirectMap2M:     3919872 kB
+DirectMap1G:     6291456 kB

[PATCH v2 2/2] module: Avoid unnecessary return value initialization in move_module()

[Petr Pavlu]

All error conditions in move_module() set the return value by updating the
ret variable. Therefore, it is not necessary to the initialize the variable
when declaring it.

Remove the unnecessary initialization.

Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
Reviewed-by: Sami Tolvanen <samitolvanen@google.com>
---
 kernel/module/main.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/kernel/module/main.c b/kernel/module/main.c
index 9ac994b2f354..7822b91fca6b 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2696,9 +2696,8 @@ static int find_module_sections(struct module *mod, struct load_info *info)
 
 static int move_module(struct module *mod, struct load_info *info)
 {
-	int i;
+	int i, ret;
 	enum mod_mem_type t = MOD_MEM_NUM_TYPES;
-	int ret = -ENOMEM;
 	bool codetag_section_found = false;
 
 	for_each_mod_mem_type(type) {
-- 
2.49.0

Re: [PATCH v2 2/2] module: Avoid unnecessary return value initialization in move_module()

[Daniel Gomez]

On 18/06/2025 14.26, Petr Pavlu wrote:
> All error conditions in move_module() set the return value by updating the
> ret variable. Therefore, it is not necessary to the initialize the variable
> when declaring it.
> 
> Remove the unnecessary initialization.
> 
> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
> Reviewed-by: Sami Tolvanen <samitolvanen@google.com>
> ---
>  kernel/module/main.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 9ac994b2f354..7822b91fca6b 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -2696,9 +2696,8 @@ static int find_module_sections(struct module *mod, struct load_info *info)
>  
>  static int move_module(struct module *mod, struct load_info *info)
>  {
> -	int i;
> +	int i, ret;
>  	enum mod_mem_type t = MOD_MEM_NUM_TYPES;
> -	int ret = -ENOMEM;
>  	bool codetag_section_found = false;
>  
>  	for_each_mod_mem_type(type) {

Reviewed-by: Daniel Gomez <da.gomez@samsung.com>

Re: [PATCH v2 0/2] module: Fix memory deallocation on error path in move_module()

[Daniel Gomez]

From: Daniel Gomez <da.gomez@samsung.com>


On Wed, 18 Jun 2025 14:26:25 +0200, Petr Pavlu wrote:
> The first patch is an actual fix. The second patch is a minor related
> cleanup.
> 
> Changes since v1 [1]:
> - Initialize t to MOD_MEM_NUM_TYPES in move_module(), instead of assigning
>   the value later.
> - Merge the definitions of the variables i and ret in move_module().
> 
> [...]

Applied, thanks!

[1/2] module: Fix memory deallocation on error path in move_module()
      commit: caca01f251ac37807ff10380d5dafb9576cdfef0
[2/2] module: Avoid unnecessary return value initialization in move_module()
      commit: f4e47f3ac79849846189058491ff885491223ab8

Best regards,
-- 
Daniel Gomez <da.gomez@samsung.com>