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 <[email protected]> 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. > 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 [ ... ] > @@ -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; > } [ ... ] > +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? > + > + return 0; > +} [ ... ] > 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 [ ... ] > @@ -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/[email protected]?part=8
