Re: [PATCH v4 11/39] unwind_user: Add user space unwinding API
On Fri, Jan 24, 2025 at 03:02:11PM -0500, Steven Rostedt wrote:
> On Tue, 21 Jan 2025 18:31:03 -0800
> Josh Poimboeuf 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.
--
Josh
Re: [PATCH v4 11/39] unwind_user: Add user space unwinding API
On Tue, 21 Jan 2025 18:31:03 -0800
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;
> +
> + /* 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;
> +}
Re: [PATCH v4 11/39] unwind_user: Add user space unwinding API
On Tue, Jan 21, 2025 at 6:32 PM Josh Poimboeuf wrote:
>
> Introduce a generic API for unwinding user stacks.
>
> Signed-off-by: Josh Poimboeuf
> ---
> 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
> +#include
> +#include
> +#include
> +
> +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?
> +}
[...]
Re: [PATCH v4 11/39] unwind_user: Add user space unwinding API
On Fri, Jan 24, 2025 at 09:59:28AM -0800, Andrii Nakryiko wrote:
> On Tue, Jan 21, 2025 at 6:32 PM 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;
>
> 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 :-)
--
Josh
Re: [PATCH v4 11/39] unwind_user: Add user space unwinding API
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.
--
Josh
Re: [PATCH v4 11/39] unwind_user: Add user space unwinding API
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
+#include
+#include
+#include
+
+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
--
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/
[PATCH v4 11/39] unwind_user: Add user space unwinding API
Introduce a generic API for unwinding user stacks.
Signed-off-by: Josh Poimboeuf
---
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
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 ..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
+
+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 ..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
+
+enum unwind_user_type {
+ UNWIND_USER_TYPE_NONE,
+};
+
+struct unwind_stacktrace {
+ unsigned intnr;
+ 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 ..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 ..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
+#include
+#include
+#include
+
+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;
+}
--
2.48.1
