unwind_deferred: Implement sframe handling
Subsystems (10)
v8 cover unwind_deferred: Implement sframe handling
This code is based on top of:
https://lore.kernel.org/linux-trace-kernel/[email protected]/
git://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace.git
unwind/core
This is the implementation of parsing the SFrame section in an ELF file. It's a continuation of Josh's last work that can be found here:
https://lore.kernel.org/all/[email protected]/
Currently the only way to get a user space stack trace from a stack walk (and not just copying large amount of user stack into the kernel ring buffer) is to use frame pointers. This has a few issues. The biggest one is that compiling frame pointers into every application and library has been shown to cause performance overhead.
Another issue is that the format of the frames may not always be consistent between different compilers and some architectures (s390) has no defined format to do a reliable stack walk. The only way to perform user space profiling on these architectures is to copy the user stack into the kernel buffer.
SFrames[1] is now supported in gcc binutils and soon will also be supported by LLVM. SFrames acts more like ORC, and lives in the ELF executable file as its own section. Like ORC it has two tables where the first table is sorted by instruction pointers (IP) and using the current IP and finding it's entry in the first table, it will take you to the second table which will tell you where the return address of the current function is located and then you can use that address to look it up in the first table to find the return address of that function, and so on. This performs a user space stack walk.
Now because the SFrame section lives in the ELF file it needs to be faulted into memory when it is used. This means that walking the user space stack requires being in a faultable context. As profilers like perf request a stack trace in interrupt or NMI context, it cannot do the walking when it is requested. Instead it must be deferred until it is safe to fault in user space. One place this is known to be safe is when the task is about to return back to user space.
This series makes the deferred unwind code implement SFrames.
[1] https://sourceware.org/binutils/wiki/sframe
git://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace.git
unwind/sframe
The unwind/main branch is just unwind/sframe merged into unwind/perf and can be used for testing with perf.
Changes since v7: https://lore.kernel.org/linux-trace-kernel/[email protected]/
- Simply rebased on top of the latest unwind/core
Josh Poimboeuf (12):
unwind_user/sframe: Add support for reading .sframe headers unwind_user/sframe: Store sframe section data in per-mm maple tree x86/uaccess: Add unsafe_copy_from_user() implementation unwind_user/sframe: Add support for reading .sframe contents unwind_user/sframe: Detect .sframe sections in executables unwind_user/sframe: Wire up unwind_user to sframe unwind_user/sframe/x86: Enable sframe unwinding on x86 unwind_user/sframe: Remove .sframe section on detected corruption unwind_user/sframe: Show file name in debug output unwind_user/sframe: Enable debugging in uaccess regions unwind_user/sframe: Add .sframe validation option unwind_user/sframe: Add prctl() interface for registering .sframe sections
----
MAINTAINERS | 1 + arch/Kconfig | 23 ++ arch/x86/Kconfig | 1 + arch/x86/include/asm/mmu.h | 2 +- arch/x86/include/asm/uaccess.h | 39 ++- fs/binfmt_elf.c | 49 ++- include/linux/mm_types.h | 3 + include/linux/sframe.h | 60 ++++ include/linux/unwind_user_types.h | 1 + include/uapi/linux/elf.h | 1 + include/uapi/linux/prctl.h | 6 +- kernel/fork.c | 10 + kernel/sys.c | 9 + kernel/unwind/Makefile | 3 +- kernel/unwind/sframe.c | 612 ++++++++++++++++++++++++++++++++++++++ kernel/unwind/sframe.h | 71 +++++ kernel/unwind/sframe_debug.h | 99 ++++++ kernel/unwind/user.c | 25 +- mm/init-mm.c | 2 + 19 files changed, 998 insertions(+), 19 deletions(-) create mode 100644 include/linux/sframe.h create mode 100644 kernel/unwind/sframe.c create mode 100644 kernel/unwind/sframe.h create mode 100644 kernel/unwind/sframe_debug.h
PATCH v8 1/12 unwind_user/sframe: Add support for reading .sframe headers
From: Josh Poimboeuf <[email protected]>
In preparation for unwinding user space stacks with sframe, add basic sframe compile infrastructure and support for reading the .sframe section header.
sframe_add_section() reads the header and unconditionally returns an error, so it's not very useful yet. A subsequent patch will improve that.
--- MAINTAINERS | 1 + arch/Kconfig | 3 + include/linux/sframe.h | 40 ++++++++++++ kernel/unwind/Makefile | 3 +- kernel/unwind/sframe.c | 136 +++++++++++++++++++++++++++++++++++++++++ kernel/unwind/sframe.h | 71 +++++++++++++++++++++ 6 files changed, 253 insertions(+), 1 deletion(-) create mode 100644 include/linux/sframe.h create mode 100644 kernel/unwind/sframe.c create mode 100644 kernel/unwind/sframe.h
MAINTAINERS +1 -0
@@ -25932,6 +25932,7 @@ USERSPACE STACK UNWINDING M: Josh Poimboeuf <[email protected]> M: Steven Rostedt <[email protected]> S: Maintained +F: include/linux/sframe.h F: include/linux/unwind*.h F: kernel/unwind/
arch/Kconfig +3 -0
@@ -446,6 +446,9 @@ config HAVE_UNWIND_USER_COMPAT_FP bool depends on HAVE_UNWIND_USER_FP +config HAVE_UNWIND_USER_SFRAME + bool + config HAVE_PERF_REGS bool help
include/linux/sframe.h new file +40 -0
@@ -0,0 +1,40 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef _LINUX_SFRAME_H +#define _LINUX_SFRAME_H + +#include <linux/mm_types.h> +#include <linux/unwind_user_types.h> + +#ifdef CONFIG_HAVE_UNWIND_USER_SFRAME + +struct sframe_section { +unsigned long sframe_start; +unsigned long sframe_end; +unsigned long text_start; +unsigned long text_end; + +unsigned long fdes_start; +unsigned long fres_start; +unsigned long fres_end; +unsigned int num_fdes; + +signed char ra_off; +signed char fp_off; +}; + +extern int sframe_add_section (unsigned long sframe_start,unsigned long sframe_end, +unsigned long text_start,unsigned long text_end); +extern int sframe_remove_section (unsigned long sframe_addr); + +#else /* !CONFIG_HAVE_UNWIND_USER_SFRAME */ + +static inline int sframe_add_section (unsigned long sframe_start,unsigned long sframe_end, +unsigned long text_start,unsigned long text_end) +{ +return -ENOSYS ; +} +static inline int sframe_remove_section (unsigned long sframe_addr) {return -ENOSYS ; } + +#endif /* CONFIG_HAVE_UNWIND_USER_SFRAME */ + +#endif /* _LINUX_SFRAME_H */
kernel/unwind/Makefile +2 -1
@@ -1 +1,2 @@ - obj-$(CONFIG_UNWIND_USER) += user.o deferred.o + obj-$(CONFIG_UNWIND_USER) += user.o deferred.o + obj-$(CONFIG_HAVE_UNWIND_USER_SFRAME) += sframe.o
kernel/unwind/sframe.c new file +136 -0
136 lines not on this page · raw message
kernel/unwind/sframe.h new file +71 -0
71 lines not on this page · raw message
Signature · 1 line
2.47.2
PATCH v8 2/12 unwind_user/sframe: Store sframe section data in per-mm maple tree
In reply to Steven RostedtFrom: Josh Poimboeuf <[email protected]>
Associate an sframe section with its mm by adding it to a per-mm maple tree which is indexed by the corresponding text address range. A single sframe section can be associated with multiple text ranges.
--- arch/x86/include/asm/mmu.h | 2 +- include/linux/mm_types.h | 3 +++ include/linux/sframe.h | 13 +++++++++ kernel/fork.c | 10 +++++++ kernel/unwind/sframe.c | 55 +++++++++++++++++++++++++++++++++++--- mm/init-mm.c | 2 ++ 6 files changed, 81 insertions(+), 4 deletions(-)
arch/x86/include/asm/mmu.h +1 -1
@@ -87,7 +87,7 @@ typedef struct { .context = { \ .ctx_id = 1, \ .lock = __MUTEX_INITIALIZER(mm.context.lock), \ - } + },void leave_mm (void );#define leave_mm leave_mm
include/linux/mm_types.h +3 -0
@@ -1206,6 +1206,9 @@ struct mm_struct {#ifdef CONFIG_MM_ID mm_id_t mm_id;#endif /* CONFIG_MM_ID */ +#ifdef CONFIG_HAVE_UNWIND_USER_SFRAME +struct maple_tree sframe_mt; +#endif } __randomize_layout;/*
include/linux/sframe.h +13 -0
@@ -22,18 +22,31 @@ struct sframe_section {signed char fp_off; }; +#define INIT_MM_SFRAME .sframe_mt = MTREE_INIT(sframe_mt, 0), +extern void sframe_free_mm (struct mm_struct *mm); +extern int sframe_add_section (unsigned long sframe_start,unsigned long sframe_end,unsigned long text_start,unsigned long text_end);extern int sframe_remove_section (unsigned long sframe_addr); +static inline bool current_has_sframe (void ) +{ +struct mm_struct *mm = current->mm; + +return mm && !mtree_empty (&mm->sframe_mt); +} +#else /* !CONFIG_HAVE_UNWIND_USER_SFRAME */ +#define INIT_MM_SFRAME +static inline void sframe_free_mm (struct mm_struct *mm) {}static inline int sframe_add_section (unsigned long sframe_start,unsigned long sframe_end,unsigned long text_start,unsigned long text_end) {return -ENOSYS ; }static inline int sframe_remove_section (unsigned long sframe_addr) {return -ENOSYS ; } +static inline bool current_has_sframe (void ) {return false; }#endif /* CONFIG_HAVE_UNWIND_USER_SFRAME */
kernel/fork.c +10 -0
@@ -106,6 +106,7 @@#include <linux/pidfs.h> #include <linux/tick.h> #include <linux/unwind_deferred.h> +#include <linux/sframe.h> #include <asm/pgalloc.h> #include <linux/uaccess.h>
@@ -687,6 +688,7 @@ void __mmdrop(struct mm_struct *mm)mm_pasid_drop (mm);mm_destroy_cid (mm);percpu_counter_destroy_many (mm->rss_stat,NR_MM_COUNTERS ); +sframe_free_mm (mm);free_mm (mm); }
@@ -1024,6 +1026,13 @@ static void mmap_init_lock(struct mm_struct *mm)#endif } +static void mm_init_sframe (struct mm_struct *mm) +{ +#ifdef CONFIG_HAVE_UNWIND_USER_SFRAME +mt_init (&mm->sframe_mt); +#endif +} +static struct mm_struct *mm_init (struct mm_struct *mm,struct task_struct *p,struct user_namespace *user_ns) {
@@ -1053,6 +1062,7 @@ static struct mm_struct *mm_init(struct mm_struct *mm, struct task_struct *p, mm->pmd_huge_pte =NULL ;#endif mm_init_uprobes_state (mm); +mm_init_sframe (mm);hugetlb_count_init (mm);if (current->mm) {
kernel/unwind/sframe.c +52 -3
67 lines not on this page · raw message
mm/init-mm.c +2 -0
@@ -11,6 +11,7 @@#include <linux/atomic.h> #include <linux/user_namespace.h> #include <linux/iommu.h> +#include <linux/sframe.h> #include <asm/mmu.h> #ifndef INIT_MM_CONTEXT
@@ -46,6 +47,7 @@ struct mm_struct init_mm = { .user_ns = &init_user_ns, .cpu_bitmap =CPU_BITS_NONE ,INIT_MM_CONTEXT (init_mm) + INIT_MM_SFRAME }; void setup_initial_init_mm(void *start_code, void *end_code,
Signature · 1 line
2.47.2
PATCH v8 3/12 x86/uaccess: Add unsafe_copy_from_user() implementation
In reply to Steven RostedtFrom: Josh Poimboeuf <[email protected]>
Add an x86 implementation of unsafe_copy_from_user() similar to the existing unsafe_copy_to_user().
--- arch/x86/include/asm/uaccess.h | 39 +++++++++++++++++++++++++--------- 1 file changed, 29 insertions(+), 10 deletions(-)
arch/x86/include/asm/uaccess.h +29 -10
@@ -599,7 +599,7 @@ _label: \* We want the unsafe accessors to always be inlined and use * the error labels - thus the macro games. */ -#define unsafe_copy_loop (dst, src, len, type, label) \ +#define unsafe_copy_to_user_loop (dst, src, len, type, label) \ while (len >= sizeof(type)) { \ unsafe_put_user(*(type *)(src),(type __user *)(dst),label); \ dst += sizeof(type); \
@@ -607,15 +607,34 @@ _label: \ len -=sizeof (type); \ } -#define unsafe_copy_to_user (_dst,_src,_len,label) \ -do { \ - char __user *__ucu_dst = (_dst); \ - const char *__ucu_src = (_src); \ - size_t __ucu_len = (_len); \ - unsafe_copy_loop(__ucu_dst, __ucu_src, __ucu_len, u64, label); \ - unsafe_copy_loop(__ucu_dst, __ucu_src, __ucu_len, u32, label); \ - unsafe_copy_loop(__ucu_dst, __ucu_src, __ucu_len, u16, label); \ - unsafe_copy_loop(__ucu_dst, __ucu_src, __ucu_len, u8, label); \ +#define unsafe_copy_to_user (_dst, _src, _len, label) \ +do { \ + void __user *__dst = (_dst); \ + const void *__src = (_src); \ + size_t __len = (_len); \ + unsafe_copy_to_user_loop(__dst, __src, __len, u64, label); \ + unsafe_copy_to_user_loop(__dst, __src, __len, u32, label); \ + unsafe_copy_to_user_loop(__dst, __src, __len, u16, label); \ + unsafe_copy_to_user_loop(__dst, __src, __len, u8, label); \ +} while (0) + +#define unsafe_copy_from_user_loop (dst, src, len, type, label) \ + while (len >= sizeof(type)) { \ + unsafe_get_user(*(type *)(dst), (type __user *)(src), label); \ + dst += sizeof(type); \ + src += sizeof(type); \ + len -= sizeof(type); \ + } + +#define unsafe_copy_from_user (_dst, _src, _len, label) \ +do { \ + void *__dst = (_dst); \ + void __user *__src = (_src); \ + size_t __len = (_len); \ + unsafe_copy_from_user_loop(__dst, __src, __len, u64, label); \ + unsafe_copy_from_user_loop(__dst, __src, __len, u32, label); \ + unsafe_copy_from_user_loop(__dst, __src, __len, u16, label); \ + unsafe_copy_from_user_loop(__dst, __src, __len, u8, label); \ } while (0)#ifdef CONFIG_CC_HAS_ASM_GOTO_OUTPUT
Signature · 1 line
2.47.2
PATCH v8 4/12 unwind_user/sframe: Add support for reading .sframe contents
In reply to Steven RostedtFrom: Josh Poimboeuf <[email protected]>
In preparation for using sframe to unwind user space stacks, add an sframe_find() interface for finding the sframe information associated with a given text address.
For performance, use user_read_access_begin() and the corresponding unsafe_*() accessors. Note that use of pr_debug() in uaccess-enabled regions would break noinstr validation, so there aren't any debug messages yet. That will be added in a subsequent commit.
--- include/linux/sframe.h | 5 + kernel/unwind/sframe.c | 311 ++++++++++++++++++++++++++++++++++- kernel/unwind/sframe_debug.h | 35 ++++ 3 files changed, 347 insertions(+), 4 deletions(-) create mode 100644 kernel/unwind/sframe_debug.h
include/linux/sframe.h +5 -0
@@ -3,11 +3,14 @@#define _LINUX_SFRAME_H#include <linux/mm_types.h> +#include <linux/srcu.h> #include <linux/unwind_user_types.h> #ifdef CONFIG_HAVE_UNWIND_USER_SFRAME struct sframe_section { + struct rcu_head rcu; + unsigned long sframe_start; unsigned long sframe_end; unsigned long text_start;
@@ -28,6 +31,7 @@ extern void sframe_free_mm(struct mm_struct *mm);extern int sframe_add_section (unsigned long sframe_start,unsigned long sframe_end,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 ) {
@@ -46,6 +50,7 @@ static inline int sframe_add_section(unsigned long sframe_start, unsigned long s return -ENOSYS; }static inline int sframe_remove_section (unsigned long sframe_addr) {return -ENOSYS ; } +static inline int sframe_find (unsigned long ip,struct unwind_user_frame *frame) {return -ENOSYS ; }static inline bool current_has_sframe (void ) {return false; }#endif /* CONFIG_HAVE_UNWIND_USER_SFRAME */
kernel/unwind/sframe.c +307 -4
337 lines not on this page · raw message
kernel/unwind/sframe_debug.h new file +35 -0
@@ -0,0 +1,35 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef _SFRAME_DEBUG_H +#define _SFRAME_DEBUG_H + +#include <linux/sframe.h> +#include "sframe.h" + +#ifdef CONFIG_DYNAMIC_DEBUG + +#define dbg (fmt, ...) \ + pr_debug("%s (%d): " fmt, current->comm, current->pid, ##__VA_ARGS__) + +static __always_inline void dbg_print_header(struct sframe_section *sec) +{ +unsigned long fdes_end; + + fdes_end = sec->fdes_start + (sec->num_fdes *sizeof (struct sframe_fde )); + +dbg ("SEC: sframe:0x%lx-0x%lx text:0x%lx-0x%lx " +"fdes:0x%lx-0x%lx fres:0x%lx-0x%lx " +"ra_off:%d fp_off:%d\n" , + sec->sframe_start, sec->sframe_end, sec->text_start, sec->text_end, + sec->fdes_start, fdes_end, sec->fres_start, sec->fres_end, + sec->ra_off, sec->fp_off); +} + +#else /* !CONFIG_DYNAMIC_DEBUG */ + +#define dbg(args...) no_printk(args) + +static inline void dbg_print_header (struct sframe_section *sec) {} + +#endif /* !CONFIG_DYNAMIC_DEBUG */ + +#endif /* _SFRAME_DEBUG_H */
Signature · 1 line
2.47.2
PATCH v8 5/12 unwind_user/sframe: Detect .sframe sections in executables
In reply to Steven RostedtFrom: Josh Poimboeuf <[email protected]>
When loading an ELF executable, automatically detect an .sframe section and associate it with the mm_struct.
--- fs/binfmt_elf.c | 49 +++++++++++++++++++++++++++++++++++++--- include/uapi/linux/elf.h | 1 + 2 files changed, 47 insertions(+), 3 deletions(-)
fs/binfmt_elf.c +46 -3
@@ -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>
@@ -622,6 +623,21 @@ 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_sframe (struct elf_phdr *text,struct elf_phdr *sframe, +unsigned long base_addr) +{ +unsigned long sframe_start, sframe_end, text_start, text_end; + + sframe_start = base_addr + sframe->p_vaddr; + sframe_end = sframe_start + sframe->p_memsz; + + text_start = base_addr + text->p_vaddr; + text_end = text_start + text->p_memsz; + +/* Ignore return value, sframe section isn't critical */ +sframe_add_section (sframe_start, sframe_end, text_start, text_end); +} +/* This is much more generalized than the library routine read function, so we keep this separate. Technically the library read function is only provided so that we can read a.out libraries that have
@@ -632,7 +648,7 @@ 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 ;
@@ -658,7 +674,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);
@@ -697,6 +714,20 @@ static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex, error = -ENOMEM ; goto out; } +break ; + } +case PT_GNU_SFRAME : + sframe_phdr = eppnt; +break ; + } + } + +if (sframe_phdr) { + eppnt = interp_elf_phdata; +for (i =0 ; i < interp_elf_ex->e_phnum; i++, eppnt++) { +if (eppnt->p_flags &PF_X ) { +elf_add_sframe (eppnt, sframe_phdr, load_addr); + } } }
@@ -821,7 +852,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;bool brk_moved = false;int retval, i;
@@ -930,6 +961,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,
@@ -1227,6 +1262,14 @@ static int load_elf_binary(struct linux_binprm *bprm) elf_brk = k; } +if (sframe_phdr) { +for (i =0 , elf_ppnt = elf_phdata; + i < elf_ex->e_phnum; i++, elf_ppnt++) { +if ((elf_ppnt->p_flags &PF_X )) +elf_add_sframe (elf_ppnt, sframe_phdr, load_bias); + } + } + e_entry = elf_ex->e_entry + load_bias; phdr_addr += load_bias; elf_brk += load_bias;
include/uapi/linux/elf.h +1 -0
@@ -41,6 +41,7 @@ typedef __u16 Elf64_Versym;#define PT_GNU_STACK (PT_LOOS + 0x474e551)#define PT_GNU_RELRO (PT_LOOS + 0x474e552)#define PT_GNU_PROPERTY (PT_LOOS + 0x474e553) +#define PT_GNU_SFRAME (PT_LOOS + 0x474e554)/* ARM MTE memory tag segment type */
Signature · 1 line
2.47.2
PATCH v8 6/12 unwind_user/sframe: Wire up unwind_user to sframe
In reply to Steven RostedtFrom: Josh Poimboeuf <[email protected]>
Now that the sframe infrastructure is fully in place, make it work by hooking it up to the unwind_user interface.
--- arch/Kconfig | 1 + include/linux/unwind_user_types.h | 1 + kernel/unwind/user.c | 25 ++++++++++++++++++++++--- 3 files changed, 24 insertions(+), 3 deletions(-)
arch/Kconfig +1 -0
@@ -448,6 +448,7 @@ config HAVE_UNWIND_USER_COMPAT_FP config HAVE_UNWIND_USER_SFRAME bool + select UNWIND_USER config HAVE_PERF_REGS bool
include/linux/unwind_user_types.h +1 -0
@@ -13,6 +13,7 @@ enum unwind_user_type {UNWIND_USER_TYPE_NONE ,UNWIND_USER_TYPE_FP ,UNWIND_USER_TYPE_COMPAT_FP , + UNWIND_USER_TYPE_SFRAME, }; struct unwind_stacktrace {
kernel/unwind/user.c +22 -3
@@ -7,6 +7,7 @@#include <linux/sched/task_stack.h> #include <linux/unwind_user.h> #include <linux/uaccess.h> +#include <linux/sframe.h> static struct unwind_user_frame fp_frame = {ARCH_INIT_USER_FP_FRAME
@@ -31,6 +32,12 @@ static inline bool compat_fp_state(struct unwind_user_state *state) state->type ==UNWIND_USER_TYPE_COMPAT_FP ; } +static inline bool sframe_state (struct unwind_user_state *state) +{ +return IS_ENABLED (CONFIG_HAVE_UNWIND_USER_SFRAME ) && + state->type ==UNWIND_USER_TYPE_SFRAME ; +} +#define unwind_get_user_long (to, from, state) \ ({ \ int __ret; \
@@ -44,18 +51,28 @@ static inline bool compat_fp_state(struct unwind_user_state *state)static int unwind_user_next (struct unwind_user_state *state) {struct unwind_user_frame *frame; +struct unwind_user_frame _frame;unsigned long cfa =0 , fp, ra =0 ;unsigned int shift;if (state->done)return -EINVAL ; -if (compat_fp_state (state)) +if (compat_fp_state (state)) { frame = &compat_fp_frame; -else if (fp_state (state)) + }else if (sframe_state (state)) { +/* sframe expects the frame to be local storage */ + frame = &_frame; +if (sframe_find (state->ip, frame)) { +if (!IS_ENABLED (CONFIG_HAVE_UNWIND_USER_FP )) + goto done; + frame = &fp_frame; + } + }else if (fp_state (state)) { frame = &fp_frame; -else + }else { goto done; + }if (frame->use_fp) {if (state->fp < state->sp)
@@ -111,6 +128,8 @@ static int unwind_user_start(struct unwind_user_state *state)if (IS_ENABLED (CONFIG_HAVE_UNWIND_USER_COMPAT_FP ) &&in_compat_mode (regs)) state->type =UNWIND_USER_TYPE_COMPAT_FP ; +else if (current_has_sframe ()) + state->type =UNWIND_USER_TYPE_SFRAME ;else if (IS_ENABLED (CONFIG_HAVE_UNWIND_USER_FP )) state->type =UNWIND_USER_TYPE_FP ;else
Signature · 1 line
2.47.2
Mathieu Desnoyers
reviewerRe: [PATCH v8 06/12] unwind_user/sframe: Wire up unwind_user to sframe
Quoted 95 lines · Steven Rostedt
From: Josh Poimboeuf <[email protected]>
Now that the sframe infrastructure is fully in place, make it work by hooking it up to the
unwind_userinterface.Signed-off-by Josh Poimboeuf <[email protected]>Signed-off-by Steven Rostedt (Google) <[email protected]>--- arch/Kconfig | 1 + include/linux/unwind_user_types.h | 1 + kernel/unwind/user.c | 25 ++++++++++++++++++++++--- 3 files changed, 24 insertions(+), 3 deletions(-)arch/Kconfig +1
-0@@ -448,6 +448,7 @@ config HAVE_UNWIND_USER_COMPAT_FP config HAVE_UNWIND_USER_SFRAME bool + select UNWIND_USER config HAVE_PERF_REGS boolinclude/linux/unwind_user_types.h +1
-0@@ -13,6 +13,7 @@ enum unwind_user_type {UNWIND_USER_TYPE_NONE ,UNWIND_USER_TYPE_FP ,UNWIND_USER_TYPE_COMPAT_FP , + UNWIND_USER_TYPE_SFRAME, }; struct unwind_stacktrace {kernel/unwind/user.c +20
-3@@ -7,6 +7,7 @@#include <linux/sched/task_stack.h> #include <linux/unwind_user.h> #include <linux/uaccess.h> +#include <linux/sframe.h> static struct unwind_user_frame fp_frame = {ARCH_INIT_USER_FP_FRAME @@ -31,6 +32,12 @@ static inline bool compat_fp_state(struct unwind_user_state *state) state->type ==UNWIND_USER_TYPE_COMPAT_FP ; } +static inline bool sframe_state (struct unwind_user_state *state) +{ +return IS_ENABLED (CONFIG_HAVE_UNWIND_USER_SFRAME ) && + state->type ==UNWIND_USER_TYPE_SFRAME ; +} +#define unwind_get_user_long (to, from, state) \ ({ \ int __ret; \@@ -44,18 +51,28 @@ static inline bool compat_fp_state(struct unwind_user_state *state)static int unwind_user_next (struct unwind_user_state *state) {struct unwind_user_frame *frame; +struct unwind_user_frame _frame;unsigned long cfa =0 , fp, ra =0 ;unsigned int shift;if (state->done)return -EINVAL ; -if (compat_fp_state (state)) +if (compat_fp_state (state)) { frame = &compat_fp_frame; -else if (fp_state (state)) + }else if (sframe_state (state)) { +/* sframe expects the frame to be local storage */ + frame = &_frame; +if (sframe_find (state->ip, frame)) { +if (!IS_ENABLED (CONFIG_HAVE_UNWIND_USER_FP )) + goto done; + frame = &fp_frame; + } + }else if (fp_state (state)) { frame = &fp_frame; -else + }else { goto done; + }if (frame->use_fp) {if (state->fp < state->sp)
kernel/unwind/user.c static int unwind_user_start(struct unwind_user_state *state) · line 132
Quoted 5 lines
kernel/unwind/user.c +2
-0@@ -111,6 +128,8 @@ static int unwind_user_start(struct unwind_user_state *state)if (IS_ENABLED (CONFIG_HAVE_UNWIND_USER_COMPAT_FP ) &&in_compat_mode (regs)) state->type =UNWIND_USER_TYPE_COMPAT_FP ; +else if (current_has_sframe ()) + state->type =UNWIND_USER_TYPE_SFRAME ;
I think you'll want to update the state->type during the traversal (in next()), because depending on whether sframe is available for a given memory area of code or not, the next() function can use either frame pointers or sframe during the same traversal. It would be good to know which is used after each specific call to next().
Thanks,
Mathieu
Quoted 3 lines
kernel/unwind/user.c +0
-0else if (IS_ENABLED (CONFIG_HAVE_UNWIND_USER_FP )) state->type =UNWIND_USER_TYPE_FP ;else
Signature · 3 lines
Mathieu Desnoyers EfficiOS Inc. https://www.efficios.com
Steven Rostedt
maintainerRe: [PATCH v8 06/12] unwind_user/sframe: Wire up unwind_user to sframe
Quoted 13 lines · On Tue, 8 Jul 2025 15:58:56 -0400 Mathieu Desnoyers…
kernel/unwind/user.c static int unwind_user_start(struct unwind_user_state *state) · line 132
Quoted 6 lines
kernel/unwind/user.c +2
-0@@ -111,6 +128,8 @@ static int unwind_user_start(struct unwind_user_state *state)if (IS_ENABLED (CONFIG_HAVE_UNWIND_USER_COMPAT_FP ) &&in_compat_mode (regs)) state->type =UNWIND_USER_TYPE_COMPAT_FP ; +else if (current_has_sframe ()) + state->type =UNWIND_USER_TYPE_SFRAME ;I think you'll want to update the
state->typeduring the traversal (innext()), because depending on whether sframe is available for a given memory area of code or not, thenext()function can use either frame pointers or sframe during the same traversal. It would be good to know which is used after each specific call tonext().
From my understanding this sets up what is available for the task at the beginning.
So once we say "this task has sframes" it will try to use it every time. In next we have:
if (compat_fp_state(state)) {
frame = &compat_fp_frame;
} else if (sframe_state(state)) {
/* sframe expects the frame to be local storage */
frame = &_frame;
if (sframe_find(state->ip, frame)) {
if (!IS_ENABLED(CONFIG_HAVE_UNWIND_USER_FP))
goto done;
frame = &fp_frame;
}
} else if (fp_state(state)) {
frame = &fp_frame;
} else {
goto done;
}Where if sframe_find() fails and we switch over to frame pointers, if frame pointers works, we can continue. But the next iteration, where the frame pointer finds the previous ip, that ip may be in the sframe section again.
I've seen this work with my trace_printk()s. A function from code that is running sframes calls into a library function that has frame pointers. The walk walks through the frame pointers in the library, and when it hits the code that has sframes, it starts using that again.
If we switched the state to just FP, it will never try to use sframes.
So this state is more about "what does this task have" than what was used per iteration.
-- Steve
Re: [PATCH v8 06/12] unwind_user/sframe: Wire up unwind_user to sframe
Quoted 39 lines · Steven Rostedt
Quoted 13 lines · On Tue, 8 Jul 2025 15:58:56 -0400 Mathieu Desnoyers…
kernel/unwind/user.c static int unwind_user_start(struct unwind_user_state *state) · line 132
Quoted 6 lines
kernel/unwind/user.c +2
-0@@ -111,6 +128,8 @@ static int unwind_user_start(struct unwind_user_state *state)if (IS_ENABLED (CONFIG_HAVE_UNWIND_USER_COMPAT_FP ) &&in_compat_mode (regs)) state->type =UNWIND_USER_TYPE_COMPAT_FP ; +else if (current_has_sframe ()) + state->type =UNWIND_USER_TYPE_SFRAME ;I think you'll want to update the
state->typeduring the traversal (innext()), because depending on whether sframe is available for a given memory area of code or not, thenext()function can use either frame pointers or sframe during the same traversal. It would be good to know which is used after each specific call tonext().From my understanding this sets up what is available for the task at the beginning.
So once we say "this task has sframes" it will try to use it every time. In next we have:
if (compat_fp_state(state)) { frame = &compat_fp_frame; } else if (sframe_state(state)) { /* sframe expects the frame to be local storage */ frame = &_frame; if (sframe_find(state->ip, frame)) { if (!IS_ENABLED(CONFIG_HAVE_UNWIND_USER_FP)) goto done; frame = &fp_frame; } } else if (fp_state(state)) { frame = &fp_frame; } else { goto done; }
Quoted 8 lines
Where if
sframe_find()fails and we switch over to frame pointers, if frame pointers works, we can continue. But the next iteration, where the frame pointer finds the previous ip, that ip may be in the sframe section again.I've seen this work with my trace_printk()s. A function from code that is running sframes calls into a library function that has frame pointers. The walk walks through the frame pointers in the library, and when it hits the code that has sframes, it starts using that again.
I think Mathieu has a point, as unwind_user_next() calls the optional architecture-specific arch_unwind_user_next() at the end. The x86 implementation does state->type specific processing (for UNWIND_USER_TYPE_COMPAT_FP).
Quoted 4 lines
If we switched the state to just FP, it will never try to use sframes.
So this state is more about "what does this task have" than what was used per iteration.
While there is currently no fallback to UNWIND_USER_TYPE_COMPAT_FP that would strictly require this, it could be useful to have both information.
Or the logic in unwind_user_start(), unwind_user_next(), and *_state() may need to be adjusted so that state->type reflects the currently used method, which unwind_user_next() determines and sets anew for every step.
Regards,
Jens
Signature · 9 lines
Jens Remus Linux on Z Development (D3303) +49-7031-16-1128 Office [email protected] IBM IBM Deutschland Research & Development GmbH; Vorsitzender des Aufsichtsrats: Wolfgang Wendt; Geschäftsführung: David Faller; Sitz der Gesellschaft: Böblingen; Registergericht: Amtsgericht Stuttgart, HRB 243294 IBM Data Privacy Statement: https://www.ibm.com/privacy/
Mathieu Desnoyers
reviewerRe: [PATCH v8 06/12] unwind_user/sframe: Wire up unwind_user to sframe
Quoted 59 lines · Jens Remus
Quoted 39 lines · Steven Rostedt
Quoted 13 lines · On Tue, 8 Jul 2025 15:58:56 -0400 Mathieu Desnoyers…
kernel/unwind/user.c static int unwind_user_start(struct unwind_user_state *state) · line 132
Quoted 6 lines
kernel/unwind/user.c +2
-0@@ -111,6 +128,8 @@ static int unwind_user_start(struct unwind_user_state *state)if (IS_ENABLED (CONFIG_HAVE_UNWIND_USER_COMPAT_FP ) &&in_compat_mode (regs)) state->type =UNWIND_USER_TYPE_COMPAT_FP ; +else if (current_has_sframe ()) + state->type =UNWIND_USER_TYPE_SFRAME ;I think you'll want to update the
state->typeduring the traversal (innext()), because depending on whether sframe is available for a given memory area of code or not, thenext()function can use either frame pointers or sframe during the same traversal. It would be good to know which is used after each specific call tonext().From my understanding this sets up what is available for the task at the
beginning.So once we say "this task has sframes" it will try to use it every time. In next we have:
if (compat_fp_state(state)) { frame = &compat_fp_frame; } else if (sframe_state(state)) { /* sframe expects the frame to be local storage */ frame = &_frame; if (sframe_find(state->ip, frame)) { if (!IS_ENABLED(CONFIG_HAVE_UNWIND_USER_FP)) goto done; frame = &fp_frame; } } else if (fp_state(state)) { frame = &fp_frame; } else { goto done; }Quoted 8 lines
Where if
sframe_find()fails and we switch over to frame pointers, if frame pointers works, we can continue. But the next iteration, where the frame pointer finds the previous ip, that ip may be in the sframe section again.I've seen this work with my trace_printk()s. A function from code that is running sframes calls into a library function that has frame pointers. The walk walks through the frame pointers in the library, and when it hits the code that has sframes, it starts using that again.
I think Mathieu has a point, as
unwind_user_next()calls the optional architecture-specificarch_unwind_user_next()at the end. The x86 implementation doesstate->typespecific processing (forUNWIND_USER_TYPE_COMPAT_FP).Quoted 4 lines
If we switched the state to just FP, it will never try to use sframes.
So this state is more about "what does this task have" than what was used per iteration.
Quoted 6 lines
While there is currently no fallback to
UNWIND_USER_TYPE_COMPAT_FPthat would strictly require this, it could be useful to have both information.Or the logic in
unwind_user_start(),unwind_user_next(), and*_state()may need to be adjusted so thatstate->typereflects the currently used method, whichunwind_user_next()determines and sets anew for every step.
I concur with Jens. I think we should keep track of both:
1) available unwind methods,
2) unwind method used for the current frame.
E.g.:
/*
* unwind types, listed in priority order: lower numbers are
* attempted first if available.
*/
enum unwind_user_type_bits {
UNWIND_USER_TYPE_SFRAME_BIT = 0, UNWIND_USER_TYPE_FP_BIT = 1, UNWIND_USER_TYPE_COMPAT_FP_BIT = 2,
_NR_UNWIND_USER_TYPE_BITS,
};
enum unwind_user_type {
UNWIND_USER_TYPE_NONE = 0,
UNWIND_USER_TYPE_SFRAME = (1U << UNWIND_USER_TYPE_SFRAME_BIT), UNWIND_USER_TYPE_FP = (1U << UNWIND_USER_TYPE_FP_BIT), UNWIND_USER_TYPE_COMPAT_FP = (1U << UNWIND_USER_TYPE_COMPAT_FP_BIT),
};
And have the following fields in struct unwind_user_state:
/* Unwind time used for the most recent unwind traversal iteration. */ enum unwind_user_type current_type;
/* Unwind types available in the current context. Bitmask of enum unwind_user_type. */ unsigned int available_types;
So as we end up adding stuff like registered JIT unwind info, we will want to expand the "available types". And it makes sense to both keep track of all available types (as a way to quickly know which mechanisms we need to query for the current task) *and* to let the caller know which unwind type was used for the current frame.
And AFAIU we'd be inserting a "jit unwind info" type between SFRAME and FP in the future, because the jit unwind info would be more reliable than FP. This would require that we bump the number for FP and COMPAT_FP, but that would be OK because this is not ABI.
Thoughts ?
Thanks,
Mathieu
Quoted 3 lines
Regards,
Jens
Signature · 3 lines
Mathieu Desnoyers EfficiOS Inc. https://www.efficios.com
Mathieu Desnoyers
reviewerRe: [PATCH v8 06/12] unwind_user/sframe: Wire up unwind_user to sframe
Quoted 109 lines · Mathieu Desnoyers
Quoted 59 lines · Jens Remus
Quoted 39 lines · Steven Rostedt
Quoted 5 lines · On Tue, 8 Jul 2025 15:58:56 -0400 Mathieu Desnoyers…
Quoted 5 lines
kernel/unwind/user.c +1
-0@@ -111,6 +128,8 @@ static int unwind_user_start(struct unwind_user_state *state) if (IS_ENABLED(CONFIG_HAVE_UNWIND_USER_COMPAT_FP) && in_compat_mode(regs)) state->type = UNWIND_USER_TYPE_COMPAT_FP; + else if (current_has_sframe())Quoted 8 lines
kernel/unwind/user.c static int unwind_user_start(struct unwind_user_state *state) · line 132
Quoted 1 line
kernel/unwind/user.c +1
-0+ state->type = UNWIND_USER_TYPE_SFRAME;I think you'll want to update the
state->typeduring the traversal (innext()), because depending on whether sframe is available for a given memory area of code or not, thenext()function can use either frame pointers or sframe during the same traversal. It would be good to know which is used after each specific call tonext().From my understanding this sets up what is available for the task at the beginning.
So once we say "this task has sframes" it will try to use it every time. In next we have:
if (
compat_fp_state(state)) {
frame =&compat_fp_frame;
} else if (sframe_state(state)) {
/* sframe expects the frame to be local storage */ frame =&_frame;
if (sframe_find(state->ip, frame)) {
if (!IS_ENABLED(CONFIG_HAVE_UNWIND_USER_FP)) goto done;
frame =&fp_frame;
}
} else if (fp_state(state)) {
frame =&fp_frame;
} else {
goto done;
}Quoted 8 lines
Where if
sframe_find()fails and we switch over to frame pointers, if frame pointers works, we can continue. But the next iteration, where the frame pointer finds the previous ip, that ip may be in the sframe section again.I've seen this work with my trace_printk()s. A function from code that is running sframes calls into a library function that has frame pointers. The walk walks through the frame pointers in the library, and when it hits the code that has sframes, it starts using that again.
I think Mathieu has a point, as
unwind_user_next()calls the optional architecture-specificarch_unwind_user_next()at the end. The x86 implementation doesstate->typespecific processing (forUNWIND_USER_TYPE_COMPAT_FP).Quoted 4 lines
If we switched the state to just FP, it will never try to use sframes.
So this state is more about "what does this task have" than what was used per iteration.
Quoted 6 lines
While there is currently no fallback to
UNWIND_USER_TYPE_COMPAT_FPthat would strictly require this, it could be useful to have both information.Or the logic in
unwind_user_start(),unwind_user_next(), and*_state()may need to be adjusted so thatstate->typereflects the currently used method, whichunwind_user_next()determines and sets anew for every step.I concur with Jens. I think we should keep track of both:
1) available unwind methods,
2) unwind method used for the current frame.
E.g.:
/*
* unwind types, listed in priority order: lower numbers are
* attempted first if available.
*/
enumunwind_user_type_bits{
UNWIND_USER_TYPE_SFRAME_BIT= 0,UNWIND_USER_TYPE_FP_BIT= 1,UNWIND_USER_TYPE_COMPAT_FP_BIT= 2,_NR_UNWIND_USER_TYPE_BITS,
};enum
unwind_user_type{
UNWIND_USER_TYPE_NONE= 0,
UNWIND_USER_TYPE_SFRAME= (1U <<UNWIND_USER_TYPE_SFRAME_BIT),UNWIND_USER_TYPE_FP= (1U <<UNWIND_USER_TYPE_FP_BIT),UNWIND_USER_TYPE_COMPAT_FP= (1U <<UNWIND_USER_TYPE_COMPAT_FP_BIT),
};And have the following fields in struct
unwind_user_state:/* Unwind time used for the most recent unwind traversal iteration. */ enum
unwind_user_typecurrent_type;/* Unwind types available in the current context. Bitmask of enum
unwind_user_type. */ unsigned intavailable_types;So as we end up adding stuff like registered JIT unwind info, we will want to expand the "available types". And it makes sense to both keep track of all available types (as a way to quickly know which mechanisms we need to query for the current task) *and* to let the caller know which unwind type was used for the current frame.
Quoted 6 lines
And AFAIU we'd be inserting a "jit unwind info" type between SFRAME and FP in the future, because the jit unwind info would be more reliable than FP. This would require that we bump the number for FP and
COMPAT_FP, but that would be OK because this is not ABI.Thoughts ?
One use-case for giving the "current_type" to iteration callers is to let end users know whether they should trust the frame info. If it comes from sframe, then it should be pretty solid. However, if it comes from frame pointers used as a fallback on a system that omits frame pointers, the user should consider the resulting data with a high level of skepticism.
Thanks,
Mathieu
Quoted 10 lines
Thanks,
Mathieu
Quoted 3 lines
Regards,
Jens
Signature · 3 lines
Mathieu Desnoyers EfficiOS Inc. https://www.efficios.com
Steven Rostedt
maintainerRe: [PATCH v8 06/12] unwind_user/sframe: Wire up unwind_user to sframe
Quoted 6 lines · On Wed, 9 Jul 2025 09:51:09 -0400 Mathieu Desnoyers…
One use-case for giving the "
current_type" to iteration callers is to let end users know whether they should trust the frame info. If it comes from sframe, then it should be pretty solid. However, if it comes from frame pointers used as a fallback on a system that omits frame pointers, the user should consider the resulting data with a high level of skepticism.
That would be in the trace sent to the callback. We could add something like the '?' if it's not trusted.
But for now, until we have a use case that we are implementing, I want to keep this simple, otherwise it will never get done. I don't want to add features for hypothetical scenarios.
Currently, the traceback is just an array of addresses. But this could change in the future. What we are discussing right now is the internal functionality of the user unwind code where I have made most of theses functions static.
The only external functions that get called during the iteration is the architecture specific code. If that code needs to know the difference between sframes and frame pointers then we can modify it, but until then, I rather keep this as is.
Jens, is there something that the architecture code needs now? If so, then lets fix it, otherwise lets do it when there is something. This isn't user API, it can change in the future.
-- Steve
Mathieu Desnoyers
reviewerRe: [PATCH v8 06/12] unwind_user/sframe: Wire up unwind_user to sframe
Quoted 22 lines · Steven Rostedt
Quoted 6 lines · On Wed, 9 Jul 2025 09:51:09 -0400 Mathieu Desnoyers…
One use-case for giving the "
current_type" to iteration callers is to let end users know whether they should trust the frame info. If it comes from sframe, then it should be pretty solid. However, if it comes from frame pointers used as a fallback on a system that omits frame pointers, the user should consider the resulting data with a high level of skepticism.That would be in the trace sent to the callback. We could add something like the '?' if it's not trusted.
But for now, until we have a use case that we are implementing, I want to keep this simple, otherwise it will never get done. I don't want to add features for hypothetical scenarios.
Currently, the traceback is just an array of addresses. But this could change in the future. What we are discussing right now is the internal functionality of the user unwind code where I have made most of theses functions static.
Quoted 4 lines
The only external functions that get called during the iteration is the architecture specific code. If that code needs to know the difference between sframes and frame pointers then we can modify it, but until then, I rather keep this as is.
Indeed it's only kernel internal API, but this is API that will be expected by each architecture supporting unwind_user. Changing this later on will cause a lot of friction and cross-architecture churn compared to doing it right in the first place.
Thanks,
Mathieu
Quoted 6 lines
Jens, is there something that the architecture code needs now? If so, then lets fix it, otherwise lets do it when there is something. This isn't user API, it can change in the future.
-- Steve
Signature · 3 lines
Mathieu Desnoyers EfficiOS Inc. https://www.efficios.com
Steven Rostedt
maintainerRe: [PATCH v8 06/12] unwind_user/sframe: Wire up unwind_user to sframe
Quoted 4 lines · On Wed, 9 Jul 2025 10:10:30 -0400 Mathieu Desnoyers…
Indeed it's only kernel internal API, but this is API that will be expected by each architecture supporting
unwind_user. Changing this later on will cause a lot of friction and cross-architecture churn compared to doing it right in the first place.
The changes you are suggesting is added info if an architecture needs it. That is easy to do. All you need is to add an extra field in the state structure and the architectures that need it can use it, and the rest can ignore it.
Again, I'm not worried about it. If you want to send me a patch, feel free, but I'm not doing this extra work, until I see a real problem.
-- Steve.
Mathieu Desnoyers
reviewerRe: [PATCH v8 06/12] unwind_user/sframe: Wire up unwind_user to sframe
Quoted 8 lines · Steven Rostedt
Quoted 4 lines · On Wed, 9 Jul 2025 10:10:30 -0400 Mathieu Desnoyers…
Indeed it's only kernel internal API, but this is API that will be expected by each architecture supporting
unwind_user. Changing this later on will cause a lot of friction and cross-architecture churn compared to doing it right in the first place.
Quoted 7 lines
The changes you are suggesting is added info if an architecture needs it. That is easy to do. All you need is to add an extra field in the state structure and the architectures that need it can use it, and the rest can ignore it.
Again, I'm not worried about it. If you want to send me a patch, feel free, but I'm not doing this extra work, until I see a real problem.
OK, I'll do it. As I look into the existing state, the priority order appears to be incorrect on compat mode: if we have both compat mode and sframe available, COMPAT_FP is preferred. I think we want to favor using sframe first. But then if you select "sframe" in start(), then you don't have the "compat state" information for the compat-fp fallback.
So the "type" logic is all intermingled between "fp vs sframe" and "compat vs non-compat" there. My changes will clean this up.
Thanks,
Mathieu
Signature · 3 lines
Mathieu Desnoyers EfficiOS Inc. https://www.efficios.com
Re: [PATCH v8 06/12] unwind_user/sframe: Wire up unwind_user to sframe
In reply to Steven RostedtQuoted 7 lines · Steven Rostedt
But for now, until we have a use case that we are implementing, I want to keep this simple, otherwise it will never get done. I don't want to add features for hypothetical scenarios.
Jens, is there something that the architecture code needs now? If so, then lets fix it, otherwise lets do it when there is something. This isn't user API, it can change in the future.
I am fine with your suggestion. My s390 sframe support does not require any changes in this regard.
Regards,
Jens
Signature · 9 lines
Jens Remus Linux on Z Development (D3303) +49-7031-16-1128 Office [email protected] IBM IBM Deutschland Research & Development GmbH; Vorsitzender des Aufsichtsrats: Wolfgang Wendt; Geschäftsführung: David Faller; Sitz der Gesellschaft: Böblingen; Registergericht: Amtsgericht Stuttgart, HRB 243294 IBM Data Privacy Statement: https://www.ibm.com/privacy/
Re: [PATCH v8 06/12] unwind_user/sframe: Wire up unwind_user to sframe
In reply to Mathieu DesnoyersQuoted 47 lines · Mathieu Desnoyers
Quoted 46 lines · Mathieu Desnoyers
I concur with Jens. I think we should keep track of both:
1) available unwind methods,
2) unwind method used for the current frame.
E.g.:
/*
* unwind types, listed in priority order: lower numbers are
* attempted first if available.
*/
enumunwind_user_type_bits{UNWIND_USER_TYPE_SFRAME_BIT= 0,UNWIND_USER_TYPE_FP_BIT= 1,UNWIND_USER_TYPE_COMPAT_FP_BIT= 2,_NR_UNWIND_USER_TYPE_BITS,
};enum
unwind_user_type{
UNWIND_USER_TYPE_NONE= 0,
UNWIND_USER_TYPE_SFRAME= (1U <<UNWIND_USER_TYPE_SFRAME_BIT),UNWIND_USER_TYPE_FP= (1U <<UNWIND_USER_TYPE_FP_BIT),UNWIND_USER_TYPE_COMPAT_FP= (1U <<UNWIND_USER_TYPE_COMPAT_FP_BIT), };And have the following fields in struct
unwind_user_state:/* Unwind time used for the most recent unwind traversal iteration. */ enum
unwind_user_typecurrent_type;/* Unwind types available in the current context. Bitmask of enum
unwind_user_type. */ unsigned intavailable_types;So as we end up adding stuff like registered JIT unwind info, we will want to expand the "available types". And it makes sense to both keep track of all available types (as a way to quickly know which mechanisms we need to query for the current task) *and* to let the caller know which unwind type was used for the current frame.
And AFAIU we'd be inserting a "jit unwind info" type between SFRAME and FP in the future, because the jit unwind info would be more reliable than FP. This would require that we bump the number for FP and
COMPAT_FP, but that would be OK because this is not ABI.
Quoted 8 lines
Quoted 1 line
Thoughts ?
One use-case for giving the "
current_type" to iteration callers is to let end users know whether they should trust the frame info. If it comes from sframe, then it should be pretty solid. However, if it comes from frame pointers used as a fallback on a system that omits frame pointers, the user should consider the resulting data with a high level of skepticism.
The current_type may be different for every unwind step (frame). So struct unwind_stacktrace would probably need the following added:
/* Unwind types used for taking the stack trace. */ unsigned int used_types;
So that the user of unwind_user() could decide whether to trust the stack trace. But as Steve suggested in his reply, all of this could be added later, once there is a need.
Regards,
Jens
Signature · 9 lines
Jens Remus Linux on Z Development (D3303) +49-7031-16-1128 Office [email protected] IBM IBM Deutschland Research & Development GmbH; Vorsitzender des Aufsichtsrats: Wolfgang Wendt; Geschäftsführung: David Faller; Sitz der Gesellschaft: Böblingen; Registergericht: Amtsgericht Stuttgart, HRB 243294 IBM Data Privacy Statement: https://www.ibm.com/privacy/
Steven Rostedt
maintainerRe: [PATCH v8 06/12] unwind_user/sframe: Wire up unwind_user to sframe
In reply to Jens RemusQuoted 4 lines · On Wed, 9 Jul 2025 09:58:26 +0200 Jens Remus <[email protected]> wrote:
I think Mathieu has a point, as
unwind_user_next()calls the optional architecture-specificarch_unwind_user_next()at the end. The x86 implementation doesstate->typespecific processing (forUNWIND_USER_TYPE_COMPAT_FP).
I'm not too comfortable with the compat patches at this stage. I'm thinking of separating out the compat patches, and just reject the deferred unwind if the task is in compat mode (forcing perf or other tracers to use whatever it uses today).
I'll take Mathieu's patches and merge them with Josh's, but make them a separate series.
I'm aiming to get the core series into this merge window, and the less complexity we have, the better. Then everything can be worked on simultaneously in the next merge window as all the other patches will not have any dependency on each other. They all have dependency on the core set.
-- Steve
Steven Rostedt
maintainerRe: [PATCH v8 06/12] unwind_user/sframe: Wire up unwind_user to sframe
Quoted 7 lines · On Thu, 10 Jul 2025 11:30:39 -0400 Steven Rostedt <[email protected]> wrote:
I'm not too comfortable with the compat patches at this stage. I'm thinking of separating out the compat patches, and just reject the deferred unwind if the task is in compat mode (forcing perf or other tracers to use whatever it uses today).
I'll take Mathieu's patches and merge them with Josh's, but make them a separate series.
So I'm removing the two compat patches from the series for now and plan to replace it with this:
-- Steve
From: Steven Rostedt <[email protected]>
Subject: [PATCH] unwind deferred/x86: Do not defer stack tracing for compat
tasks
Currently compat tasks are not supported. If a deferred user space stack trace is requested on a compat task, it should fail and return an error so that the profiler can use an alternative approach (whatever it uses today).
Add a arch_unwind_can_defer() macro that is called in unwind_deferred_request(). Have x86 define it to a function that makes sure that the current task is running in 64bit mode, and if it is not, it returns false. This will cause unwind_deferred_request() to error out and the caller can use the current method of user space stack tracing.
--- arch/x86/include/asm/unwind_user.h | 11 +++++++++++ include/linux/unwind_deferred.h | 5 +++++ kernel/unwind/deferred.c | 3 +++ 3 files changed, 19 insertions(+)
arch/x86/include/asm/unwind_user.h +11 -0
@@ -2,6 +2,17 @@#ifndef _ASM_X86_UNWIND_USER_H#define _ASM_X86_UNWIND_USER_H +#ifdef CONFIG_IA32_EMULATION +/* Currently compat mode is not supported for deferred stack trace */ +static inline bool arch_unwind_can_defer (void ) +{ +struct pt_regs *regs =task_pt_regs (current); + +return user_64bit_mode (regs); +} +# define arch_unwind_can_defer arch_unwind_can_defer +#endif /* CONFIG_IA32_EMULATION */ +#define ARCH_INIT_USER_FP_FRAME \ .cfa_off = (s32)sizeof(long) * 2, \ .ra_off = (s32)sizeof(long) * -1, \
include/linux/unwind_deferred.h +5 -0
@@ -16,6 +16,11 @@ struct unwind_work {int bit; }; +/* Architectures can add a test to not defer unwinding */ +#ifndef arch_unwind_can_defer +# define arch_unwind_can_defer () (true) +#endif +#ifdef CONFIG_UNWIND_USER #define UNWIND_PENDING_BIT (BITS_PER_LONG - 1)
kernel/unwind/deferred.c +3 -0
@@ -236,6 +236,9 @@ int unwind_deferred_request(struct unwind_work *work, u64 *cookie) *cookie =0 ; +if (!arch_unwind_can_defer ()) +return -EINVAL ; +if ((current->flags & (PF_KTHREAD |PF_EXITING )) || !user_mode (task_pt_regs (current)))return -EINVAL ;
Signature · 1 line
2.47.2
PATCH v8 7/12 unwind_user/sframe/x86: Enable sframe unwinding on x86
In reply to Steven RostedtFrom: Josh Poimboeuf <[email protected]>
The x86 sframe 2.0 implementation works fairly well, starting with binutils 2.41 (though some bugs are getting fixed in later versions). Enable it.
--- arch/x86/Kconfig | 1 + 1 file changed, 1 insertion(+)
arch/x86/Kconfig +1 -0
@@ -304,6 +304,7 @@ config X86 select HAVE_UNSTABLE_SCHED_CLOCK select HAVE_UNWIND_USER_COMPAT_FP if IA32_EMULATION select HAVE_UNWIND_USER_FP if X86_64 + select HAVE_UNWIND_USER_SFRAME if X86_64 select HAVE_USER_RETURN_NOTIFIER select HAVE_GENERIC_VDSO select VDSO_GETRANDOM if X86_64
Signature · 1 line
2.47.2
PATCH v8 8/12 unwind_user/sframe: Remove .sframe section on detected corruption
In reply to Steven RostedtFrom: Josh Poimboeuf <[email protected]>
To avoid continued attempted use of a bad .sframe section, remove it on demand when the first sign of corruption is detected.
--- kernel/unwind/sframe.c | 4 ++++ 1 file changed, 4 insertions(+)
kernel/unwind/sframe.c +4 -0
@@ -310,6 +310,10 @@ int sframe_find(unsigned long ip, struct unwind_user_frame *frame) ret =__find_fre (sec, &fde, ip, frame); end:user_read_access_end (); + +if (ret == -EFAULT ) +WARN_ON_ONCE (sframe_remove_section (sec->sframe_start)); +return ret; }
Signature · 1 line
2.47.2
PATCH v8 9/12 unwind_user/sframe: Show file name in debug output
In reply to Steven RostedtFrom: Josh Poimboeuf <[email protected]>
When debugging sframe issues, the error messages aren't all that helpful without knowing what file a corresponding .sframe section belongs to. Prefix debug output strings with the file name.
--- include/linux/sframe.h | 4 +++- kernel/unwind/sframe.c | 23 ++++++++++-------- kernel/unwind/sframe_debug.h | 45 +++++++++++++++++++++++++++++++----- 3 files changed, 56 insertions(+), 16 deletions(-)
include/linux/sframe.h +3 -1
@@ -10,7 +10,9 @@struct sframe_section {struct rcu_head rcu; - +#ifdef CONFIG_DYNAMIC_DEBUG +const char *filename; +#endif unsigned long sframe_start;unsigned long sframe_end;unsigned long text_start;
kernel/unwind/sframe.c +14 -9
@@ -311,14 +311,17 @@ int sframe_find(unsigned long ip, struct unwind_user_frame *frame) end:user_read_access_end (); -if (ret == -EFAULT ) +if (ret == -EFAULT ) { +dbg_sec ("removing bad .sframe section\n" );WARN_ON_ONCE (sframe_remove_section (sec->sframe_start)); + }return ret; }static void free_section (struct sframe_section *sec) { +dbg_free (sec);kfree (sec); }
@@ -329,7 +332,7 @@ static int sframe_read_header(struct sframe_section *sec)unsigned int num_fdes;if (copy_from_user (&shdr, (void __user *)sec->sframe_start,sizeof (shdr))) { -dbg ("header usercopy failed\n" ); +dbg_sec ("header usercopy failed\n" );return -EFAULT ; }
@@ -337,18 +340,18 @@ static int sframe_read_header(struct sframe_section *sec) shdr.preamble.version != SFRAME_VERSION_2 || !(shdr.preamble.flags & SFRAME_F_FDE_SORTED) || shdr.auxhdr_len) { -dbg ("bad/unsupported sframe header\n" ); +dbg_sec ("bad/unsupported sframe header\n" );return -EINVAL ; }if (!shdr.num_fdes || !shdr.num_fres) { -dbg ("no fde/fre entries\n" ); +dbg_sec ("no fde/fre entries\n" );return -EINVAL ; } header_end = sec->sframe_start +SFRAME_HEADER_SIZE (shdr);if (header_end >= sec->sframe_end) { -dbg ("header doesn't fit in section\n" ); +dbg_sec ("header doesn't fit in section\n" );return -EINVAL ; }
@@ -360,7 +363,7 @@ static int sframe_read_header(struct sframe_section *sec) fres_end = fres_start + shdr.fre_len;if (fres_start < fdes_end || fres_end > sec->sframe_end) { -dbg ("inconsistent fde/fre offsets\n" ); +dbg_sec ("inconsistent fde/fre offsets\n" );return -EINVAL ; }
@@ -416,6 +419,8 @@ int sframe_add_section(unsigned long sframe_start, unsigned long sframe_end, sec->text_start = text_start; sec->text_end = text_end; +dbg_init (sec); + ret =sframe_read_header (sec);if (ret) {dbg_print_header (sec);
@@ -424,8 +429,8 @@ int sframe_add_section(unsigned long sframe_start, unsigned long sframe_end, ret =mtree_insert_range (sframe_mt, sec->text_start, sec->text_end, sec,GFP_KERNEL );if (ret) { -dbg ("mtree_insert_range failed: text=%lx-%lx\n" , - sec->text_start, sec->text_end); +dbg_sec ("mtree_insert_range failed: text=%lx-%lx\n" , + sec->text_start, sec->text_end); goto err_free; }
@@ -447,7 +452,7 @@ static int __sframe_remove_section(struct mm_struct *mm, struct sframe_section *sec) {if (!mtree_erase (&mm->sframe_mt, sec->text_start)) { -dbg ("mtree_erase failed: text=%lx\n" , sec->text_start); +dbg_sec ("mtree_erase failed: text=%lx\n" , sec->text_start);return -EINVAL ; }
kernel/unwind/sframe_debug.h +39 -6
65 lines not on this page · raw message
Signature · 1 line
2.47.2
PATCH v8 10/12 unwind_user/sframe: Enable debugging in uaccess regions
In reply to Steven RostedtFrom: Josh Poimboeuf <[email protected]>
Objtool warns about calling pr_debug() from uaccess-enabled regions, and rightfully so. Add a dbg_sec_uaccess() macro which temporarily disables uaccess before doing the dynamic printk, and use that to add debug messages throughout the uaccess-enabled regions.
--- kernel/unwind/sframe.c | 60 ++++++++++++++++++++++++++++-------- kernel/unwind/sframe_debug.h | 31 +++++++++++++++++++ 2 files changed, 78 insertions(+), 13 deletions(-)
kernel/unwind/sframe.c +47 -13
@@ -53,12 +53,15 @@ static __always_inline int __read_fde(struct sframe_section *sec, sizeof(struct sframe_fde), Efault); ip = sec->sframe_start + fde->start_addr; -if (ip < sec->text_start || ip > sec->text_end) +if (ip < sec->text_start || ip > sec->text_end) { +dbg_sec_uaccess ("bad fde num %d\n" , fde_num);return -EINVAL ; + }return 0 ; Efault: +dbg_sec_uaccess ("fde %d usercopy failed\n" , fde_num);return -EFAULT ; }
@@ -85,16 +88,22 @@ static __always_inline int __find_fde(struct sframe_section *sec,unsafe_get_user (func_off, (s32 __user *)mid, Efault);if (ip_off >= func_off) { -if (func_off < func_off_low) +if (func_off < func_off_low) { +dbg_sec_uaccess ("fde %u not sorted\n" , + (unsigned int )(mid - first));return -EFAULT ; + } func_off_low = func_off; found = mid; low = mid +1 ; }else { -if (func_off > func_off_high) +if (func_off > func_off_high) { +dbg_sec_uaccess ("fde %u not sorted\n" , + (unsigned int )(mid - first));return -EFAULT ; + } func_off_high = func_off;
@@ -116,6 +125,7 @@ static __always_inline int __find_fde(struct sframe_section *sec,return 0 ; Efault: +dbg_sec_uaccess ("fde usercopy failed\n" );return -EFAULT ; }
@@ -140,6 +150,8 @@ static __always_inline int __find_fde(struct sframe_section *sec, ____UNSAFE_GET_USER_INC(to, from, u_or_s##32, label); \ break; \ default: \ + dbg_sec_uaccess("%d: bad UNSAFE_GET_USER_INC size %u\n",\ + __LINE__, size); \ return -EFAULT; \ } \ })
@@ -174,24 +186,34 @@ static __always_inline int __read_fre(struct sframe_section *sec,u8 info; addr_size =fre_type_to_size (fre_type); -if (!addr_size) +if (!addr_size) { +dbg_sec_uaccess ("bad addr_size in fde info %u\n" , fde->info);return -EFAULT ; + } -if (fre_addr + addr_size +1 > sec->fres_end) +if (fre_addr + addr_size +1 > sec->fres_end) { +dbg_sec_uaccess ("fre addr+info goes past end of subsection\n" );return -EFAULT ; + }UNSAFE_GET_USER_INC (ip_off, cur, addr_size, Efault); -if (fde_type ==SFRAME_FDE_TYPE_PCINC && ip_off > fde->func_size) +if (fde_type ==SFRAME_FDE_TYPE_PCINC && ip_off > fde->func_size) { +dbg_sec_uaccess ("fre starts past end of function: ip_off=0x%x, func_size=0x%x\n" , + ip_off, fde->func_size);return -EFAULT ; + }UNSAFE_GET_USER_INC (info, cur,1 , Efault); offset_count =SFRAME_FRE_OFFSET_COUNT (info); offset_size =offset_size_enum_to_size (SFRAME_FRE_OFFSET_SIZE (info)); -if (!offset_count || !offset_size) +if (!offset_count || !offset_size) { +dbg_sec_uaccess ("zero offset_count or size in fre info %u\n" ,info);return -EFAULT ; - -if (cur + (offset_count * offset_size) > sec->fres_end) + } +if (cur + (offset_count * offset_size) > sec->fres_end) { +dbg_sec_uaccess ("fre goes past end of subsection\n" );return -EFAULT ; + } fre->size = addr_size +1 + (offset_count * offset_size);
@@ -200,8 +222,10 @@ static __always_inline int __read_fre(struct sframe_section *sec, ra_off = sec->ra_off;if (!ra_off) { -if (!offset_count--) +if (!offset_count--) { +dbg_sec_uaccess ("zero offset_count, can't find ra_off\n" );return -EFAULT ; + }UNSAFE_GET_USER_INC (ra_off, cur, offset_size, Efault); }
@@ -212,8 +236,10 @@ static __always_inline int __read_fre(struct sframe_section *sec,UNSAFE_GET_USER_INC (fp_off, cur, offset_size, Efault); } -if (offset_count) +if (offset_count) { +dbg_sec_uaccess ("non-zero offset_count after reading fre\n" );return -EFAULT ; + } fre->ip_off = ip_off; fre->cfa_off = cfa_off;
@@ -224,6 +250,7 @@ static __always_inline int __read_fre(struct sframe_section *sec,return 0 ; Efault: +dbg_sec_uaccess ("fre usercopy failed\n" );return -EFAULT ; }
@@ -257,13 +284,20 @@ static __always_inline int __find_fre(struct sframe_section *sec, which = !which; ret =__read_fre (sec, fde, fre_addr, fre); -if (ret) +if (ret) { +dbg_sec_uaccess ("fde addr 0x%x: __read_fre(%u) failed\n" , + fde->start_addr, i); +dbg_print_fde_uaccess (sec, fde);return ret; + } fre_addr += fre->size; -if (prev_fre && fre->ip_off <= prev_fre->ip_off) +if (prev_fre && fre->ip_off <= prev_fre->ip_off) { +dbg_sec_uaccess ("fde addr 0x%x: fre %u not sorted\n" , + fde->start_addr, i);return -EFAULT ; + }if (fre->ip_off > ip_off)break ;
kernel/unwind/sframe_debug.h +31 -0
@@ -13,6 +13,26 @@#define dbg_sec (fmt, ...) \ dbg("%s: " fmt, sec->filename, ##__VA_ARGS__) +#define __dbg_sec_descriptor (fmt, ...) \ + __dynamic_pr_debug(&descriptor, "sframe: %s: " fmt, \ + sec->filename, ##__VA_ARGS__) + +/* +* To avoid breaking uaccess rules, temporarily disable uaccess +* before calling printk. +*/ +#define dbg_sec_uaccess (fmt, ...) \ +({ \ + DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt); \ + if (DYNAMIC_DEBUG_BRANCH(descriptor)) { \ + user_read_access_end(); \ + __dbg_sec_descriptor(fmt, ##__VA_ARGS__); \ + BUG_ON(!user_read_access_begin( \ + (void __user *)sec->sframe_start, \ + sec->sframe_end - sec->sframe_start)); \ + } \ +}) + static __always_inline void dbg_print_header(struct sframe_section *sec) {unsigned long fdes_end;
@@ -27,6 +47,15 @@ static __always_inline void dbg_print_header(struct sframe_section *sec) sec->ra_off, sec->fp_off); } +static __always_inline void dbg_print_fde_uaccess(struct sframe_section *sec, +struct sframe_fde *fde) +{ +dbg_sec_uaccess ("FDE: start_addr:0x%x func_size:0x%x " +"fres_off:0x%x fres_num:%d info:%u rep_size:%u\n" , + fde->start_addr, fde->func_size, + fde->fres_off, fde->fres_num, fde->info, fde->rep_size); +} +static inline void dbg_init (struct sframe_section *sec) {struct mm_struct *mm = current->mm;
@@ -57,8 +86,10 @@ static inline void dbg_free(struct sframe_section *sec) #define dbg(args...) no_printk(args) #define dbg_sec(args... ) no_printk(args) +#define dbg_sec_uaccess(args...) no_printk(args)static inline void dbg_print_header (struct sframe_section *sec) {} +static inline void dbg_print_fde_uaccess (struct sframe_section *sec,struct sframe_fde *fde) {}static inline void dbg_init (struct sframe_section *sec) {}static inline void dbg_free (struct sframe_section *sec) {}
Signature · 1 line
2.47.2
Re: [PATCH v8 10/12] unwind_user/sframe: Enable debugging in uaccess regions
Quoted 7 lines · Steven Rostedt
From: Josh Poimboeuf <[email protected]>
Objtool warns about calling
pr_debug()from uaccess-enabled regions, and rightfully so. Add adbg_sec_uaccess()macro which temporarily disables uaccess before doing the dynamic printk, and use that to add debug messages throughout the uaccess-enabled regions.
Please kill this patch, and stop doing stupid things.
The whole AND ONLY point of using the unsafe user access macros is performance. You are now actively breaking the whole point of them.
If you need to add debug printouts to user accesses, just don't use the 'unsafe' ones.
Or add the debug to after the unsafe region has finished. Not this way.
This patch is disgusting, in other words. It's wrong. STOP IT.
Linus
Steven Rostedt
maintainerRe: [PATCH v8 10/12] unwind_user/sframe: Enable debugging in uaccess regions
Quoted 4 lines · On Mon, 7 Jul 2025 20:38:35 -0700 Linus Torvalds…
On Mon, 7 Jul 2025 at 19:12, Steven Rostedt <[email protected]> wrote:
This patch is disgusting, in other words. It's wrong. STOP IT.
No problem, I can easily drop it.
I just took Josh's PoC patches and posted them. This particular series still needs a bit of work. That's one of the reasons I split it out of the other series. The core series is at the stage for acceptance and looking for feedback from other maintainers. This series is still a work in progress. Others have asked me to post them so they can start playing with it.
We still need to come up with a system call with a robust API to allow the dynamic linker to tell the kernel where the SFrames are for the libraries it loads. Hence the "DO NOT APPLY" patch at the end (which I just noticed the subject text got dropped when I pulled it into git from patchwork and sent out this version, at least the change long still suggest it shouldn't be applied).
But I will remove this patch from the queue. I never even used this debugging. What I did was inject trace_printk() all over the place to make sure it was working as expected.
Thanks,
-- Steve
Josh Poimboeuf
maintainerRe: [PATCH v8 10/12] unwind_user/sframe: Enable debugging in uaccess regions
Quoted 24 lines · Steven Rostedt
Quoted 4 lines · On Mon, 7 Jul 2025 20:38:35 -0700 Linus Torvalds…
On Mon, 7 Jul 2025 at 19:12, Steven Rostedt <[email protected]> wrote:
This patch is disgusting, in other words. It's wrong. STOP IT.
No problem, I can easily drop it.
I just took Josh's PoC patches and posted them. This particular series still needs a bit of work. That's one of the reasons I split it out of the other series. The core series is at the stage for acceptance and looking for feedback from other maintainers. This series is still a work in progress. Others have asked me to post them so they can start playing with it.
We still need to come up with a system call with a robust API to allow the dynamic linker to tell the kernel where the SFrames are for the libraries it loads. Hence the "DO NOT APPLY" patch at the end (which I just noticed the subject text got dropped when I pulled it into git from patchwork and sent out this version, at least the change long still suggest it shouldn't be applied).
Quoted 3 lines
But I will remove this patch from the queue. I never even used this debugging. What I did was inject
trace_printk()all over the place to make sure it was working as expected.
I had found those debug printks really useful for debugging corrupt/missing .sframe data, but yeah, this patch is ridiculously bad. Sorry for putting that out into the world ;-)
And those are all error paths, so it's rather pointless to do that whole uaccess disable/enable/disable dance.
So yeah, drop it for now and I can replace it with something better later on.
Signature · 1 line
Josh
Steven Rostedt
maintainerRe: [PATCH v8 10/12] unwind_user/sframe: Enable debugging in uaccess regions
Quoted 9 lines · On Tue, 8 Jul 2025 07:34:36 -0700 Josh Poimboeuf <[email protected]> wrote:
I had found those debug printks really useful for debugging corrupt/missing .sframe data, but yeah, this patch is ridiculously bad. Sorry for putting that out into the world ;-)
And those are all error paths, so it's rather pointless to do that whole uaccess disable/enable/disable dance.
So yeah, drop it for now and I can replace it with something better later on.
Would something like this work? If someone enables the config to enable the validation, I don't think we need dynamic printk to do it (as that requires passing in the format directly and not via a pointer).
-- Steve
kernel/unwind/sframe.c +39 -7
@@ -321,11 +321,24 @@ int sframe_find(unsigned long ip, struct unwind_user_frame *frame)#ifdef CONFIG_SFRAME_VALIDATION -static __always_inline int __sframe_validate_section(struct sframe_section *sec) +/* Used to save error messages in uaccess sections */ +struct err_msg { +const char *fmt; +int param1; +int param2; +long param3; +long param4; +}; + +static __always_inline +int __sframe_validate_section(struct sframe_section *sec, struct err_msg *err) {unsigned long prev_ip =0 ;unsigned int i; +/* current->comm, current->pid, sec->filename */ +#define ERR_HDR KERN_WARNING "%s (%d) %s: " +for (i =0 ; i < sec->num_fdes; i++) {struct sframe_fre *fre, *prev_fre =NULL ;unsigned long ip, fre_addr;
@@ -341,7 +354,8 @@ static __always_inline int __sframe_validate_section(struct sframe_section *sec) ip = sec->sframe_start + fde.start_addr;if (ip <= prev_ip) { -dbg_sec_uaccess ("fde %u not sorted\n" , i); + err->fmt =ERR_HDR "fde %u not sorted\n" ; + err->param1 = i;return -EFAULT ; } prev_ip = ip;
@@ -355,15 +369,23 @@ static __always_inline int __sframe_validate_section(struct sframe_section *sec) ret =__read_fre (sec, &fde, fre_addr, fre);if (ret) { -dbg_sec_uaccess ("fde %u: __read_fre(%u) failed\n" , i, j); -dbg_print_fde_uaccess (sec, &fde); + err->fmt =ERR_HDR +"fde %u: __read_fre(%u) failed\n" +" frame_start=%lx frame_end=%lx\n" ; + err->param1 = i; + err->param2 = j; + err->param3 = (long )sec->sframe_start; + err->param4 = (long )sec->sframe_end;return ret; } fre_addr += fre->size;if (prev_fre && fre->ip_off <= prev_fre->ip_off) { -dbg_sec_uaccess ("fde %u: fre %u not sorted\n" , i, j); + err->fmt =ERR_HDR +"fde %u: fre %u not sorted\n" ; + err->param1 = i; + err->param2 = j;return -EFAULT ; }
@@ -376,16 +398,26 @@ static __always_inline int __sframe_validate_section(struct sframe_section *sec)static int sframe_validate_section (struct sframe_section *sec) { +struct err_msg err;int ret; +memset (&err,0 ,sizeof (err)); +if (!user_read_access_begin ((void __user *)sec->sframe_start, sec->sframe_end - sec->sframe_start)) { -dbg_sec ("section usercopy failed\n" ); +pr_warn ("%s (%d): section usercopy failed\n" , + current->comm, current->pid);return -EFAULT ; } - ret =__sframe_validate_section (sec); + ret =__sframe_validate_section (sec, &err);user_read_access_end (); + +if (ret) { +printk (err.fmt, current->comm, current->pid, + sec->filename, err.param1, err.param2, + err.param3, err.param4); + }return ret; }
Re: [PATCH v8 10/12] unwind_user/sframe: Enable debugging in uaccess regions
Quoted 4 lines · Steven Rostedt
Would something like this work? If someone enables the config to enable the validation, I don't think we need dynamic printk to do it (as that requires passing in the format directly and not via a pointer).
I really think you should just not use 'user_access_begin()" AT ALL if you need to play these kinds of games.
Linus
Steven Rostedt
maintainerRe: [PATCH v8 10/12] unwind_user/sframe: Enable debugging in uaccess regions
Quoted 9 lines · On Tue, 8 Jul 2025 08:53:56 -0700 Linus Torvalds…
Quoted 4 lines · Steven Rostedt
Would something like this work? If someone enables the config to enable the validation, I don't think we need dynamic printk to do it (as that requires passing in the format directly and not via a pointer).
I really think you should just not use '
user_access_begin()" AT ALL if you need to play these kinds of games.
Looking at the code a bit deeper, I don't think we need to play these games and still keep the user_read_access_begin().
The places that are more performance critical (where it reads the sframe during normal stack walking during profiling) has no debug output, and there's nothing there that needs to take it out of the user_read_access area.
It's the validator that adds these hacks. I don't think it needs to. It can just wrap the calls to the code that requires user_read_access and then check the return value. The validator is just a debugging feature and performance should not be an issue.
But I do think performance is something to care about during normal operations where the one big user_read_access_begin() can help.
What about something like this? It adds "safe" versions of the user space access functions and uses them only in the slow (we don't care about performance) validator:
-- Steve
kernel/unwind/sframe.c +37 -22
@@ -321,7 +321,34 @@ int sframe_find(unsigned long ip, struct unwind_user_frame *frame)#ifdef CONFIG_SFRAME_VALIDATION -static __always_inline int __sframe_validate_section(struct sframe_section *sec) +static int safe_read_fde (struct sframe_section *sec, +unsigned int fde_num,struct sframe_fde *fde) +{ +int ret; + +if (!user_read_access_begin ((void __user *)sec->sframe_start, + sec->sframe_end - sec->sframe_start)) +return -EFAULT ; + ret =__read_fde (sec, fde_num, fde); +user_read_access_end (); +return ret; +} + +static int safe_read_fre (struct sframe_section *sec, +struct sframe_fde *fde,unsigned long fre_addr, +struct sframe_fre *fre) +{ +int ret; + +if (!user_read_access_begin ((void __user *)sec->sframe_start, + sec->sframe_end - sec->sframe_start)) +return -EFAULT ; + ret =__read_fre (sec, fde, fre_addr, fre); +user_read_access_end (); +return ret; +} + +static int sframe_validate_section (struct sframe_section *sec) {unsigned long prev_ip =0 ;unsigned int i;
@@ -335,13 +362,13 @@ static __always_inline int __sframe_validate_section(struct sframe_section *sec)unsigned int j;int ret; - ret =__read_fde (sec, i, &fde); + ret =safe_read_fde (sec, i, &fde);if (ret)return ret; ip = sec->sframe_start + fde.start_addr;if (ip <= prev_ip) { -dbg_sec_uaccess ("fde %u not sorted\n" , i); +dbg_sec ("fde %u not sorted\n" , i);return -EFAULT ; } prev_ip = ip;
@@ -353,17 +380,20 @@ static __always_inline int __sframe_validate_section(struct sframe_section *sec) fre = which ? fres : fres +1 ; which = !which; - ret =__read_fre (sec, &fde, fre_addr, fre); + ret =safe_read_fre (sec, &fde, fre_addr, fre);if (ret) { -dbg_sec_uaccess ("fde %u: __read_fre(%u) failed\n" , i, j); -dbg_print_fde_uaccess (sec, &fde); +dbg_sec ("fde %u: __read_fre(%u) failed\n" , i, j); +dbg_sec ("FDE: start_addr:0x%x func_size:0x%x fres_off:0x%x fres_num:%d info:%u rep_size:%u\n" , + fde.start_addr, fde.func_size, + fde.fres_off, fde.fres_num, + fde.info, fde.rep_size);return ret; } fre_addr += fre->size;if (prev_fre && fre->ip_off <= prev_fre->ip_off) { -dbg_sec_uaccess ("fde %u: fre %u not sorted\n" , i, j); +dbg_sec ("fde %u: fre %u not sorted\n" , i, j);return -EFAULT ; }
@@ -374,21 +404,6 @@ static __always_inline int __sframe_validate_section(struct sframe_section *sec) return 0; } -static int sframe_validate_section (struct sframe_section *sec) -{ -int ret; - -if (!user_read_access_begin ((void __user *)sec->sframe_start, - sec->sframe_end - sec->sframe_start)) { -dbg_sec ("section usercopy failed\n" ); -return -EFAULT ; - } - - ret =__sframe_validate_section (sec); -user_read_access_end (); -return ret; -} -#else /* !CONFIG_SFRAME_VALIDATION */ static int sframe_validate_section (struct sframe_section *sec) {return 0 ; }
Josh Poimboeuf
maintainerRe: [PATCH v8 10/12] unwind_user/sframe: Enable debugging in uaccess regions
Quoted 26 lines · Steven Rostedt
Quoted 9 lines · On Tue, 8 Jul 2025 08:53:56 -0700 Linus Torvalds…
Quoted 4 lines · Steven Rostedt
Would something like this work? If someone enables the config to enable the validation, I don't think we need dynamic printk to do it (as that requires passing in the format directly and not via a pointer).
I really think you should just not use '
user_access_begin()" AT ALL if you need to play these kinds of games.Looking at the code a bit deeper, I don't think we need to play these games and still keep the
user_read_access_begin().The places that are more performance critical (where it reads the sframe during normal stack walking during profiling) has no debug output, and there's nothing there that needs to take it out of the
user_read_accessarea.It's the validator that adds these hacks. I don't think it needs to. It can just wrap the calls to the code that requires
user_read_accessand then check the return value. The validator is just a debugging feature and performance should not be an issue.
Quoted 6 lines
But I do think performance is something to care about during normal operations where the one big
user_read_access_begin()can help.What about something like this? It adds "safe" versions of the user space access functions and uses them only in the slow (we don't care about performance) validator:
Looks good to me, thanks!
Signature · 1 line
Josh
Re: [PATCH v8 10/12] unwind_user/sframe: Enable debugging in uaccess regions
In reply to Josh PoimboeufQuoted 4 lines · Josh Poimboeuf
I had found those debug printks really useful for debugging corrupt/missing .sframe data, but yeah, this patch is ridiculously bad. Sorry for putting that out into the world ;-)
I suspect that code that is still needs that level of debugging should just not use the 'unsafe' user access helpers.
They really are meant for "this sequence turns into three CPU instructions" kind of uses, and the "unsafe" part of the naming was very much intended to be a "please don't use this unless you are being very careful and limited" marker.
Now, I do think that the "goto label for exceptions" part of the unsafe accessors can be very convenient, so maybe we should make _that_ part of the interface more widely available. IOW, without the whole user_read_access_begin/user_read_access_end dance?
That model is already used by "__{get,put}_kernel_nofault()", but I think it's limited to just some unusual code in mm/maccess.c.
Linus
PATCH v8 11/12 unwind_user/sframe: Add .sframe validation option
In reply to Steven RostedtFrom: Josh Poimboeuf <[email protected]>
Add a debug feature to validate all .sframe sections when first loading the file rather than on demand.
--- arch/Kconfig | 19 ++++++++++ kernel/unwind/sframe.c | 81 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 100 insertions(+)
arch/Kconfig +19 -0
@@ -450,6 +450,25 @@ config HAVE_UNWIND_USER_SFRAME bool select UNWIND_USER +config SFRAME_VALIDATION + bool "Enable .sframe section debugging" + depends on HAVE_UNWIND_USER_SFRAME + depends on DYNAMIC_DEBUG + help + When adding an .sframe section for a task, validate the entire + section immediately rather than on demand. + + This is a debug feature which is helpful for rooting out .sframe + section issues. If the .sframe section is corrupt, it will fail to + load immediately, with more information provided in dynamic printks. + + This has a significant page cache footprint due to its reading of the + entire .sframe section for every loaded executable and shared + library. Also, it's done for all processes, even those which don't + get stack traced by the kernel. Not recommended for general use. + + If unsure, say N. + config HAVE_PERF_REGS bool help
kernel/unwind/sframe.c +81 -0
93 lines not on this page · raw message
Signature · 1 line
2.47.2
PATCH v8 12/12 unwind_user/sframe: Add prctl() interface for registering .sframe sections
In reply to Steven RostedtFrom: Josh Poimboeuf <[email protected]>
The kernel doesn't have direct visibility to the ELF contents of shared libraries. Add some prctl() interfaces which allow glibc to tell the kernel where to find .sframe sections.
[
This adds an interface for prctl() for testing loading of sframes for libraries. But this interface should really be a system call. This patch is for testing purposes only and should not be applied to mainline.
]
--- include/uapi/linux/prctl.h | 6 +++++- kernel/sys.c | 9 +++++++++ 2 files changed, 14 insertions(+), 1 deletion(-)
include/uapi/linux/prctl.h +5 -1
@@ -351,7 +351,7 @@ struct prctl_mm_map {* configuration. All bits may be locked via this call, including * undefined bits. */ -#define PR_LOCK_SHADOW_STACK_STATUS 76 +#define PR_LOCK_SHADOW_STACK_STATUS 76/* * Controls the mode of timer_create() for CRIU restore operations.
@@ -371,4 +371,8 @@ struct prctl_mm_map {# define PR_FUTEX_HASH_GET_SLOTS 2# define PR_FUTEX_HASH_GET_IMMUTABLE 3 +/* SFRAME management */ +#define PR_ADD_SFRAME 79 +#define PR_REMOVE_SFRAME 80 +#endif /* _LINUX_PRCTL_H */
kernel/sys.c +9 -0
@@ -65,6 +65,7 @@#include <linux/rcupdate.h> #include <linux/uidgid.h> #include <linux/cred.h> +#include <linux/sframe.h> #include <linux/nospec.h>
@@ -2824,6 +2825,14 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,case PR_FUTEX_HASH : error =futex_hash_prctl (arg2, arg3, arg4);break ; +case PR_ADD_SFRAME : + error =sframe_add_section (arg2, arg3, arg4, arg5); +break ; +case PR_REMOVE_SFRAME : +if (arg3 || arg4 || arg5) +return -EINVAL ; + error =sframe_remove_section (arg2); +break ;default :trace_task_prctl_unknown (option, arg2, arg3, arg4, arg5); error = -EINVAL ;
Signature · 1 line
2.47.2