Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] If mas_erase() fails due to an Out-Of-Memory (OOM) condition, it leaves the entry in the tree but __eh_frame_remove_section() still frees it via call_srcu().
- [Medium] Mixing `goto`-based error handling with scope-based cleanup helpers in the same function.
- [Low] `efi_mm` is missing the `INIT_MM_EH_FRAME` initializer.
--
--- Patch [8]: [RFC PATCH v2 08/24] unwind_user/eh_frame: Store .eh_frame_hdr section data in per-mm maple tree ---
commit 428d78732477ec23211b2b4732a9923dae762c55
Author: Jens Remus [off-list ref]
unwind_user/eh_frame: Store .eh_frame_hdr section data in per-mm maple tree
This commit associates .eh_frame_hdr sections with their mm_struct by
adding them to a per-mm maple tree. The tree is indexed by the
corresponding text address ranges.
quoted hunk ↗ jump to hunk
diff --git a/kernel/unwind/eh_frame.c b/kernel/unwind/eh_frame.c
index f25a0923e315b..8cf19c5298fde 100644
--- a/kernel/unwind/eh_frame.c
+++ b/kernel/unwind/eh_frame.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -352,15 +355,76 @@ int eh_frame_add_section(unsigned long eh_frame_hdr_start,
if (ret)
goto err_free;
- /* TODO nowhere to store it yet - just free it and return an error */
- ret = -ENOSYS;
+ ret = mtree_insert_range(eh_frame_mt, sec->text_start, sec->text_end - 1,
+ sec, GFP_KERNEL_ACCOUNT);
+ if (ret) {
+ dbg("mtree_insert_range failed: text=%lx-%lx\n",
+ sec->text_start, sec->text_end);
+ goto err_free;
+ }
+
+ return 0;
err_free:
[Severity: Medium]
Is it intentional to mix goto-based error handling with scope-based cleanup
helpers in the same function?
The eh_frame_add_section function uses scoped_guard for the mmap lock but
retains a goto err_free label for allocation cleanup on failure.
The cleanup subsystem guidelines suggest that a function should either convert
all resources to scope-based cleanup or convert none of them to avoid
confusing ownership semantics.
free_section(sec);
return ret;
}
[ ... ]
quoted hunk ↗ jump to hunk
+static int __eh_frame_remove_section(struct ma_state *mas,
+ struct eh_frame_section *sec)
+{
+ if (mas_erase(mas) != sec) {
+ dbg("mas_erase failed: text=%lx\n", sec->text_start);
+ return -EINVAL;
+ }
+
+ call_srcu(&eh_frame_srcu, &sec->rcu, eh_frame_free_srcu);
[Severity: Critical]
Can this cause a use-after-free if the maple tree erase operation fails?
If mas_erase fails due to memory exhaustion (for example, during tree
rebalancing), it leaves the entry in the tree, sets mas_is_err(mas), and
returns the pointer to the entry.
Because it returns the entry pointer, the condition mas_erase(mas) != sec
will evaluate to false, skipping the error block.
The code would then proceed to call call_srcu and free the section while
it remains linked in the maple tree, potentially causing a crash when the
tree is traversed later. Should this check mas_is_err(mas) instead?
quoted hunk ↗ jump to hunk
[ ... ]
quoted hunk ↗ jump to hunk
diff --git a/mm/init-mm.c b/mm/init-mm.c
index 3e792aad76261..25c26341fc4fb 100644
--- a/mm/init-mm.c
+++ b/mm/init-mm.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -48,6 +49,7 @@ struct mm_struct init_mm = {
#endif
.flexible_array = MM_STRUCT_FLEXIBLE_ARRAY_INIT,
INIT_MM_CONTEXT(init_mm)
+ INIT_MM_EH_FRAME
};
[Severity: Low]
Does efi_mm in drivers/firmware/efi/efi.c also need this initializer?
When a new initializer like INIT_MM_EH_FRAME is added to init_mm, it usually
needs to be added to all static mm_struct definitions.
Leaving it out means the eh_frame_mt maple tree inside efi_mm remains
zero-initialized rather than properly initialized with MTREE_INIT, which
might lead to lock splats or crashes if accessed.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260821195259.2688377-1-jremus@linux.ibm.com?part=8