| Message ID | cd54347d26967427bc831f473d76e424ba3eba95.1737511963.git.jpoimboe@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | unwind, perf: sframe user space unwinding | expand |
On 22.01.2025 03:31, Josh Poimboeuf wrote: > diff --git a/kernel/unwind/user.c b/kernel/unwind/user.c > @@ -0,0 +1,59 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > +* Generic interfaces for unwinding user space > +*/ > +#include <linux/kernel.h> > +#include <linux/sched.h> > +#include <linux/sched/task_stack.h> > +#include <linux/unwind_user.h> > + > +int unwind_user_next(struct unwind_user_state *state) > +{ > + struct unwind_user_frame _frame; > + struct unwind_user_frame *frame = &_frame; > + unsigned long cfa = 0, fp, ra = 0; Why are cfa and ra initialized to zero? Where is that important in subsequent patches? "[PATCH v4 12/39] unwind_user: Add frame pointer support" does either unconditionally set both cfa and ra or bail out. > + > + /* no implementation yet */ > + -EINVAL; > +} Thanks and regards, Jens
On Fri, Jan 24, 2025 at 05:41:29PM +0100, Jens Remus wrote: > On 22.01.2025 03:31, Josh Poimboeuf wrote: > > +int unwind_user_next(struct unwind_user_state *state) > > +{ > > + struct unwind_user_frame _frame; > > + struct unwind_user_frame *frame = &_frame; > > + unsigned long cfa = 0, fp, ra = 0; > > Why are cfa and ra initialized to zero? Where is that important in > subsequent patches? > > "[PATCH v4 12/39] unwind_user: Add frame pointer support" does either > unconditionally set both cfa and ra or bail out. Right, probably leftovers from some previous iteration. I drop those.
On Tue, Jan 21, 2025 at 6:32 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote: > > Introduce a generic API for unwinding user stacks. > > Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org> > --- > arch/Kconfig | 3 ++ > include/linux/unwind_user.h | 15 ++++++++ > include/linux/unwind_user_types.h | 31 ++++++++++++++++ > kernel/Makefile | 1 + > kernel/unwind/Makefile | 1 + > kernel/unwind/user.c | 59 +++++++++++++++++++++++++++++++ > 6 files changed, 110 insertions(+) > create mode 100644 include/linux/unwind_user.h > create mode 100644 include/linux/unwind_user_types.h > create mode 100644 kernel/unwind/Makefile > create mode 100644 kernel/unwind/user.c > [...] > --- /dev/null > +++ b/kernel/unwind/user.c > @@ -0,0 +1,59 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > +* Generic interfaces for unwinding user space > +*/ > +#include <linux/kernel.h> > +#include <linux/sched.h> > +#include <linux/sched/task_stack.h> > +#include <linux/unwind_user.h> > + > +int unwind_user_next(struct unwind_user_state *state) > +{ > + struct unwind_user_frame _frame; > + struct unwind_user_frame *frame = &_frame; > + unsigned long cfa = 0, fp, ra = 0; wouldn't all the above generate compilation warnings about unused variables, potentially breaking bisection? > + > + /* no implementation yet */ > + -EINVAL; return missing? > +} [...]
On Fri, Jan 24, 2025 at 09:59:28AM -0800, Andrii Nakryiko wrote: > On Tue, Jan 21, 2025 at 6:32 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote: > > +int unwind_user_next(struct unwind_user_state *state) > > +{ > > + struct unwind_user_frame _frame; > > + struct unwind_user_frame *frame = &_frame; > > + unsigned long cfa = 0, fp, ra = 0; > > wouldn't all the above generate compilation warnings about unused > variables, potentially breaking bisection? It doesn't break bisection because no arches support UNWIND_USER yet so this can't yet be compiled. But yeah, it's shoddy and I fixed it after Jens' comment about the unnecessary zero-initialized variables. > > > + > > + /* no implementation yet */ > > + -EINVAL; > > return missing? heh :-)
On Tue, 21 Jan 2025 18:31:03 -0800 Josh Poimboeuf <jpoimboe@kernel.org> wrote: > +int unwind_user_next(struct unwind_user_state *state) > +{ > + struct unwind_user_frame _frame; > + struct unwind_user_frame *frame = &_frame; > + unsigned long cfa = 0, fp, ra = 0; > + > + /* no implementation yet */ > + -EINVAL; > +} > + > +int unwind_user_start(struct unwind_user_state *state) > +{ > + struct pt_regs *regs = task_pt_regs(current); > + > + memset(state, 0, sizeof(*state)); > + > + if (!current->mm || !user_mode(regs)) { > + state->done = true; > + return -EINVAL; > + } > + > + state->type = UNWIND_USER_TYPE_NONE; > + > + state->ip = instruction_pointer(regs); > + state->sp = user_stack_pointer(regs); > + state->fp = frame_pointer(regs); > + > + return 0; > +} > + I know this is just an introductory of the interface, but this should really have kerneldoc attached to it, as I have no idea what these are supposed to be doing. This patch is meaningless without it. The change log is useless too. -- Steve > +int unwind_user(struct unwind_stacktrace *trace, unsigned int max_entries) > +{ > + struct unwind_user_state state; > + > + trace->nr = 0; > + > + if (!max_entries) > + return -EINVAL; > + > + if (!current->mm) > + return 0; > + > + for_each_user_frame(&state) { > + trace->entries[trace->nr++] = state.ip; > + if (trace->nr >= max_entries) > + break; > + } > + > + return 0; > +}
On Fri, Jan 24, 2025 at 03:02:11PM -0500, Steven Rostedt wrote: > On Tue, 21 Jan 2025 18:31:03 -0800 > Josh Poimboeuf <jpoimboe@kernel.org> wrote: > > +int unwind_user_start(struct unwind_user_state *state) > > +{ > > + struct pt_regs *regs = task_pt_regs(current); > > + > > + memset(state, 0, sizeof(*state)); > > + > > + if (!current->mm || !user_mode(regs)) { > > + state->done = true; > > + return -EINVAL; > > + } > > + > > + state->type = UNWIND_USER_TYPE_NONE; > > + > > + state->ip = instruction_pointer(regs); > > + state->sp = user_stack_pointer(regs); > > + state->fp = frame_pointer(regs); > > + > > + return 0; > > +} > > + > > I know this is just an introductory of the interface, but this should > really have kerneldoc attached to it, as I have no idea what these are > supposed to be doing. This patch is meaningless without it. The change log > is useless too. Yeah, sure.
diff --git a/arch/Kconfig b/arch/Kconfig index 65228c78fef0..c6fa2b3ecbc6 100644 --- a/arch/Kconfig +++ b/arch/Kconfig @@ -435,6 +435,9 @@ config HAVE_HARDLOCKUP_DETECTOR_ARCH It uses the same command line parameters, and sysctl interface, as the generic hardlockup detectors. +config UNWIND_USER + bool + config AS_SFRAME def_bool $(as-instr,.cfi_sections .sframe\n.cfi_startproc\n.cfi_endproc) diff --git a/include/linux/unwind_user.h b/include/linux/unwind_user.h new file mode 100644 index 000000000000..aa7923c1384f --- /dev/null +++ b/include/linux/unwind_user.h @@ -0,0 +1,15 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef _LINUX_UNWIND_USER_H +#define _LINUX_UNWIND_USER_H + +#include <linux/unwind_user_types.h> + +int unwind_user_start(struct unwind_user_state *state); +int unwind_user_next(struct unwind_user_state *state); + +int unwind_user(struct unwind_stacktrace *trace, unsigned int max_entries); + +#define for_each_user_frame(state) \ + for (unwind_user_start((state)); !(state)->done; unwind_user_next((state))) + +#endif /* _LINUX_UNWIND_USER_H */ diff --git a/include/linux/unwind_user_types.h b/include/linux/unwind_user_types.h new file mode 100644 index 000000000000..6ed1b4ae74e1 --- /dev/null +++ b/include/linux/unwind_user_types.h @@ -0,0 +1,31 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef _LINUX_UNWIND_USER_TYPES_H +#define _LINUX_UNWIND_USER_TYPES_H + +#include <linux/types.h> + +enum unwind_user_type { + UNWIND_USER_TYPE_NONE, +}; + +struct unwind_stacktrace { + unsigned int nr; + unsigned long *entries; +}; + +struct unwind_user_frame { + s32 cfa_off; + s32 ra_off; + s32 fp_off; + bool use_fp; +}; + +struct unwind_user_state { + unsigned long ip; + unsigned long sp; + unsigned long fp; + enum unwind_user_type type; + bool done; +}; + +#endif /* _LINUX_UNWIND_USER_TYPES_H */ diff --git a/kernel/Makefile b/kernel/Makefile index 87866b037fbe..6cb4b0e02a34 100644 --- a/kernel/Makefile +++ b/kernel/Makefile @@ -50,6 +50,7 @@ obj-y += rcu/ obj-y += livepatch/ obj-y += dma/ obj-y += entry/ +obj-y += unwind/ obj-$(CONFIG_MODULES) += module/ obj-$(CONFIG_KCMP) += kcmp.o diff --git a/kernel/unwind/Makefile b/kernel/unwind/Makefile new file mode 100644 index 000000000000..349ce3677526 --- /dev/null +++ b/kernel/unwind/Makefile @@ -0,0 +1 @@ + obj-$(CONFIG_UNWIND_USER) += user.o diff --git a/kernel/unwind/user.c b/kernel/unwind/user.c new file mode 100644 index 000000000000..456539635e49 --- /dev/null +++ b/kernel/unwind/user.c @@ -0,0 +1,59 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* Generic interfaces for unwinding user space +*/ +#include <linux/kernel.h> +#include <linux/sched.h> +#include <linux/sched/task_stack.h> +#include <linux/unwind_user.h> + +int unwind_user_next(struct unwind_user_state *state) +{ + struct unwind_user_frame _frame; + struct unwind_user_frame *frame = &_frame; + unsigned long cfa = 0, fp, ra = 0; + + /* no implementation yet */ + -EINVAL; +} + +int unwind_user_start(struct unwind_user_state *state) +{ + struct pt_regs *regs = task_pt_regs(current); + + memset(state, 0, sizeof(*state)); + + if (!current->mm || !user_mode(regs)) { + state->done = true; + return -EINVAL; + } + + state->type = UNWIND_USER_TYPE_NONE; + + state->ip = instruction_pointer(regs); + state->sp = user_stack_pointer(regs); + state->fp = frame_pointer(regs); + + return 0; +} + +int unwind_user(struct unwind_stacktrace *trace, unsigned int max_entries) +{ + struct unwind_user_state state; + + trace->nr = 0; + + if (!max_entries) + return -EINVAL; + + if (!current->mm) + return 0; + + for_each_user_frame(&state) { + trace->entries[trace->nr++] = state.ip; + if (trace->nr >= max_entries) + break; + } + + return 0; +}
Introduce a generic API for unwinding user stacks. Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org> --- arch/Kconfig | 3 ++ include/linux/unwind_user.h | 15 ++++++++ include/linux/unwind_user_types.h | 31 ++++++++++++++++ kernel/Makefile | 1 + kernel/unwind/Makefile | 1 + kernel/unwind/user.c | 59 +++++++++++++++++++++++++++++++ 6 files changed, 110 insertions(+) create mode 100644 include/linux/unwind_user.h create mode 100644 include/linux/unwind_user_types.h create mode 100644 kernel/unwind/Makefile create mode 100644 kernel/unwind/user.c