On 8/18/2026 5:08 PM, [email protected] wrote: > Thank you for your contribution! Sashiko AI review found 5 potential issue(s) > to consider: > - [Critical] The `eh_frame_mt` maple tree is initialized without > `MT_FLAGS_USE_RCU`, causing internal nodes to be synchronously freed, which > crashes concurrent lockless readers.
Josh, Steven, this seems like a valid finding. See below. > - [High] fork() fails to inherit .eh_frame_hdr registrations, breaking > unwinding for child processes. This is resolved by subsequent patch "unwind_user/eh_frame: Duplicate registered .eh_frame[_hdr] section data on clone/fork". > - [High] A TOCTOU race condition in `eh_frame_remove_section` allows a > double-free of one section and a memory leak of another. Steven, we somehow started to use the mmap_read_lock/mmap_write_lock to synchronize adding/removing sections into the maple tree in the sframe series, which my eh_frame series is missing: https://lore.kernel.org/all/[email protected]/ I think using the advanced maple tree API would be preferable. See below. > - [High] An integer overflow in `table_end` calculation allows maliciously > crafted `.eh_frame_hdr` sections to bypass bounds checking. This is fixed in preceding patch "[RFC PATCH v1 07/25] unwind_user/ eh_frame: Add support for reading .eh_frame_hdr section". > - [Low] Appending INIT_MM_EH_FRAME in init_mm causes a missing comma syntax > error on multiple architectures. IIRC this can be left for affected architectures to fix? >> diff --git a/include/linux/eh_frame.h b/include/linux/eh_frame.h >> +#define INIT_MM_EH_FRAME .eh_frame_mt = MTREE_INIT(eh_frame_mt, 0), #define EH_FRAME_MT_FLAGS (MT_FLAGS_USE_RCU) #define INIT_MM_EH_FRAME .eh_frame_mt = MTREE_INIT(eh_frame_mt, EH_FRAME_MT_FLAGS), > > [Severity: Critical] > Is the MT_FLAGS_USE_RCU flag required here? > > Since readers are locklessly iterating the tree using mt_for_each() (which > relies on rcu_read_lock()), omitting MT_FLAGS_USE_RCU causes concurrent > mtree_erase() or mtree_insert_range() to synchronously free internal nodes via > kfree() instead of kfree_rcu(). Can this cause a use-after-free for the > concurrent readers? >> diff --git a/kernel/fork.c b/kernel/fork.c >> +static void mm_init_eh_frame(struct mm_struct *mm) >> +{ >> +#ifdef CONFIG_HAVE_UNWIND_USER_EH_FRAME >> + mt_init(&mm->eh_frame_mt); mt_init_flags(&mm->eh_frame_mt, EH_FRAME_MT_FLAGS); > > [Severity: Critical] > Should this use mt_init_flags() with MT_FLAGS_USE_RCU? > > Similar to the INIT_MM_EH_FRAME macro, initializing without RCU could lead to > a use-after-free if internal maple tree nodes are freed while lockless readers > are iterating. ... >> +#endif >> +} >> diff --git a/kernel/unwind/eh_frame.c b/kernel/unwind/eh_frame.c >> +static int __eh_frame_remove_section(struct mm_struct *mm, static int __eh_frame_remove_section(struct ma_state *mas, >> + struct eh_frame_section *sec) >> +{ >> + if (!mtree_erase(&mm->eh_frame_mt, sec->text_start)) { >> + dbg("mtree_erase failed: text=%lx\n", sec->text_start); 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); >> + >> + return 0; >> +} >> + >> int eh_frame_remove_section(unsigned long eh_frame_hdr_start) >> { >> - return -ENOSYS; >> + struct mm_struct *mm = current->mm; >> + struct eh_frame_section *sec; MA_STATE(mas, &mm->eh_frame_mt, 0, 0); >> + bool found = false; >> + int ret = 0; >> + >> + guard(srcu)(&eh_frame_srcu); >> + >> + mt_for_each(&mm->eh_frame_mt, sec, index, ULONG_MAX) { mtree_lock(&mm->eh_frame_mt); mas_for_each(&mas, sec, ULONG_MAX) { >> + if (sec->eh_frame_hdr_start == eh_frame_hdr_start) { >> + found = true; >> + ret |= __eh_frame_remove_section(mm, sec); >> + } >> + } mtree_unlock(&mm->eh_frame_mt); > > [Severity: High] > Can this mtree_erase() and call_srcu() sequence race with another thread and > cause a double-free? > > If Thread A locklessly iterates and finds sec1 in mt_for_each(), and Thread B > concurrently removes sec1, Thread C could map a new section sec2 at the same > text_start address. > > When Thread A calls __eh_frame_remove_section(mm, sec1), mtree_erase() would > successfully remove and return sec2. However, Thread A then passes the > original sec1 pointer to call_srcu(), double-freeing sec1 and leaking sec2. Valid. Using the internal maple tree API to guard the whole iteration and deletion of the eh_frame sections using the internal writer spinlock should resolve this issue. >> diff --git 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 appending INIT_MM_EH_FRAME here cause a missing comma syntax error on > some architectures? > > While x86's INIT_MM_CONTEXT was updated to include a trailing comma in this > patch, other architectures (like UML or PowerPC Book3s 32-bit) may lack this > trailing comma in their INIT_MM_CONTEXT definitions, breaking the build. Thanks and regards, Jens -- Jens Remus Linux on Z Development (D3303) [email protected] / [email protected] IBM Deutschland Research & Development GmbH; Vorsitzender des Aufsichtsrats: Wolfgang Wendt; Geschäftsführung: David Faller; Sitz der Gesellschaft: Ehningen; Registergericht: Amtsgericht Stuttgart, HRB 243294 IBM Data Privacy Statement: https://www.ibm.com/privacy/
