Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Missing duplication of eh_frame_mt during fork() leaves child processes without stack unwind capability. - [Medium] Executable segments with a trailing BSS section silently fail .eh_frame_hdr registration. --
--- Patch [10]: [RFC PATCH v1 10/25] unwind_user/eh_frame: Detect .eh_frame_hdr sections in executables --- commit e178ff4c7b227a1dcb3db08f8923210e443187ac Author: Jens Remus <[email protected]> unwind_user/eh_frame: Detect .eh_frame_hdr sections in executables This commit modifies the ELF loader to automatically detect .eh_frame_hdr sections during binary and interpreter loading. It associates these sections with the mm_struct to support user-space stack unwinding. > When loading an ELF executable, automatically detect .eh_frame_hdr > sections and associate them with the mm_struct. [Severity: High] Are these eh_frame sections properly preserved for child processes during fork()? When a process forks, dup_mm() creates a new mm_struct and calls mm_init(), which appears to initialize eh_frame_mt as an empty maple tree: kernel/fork.c:mm_init_eh_frame() { ... mt_init(&mm->eh_frame_mt); ... } Is there any subsequent logic that duplicates the parent's eh_frame_mt entries during dup_mm()? If not, won't child processes inherit correctly duplicated VMAs but an empty eh_frame_mt, resulting in a complete loss of stack unwinding capabilities for all children? > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c > index 16a56b6b3f6ca..8c1cdd8318c53 100644 > --- a/fs/binfmt_elf.c > +++ b/fs/binfmt_elf.c [ ... ] > @@ -637,6 +638,23 @@ static inline int make_prot(u32 p_flags, struct > arch_elf_state *arch_state, > return arch_elf_adjust_prot(prot, arch_state, has_interp, is_interp); > } > > +static void elf_add_eh_frame(const struct elf_phdr *text, > + const struct elf_phdr *eh_frame, > + unsigned long base_addr) > +{ > + unsigned long eh_frame_start, eh_frame_end, > + text_start, text_end; > + > + eh_frame_start = base_addr + eh_frame->p_vaddr; > + eh_frame_end = eh_frame_start + eh_frame->p_memsz; > + > + text_start = base_addr + text->p_vaddr; > + text_end = text_start + text->p_memsz; [Severity: Medium] Will this silently fail to register .eh_frame_hdr for segments with a trailing BSS section? When an ELF executable segment has p_memsz > p_filesz crossing a page boundary, the loader maps the file-backed portion and creates a separate anonymous VMA for the BSS. Because text_end is calculated using p_memsz, it may extend beyond the file-backed VMA boundary. When this is passed to eh_frame_add_section(), it uses vma_lookup() and checks if the entire segment resides in a single VMA: kernel/unwind/eh_frame.c:eh_frame_add_section() { ... if (!text_vma || !(text_vma->vm_flags & VM_EXEC) || text_end > text_vma->vm_end) { return -EINVAL; ... } Since the file-backed VMA ends before text_end in this scenario, won't the text_end > text_vma->vm_end check fail and terminate registration? > + > + /* Ignore return value, eh_frame[_hdr] section isn't critical */ > + eh_frame_add_section(eh_frame_start, eh_frame_end, text_start, > text_end); > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=10
