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/
