unwind, perf: sframe user space unwinding

VDSO is the only part of the "kernel" using DWARF CFI directives. For the kernel proper, ensure the CFI_* macros don't do anything.

These macros aren't yet being used outside of VDSO, so there's no functional change.

arch/x86/include/asm/dwarf2.h +0 -0

index 430fca13bb56..b1aa3fcd5bca 100644

arch/x86/include/asm/dwarf2.h +6 -7

@@ -29,13 +39,24 @@
  	 * useful to anyone.  Note we should not use this directive if we
  	 * ever decide to enable DWARF unwinding at runtime.
  	 */
+
  	.cfi_sections .debug_frame
-#else
-	 /*
-	  * For the vDSO, emit both runtime unwind information and debug
-	  * symbols for the .dbg file.
-	  */
-	.cfi_sections .eh_frame, .debug_frame
-#endif
+
+#define CFI_STARTPROC
+#define CFI_ENDPROC
+#define CFI_DEF_CFA
+#define CFI_DEF_CFA_REGISTER

Quoted 4 lines

arch/x86/include/asm/dwarf2.h +4 -0

+#define CFI_STARTPROC
+#define CFI_ENDPROC
+#define CFI_DEF_CFA
+#define CFI_DEF_CFA_REGISTER

arch/x86/include/asm/dwarf2.h line 58

Quoted 10 lines

arch/x86/include/asm/dwarf2.h +10 -0

+#define CFI_DEF_CFA_OFFSET
+#define CFI_ADJUST_CFA_OFFSET
+#define CFI_OFFSET
+#define CFI_REL_OFFSET
+#define CFI_REGISTER
+#define CFI_RESTORE
+#define CFI_REMEMBER_STATE
+#define CFI_RESTORE_STATE
+#define CFI_UNDEFINED
+#define CFI_ESCAPE

Don't all of the above need to be defined to "#", so that they change any parameters into an assembler comment? For example:

#define CFI_STARTPROC #

Enable sframe generation in the VDSO library so kernel and user space can unwind through it.

arch/x86/entry/vdso/Makefile quiet_cmd_vdso2c = VDSO2C $@ · line 49

Quoted 8 lines

arch/x86/entry/vdso/Makefile +1 -0

index c9216ac4fb1e..75ae9e093a2d 100644
@@ -47,13 +47,15 @@ quiet_cmd_vdso2c = VDSO2C  $@
  $(obj)/vdso-image-%.c: $(obj)/vdso%.so.dbg $(obj)/vdso%.so $(obj)/vdso2c FORCE
  	$(call if_changed,vdso2c)
+SFRAME_CFLAGS := $(call as-option,-Wa$(comma)-gsframe,)

I have the impression this test might not work as expected.

Maybe the following would be an alternative test in the Makefile?

SFRAME_CFLAGS := $(call as-instr,.cfi_sections .sframe\n.cfi_startproc\n.cfi_endproc,-DCONFIG_AS_SFRAME=1) ifneq ($(SFRAME_CFLAGS),)

       SFRAME_CFLAGS += -Wa,--gsframe

endif

Introduce a user space unwinder API which provides a generic way to unwind user stacks.

kernel/unwind/user.c +0 -0

kernel/unwind/user.c +16 -0

+int unwind_user_next(struct unwind_user_state *state)
+{
+	struct unwind_user_frame _frame;
+	struct unwind_user_frame *frame = &_frame;
+	unsigned long prev_ip, cfa, fp, ra = 0;
+
+	if (state->done)
+		return -EINVAL;
+
+	prev_ip = state->ip;
+
+	switch (state->type) {
+	case UNWIND_USER_TYPE_FP:
+		frame = &fp_frame;
+		break;
+	default:

kernel/unwind/user.c +5 -0

+
+	if (frame->fp_off && get_user(fp, (unsigned long __user *)(cfa + frame->fp_off)))
+		goto the_end;
+
+	state->sp = cfa;

Review

Quoted 2 lines · Josh Poimboeuf

(quoted diff) +2 -0

+	if (ra == prev_ip)
+		goto the_end;

This seems too restrictive to me, as it effectively prevents unwinding from recursive functions, e.g. Glibc internal merge sort msort_with_tmp():

$ perf record -F 9999 --call-graph fp /usr/bin/objdump -wdWF /usr/bin/objdump
$ perf script
...
objdump    8314 236064.515562:     100010 task-clock:ppp:
                 100630a compare_symbols+0x2a (/usr/bin/objdump)
             3ffb9e58e7c msort_with_tmp.part.0+0x15c (/usr/lib64/libc.so.6)
             3ffb9e58d76 msort_with_tmp.part.0+0x56 (/usr/lib64/libc.so.6)
[unwinding unexpectedly stops]

Would it be an option to only stop unwinding if both the IP and SP do not change?

if (sp == prev_sp && ra == prev_ra)

	gote the_end;

Quoted 17 lines · Jens Remus

Review

Quoted 2 lines · Josh Poimboeuf

(quoted diff) +2 -0

+	if (ra == prev_ip)
+		goto the_end;

This seems too restrictive to me, as it effectively prevents unwinding from recursive functions, e.g. Glibc internal merge sort msort_with_tmp():

$ perf record -F 9999 --call-graph fp /usr/bin/objdump -wdWF /usr/bin/objdump
$ perf script
...
objdump    8314 236064.515562:     100010 task-clock:ppp:
                  100630a compare_symbols+0x2a (/usr/bin/objdump)
              3ffb9e58e7c msort_with_tmp.part.0+0x15c (/usr/lib64/libc.so.6)
              3ffb9e58d76 msort_with_tmp.part.0+0x56 (/usr/lib64/libc.so.6)
[unwinding unexpectedly stops]

Quoted 5 lines

Would it be an option to only stop unwinding if both the IP and SP do not change?

if (sp == prev_sp && ra == prev_ra)

	gote the_end;

Good point, I've already fixed that for the next version (not yet posted). I believe the only thing we really need to check here is that the unwind is heading in the right direction:

if (cfa <= state->sp)

	goto the_end;

Quoted 2 lines · Josh Poimboeuf

if (cfa <= state->sp)

	goto the_end;

Assuming the x86 definition of the CFA (CFA == SP at call site) this translates into:

if (sp <= state->sp)

	goto the_end;

That won't work for architectures that pass the return address in a register instead of on the stack, such as s390. At least in the topmost frame the unwound SP may be unchanged. For instance when in the function prologue or when in a leaf function.

One of my patches for s390 support introduces a state->first flag, indicating whether it is the topmost user space frame. Using that your check could be extended to:

if ((state->first && sp < state->sp) || (!state->first && sp <= state->sp))

	goto the_end;

Which could be simplified to:

if (sp <= state->sp - state->first)

	goto the_end;

Btw. neither would work for architectures with an upwards-growing stack, such as hppa. Not sure if that needs to be considered.

Use ARCH_INIT_USER_FP_FRAME to describe how frame pointers are unwound on x86, and enable CONFIG_HAVE_UNWIND_USER_FP accordingly so the unwind_user interfaces can be used.

Signed-off-by Josh Poimboeuf <[email protected]>

---
 arch/x86/Kconfig                   |  1 +
 arch/x86/include/asm/unwind_user.h | 11 +++++++++++
 2 files changed, 12 insertions(+)
 create mode 100644 arch/x86/include/asm/unwind_user.h

arch/x86/Kconfig +1 -0

index 0bdb7a394f59..f91098d6f535 100644
@@ -289,6 +289,7 @@ config X86
 	select HAVE_SYSCALL_TRACEPOINTS
 	select HAVE_UACCESS_VALIDATION		if HAVE_OBJTOOL
 	select HAVE_UNSTABLE_SCHED_CLOCK
+	select HAVE_UNWIND_USER_FP		if X86_64
 	select HAVE_USER_RETURN_NOTIFIER
 	select HAVE_GENERIC_VDSO
 	select VDSO_GETRANDOM			if X86_64

arch/x86/include/asm/unwind_user.h new file +1 -0

new file mode 100644
index 000000000000..19df26a65132
@@ -0,0 +1,11 @@
+/* SPDX-License-Identifier: GPL-2.0 */

Quoted 34 lines · Josh Poimboeuf

Use ARCH_INIT_USER_FP_FRAME to describe how frame pointers are unwound on x86, and enable CONFIG_HAVE_UNWIND_USER_FP accordingly so the unwind_user interfaces can be used.

Signed-off-by Josh Poimboeuf <[email protected]>

---
 arch/x86/Kconfig                   |  1 +
 arch/x86/include/asm/unwind_user.h | 11 +++++++++++
 2 files changed, 12 insertions(+)
 create mode 100644 arch/x86/include/asm/unwind_user.h

arch/x86/Kconfig +1 -0

index 0bdb7a394f59..f91098d6f535 100644
@@ -289,6 +289,7 @@ config X86
 	select HAVE_SYSCALL_TRACEPOINTS
 	select HAVE_UACCESS_VALIDATION		if HAVE_OBJTOOL
 	select HAVE_UNSTABLE_SCHED_CLOCK
+	select HAVE_UNWIND_USER_FP		if X86_64
 	select HAVE_USER_RETURN_NOTIFIER
 	select HAVE_GENERIC_VDSO
 	select VDSO_GETRANDOM			if X86_64

arch/x86/include/asm/unwind_user.h new file +5 -0

new file mode 100644
index 000000000000..19df26a65132
@@ -0,0 +1,11 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_X86_UNWIND_USER_H
+#define _ASM_X86_UNWIND_USER_H
+
+#define ARCH_INIT_USER_FP_FRAME							\

arch/x86/include/asm/unwind_user.h line 11

Quoted 6 lines

arch/x86/include/asm/unwind_user.h new file +6 -0

+	.ra_off		= (s32)sizeof(long) * -1,				\
+	.cfa_off	= (s32)sizeof(long) * 2,				\
+	.fp_off		= (s32)sizeof(long) * -2,				\
+	.use_fp		= true,
+
+#endif /* _ASM_X86_UNWIND_USER_H */

What about compat / 32bit userspace?

Quoted 9 lines · Peter Zijlstra

arch/x86/include/asm/unwind_user.h line 11

Quoted 7 lines

arch/x86/include/asm/unwind_user.h new file +7 -0

+#define ARCH_INIT_USER_FP_FRAME							\
+	.ra_off		= (s32)sizeof(long) * -1,				\
+	.cfa_off	= (s32)sizeof(long) * 2,				\
+	.fp_off		= (s32)sizeof(long) * -2,				\
+	.use_fp		= true,
+
+#endif /* _ASM_X86_UNWIND_USER_H */

What about compat / 32bit userspace?

Sframe doesn't support 32-bit binaries at the moment. Does anybody actually care?

You're right this regresses existing perf behavior for the frame pointer case. I'll try to fix that.

kernel/unwind/sframe.c new file +41 -0

+static int __sframe_add_section(unsigned long sframe_addr,
+				unsigned long text_start,
+				unsigned long text_end)
+{
+	struct maple_tree *sframe_mt = &current->mm->sframe_mt;
+	struct sframe_section *sec;
+	struct sframe_header shdr;
+	unsigned long header_end;
+	int ret;
+
+	if (copy_from_user(&shdr, (void __user *)sframe_addr, sizeof(shdr)))
+		return -EFAULT;
+
+	if (shdr.preamble.magic != SFRAME_MAGIC ||
+	    shdr.preamble.version != SFRAME_VERSION_2 ||
+	    !(shdr.preamble.flags & SFRAME_F_FDE_SORTED) ||
+	    shdr.auxhdr_len || !shdr.num_fdes || !shdr.num_fres ||
+	    shdr.fdes_off > shdr.fres_off) {
+		return -EINVAL;
+	}
+
+	sec = kmalloc(sizeof(*sec), GFP_KERNEL);
+	if (!sec)
+		return -ENOMEM;
+
+	header_end = sframe_addr + SFRAME_HDR_SIZE(shdr);
+
+	sec->sframe_addr	= sframe_addr;
+	sec->text_addr		= text_start;
+	sec->fdes_addr		= header_end + shdr.fdes_off;
+	sec->fres_addr		= header_end + shdr.fres_off;
+	sec->fdes_nr		= shdr.num_fdes;
+	sec->ra_off		= shdr.cfa_fixed_ra_offset;
+	sec->fp_off		= shdr.cfa_fixed_fp_offset;
+
+	ret = mtree_insert_range(sframe_mt, text_start, text_end, sec, GFP_KERNEL);
+	if (ret) {
+		kfree(sec);
+		return ret;
+	}
+

kernel/unwind/sframe.c new file +60 -0

+
+	sframe_vma = vma_lookup(mm, sframe_addr);
+	if (!sframe_vma)
+		goto err_unlock;
+
+	if (text_start && text_end) {
+		struct vm_area_struct *text_vma;
+
+		text_vma = vma_lookup(mm, text_start);
+		if (!(text_vma->vm_flags & VM_EXEC))
+			goto err_unlock;
+
+		if (PAGE_ALIGN(text_end) != text_vma->vm_end)
+			goto err_unlock;
+	} else {
+		struct vm_area_struct *vma, *text_vma = NULL;
+		VMA_ITERATOR(vmi, mm, 0);
+
+		for_each_vma(vmi, vma) {
+			if (vma->vm_file != sframe_vma->vm_file ||
+			    !(vma->vm_flags & VM_EXEC))
+				continue;
+
+			if (text_vma) {
+				pr_warn_once("%s[%d]: multiple EXEC segments unsupported\n",
+					     current->comm, current->pid);
+				goto err_unlock;
+			}
+
+			text_vma = vma;
+		}
+
+		if (!text_vma)
+			goto err_unlock;
+
+		text_start = text_vma->vm_start;
+		text_end   = text_vma->vm_end;
+	}
+
+	mmap_read_unlock(mm);
+
+	return __sframe_add_section(sframe_addr, text_start, text_end);
+
+err_unlock:
+	mmap_read_unlock(mm);
+	return -EINVAL;
+}
 
+int sframe_remove_section(unsigned long sframe_addr)
+{
+	struct mm_struct *mm = current->mm;
+	struct sframe_section *sec;
+	unsigned long index = 0;
+
+	mt_for_each(&mm->sframe_mt, sec, index, ULONG_MAX) {
+		if (sec->sframe_addr == sframe_addr)
+			return __sframe_remove_section(mm, sec);
+	}
+
+	return -EINVAL;
+}

kernel/sys.c +4 -0

@@ -64,6 +64,7 @@
 #include <linux/rcupdate.h>
 #include <linux/uidgid.h>
 #include <linux/cred.h>
+#include <linux/sframe.h>
 
 #include <linux/nospec.h>
 

@@ -2784,6 +2785,16 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
 	case PR_RISCV_SET_ICACHE_FLUSH_CTX:
 		error = RISCV_SET_ICACHE_FLUSH_CTX(arg2, arg3);
 		break;
+	case PR_ADD_SFRAME:
+		if (arg5)
+			return -EINVAL;

kernel/unwind/sframe.c line 286

Quoted 7 lines

kernel/unwind/sframe.c new file +7 -0

+int sframe_add_section(unsigned long sframe_addr, unsigned long text_start,
+		       unsigned long text_end)
+{
+	struct mm_struct *mm = current->mm;
+	struct vm_area_struct *sframe_vma;
+
+	mmap_read_lock(mm);

DEFINE_GUARD(mmap_read_lock, struct mm_struct *,

	     mmap_read_lock(_T), mmap_read_unlock(_T))

in include/linux/mmap_lock.h ?

Quoted 12 lines

kernel/sys.c +8 -0

@@ -2784,6 +2785,16 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
 	case PR_RISCV_SET_ICACHE_FLUSH_CTX:
 		error = RISCV_SET_ICACHE_FLUSH_CTX(arg2, arg3);
 		break;
+	case PR_ADD_SFRAME:
+		if (arg5)
+			return -EINVAL;
+		error = sframe_add_section(arg2, arg3, arg4);
+		break;
+	case PR_REMOVE_SFRAME:
+		if (arg3 || arg4 || arg5)
+			return -EINVAL;

kernel/sys.c SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, · line 2800

Quoted 5 lines

kernel/sys.c +2 -0

+		error = sframe_remove_section(arg2);
+		break;
 	default:
 		error = -EINVAL;
 		break;

So I realize that mtree has an internal lock, but are we sure we don't want a lock around those prctl()s?

Quoted 9 lines · Peter Zijlstra

kernel/unwind/sframe.c line 286

Quoted 7 lines

kernel/unwind/sframe.c new file +7 -0

+int sframe_add_section(unsigned long sframe_addr, unsigned long text_start,
+		       unsigned long text_end)
+{
+	struct mm_struct *mm = current->mm;
+	struct vm_area_struct *sframe_vma;
+
+	mmap_read_lock(mm);

DEFINE_GUARD(mmap_read_lock, struct mm_struct *,

Quoted 3 lines

	     mmap_read_lock(_T), mmap_read_unlock(_T))

in include/linux/mmap_lock.h ?

Will do.

Quoted 12 lines

Quoted 12 lines

kernel/sys.c +8 -0

@@ -2784,6 +2785,16 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
 	case PR_RISCV_SET_ICACHE_FLUSH_CTX:
 		error = RISCV_SET_ICACHE_FLUSH_CTX(arg2, arg3);
 		break;
+	case PR_ADD_SFRAME:
+		if (arg5)
+			return -EINVAL;
+		error = sframe_add_section(arg2, arg3, arg4);
+		break;
+	case PR_REMOVE_SFRAME:
+		if (arg3 || arg4 || arg5)
+			return -EINVAL;

Quoted 8 lines

kernel/sys.c SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, · line 2800

Quoted 5 lines

kernel/sys.c +2 -0

+		error = sframe_remove_section(arg2);
+		break;
 	default:
 		error = -EINVAL;
 		break;

So I realize that mtree has an internal lock, but are we sure we don't want a lock around those prctl()s?

Not that I can tell? It relies on the mtree internal locking for atomicity.

For sframe_remove_section() it uses srcu to delay the freeing until all sframe_find()'s have completed.

Some distros have started compiling frame pointers into all their packages to enable the kernel to do system-wide profiling of user space. Unfortunately that creates a runtime performance penalty across the entire system. Using DWARF instead isn't feasible due to the complexity it would add to the kernel.

For in-kernel unwinding we solved this problem with the creation of the ORC unwinder for x86_64. Similarly, for user space the GNU assembler has created the sframe format starting with binutils 2.41 for sframe v2. Sframe is a simpler version of .eh_frame. It gets placed in the .sframe section.

Add support for unwinding user space using sframe.

More information about sframe can be found here:

- https://lwn.net/Articles/932209/
- https://lwn.net/Articles/940686/
- https://sourceware.org/binutils/docs/sframe-spec.html

Glibc support is needed to implement the prctl() calls to tell the kernel where the .sframe segments are.

Signed-off-by Josh Poimboeuf <[email protected]>

---
 arch/Kconfig                |   4 +
 arch/x86/include/asm/mmu.h  |   2 +-
 fs/binfmt_elf.c             |  35 +++-
 include/linux/mm_types.h    |   3 +
 include/linux/sframe.h      |  41 ++++
 include/linux/unwind_user.h |   2 +
 include/uapi/linux/elf.h    |   1 +
 include/uapi/linux/prctl.h  |   3 +
 kernel/fork.c               |  10 +
 kernel/sys.c                |  11 ++
 kernel/unwind/Makefile      |   3 +-

 kernel/unwind/sframe.c      | 380 ++++++++++++++++++++++++++++++++++++
 kernel/unwind/sframe.h      | 215 ++++++++++++++++++++
 kernel/unwind/user.c        |  24 ++-
 mm/init-mm.c                |   6 +
 15 files changed, 732 insertions(+), 8 deletions(-)
 create mode 100644 include/linux/sframe.h
 create mode 100644 kernel/unwind/sframe.c
 create mode 100644 kernel/unwind/sframe.h

fs/binfmt_elf.c +1 -1

@@ -688,7 +692,7 @@ static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex,
                        /*
                         * Check to see if the section's size will overflow the
                         * allowed task size. Note that p_filesz must always be
-                        * <= p_memsize so it's only necessary to check p_memsz.
+                        * <= p_memsz so it's only necessary to check p_memsz.
                         */
                        k = load_addr + eppnt->p_vaddr;
                        if (BAD_ADDR(k) ||

@@ -698,9 +702,24 @@ static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex,
                                error = -ENOMEM;
                                goto out;
                        }

fs/binfmt_elf.c +5 -1

        error = load_addr;
 out:
        return error;

@@ -823,7 +842,7 @@ static int load_elf_binary(struct linux_binprm *bprm)
        int first_pt_load = 1;
        unsigned long error;
        struct elf_phdr *elf_ppnt, *elf_phdata, *interp_elf_phdata = NULL;
-       struct elf_phdr *elf_property_phdata = NULL;
+       struct elf_phdr *elf_property_phdata = NULL, *sframe_phdr = NULL;
        unsigned long elf_brk;
        int retval, i;
        unsigned long elf_entry;

@@ -931,6 +950,10 @@ static int load_elf_binary(struct linux_binprm *bprm)
                                executable_stack = EXSTACK_DISABLE_X;
                        break;
 
+               case PT_GNU_SFRAME:
+                       sframe_phdr = elf_ppnt;
+                       break;
+
                case PT_LOPROC ... PT_HIPROC:
                        retval = arch_elf_pt_proc(elf_ex, elf_ppnt,
                                                  bprm->file, false,

fs/binfmt_elf.c +0 -0

        regs = current_pt_regs();
 #ifdef ELF_PLAT_INIT
        /*

include/linux/mm_types.h +3 -0

index 381d22eba088..6e7561c1a5fc 100644
@@ -1052,6 +1052,9 @@ struct mm_struct {
 #endif
                } lru_gen;
 #endif /* CONFIG_LRU_GEN_WALKS_MMU */
+#ifdef CONFIG_HAVE_UNWIND_USER_SFRAME
+               struct maple_tree sframe_mt;
+#endif
        } __randomize_layout;
 
        /*

include/linux/sframe.h new file +5 -0

new file mode 100644
index 000000000000..d167e01817c4
@@ -0,0 +1,41 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_SFRAME_H
+#define _LINUX_SFRAME_H
+
+#include <linux/mm_types.h>

include/linux/sframe.h new file +13 -0

+extern int sframe_add_section(unsigned long sframe_addr, unsigned long text_start,
+                             unsigned long text_end);
+
+extern int sframe_remove_section(unsigned long sframe_addr);
+extern int sframe_find(unsigned long ip, struct unwind_user_frame *frame);
+
+static inline bool current_has_sframe(void)
+{
+       struct mm_struct *mm = current->mm;
+
+       return mm && !mtree_empty(&mm->sframe_mt);
+}
+

kernel/sys.c +1 -0

index 4da31f28fda8..7d05a67727db 100644
@@ -64,6 +64,7 @@
 #include <linux/rcupdate.h>
 #include <linux/uidgid.h>
 #include <linux/cred.h>
+#include <linux/sframe.h>
 
 #include <linux/nospec.h>
 

kernel/sys.c +6 -0

+               break;
+       case PR_REMOVE_SFRAME:
+               if (arg3 || arg4 || arg5)
+                       return -EINVAL;
+               error = sframe_remove_section(arg2);
+               break;
        default:
                error = -EINVAL;
                break;

kernel/sys.c +11 -0

+static unsigned char fre_type_to_size(unsigned char fre_type)
+{
+       if (fre_type > 2)
+               return 0;
+       return 1 << fre_type;
+}
+
+static unsigned char offset_size_enum_to_size(unsigned char off_size)
+{
+       if (off_size > 2)
+               return 0;

kernel/sys.c +45 -0

+
+       first = (void __user *)sec->fdes_addr;
+       last = first + sec->fdes_nr;
+       while (first <= last) {
+               struct sframe_fde __user *mid;
+               u32 func_off;
+
+               mid = first + ((last - first) / 2);
+
+               if (get_user(func_off, (s32 __user *)mid))
+                       return -EFAULT;
+
+               if (ip_off >= func_off) {
+                       /* validate sort order */
+                       if (func_off < func_off_low)
+                               return -EINVAL;
+
+                       func_off_low = func_off;
+
+                       found = mid;
+                       first = mid + 1;
+               } else {
+                       /* validate sort order */
+                       if (func_off > func_off_high)
+                               return -EINVAL;
+
+                       func_off_high = func_off;
+
+                       last = mid - 1;
+               }
+       }
+
+       if (!found)
+               return -EINVAL;
+
+       if (copy_from_user(fde, found, sizeof(*fde)))
+               return -EFAULT;
+
+       /* check for gaps */
+       if (ip_off < fde->start_addr || ip_off >= fde->start_addr + fde->size)
+               return -EINVAL;
+
+       return 0;
+}
+

kernel/sys.c +23 -0

+int sframe_add_section(unsigned long sframe_addr, unsigned long text_start,
+                      unsigned long text_end)
+{
+       struct mm_struct *mm = current->mm;
+       struct vm_area_struct *sframe_vma;
+
+       mmap_read_lock(mm);
+
+       sframe_vma = vma_lookup(mm, sframe_addr);
+       if (!sframe_vma)
+               goto err_unlock;
+
+       if (text_start && text_end) {
+               struct vm_area_struct *text_vma;
+
+               text_vma = vma_lookup(mm, text_start);
+               if (!(text_vma->vm_flags & VM_EXEC))
+                       goto err_unlock;
+
+               if (PAGE_ALIGN(text_end) != text_vma->vm_end)
+                       goto err_unlock;
+       } else {
+               struct vm_area_struct *vma, *text_vma = NULL;

kernel/sys.c +17 -0

+                               goto err_unlock;
+                       }
+
+                       text_vma = vma;
+               }
+
+               if (!text_vma)
+                       goto err_unlock;
+
+               text_start = text_vma->vm_start;
+               text_end   = text_vma->vm_end;
+       }
+
+       mmap_read_unlock(mm);
+
+       return __sframe_add_section(sframe_addr, text_start, text_end);
+

It feels like this patch is trying to do too much. There is both new UAPI introduction, and SFrame format definition, and unwinder integration, etc, etc. Do you think it can be split further into more focused smaller patches?

Quoted 5 lines

fs/binfmt_elf.c +5 -0

+
+                       if ((eppnt->p_flags & PF_X) && k < start_code)
+                               start_code = k;
+
+                       if ((eppnt->p_flags & PF_X) && k + eppnt->p_filesz > end_code)

fs/binfmt_elf.c static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex, · line 714

Quoted 5 lines

fs/binfmt_elf.c +5 -0

+                               end_code = k + eppnt->p_filesz;
+                       break;
+               }
+               case PT_GNU_SFRAME:
+                       sframe_phdr = eppnt;

if I understand correctly, there has to be only one sframe, is that right? Should we validate that?

fs/binfmt_elf.c static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex, · line 722

Quoted 8 lines

fs/binfmt_elf.c +5 -0

+                       break;
                }
        }
 
+       if (sframe_phdr)
+               sframe_add_section(load_addr + sframe_phdr->p_vaddr,
+                                  start_code, end_code);
+

no error checking?

include/linux/sframe.h line 15

Quoted 7 lines

include/linux/sframe.h new file +7 -0

+#ifdef CONFIG_HAVE_UNWIND_USER_SFRAME
+
+#define INIT_MM_SFRAME .sframe_mt = MTREE_INIT(sframe_mt, 0),
+
+extern void sframe_free_mm(struct mm_struct *mm);
+
+/* text_start, text_end, file_name are optional */

what file_name? was that an extra argument that got removed?

kernel/sys.c SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, · line 2791

Quoted 7 lines

        case PR_RISCV_SET_ICACHE_FLUSH_CTX:
                error = RISCV_SET_ICACHE_FLUSH_CTX(arg2, arg3);
                break;

kernel/sys.c +4 -0

+       case PR_ADD_SFRAME:
+               if (arg5)
+                       return -EINVAL;
+               error = sframe_add_section(arg2, arg3, arg4);

wouldn't it be better to make this interface extendable from the get go? Instead of passing 3 arguments with fixed meaning, why not pass a pointer to an extendable binary struct like seems to be the trend nowadays with nicely extensible APIs. See [0] for one such example (specifically, struct procmap_query). Seems more prudent, as we'll most probably will be adding flags, options, extra information, etc)

[0] https://lore.kernel.org/linux-mm/[email protected]/

kernel/unwind/sframe.c line 78

Quoted 7 lines

kernel/unwind/sframe.c new file +7 -0

+static int find_fde(struct sframe_section *sec, unsigned long ip,
+                   struct sframe_fde *fde)
+{
+       struct sframe_fde __user *first, *last, *found = NULL;
+       u32 ip_off, func_off_low = 0, func_off_high = -1;
+
+       ip_off = ip - sec->sframe_addr;

what if ip_off is larger than 4GB? ELF section can be bigger than 4GB, right?

and also, does it mean that SFrame doesn't support executables with text bigger than 4GB?

Quoted 7 lines

kernel/unwind/sframe.c new file +7 -0

+       } else {
+               struct vm_area_struct *vma, *text_vma = NULL;
+               VMA_ITERATOR(vmi, mm, 0);
+
+               for_each_vma(vmi, vma) {
+                       if (vma->vm_file != sframe_vma->vm_file ||
+                           !(vma->vm_flags & VM_EXEC))

kernel/unwind/sframe.c line 312

Quoted 5 lines

kernel/unwind/sframe.c new file +5 -0

+                               continue;
+
+                       if (text_vma) {
+                               pr_warn_once("%s[%d]: multiple EXEC segments unsupported\n",
+                                            current->comm, current->pid);

is this just something that fundamentally can't be supported by SFrame format? Or just an implementation simplification?

It's not illegal to have an executable with multiple VM_EXEC segments, no? Should this be a pr_warn_once() then?

Quoted 4 lines · Andrii Nakryiko

It feels like this patch is trying to do too much. There is both new UAPI introduction, and SFrame format definition, and unwinder integration, etc, etc. Do you think it can be split further into more focused smaller patches?

True, let me see if I can split it up.

Quoted 5 lines

Quoted 5 lines

fs/binfmt_elf.c +5 -0

+
+                       if ((eppnt->p_flags & PF_X) && k < start_code)
+                               start_code = k;
+
+                       if ((eppnt->p_flags & PF_X) && k + eppnt->p_filesz > end_code)

Quoted 8 lines

fs/binfmt_elf.c static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex, · line 714

Quoted 5 lines

fs/binfmt_elf.c +5 -0

+                               end_code = k + eppnt->p_filesz;
+                       break;
+               }
+               case PT_GNU_SFRAME:
+                       sframe_phdr = eppnt;

if I understand correctly, there has to be only one sframe, is that right? Should we validate that?

Yes, there shouldn't be more than one PT_GNU_SFRAME for the executable itself. I can validate that.

Quoted 10 lines

fs/binfmt_elf.c static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex, · line 722

Quoted 8 lines

fs/binfmt_elf.c +5 -0

+                       break;
                }
        }
 
+       if (sframe_phdr)
+               sframe_add_section(load_addr + sframe_phdr->p_vaddr,
+                                  start_code, end_code);
+

no error checking?

Good point. I remember discussing this with some people at Cauldon/LPC, I just forgot to do it!

Right now it does all the validation at unwind, which could really slow things down unnecessarily if the sframe isn't valid.

Quoted 9 lines

include/linux/sframe.h line 15

Quoted 7 lines

include/linux/sframe.h new file +7 -0

+#ifdef CONFIG_HAVE_UNWIND_USER_SFRAME
+
+#define INIT_MM_SFRAME .sframe_mt = MTREE_INIT(sframe_mt, 0),
+
+extern void sframe_free_mm(struct mm_struct *mm);
+
+/* text_start, text_end, file_name are optional */

what file_name? was that an extra argument that got removed?

Indeed, that was for some old code.

Quoted 8 lines

kernel/sys.c SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, · line 2791

Quoted 7 lines

        case PR_RISCV_SET_ICACHE_FLUSH_CTX:
                error = RISCV_SET_ICACHE_FLUSH_CTX(arg2, arg3);
                break;

kernel/sys.c +4 -0

+       case PR_ADD_SFRAME:
+               if (arg5)
+                       return -EINVAL;
+               error = sframe_add_section(arg2, arg3, arg4);

Quoted 8 lines

wouldn't it be better to make this interface extendable from the get go? Instead of passing 3 arguments with fixed meaning, why not pass a pointer to an extendable binary struct like seems to be the trend nowadays with nicely extensible APIs. See [0] for one such example (specifically, struct procmap_query). Seems more prudent, as we'll most probably will be adding flags, options, extra information, etc)

[0] https://lore.kernel.org/linux-mm/[email protected]/

This ioctl interface was admittedly hacked together. I was hoping somebody would suggest something better :-) I'll take a look.

Quoted 9 lines

kernel/unwind/sframe.c line 78

Quoted 7 lines

kernel/unwind/sframe.c new file +7 -0

+static int find_fde(struct sframe_section *sec, unsigned long ip,
+                   struct sframe_fde *fde)
+{
+       struct sframe_fde __user *first, *last, *found = NULL;
+       u32 ip_off, func_off_low = 0, func_off_high = -1;
+
+       ip_off = ip - sec->sframe_addr;

what if ip_off is larger than 4GB? ELF section can be bigger than 4GB, right?

That's baked into sframe v2.

Quoted 3 lines

and also, does it mean that SFrame doesn't support executables with text bigger than 4GB?

Yes, but is that a realistic concern?

Quoted 16 lines

kernel/unwind/sframe.c line 312

Quoted 13 lines

kernel/unwind/sframe.c new file +12 -0

+       } else {
+               struct vm_area_struct *vma, *text_vma = NULL;
+               VMA_ITERATOR(vmi, mm, 0);
+
+               for_each_vma(vmi, vma) {
+                       if (vma->vm_file != sframe_vma->vm_file ||
+                           !(vma->vm_flags & VM_EXEC))
+                               continue;
+
+                       if (text_vma) {
+                               pr_warn_once("%s[%d]: multiple EXEC segments unsupported\n",
+                                            current->comm, current->pid);

is this just something that fundamentally can't be supported by SFrame format? Or just an implementation simplification?

It's a simplification I suppose.

Quoted 3 lines

It's not illegal to have an executable with multiple VM_EXEC segments, no? Should this be a pr_warn_once() then?

I don't know, is it allowed? I've never seen it in practice. The

pr_warn_once() is not reporting that it's illegal but rather that this corner case actually exists and maybe needs to be looked at.

--
Josh

Quoted 12 lines · Josh Poimboeuf

Quoted 9 lines · Andrii Nakryiko

kernel/unwind/sframe.c line 78

Quoted 7 lines

kernel/unwind/sframe.c new file +7 -0

+static int find_fde(struct sframe_section *sec, unsigned long ip,
+                   struct sframe_fde *fde)
+{
+       struct sframe_fde __user *first, *last, *found = NULL;
+       u32 ip_off, func_off_low = 0, func_off_high = -1;
+
+       ip_off = ip - sec->sframe_addr;

what if ip_off is larger than 4GB? ELF section can be bigger than 4GB, right?

Quoted 1 line

That's baked into sframe v2.

I believe we do have large production binaries with more than 4GB of text, what are we going to do about them? It would be interesting to hear sframe people's opinion. Adding such a far-reaching new format in 2024 with these limitations is kind of sad. At the very least maybe we should allow some form of chaining sframe definitions to cover more than 4GB segments? Please CC relevant folks, I'm wondering what they're thinking about this.

Quoted 85 lines · Josh Poimboeuf

Quoted 4 lines · Andrii Nakryiko

It feels like this patch is trying to do too much. There is both new UAPI introduction, and SFrame format definition, and unwinder integration, etc, etc. Do you think it can be split further into more focused smaller patches?

True, let me see if I can split it up.

Quoted 5 lines

Quoted 5 lines

fs/binfmt_elf.c +5 -0

+
+                       if ((eppnt->p_flags & PF_X) && k < start_code)
+                               start_code = k;
+
+                       if ((eppnt->p_flags & PF_X) && k + eppnt->p_filesz > end_code)

Quoted 8 lines

fs/binfmt_elf.c static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex, · line 714

Quoted 5 lines

fs/binfmt_elf.c +5 -0

+                               end_code = k + eppnt->p_filesz;
+                       break;
+               }
+               case PT_GNU_SFRAME:
+                       sframe_phdr = eppnt;

if I understand correctly, there has to be only one sframe, is that right? Should we validate that?

Yes, there shouldn't be more than one PT_GNU_SFRAME for the executable itself. I can validate that.

Quoted 10 lines

fs/binfmt_elf.c static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex, · line 722

Quoted 8 lines

fs/binfmt_elf.c +5 -0

+                       break;
                 }
         }
 
+       if (sframe_phdr)
+               sframe_add_section(load_addr + sframe_phdr->p_vaddr,
+                                  start_code, end_code);
+

no error checking?

Good point. I remember discussing this with some people at Cauldon/LPC, I just forgot to do it!

Right now it does all the validation at unwind, which could really slow things down unnecessarily if the sframe isn't valid.

Quoted 9 lines

include/linux/sframe.h line 15

Quoted 7 lines

include/linux/sframe.h new file +7 -0

+#ifdef CONFIG_HAVE_UNWIND_USER_SFRAME
+
+#define INIT_MM_SFRAME .sframe_mt = MTREE_INIT(sframe_mt, 0),
+
+extern void sframe_free_mm(struct mm_struct *mm);
+
+/* text_start, text_end, file_name are optional */

what file_name? was that an extra argument that got removed?

Indeed, that was for some old code.

Quoted 8 lines

kernel/sys.c SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, · line 2791

Quoted 7 lines

         case PR_RISCV_SET_ICACHE_FLUSH_CTX:
                 error = RISCV_SET_ICACHE_FLUSH_CTX(arg2, arg3);
                 break;

kernel/sys.c +4 -0

+       case PR_ADD_SFRAME:
+               if (arg5)
+                       return -EINVAL;
+               error = sframe_add_section(arg2, arg3, arg4);

Quoted 8 lines

wouldn't it be better to make this interface extendable from the get go? Instead of passing 3 arguments with fixed meaning, why not pass a pointer to an extendable binary struct like seems to be the trend nowadays with nicely extensible APIs. See [0] for one such example (specifically, struct procmap_query). Seems more prudent, as we'll most probably will be adding flags, options, extra information, etc)

   [0] https://lore.kernel.org/linux-mm/[email protected]/

This ioctl interface was admittedly hacked together. I was hoping somebody would suggest something better :-) I'll take a look.

Quoted 9 lines

kernel/unwind/sframe.c line 78

Quoted 7 lines

kernel/unwind/sframe.c new file +7 -0

+static int find_fde(struct sframe_section *sec, unsigned long ip,
+                   struct sframe_fde *fde)
+{
+       struct sframe_fde __user *first, *last, *found = NULL;
+       u32 ip_off, func_off_low = 0, func_off_high = -1;
+
+       ip_off = ip - sec->sframe_addr;

what if ip_off is larger than 4GB? ELF section can be bigger than 4GB, right?

Quoted 1 line

That's baked into sframe v2.

I believe we do have large production binaries with more than 4GB of text, what are we going to do about them? It would be interesting to hear sframe people's opinion. Adding such a far-reaching new format in 2024 with these limitations is kind of sad. At the very least maybe we should allow some form of chaining sframe definitions to cover more than 4GB segments? Please CC relevant folks, I'm wondering what they're thinking about this.

Quoted 5 lines

Quoted 3 lines

and also, does it mean that SFrame doesn't support executables with text bigger than 4GB?

Yes, but is that a realistic concern?

See above, yes. You'd be surprised. As somewhat corroborating evidence, there were tons of problems and churn (within at least Meta) with DWARF not supporting more than 2GB sizes, so yes, this is not an abstract problem for sure. Modern production applications can be ridiculously big.

Quoted 17 lines

Quoted 16 lines

kernel/unwind/sframe.c line 312

Quoted 13 lines

kernel/unwind/sframe.c new file +12 -0

+       } else {
+               struct vm_area_struct *vma, *text_vma = NULL;
+               VMA_ITERATOR(vmi, mm, 0);
+
+               for_each_vma(vmi, vma) {
+                       if (vma->vm_file != sframe_vma->vm_file ||
+                           !(vma->vm_flags & VM_EXEC))
+                               continue;
+
+                       if (text_vma) {
+                               pr_warn_once("%s[%d]: multiple EXEC segments unsupported\n",
+                                            current->comm, current->pid);

is this just something that fundamentally can't be supported by SFrame format? Or just an implementation simplification?

Quoted 1 line

It's a simplification I suppose.

That's a rather random limitation, IMO... How hard would it be to not make that assumption?

Quoted 5 lines

Quoted 3 lines

It's not illegal to have an executable with multiple VM_EXEC segments, no? Should this be a pr_warn_once() then?

I don't know, is it allowed? I've never seen it in practice. The

I'm pretty sure you can do that with a custom linker script, at the very least. Normally this probably won't happen, but I don't think Linux dictates how many executable VMAs an application can have. And it probably just naturally happens for JIT-ted applications (Java, Go, etc).

Linux kernel itself has two executable segments, for instance (though kernel is special, of course, but still).

Quoted 2 lines

pr_warn_once() is not reporting that it's illegal but rather that this corner case actually exists and maybe needs to be looked at.

This warn() will be logged across millions of machines in the fleet, triggering alarms, people looking at this, making custom internal patches to disable the known-to-happen warn. Why do we need all this? This is an issue that is trivial to trigger by user process that's not doing anything illegal. Why?

Quoted 3 lines

--
Josh

Quoted 88 lines · Andrii Nakryiko

Quoted 85 lines · Josh Poimboeuf

Quoted 4 lines · Andrii Nakryiko

It feels like this patch is trying to do too much. There is both new UAPI introduction, and SFrame format definition, and unwinder integration, etc, etc. Do you think it can be split further into more focused smaller patches?

True, let me see if I can split it up.

Quoted 5 lines

Quoted 5 lines

fs/binfmt_elf.c +5 -0

+
+                       if ((eppnt->p_flags & PF_X) && k < start_code)
+                               start_code = k;
+
+                       if ((eppnt->p_flags & PF_X) && k + eppnt->p_filesz > end_code)

Quoted 8 lines

fs/binfmt_elf.c static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex, · line 714

Quoted 5 lines

fs/binfmt_elf.c +5 -0

+                               end_code = k + eppnt->p_filesz;
+                       break;
+               }
+               case PT_GNU_SFRAME:
+                       sframe_phdr = eppnt;

if I understand correctly, there has to be only one sframe, is that right? Should we validate that?

Yes, there shouldn't be more than one PT_GNU_SFRAME for the executable itself. I can validate that.

Quoted 10 lines

fs/binfmt_elf.c static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex, · line 722

Quoted 8 lines

fs/binfmt_elf.c +5 -0

+                       break;
                 }
         }
 
+       if (sframe_phdr)
+               sframe_add_section(load_addr + sframe_phdr->p_vaddr,
+                                  start_code, end_code);
+

no error checking?

Good point. I remember discussing this with some people at Cauldon/LPC, I just forgot to do it!

Right now it does all the validation at unwind, which could really slow things down unnecessarily if the sframe isn't valid.

Quoted 9 lines

include/linux/sframe.h line 15

Quoted 7 lines

include/linux/sframe.h new file +7 -0

+#ifdef CONFIG_HAVE_UNWIND_USER_SFRAME
+
+#define INIT_MM_SFRAME .sframe_mt = MTREE_INIT(sframe_mt, 0),
+
+extern void sframe_free_mm(struct mm_struct *mm);
+
+/* text_start, text_end, file_name are optional */

what file_name? was that an extra argument that got removed?

Indeed, that was for some old code.

Quoted 8 lines

kernel/sys.c SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, · line 2791

Quoted 7 lines

         case PR_RISCV_SET_ICACHE_FLUSH_CTX:
                 error = RISCV_SET_ICACHE_FLUSH_CTX(arg2, arg3);
                 break;

kernel/sys.c +4 -0

+       case PR_ADD_SFRAME:
+               if (arg5)
+                       return -EINVAL;
+               error = sframe_add_section(arg2, arg3, arg4);

Quoted 8 lines

wouldn't it be better to make this interface extendable from the get go? Instead of passing 3 arguments with fixed meaning, why not pass a pointer to an extendable binary struct like seems to be the trend nowadays with nicely extensible APIs. See [0] for one such example (specifically, struct procmap_query). Seems more prudent, as we'll most probably will be adding flags, options, extra information, etc)

   [0] https://lore.kernel.org/linux-mm/[email protected]/

This ioctl interface was admittedly hacked together. I was hoping somebody would suggest something better :-) I'll take a look.

Quoted 9 lines

kernel/unwind/sframe.c line 78

Quoted 7 lines

kernel/unwind/sframe.c new file +7 -0

+static int find_fde(struct sframe_section *sec, unsigned long ip,
+                   struct sframe_fde *fde)
+{
+       struct sframe_fde __user *first, *last, *found = NULL;
+       u32 ip_off, func_off_low = 0, func_off_high = -1;
+
+       ip_off = ip - sec->sframe_addr;

what if ip_off is larger than 4GB? ELF section can be bigger than 4GB, right?

Quoted 1 line

That's baked into sframe v2.

Quoted 8 lines

I believe we do have large production binaries with more than 4GB of text, what are we going to do about them? It would be interesting to hear sframe people's opinion. Adding such a far-reaching new format in 2024 with these limitations is kind of sad. At the very least maybe we should allow some form of chaining sframe definitions to cover more than 4GB segments? Please CC relevant folks, I'm wondering what they're thinking about this.

SFrame V2 does have that limitation. We can try to have 64-bit representation for the 'ip' in the SFrame FDE and conditionalize it somehow (say, with a flag in the header) so as to not bloat the majority of applications.

Quoted 62 lines

Quoted 6 lines

Quoted 4 lines

and also, does it mean that SFrame doesn't support executables with text bigger than 4GB?

Yes, but is that a realistic concern?

See above, yes. You'd be surprised. As somewhat corroborating evidence, there were tons of problems and churn (within at least Meta) with DWARF not supporting more than 2GB sizes, so yes, this is not an abstract problem for sure. Modern production applications can be ridiculously big.

Quoted 17 lines

Quoted 16 lines

kernel/unwind/sframe.c line 312

Quoted 13 lines

kernel/unwind/sframe.c new file +12 -0

+       } else {
+               struct vm_area_struct *vma, *text_vma = NULL;
+               VMA_ITERATOR(vmi, mm, 0);
+
+               for_each_vma(vmi, vma) {
+                       if (vma->vm_file != sframe_vma->vm_file ||
+                           !(vma->vm_flags & VM_EXEC))
+                               continue;
+
+                       if (text_vma) {
+                               pr_warn_once("%s[%d]: multiple EXEC segments unsupported\n",
+                                            current->comm, current->pid);

is this just something that fundamentally can't be supported by SFrame format? Or just an implementation simplification?

Quoted 1 line

It's a simplification I suppose.

That's a rather random limitation, IMO... How hard would it be to not make that assumption?

Quoted 5 lines

Quoted 3 lines

It's not illegal to have an executable with multiple VM_EXEC segments, no? Should this be a pr_warn_once() then?

I don't know, is it allowed? I've never seen it in practice. The

I'm pretty sure you can do that with a custom linker script, at the very least. Normally this probably won't happen, but I don't think Linux dictates how many executable VMAs an application can have. And it probably just naturally happens for JIT-ted applications (Java, Go, etc).

Linux kernel itself has two executable segments, for instance (though kernel is special, of course, but still).

Quoted 2 lines

pr_warn_once() is not reporting that it's illegal but rather that this corner case actually exists and maybe needs to be looked at.

This warn() will be logged across millions of machines in the fleet, triggering alarms, people looking at this, making custom internal patches to disable the known-to-happen warn. Why do we need all this? This is an issue that is trivial to trigger by user process that's not doing anything illegal. Why?

Quoted 3 lines

--
Josh

BTW, I wanted to ask. Are there any plans to add SFrame support to Clang as well? It feels like without that there is no future for SFrame as a general-purpose solution for stack traces.

Quoted 3 lines · On Fri, 1 Nov 2024 11:38:47 -0700 Andrii Nakryiko <[email protected]>…

BTW, I wanted to ask. Are there any plans to add SFrame support to Clang as well? It feels like without that there is no future for SFrame as a general-purpose solution for stack traces.

We want to use SFrames inside Google, and having Clang support it is a requirement for that. I'm working on getting people to support it in Clang.

-- Steve

Quoted 99 lines · Indu Bhagat

Quoted 88 lines · Andrii Nakryiko

Quoted 85 lines · Josh Poimboeuf

Quoted 4 lines · Andrii Nakryiko

It feels like this patch is trying to do too much. There is both new UAPI introduction, and SFrame format definition, and unwinder integration, etc, etc. Do you think it can be split further into more focused smaller patches?

True, let me see if I can split it up.

Quoted 5 lines

Quoted 5 lines

fs/binfmt_elf.c +5 -0

+
+                       if ((eppnt->p_flags & PF_X) && k < start_code)
+                               start_code = k;
+
+                       if ((eppnt->p_flags & PF_X) && k + eppnt->p_filesz > end_code)

Quoted 8 lines

fs/binfmt_elf.c static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex, · line 714

Quoted 5 lines

fs/binfmt_elf.c +5 -0

+                               end_code = k + eppnt->p_filesz;
+                       break;
+               }
+               case PT_GNU_SFRAME:
+                       sframe_phdr = eppnt;

if I understand correctly, there has to be only one sframe, is that right? Should we validate that?

Yes, there shouldn't be more than one PT_GNU_SFRAME for the executable itself. I can validate that.

Quoted 10 lines

fs/binfmt_elf.c static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex, · line 722

Quoted 8 lines

fs/binfmt_elf.c +5 -0

+                       break;
                  }
          }
 
+       if (sframe_phdr)
+               sframe_add_section(load_addr + sframe_phdr->p_vaddr,
+                                  start_code, end_code);
+

no error checking?

Good point. I remember discussing this with some people at Cauldon/LPC, I just forgot to do it!

Right now it does all the validation at unwind, which could really slow things down unnecessarily if the sframe isn't valid.

Quoted 9 lines

include/linux/sframe.h line 15

Quoted 7 lines

include/linux/sframe.h new file +7 -0

+#ifdef CONFIG_HAVE_UNWIND_USER_SFRAME
+
+#define INIT_MM_SFRAME .sframe_mt = MTREE_INIT(sframe_mt, 0),
+
+extern void sframe_free_mm(struct mm_struct *mm);
+
+/* text_start, text_end, file_name are optional */

what file_name? was that an extra argument that got removed?

Indeed, that was for some old code.

Quoted 8 lines

kernel/sys.c SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, · line 2791

Quoted 7 lines

          case PR_RISCV_SET_ICACHE_FLUSH_CTX:
                  error = RISCV_SET_ICACHE_FLUSH_CTX(arg2, arg3);
                  break;

kernel/sys.c +4 -0

+       case PR_ADD_SFRAME:
+               if (arg5)
+                       return -EINVAL;
+               error = sframe_add_section(arg2, arg3, arg4);

Quoted 8 lines

wouldn't it be better to make this interface extendable from the get go? Instead of passing 3 arguments with fixed meaning, why not pass a pointer to an extendable binary struct like seems to be the trend nowadays with nicely extensible APIs. See [0] for one such example (specifically, struct procmap_query). Seems more prudent, as we'll most probably will be adding flags, options, extra information, etc)

    [0] https://lore.kernel.org/linux-mm/[email protected]/

This ioctl interface was admittedly hacked together. I was hoping somebody would suggest something better :-) I'll take a look.

Quoted 9 lines

kernel/unwind/sframe.c line 78

Quoted 7 lines

kernel/unwind/sframe.c new file +7 -0

+static int find_fde(struct sframe_section *sec, unsigned long ip,
+                   struct sframe_fde *fde)
+{
+       struct sframe_fde __user *first, *last, *found = NULL;
+       u32 ip_off, func_off_low = 0, func_off_high = -1;
+
+       ip_off = ip - sec->sframe_addr;

what if ip_off is larger than 4GB? ELF section can be bigger than 4GB, right?

Quoted 1 line

That's baked into sframe v2.

Quoted 8 lines

I believe we do have large production binaries with more than 4GB of text, what are we going to do about them? It would be interesting to hear sframe people's opinion. Adding such a far-reaching new format in 2024 with these limitations is kind of sad. At the very least maybe we should allow some form of chaining sframe definitions to cover more than 4GB segments? Please CC relevant folks, I'm wondering what they're thinking about this.

Quoted 4 lines

SFrame V2 does have that limitation. We can try to have 64-bit representation for the 'ip' in the SFrame FDE and conditionalize it somehow (say, with a flag in the header) so as to not bloat the majority of applications.

Hi Indu,

I think that's prudent if we believe that SFrame is the solution here. See my reply to Josh. Real-world already approach 4GB limits, and things are not going to shrink in the years to come. So yeah, probably we need some adjustments to the format to at least allow 64-bit offsets (though trying to stick to 32-bit as much as possible, of course, if they work).

I'm not really familiar with the nuances of the format just yet, so can't really provide anything more useful at this point. What would be the sort of gold reference for Sframe format to familiarize myself thoroughly?

BTW, I wanted to ask. Are there any plans to add SFrame support to Clang as well? It feels like without that there is no future for SFrame as a general-purpose solution for stack traces.

Quoted 63 lines

Quoted 62 lines

Quoted 6 lines

Quoted 4 lines

and also, does it mean that SFrame doesn't support executables with text bigger than 4GB?

Yes, but is that a realistic concern?

See above, yes. You'd be surprised. As somewhat corroborating evidence, there were tons of problems and churn (within at least Meta) with DWARF not supporting more than 2GB sizes, so yes, this is not an abstract problem for sure. Modern production applications can be ridiculously big.

Quoted 17 lines

Quoted 16 lines

kernel/unwind/sframe.c line 312

Quoted 13 lines

kernel/unwind/sframe.c new file +12 -0

+       } else {
+               struct vm_area_struct *vma, *text_vma = NULL;
+               VMA_ITERATOR(vmi, mm, 0);
+
+               for_each_vma(vmi, vma) {
+                       if (vma->vm_file != sframe_vma->vm_file ||
+                           !(vma->vm_flags & VM_EXEC))
+                               continue;
+
+                       if (text_vma) {
+                               pr_warn_once("%s[%d]: multiple EXEC segments unsupported\n",
+                                            current->comm, current->pid);

is this just something that fundamentally can't be supported by SFrame format? Or just an implementation simplification?

Quoted 1 line

It's a simplification I suppose.

That's a rather random limitation, IMO... How hard would it be to not make that assumption?

Quoted 5 lines

Quoted 3 lines

It's not illegal to have an executable with multiple VM_EXEC segments, no? Should this be a pr_warn_once() then?

I don't know, is it allowed? I've never seen it in practice. The

I'm pretty sure you can do that with a custom linker script, at the very least. Normally this probably won't happen, but I don't think Linux dictates how many executable VMAs an application can have. And it probably just naturally happens for JIT-ted applications (Java, Go, etc).

Linux kernel itself has two executable segments, for instance (though kernel is special, of course, but still).

Quoted 2 lines

pr_warn_once() is not reporting that it's illegal but rather that this corner case actually exists and maybe needs to be looked at.

This warn() will be logged across millions of machines in the fleet, triggering alarms, people looking at this, making custom internal patches to disable the known-to-happen warn. Why do we need all this? This is an issue that is trivial to trigger by user process that's not doing anything illegal. Why?

Quoted 3 lines

--
Josh

Quoted 3 lines

Quoted 1 line

what if ip_off is larger than 4GB? ELF section can be bigger than 4GB, right?

That's baked into sframe v2.

I believe we do have large production binaries with more than 4GB of text, what are we going to do about them? It would be interesting to hear sframe people's opinion. Adding such a far-reaching new format in 2024 with these limitations is kind of sad. At the very least maybe we should allow some form of chaining sframe definitions to cover more than 4GB segments? Please CC relevant folks, I'm wondering what they're thinking about this.

Quoted 8 lines

Quoted 6 lines

kernel/unwind/sframe.c line 312

Quoted 3 lines

kernel/unwind/sframe.c new file +3 -0

+                       if (text_vma) {
+                               pr_warn_once("%s[%d]: multiple EXEC segments unsupported\n",
+                                            current->comm, current->pid);

is this just something that fundamentally can't be supported by SFrame format? Or just an implementation simplification?

It's a simplification I suppose.

That's a rather random limitation, IMO... How hard would it be to not make that assumption?

Quoted 4 lines

Quoted 2 lines

It's not illegal to have an executable with multiple VM_EXEC segments, no? Should this be a pr_warn_once() then?

I don't know, is it allowed? I've never seen it in practice. The

I'm pretty sure you can do that with a custom linker script, at the very least. Normally this probably won't happen, but I don't think Linux dictates how many executable VMAs an application can have. And it probably just naturally happens for JIT-ted applications (Java, Go, etc).

Quoted 2 lines

pr_warn_once() is not reporting that it's illegal but rather that this corner case actually exists and maybe needs to be looked at.

This warn() will be logged across millions of machines in the fleet, triggering alarms, people looking at this, making custom internal patches to disable the known-to-happen warn. Why do we need all this? This is an issue that is trivial to trigger by user process that's not doing anything illegal. Why?

Quoted 11 lines · Andrii Nakryiko

Quoted 3 lines

Quoted 1 line

what if ip_off is larger than 4GB? ELF section can be bigger than 4GB, right?

That's baked into sframe v2.

I believe we do have large production binaries with more than 4GB of text, what are we going to do about them? It would be interesting to hear sframe people's opinion. Adding such a far-reaching new format in 2024 with these limitations is kind of sad. At the very least maybe we should allow some form of chaining sframe definitions to cover more than 4GB segments? Please CC relevant folks, I'm wondering what they're thinking about this.

Personally I find the idea of a single 4GB+ text segment pretty surprising as I've never seen anything even close to that.

Quoted 1 line

Quoted 1 line

((0x170ad000 - 0x200000) + (0x5b270000 - 0x4f600000) + (0x498e7000 - 0x172ac000)) / 1024 / 1024

Anyway it's iterative development and not everybody's requirements are clear from day 1. Which is why we're discussing it now. I think there are already plans to do an sframe v3.

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

kernel/unwind/sframe.c line 312

Quoted 4 lines

kernel/unwind/sframe.c new file +3 -0

+                       if (text_vma) {
+                               pr_warn_once("%s[%d]: multiple EXEC segments unsupported\n",
+                                            current->comm, current->pid);

is this just something that fundamentally can't be supported by SFrame format? Or just an implementation simplification?

Quoted 4 lines

Quoted 1 line

It's a simplification I suppose.

That's a rather random limitation, IMO... How hard would it be to not make that assumption?

It's definitely not random, there's no need to complicate the code if this condition doesn't exist.

Quoted 6 lines

Quoted 5 lines

Quoted 3 lines

It's not illegal to have an executable with multiple VM_EXEC segments, no? Should this be a pr_warn_once() then?

I don't know, is it allowed? I've never seen it in practice. The

Quoted 5 lines

I'm pretty sure you can do that with a custom linker script, at the very least. Normally this probably won't happen, but I don't think Linux dictates how many executable VMAs an application can have. And it probably just naturally happens for JIT-ted applications (Java, Go, etc).

Actually I just double checked and even the kernel's ELF loader assumes that each executable has only a single text start+end address pair.

Quoted 4 lines

Quoted 3 lines

pr_warn_once() is not reporting that it's illegal but rather that this corner case actually exists and maybe needs to be looked at.

Quoted 5 lines

This warn() will be logged across millions of machines in the fleet, triggering alarms, people looking at this, making custom internal patches to disable the known-to-happen warn. Why do we need all this? This is an issue that is trivial to trigger by user process that's not doing anything illegal. Why?

There's no point in adding complexity to support some hypothetical. I can remove the printk though.

--
Josh

00200000-170ad000 r--p 00000000 07:01 5
172ac000-498e7000 r-xp 16eac000 07:01 5
49ae7000-49b8b000 r--p 494e7000 07:01 5
49d8b000-4a228000 rw-p 4958b000 07:01 5
4a228000-4c677000 rw-p 00000000 00:00 0
4c800000-4ca00000 r-xp 49c00000 07:01 5
4ca00000-4f600000 r-xp 49e00000 07:01 5
4f600000-5b270000 r-xp 4ca00000 07:01 5

Sorry, I'm probably dense and missing something. But from the example process above, isn't this check violated already? Or it's two different things? Not sure, honestly.

172ac000-498e7000 r-xp 16eac000 07:01 5

4c800000-4ca00000 r-xp 49c00000 07:01 5
4ca00000-4f600000 r-xp 49e00000 07:01 5
4f600000-5b270000 r-xp 4ca00000 07:01 5

Quoted 2 lines

Actually I just double checked and even the kernel's ELF loader assumes that each executable has only a single text start+end address pair.

See above, very confused by such assumptions, but I'm hoping we are talking about two different things here.

Quoted 2 lines

There's no point in adding complexity to support some hypothetical. I can remove the printk though.

We are talking about fundamental things like format for supporting frame pointer-less stack trace capture. It will take years to adopt SFrame everywhere, so I think it's prudent to think a bit ahead beyond just saying "no real application should need more than 4GB text", IMO.

Quoted 9 lines · Andrii Nakryiko

00200000-170ad000 r--p 00000000 07:01 5
172ac000-498e7000 r-xp 16eac000 07:01 5
49ae7000-49b8b000 r--p 494e7000 07:01 5
49d8b000-4a228000 rw-p 4958b000 07:01 5
4a228000-4c677000 rw-p 00000000 00:00 0
4c800000-4ca00000 r-xp 49c00000 07:01 5
4ca00000-4f600000 r-xp 49e00000 07:01 5
4f600000-5b270000 r-xp 4ca00000 07:01 5

Quoted 3 lines

Sorry, I'm probably dense and missing something. But from the example process above, isn't this check violated already? Or it's two different things? Not sure, honestly.

It's hard to tell exactly what's going on, did you strip the file names?

The sframe limitation is per file, not per address space. I assume these are one file:

Quoted 1 line

172ac000-498e7000 r-xp 16eac000 07:01 5

and these are another:

Quoted 3 lines

4c800000-4ca00000 r-xp 49c00000 07:01 5
4ca00000-4f600000 r-xp 49e00000 07:01 5
4f600000-5b270000 r-xp 4ca00000 07:01 5

Multiple mappings for a single file is fine, as long as they're contiguous.

Quoted 6 lines

Quoted 3 lines

Actually I just double checked and even the kernel's ELF loader assumes that each executable has only a single text start+end address pair.

See above, very confused by such assumptions, but I'm hoping we are talking about two different things here.

The "contiguous text" thing seems enforced by the kernel for executables. However it doesn't manage shared libraries, those are mapped by the loader, e.g. /lib64/ld-linux-x86-64.so.2.

At a quick glance I can't tell if /lib64/ld-linux-x86-64.so.2 enforces that.

Quoted 7 lines

Quoted 2 lines

There's no point in adding complexity to support some hypothetical. I can remove the printk though.

We are talking about fundamental things like format for supporting frame pointer-less stack trace capture. It will take years to adopt SFrame everywhere, so I think it's prudent to think a bit ahead beyond just saying "no real application should need more than 4GB text", IMO.

I don't think anybody is saying that...

--
Josh

Quoted 2 lines · Josh Poimboeuf

On Fri, Nov 01, 2024 at 11:34:48AM -0700, Andrii Nakryiko wrote:

Quoted 9 lines

Quoted 9 lines

00200000-170ad000 r--p 00000000 07:01 5
172ac000-498e7000 r-xp 16eac000 07:01 5
49ae7000-49b8b000 r--p 494e7000 07:01 5
49d8b000-4a228000 rw-p 4958b000 07:01 5
4a228000-4c677000 rw-p 00000000 00:00 0
4c800000-4ca00000 r-xp 49c00000 07:01 5
4ca00000-4f600000 r-xp 49e00000 07:01 5
4f600000-5b270000 r-xp 4ca00000 07:01 5

Quoted 5 lines

Quoted 3 lines

Sorry, I'm probably dense and missing something. But from the example process above, isn't this check violated already? Or it's two different things? Not sure, honestly.

It's hard to tell exactly what's going on, did you strip the file names?

Yes, I did, of course. But as I said, they all belong to the same main binary of the process.

Quoted 6 lines

The sframe limitation is per file, not per address space. I assume these are one file:

Quoted 1 line

172ac000-498e7000 r-xp 16eac000 07:01 5

Quoted 8 lines

and these are another:

Quoted 3 lines

4c800000-4ca00000 r-xp 49c00000 07:01 5
4ca00000-4f600000 r-xp 49e00000 07:01 5
4f600000-5b270000 r-xp 4ca00000 07:01 5

Multiple mappings for a single file is fine, as long as they're contiguous.

No all of what I posted above belongs to the same file (except "4a228000-4c677000 rw-p 00000000 00:00 0" which doesn't have associated file, but I suspect it originally was part of this file, we do some tricks with re-mmap()'ing stuff due to huge pages usage).

Quoted 26 lines

Quoted 6 lines

Quoted 3 lines

Actually I just double checked and even the kernel's ELF loader assumes that each executable has only a single text start+end address pair.

See above, very confused by such assumptions, but I'm hoping we are talking about two different things here.

The "contiguous text" thing seems enforced by the kernel for executables. However it doesn't manage shared libraries, those are mapped by the loader, e.g. /lib64/ld-linux-x86-64.so.2.

At a quick glance I can't tell if /lib64/ld-linux-x86-64.so.2 enforces that.

Quoted 7 lines

Quoted 2 lines

There's no point in adding complexity to support some hypothetical. I can remove the printk though.

We are talking about fundamental things like format for supporting frame pointer-less stack trace capture. It will take years to adopt SFrame everywhere, so I think it's prudent to think a bit ahead beyond just saying "no real application should need more than 4GB text", IMO.

I don't think anybody is saying that...

--
Josh

Quoted 4 lines · Andrii Nakryiko

Quoted 2 lines · Josh Poimboeuf

On Fri, Nov 01, 2024 at 11:34:48AM -0700, Andrii Nakryiko wrote:

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

00200000-170ad000 r--p 00000000 07:01 5 172ac000-498e7000 r-xp 16eac000 07:01 5 49ae7000-49b8b000 r--p 494e7000 07:01 5 49d8b000-4a228000 rw-p 4958b000 07:01 5 4a228000-4c677000 rw-p 00000000 00:00 0 4c800000-4ca00000 r-xp 49c00000 07:01 5 4ca00000-4f600000 r-xp 49e00000 07:01 5 4f600000-5b270000 r-xp 4ca00000 07:01 5

I should have maybe posted this in this form:

00200000-170ad000 r--p 00000000 07:01 5  /packages/obfuscated_file
172ac000-498e7000 r-xp 16eac000 07:01 5  /packages/obfuscated_file
49ae7000-49b8b000 r--p 494e7000 07:01 5  /packages/obfuscated_file
49d8b000-4a228000 rw-p 4958b000 07:01 5  /packages/obfuscated_file
4a228000-4c677000 rw-p 00000000 00:00 0
4c800000-4ca00000 r-xp 49c00000 07:01 5  /packages/obfuscated_file
4ca00000-4f600000 r-xp 49e00000 07:01 5  /packages/obfuscated_file
4f600000-5b270000 r-xp 4ca00000 07:01 5  /packages/obfuscated_file

Those paths are pointing to the same binary.

Personally I find the idea of a single 4GB+ text segment pretty surprising as I've never seen anything even close to that.

Actually I just double checked and even the kernel's ELF loader assumes that each executable has only a single text start+end address pair.

Quoted 2 lines · Josh Poimboeuf

Actually I just double checked and even the kernel's ELF loader assumes that each executable has only a single text start+end address pair.

Huh? What makes you think that? There can be many executable PT_LOAD segments in each and every binary.

Quoted 6 lines · Segher Boessenkool

Quoted 2 lines · Josh Poimboeuf

Actually I just double checked and even the kernel's ELF loader assumes that each executable has only a single text start+end address pair.

Huh? What makes you think that? There can be many executable PT_LOAD segments in each and every binary.

Right, but for executables (not shared libraries) the kernel seems to assume they're contiguous? See the 'start_code' and 'end_code' variables in load_elf_binary() load_elf_interp().

Quoted 5 lines · Josh Poimboeuf

Quoted 4 lines · Segher Boessenkool

Quoted 2 lines · Josh Poimboeuf

Actually I just double checked and even the kernel's ELF loader assumes that each executable has only a single text start+end address pair.

Quoted 6 lines

Quoted 2 lines

Huh? What makes you think that? There can be many executable PT_LOAD segments in each and every binary.

Right, but for executables (not shared libraries) the kernel seems to assume they're contiguous? See the 'start_code' and 'end_code' variables in load_elf_binary() load_elf_interp().

Typo, see load_elf_binary (not load_elf_interp).

Quoted 9 lines · Josh Poimboeuf

Quoted 8 lines · Josh Poimboeuf

Quoted 6 lines · Segher Boessenkool

Quoted 2 lines · Josh Poimboeuf

Actually I just double checked and even the kernel's ELF loader assumes that each executable has only a single text start+end address pair.

Huh? What makes you think that? There can be many executable PT_LOAD segments in each and every binary.

Quoted 5 lines

Quoted 3 lines

Right, but for executables (not shared libraries) the kernel seems to assume they're contiguous? See the 'start_code' and 'end_code' variables in load_elf_binary() load_elf_interp().

Typo, see load_elf_binary (not load_elf_interp).

Hm, actually AFAICT that's only for reporting things in sysfs/proc. So maybe it's assumed but not really "enforced".

fs/binfmt_elf.c +23 -4

index 06dc4a57ba78..434c548f0837 100644
@@ -47,6 +47,7 @@
 #include <linux/dax.h>
 #include <linux/uaccess.h>
 #include <linux/rseq.h>
+#include <linux/sframe.h>
 #include <asm/param.h>
 #include <asm/page.h>
 

@@ -633,11 +634,13 @@ static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex,
 		unsigned long no_base, struct elf_phdr *interp_elf_phdata,
 		struct arch_elf_state *arch_state)
 {
-	struct elf_phdr *eppnt;
+	struct elf_phdr *eppnt, *sframe_phdr = NULL;
 	unsigned long load_addr = 0;
 	int load_addr_set = 0;
 	unsigned long error = ~0UL;
 	unsigned long total_size;
+	unsigned long start_code = ~0UL;
+	unsigned long end_code = 0;
 	int i;
 
 	/* First of all, some simple consistency checks */

@@ -659,7 +662,8 @@ static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex,
 
 	eppnt = interp_elf_phdata;
 	for (i = 0; i < interp_elf_ex->e_phnum; i++, eppnt++) {
-		if (eppnt->p_type == PT_LOAD) {
+		switch (eppnt->p_type) {
+		case PT_LOAD: {
 			int elf_type = MAP_PRIVATE;
 			int elf_prot = make_prot(eppnt->p_flags, arch_state,
 						 true, true);

@@ -688,7 +692,7 @@ static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex,
 			/*
 			 * Check to see if the section's size will overflow the
 			 * allowed task size. Note that p_filesz must always be
-			 * <= p_memsize so it's only necessary to check p_memsz.
+			 * <= p_memsz so it's only necessary to check p_memsz.
 			 */
 			k = load_addr + eppnt->p_vaddr;
 			if (BAD_ADDR(k) ||

@@ -698,9 +702,24 @@ static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex,
 				error = -ENOMEM;
 				goto out;
 			}
+
+			if ((eppnt->p_flags & PF_X) && k < start_code)
+				start_code = k;
+
+			if ((eppnt->p_flags & PF_X) && k + eppnt->p_filesz > end_code)
+				end_code = k + eppnt->p_filesz;
+			break;
+		}
+		case PT_GNU_SFRAME:
+			sframe_phdr = eppnt;
+			break;
 		}
 	}
 
+	if (sframe_phdr)
+		sframe_add_section(load_addr + sframe_phdr->p_vaddr,
+				   start_code, end_code);
+
 	error = load_addr;
 out:
 	return error;

@@ -823,7 +842,7 @@ static int load_elf_binary(struct linux_binprm *bprm)
 	int first_pt_load = 1;
 	unsigned long error;
 	struct elf_phdr *elf_ppnt, *elf_phdata, *interp_elf_phdata = NULL;
-	struct elf_phdr *elf_property_phdata = NULL;
+	struct elf_phdr *elf_property_phdata = NULL, *sframe_phdr = NULL;
 	unsigned long elf_brk;
 	int retval, i;
 	unsigned long elf_entry;

fs/binfmt_elf.c +4 -0

+	if (sframe_phdr)
+		sframe_add_section(load_bias + sframe_phdr->p_vaddr,
+				   start_code, end_code);
+
 	regs = current_pt_regs();
 #ifdef ELF_PLAT_INIT
 	/*

include/linux/mm_types.h +3 -0

index 381d22eba088..6e7561c1a5fc 100644
@@ -1052,6 +1052,9 @@ struct mm_struct {
 #endif
 		} lru_gen;
 #endif /* CONFIG_LRU_GEN_WALKS_MMU */
+#ifdef CONFIG_HAVE_UNWIND_USER_SFRAME
+		struct maple_tree sframe_mt;
+#endif
 	} __randomize_layout;
 
 	/*

On Mon, 28 Oct 2024 14:47:56 -0700
Josh Poimboeuf <[email protected]> wrote:

kernel/unwind/sframe.c +0 -0

kernel/unwind/sframe.c +39 -0

+#define __SFRAME_GET_USER(out, user_ptr, type)				\
+({									\
+	type __tmp;							\
+	if (get_user(__tmp, (type __user *)user_ptr))			\
+		return -EFAULT;						\
+	user_ptr += sizeof(__tmp);					\
+	out = __tmp;							\
+})
+
+#define SFRAME_GET_USER(out, user_ptr, size)				\
+({									\
+	switch (size) {							\
+	case 1:								\
+		__SFRAME_GET_USER(out, user_ptr, u8);			\
+		break;							\
+	case 2:								\
+		__SFRAME_GET_USER(out, user_ptr, u16);			\
+		break;							\
+	case 4:								\
+		__SFRAME_GET_USER(out, user_ptr, u32);			\
+		break;							\
+	default:							\
+		return -EINVAL;						\
+	}								\
+})
+
+static unsigned char fre_type_to_size(unsigned char fre_type)
+{
+	if (fre_type > 2)
+		return 0;
+	return 1 << fre_type;
+}
+
+static unsigned char offset_size_enum_to_size(unsigned char off_size)
+{
+	if (off_size > 2)
+		return 0;
+	return 1 << off_size;
+}

kernel/unwind/sframe.c +48 -0

+static int find_fre(struct sframe_section *sec, struct sframe_fde *fde,
+		    unsigned long ip, struct unwind_user_frame *frame)
+{
+	unsigned char fde_type = SFRAME_FUNC_FDE_TYPE(fde->info);
+	unsigned char fre_type = SFRAME_FUNC_FRE_TYPE(fde->info);
+	unsigned char offset_count, offset_size;
+	s32 cfa_off, ra_off, fp_off, ip_off;
+	void __user *f, *last_f = NULL;
+	unsigned char addr_size;
+	u32 last_fre_ip_off = 0;
+	u8 fre_info = 0;
+	int i;
+
+	addr_size = fre_type_to_size(fre_type);
+	if (!addr_size)
+		return -EINVAL;
+
+	ip_off = ip - (sec->sframe_addr + fde->start_addr);
+
+	f = (void __user *)sec->fres_addr + fde->fres_off;
+
+	for (i = 0; i < fde->fres_num; i++) {
+		u32 fre_ip_off;
+
+		SFRAME_GET_USER(fre_ip_off, f, addr_size);
+
+		if (fre_ip_off < last_fre_ip_off)
+			return -EINVAL;
+
+		last_fre_ip_off = fre_ip_off;
+
+		if (fde_type == SFRAME_FDE_TYPE_PCINC) {
+			if (ip_off < fre_ip_off)
+				break;
+		} else {
+			/* SFRAME_FDE_TYPE_PCMASK */
+			if (ip_off % fde->rep_size < fre_ip_off)
+				break;
+		}
+
+		SFRAME_GET_USER(fre_info, f, 1);
+
+		offset_count = SFRAME_FRE_OFFSET_COUNT(fre_info);
+		offset_size  = offset_size_enum_to_size(SFRAME_FRE_OFFSET_SIZE(fre_info));
+
+		if (!offset_count || !offset_size)
+			return -EINVAL;
+

kernel/unwind/sframe.c +0 -0

kernel/unwind/sframe.c +8 -0

+static int find_fre(struct sframe_section *sec, struct sframe_fde *fde,
+		    unsigned long ip, struct unwind_user_frame *frame)
+{
+	unsigned char fde_type = SFRAME_FUNC_FDE_TYPE(fde->info);
+	unsigned char fre_type = SFRAME_FUNC_FRE_TYPE(fde->info);
+	unsigned char offset_count, offset_size;
+	s32 cfa_off, ra_off, fp_off, ip_off;
+	void __user *f, *last_f = NULL;

kernel/unwind/sframe.c +66 -0

+
+	f = (void __user *)sec->fres_addr + fde->fres_off;
+
+	for (i = 0; i < fde->fres_num; i++) {
+		u32 fre_ip_off;
+
+		SFRAME_GET_USER(fre_ip_off, f, addr_size);
+
+		if (fre_ip_off < last_fre_ip_off)
+			return -EINVAL;
+
+		last_fre_ip_off = fre_ip_off;
+
+		if (fde_type == SFRAME_FDE_TYPE_PCINC) {
+			if (ip_off < fre_ip_off)
+				break;
+		} else {
+			/* SFRAME_FDE_TYPE_PCMASK */
+			if (ip_off % fde->rep_size < fre_ip_off)
+				break;
+		}
+
+		SFRAME_GET_USER(fre_info, f, 1);
+
+		offset_count = SFRAME_FRE_OFFSET_COUNT(fre_info);
+		offset_size  = offset_size_enum_to_size(SFRAME_FRE_OFFSET_SIZE(fre_info));
+
+		if (!offset_count || !offset_size)
+			return -EINVAL;
+
+		last_f = f;
+		f += offset_count * offset_size;
+	}
+
+	if (!last_f)
+		return -EINVAL;
+
+	f = last_f;
+
+	SFRAME_GET_USER(cfa_off, f, offset_size);
+	offset_count--;
+
+	ra_off = sec->ra_off;
+	if (!ra_off) {
+		if (!offset_count--)
+			return -EINVAL;
+
+		SFRAME_GET_USER(ra_off, f, offset_size);
+	}
+
+	fp_off = sec->fp_off;
+	if (!fp_off && offset_count) {
+		offset_count--;
+		SFRAME_GET_USER(fp_off, f, offset_size);
+	}
+
+	if (offset_count)
+		return -EINVAL;
+
+	frame->cfa_off = cfa_off;
+	frame->ra_off = ra_off;
+	frame->fp_off = fp_off;
+	frame->use_fp = SFRAME_FRE_CFA_BASE_REG_ID(fre_info) == SFRAME_BASE_REG_FP;
+
+	return 0;
+}

kernel/unwind/sframe.c +0 -0

kernel/unwind/sframe.c +18 -0

+
+			func_off_low = func_off;
+
+			found = mid;
+			first = mid + 1;
+		} else {
+			/* validate sort order */
+			if (func_off > func_off_high)
+				return -EINVAL;
+
+			func_off_high = func_off;
+
+			last = mid - 1;
+		}
+	}
+
+	if (!found)
+		return -EINVAL;