Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1456760 > unrolled thread

[PATCH v2 30/44] x86/unwind: add new unwind interface and implementations

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 30/44] x86/unwind: add new unwind interface and implementations Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-05 00:30 +0200
    Re: [PATCH v2 30/44] x86/unwind: add new unwind interface and implementations Nilay Vaish <nilayvaish@gmail.com> - 2016-08-10 01:20 +0200
      Re: [PATCH v2 30/44] x86/unwind: add new unwind interface and  implementations Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-10 01:30 +0200
        Re: [PATCH v2 30/44] x86/unwind: add new unwind interface and implementations Andy Lutomirski <luto@amacapital.net> - 2016-08-10 21:10 +0200
          Re: [PATCH v2 30/44] x86/unwind: add new unwind interface and  implementations Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-10 22:10 +0200
            Re: [PATCH v2 30/44] x86/unwind: add new unwind interface and implementations Andy Lutomirski <luto@amacapital.net> - 2016-08-11 09:30 +0200
              Re: [PATCH v2 30/44] x86/unwind: add new unwind interface and  implementations Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-11 16:30 +0200
                Re: [PATCH v2 30/44] x86/unwind: add new unwind interface and implementations Andy Lutomirski <luto@amacapital.net> - 2016-08-11 17:00 +0200
                  Re: [PATCH v2 30/44] x86/unwind: add new unwind interface and  implementations Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-11 18:10 +0200
                    Re: [PATCH v2 30/44] x86/unwind: add new unwind interface and implementations Andy Lutomirski <luto@amacapital.net> - 2016-08-11 21:00 +0200
                      Re: [PATCH v2 30/44] x86/unwind: add new unwind interface and  implementations Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-11 21:20 +0200

#1456760 — [PATCH v2 30/44] x86/unwind: add new unwind interface and implementations

The x86 stack dump code is a bit of a mess.  dump_trace() uses
callbacks, and each user of it seems to have slightly different
requirements, so there are several slightly different callbacks floating
around.

Also there are some upcoming features which will require more changes to
the stack dump code: reliable stack detection for live patching,
hardened user copy, and the DWARF unwinder.  Each of those features
would at least need more callbacks and/or callback interfaces, resulting
in a much bigger mess than what we have today.

Before doing all that, we should try to clean things up and replace
dump_trace() with something cleaner and more flexible.

The new unwinder is a simple state machine which was heavily inspired by
a suggestion from Andy Lutomirski:

  https://lkml.kernel.org/r/CALCETrUbNTqaM2LRyXGRx=kVLRPeY5A3Pc6k4TtQxF320rUT=w@mail.gmail.com

It's also very similar to the libunwind API:

  http://www.nongnu.org/libunwind/man/libunwind(3).html

Some if its advantages:

- Simplicity: no more callback sprawl and less code duplication.

- Flexibility: it allows the caller to stop and inspect the stack state
  at each step in the unwinding process.

- Modularity: the unwinder code, console stack dump code, and stack
  metadata analysis code are all better separated so that changing one
  of them shouldn't have much of an impact on any of the others.

Two implementations are added which conform to the new unwind interface:

- The frame pointer unwinder which is used for CONFIG_FRAME_POINTER=y.

- The "guess" unwinder which is used for CONFIG_FRAME_POINTER=n.  This
  isn't an "unwinder" per se.  All it does is scan the stack for kernel
  text addresses.  But with no frame pointers, guesses are better than
  nothing in most cases.

Suggested-by: Andy Lutomirski <luto@amacapital.net>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/include/asm/unwind.h  | 93 ++++++++++++++++++++++++++++++++++++++++++
 arch/x86/kernel/Makefile       |  6 +++
 arch/x86/kernel/unwind_frame.c | 84 ++++++++++++++++++++++++++++++++++++++
 arch/x86/kernel/unwind_guess.c | 40 ++++++++++++++++++
 4 files changed, 223 insertions(+)
 create mode 100644 arch/x86/include/asm/unwind.h
 create mode 100644 arch/x86/kernel/unwind_frame.c
 create mode 100644 arch/x86/kernel/unwind_guess.c

diff --git a/arch/x86/include/asm/unwind.h b/arch/x86/include/asm/unwind.h
new file mode 100644
index 0000000..c4fdd58
--- /dev/null
+++ b/arch/x86/include/asm/unwind.h
@@ -0,0 +1,93 @@
+#ifndef _ASM_X86_UNWIND_H
+#define _ASM_X86_UNWIND_H
+
+#include <linux/sched.h>
+#include <linux/ftrace.h>
+#include <asm/ptrace.h>
+#include <asm/stacktrace.h>
+
+struct unwind_state {
+	struct stack_info stack_info;
+	unsigned long stack_mask;
+	struct task_struct *task;
+	int graph_idx;
+#ifdef CONFIG_FRAME_POINTER
+	unsigned long *bp;
+#else
+	unsigned long *sp;
+#endif
+};
+
+void __unwind_start(struct unwind_state *state, struct task_struct *task,
+		    struct pt_regs *regs, unsigned long *sp);
+
+bool unwind_next_frame(struct unwind_state *state);
+
+
+#ifdef CONFIG_FRAME_POINTER
+
+static inline
+unsigned long *unwind_get_return_address_ptr(struct unwind_state *state)
+{
+	if (state->stack_info.type == STACK_TYPE_UNKNOWN)
+		return NULL;
+
+	return state->bp + 1;
+}
+
+static inline unsigned long *unwind_get_stack_ptr(struct unwind_state *state)
+{
+	if (state->stack_info.type == STACK_TYPE_UNKNOWN)
+		return NULL;
+
+	return state->bp;
+}
+
+unsigned long unwind_get_return_address(struct unwind_state *state);
+
+#else /* !CONFIG_FRAME_POINTER */
+
+static inline
+unsigned long *unwind_get_return_address_ptr(struct unwind_state *state)
+{
+	return NULL;
+}
+
+static inline unsigned long *unwind_get_stack_ptr(struct unwind_state *state)
+{
+	if (state->stack_info.type == STACK_TYPE_UNKNOWN)
+		return NULL;
+
+	return state->sp;
+}
+
+static inline
+unsigned long unwind_get_return_address(struct unwind_state *state)
+{
+	if (state->stack_info.type == STACK_TYPE_UNKNOWN)
+		return 0;
+
+	return ftrace_graph_ret_addr(state->task, &state->graph_idx,
+				     *state->sp, state->sp);
+}
+
+#endif /* CONFIG_FRAME_POINTER */
+
+static inline bool unwind_done(struct unwind_state *state)
+{
+	return (state->stack_info.type == STACK_TYPE_UNKNOWN);
+}
+
+static inline
+void unwind_start(struct unwind_state *state, struct task_struct *task,
+		  struct pt_regs *regs, unsigned long *sp)
+{
+	if (!task)
+		task = current;
+
+	sp = sp ? : get_stack_pointer(task, regs);
+
+	__unwind_start(state, task, regs, sp);
+}
+
+#endif /* _ASM_X86_UNWIND_H */
diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index 0503f5b..45257cf 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -125,6 +125,12 @@ obj-$(CONFIG_EFI)			+= sysfb_efi.o
 obj-$(CONFIG_PERF_EVENTS)		+= perf_regs.o
 obj-$(CONFIG_TRACING)			+= tracepoint.o
 
+ifdef CONFIG_FRAME_POINTER
+obj-y					+= unwind_frame.o
+else
+obj-y					+= unwind_guess.o
+endif
+
 ###
 # 64 bit specific files
 ifeq ($(CONFIG_X86_64),y)
diff --git a/arch/x86/kernel/unwind_frame.c b/arch/x86/kernel/unwind_frame.c
new file mode 100644
index 0000000..f28f1b5
--- /dev/null
+++ b/arch/x86/kernel/unwind_frame.c
@@ -0,0 +1,84 @@
+#include <linux/sched.h>
+#include <asm/ptrace.h>
+#include <asm/bitops.h>
+#include <asm/stacktrace.h>
+#include <asm/unwind.h>
+
+#define FRAME_HEADER_SIZE (sizeof(long) * 2)
+
+unsigned long unwind_get_return_address(struct unwind_state *state)
+{
+	unsigned long *addr_p = unwind_get_return_address_ptr(state);
+	unsigned long addr;
+
+	if (state->stack_info.type == STACK_TYPE_UNKNOWN)
+		return 0;
+
+	addr = ftrace_graph_ret_addr(state->task, &state->graph_idx, *addr_p,
+				     addr_p);
+
+	return __kernel_text_address(addr) ? addr : 0;
+}
+EXPORT_SYMBOL_GPL(unwind_get_return_address);
+
+static bool update_stack_state(struct unwind_state *state, void *addr,
+			       size_t len)
+{
+	struct stack_info *info = &state->stack_info;
+
+	if (on_stack(info, addr, len))
+		return true;
+
+	if (get_stack_info(info->next_sp, state->task, info,
+			   &state->stack_mask))
+		goto unknown;
+
+	if (!on_stack(info, addr, len))
+		goto unknown;
+
+	return true;
+
+unknown:
+	info->type = STACK_TYPE_UNKNOWN;
+	return false;
+}
+
+bool unwind_next_frame(struct unwind_state *state)
+{
+	unsigned long *next_bp;
+
+	if (unwind_done(state))
+		return false;
+
+	next_bp = (unsigned long *)*state->bp;
+
+	/*
+	 * Make sure the next frame is on a valid stack and can be accessed
+	 * safely.
+	 */
+	if (!update_stack_state(state, next_bp, FRAME_HEADER_SIZE))
+		return false;
+
+	/* move to the next frame */
+	state->bp = next_bp;
+	return true;
+}
+EXPORT_SYMBOL_GPL(unwind_next_frame);
+
+void __unwind_start(struct unwind_state *state, struct task_struct *task,
+		    struct pt_regs *regs, unsigned long *sp)
+{
+	memset(state, 0, sizeof(*state));
+
+	state->task = task;
+	state->bp = get_frame_pointer(task, regs);
+
+	get_stack_info(state->bp, state->task, &state->stack_info,
+		       &state->stack_mask);
+	update_stack_state(state, state->bp, FRAME_HEADER_SIZE);
+
+	/* unwind to the first frame after the specified stack pointer */
+	while (state->bp < sp && !unwind_done(state))
+		unwind_next_frame(state);
+}
+EXPORT_SYMBOL_GPL(__unwind_start);
diff --git a/arch/x86/kernel/unwind_guess.c b/arch/x86/kernel/unwind_guess.c
new file mode 100644
index 0000000..e03df5a
--- /dev/null
+++ b/arch/x86/kernel/unwind_guess.c
@@ -0,0 +1,40 @@
+#include <linux/sched.h>
+#include <linux/ftrace.h>
+#include <asm/ptrace.h>
+#include <asm/bitops.h>
+#include <asm/stacktrace.h>
+#include <asm/unwind.h>
+
+bool unwind_next_frame(struct unwind_state *state)
+{
+	struct stack_info *info = &state->stack_info;
+
+	if (info->type == STACK_TYPE_UNKNOWN)
+		return false;
+
+	do {
+		for (state->sp++; state->sp < info->end; state->sp++)
+			if (__kernel_text_address(*state->sp))
+				return true;
+
+		state->sp = info->next_sp;
+
+	} while (!get_stack_info(state->sp, state->task, info,
+				 &state->stack_mask));
+
+	return false;
+}
+
+void __unwind_start(struct unwind_state *state, struct task_struct *task,
+		    struct pt_regs *regs, unsigned long *sp)
+{
+	memset(state, 0, sizeof(*state));
+
+	state->task = task;
+	state->sp   = sp;
+
+	get_stack_info(sp, state->task, &state->stack_info, &state->stack_mask);
+
+	if (!__kernel_text_address(*sp))
+		unwind_next_frame(state);
+}
-- 
2.7.4

[toc] | [next] | [standalone]

#1459171

On 4 August 2016 at 17:22, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> diff --git a/arch/x86/kernel/unwind_frame.c b/arch/x86/kernel/unwind_frame.c
> new file mode 100644
> index 0000000..f28f1b5
> --- /dev/null
> +++ b/arch/x86/kernel/unwind_frame.c
> @@ -0,0 +1,84 @@
> +#include <linux/sched.h>
> +#include <asm/ptrace.h>
> +#include <asm/bitops.h>
> +#include <asm/stacktrace.h>
> +#include <asm/unwind.h>
> +
> +#define FRAME_HEADER_SIZE (sizeof(long) * 2)
> +
> +unsigned long unwind_get_return_address(struct unwind_state *state)
> +{
> +       unsigned long *addr_p = unwind_get_return_address_ptr(state);
> +       unsigned long addr;
> +
> +       if (state->stack_info.type == STACK_TYPE_UNKNOWN)
> +               return 0;
> +
> +       addr = ftrace_graph_ret_addr(state->task, &state->graph_idx, *addr_p,
> +                                    addr_p);
> +
> +       return __kernel_text_address(addr) ? addr : 0;
> +}
> +EXPORT_SYMBOL_GPL(unwind_get_return_address);
> +
> +static bool update_stack_state(struct unwind_state *state, void *addr,
> +                              size_t len)
> +{
> +       struct stack_info *info = &state->stack_info;
> +
> +       if (on_stack(info, addr, len))
> +               return true;
> +
> +       if (get_stack_info(info->next_sp, state->task, info,
> +                          &state->stack_mask))
> +               goto unknown;
> +
> +       if (!on_stack(info, addr, len))
> +               goto unknown;
> +
> +       return true;
> +
> +unknown:
> +       info->type = STACK_TYPE_UNKNOWN;
> +       return false;
> +}
> +
> +bool unwind_next_frame(struct unwind_state *state)
> +{
> +       unsigned long *next_bp;
> +
> +       if (unwind_done(state))
> +               return false;
> +
> +       next_bp = (unsigned long *)*state->bp;
> +
> +       /*
> +        * Make sure the next frame is on a valid stack and can be accessed
> +        * safely.
> +        */
> +       if (!update_stack_state(state, next_bp, FRAME_HEADER_SIZE))
> +               return false;
> +
> +       /* move to the next frame */
> +       state->bp = next_bp;
> +       return true;
> +}
> +EXPORT_SYMBOL_GPL(unwind_next_frame);
> +
> +void __unwind_start(struct unwind_state *state, struct task_struct *task,
> +                   struct pt_regs *regs, unsigned long *sp)
> +{
> +       memset(state, 0, sizeof(*state));
> +
> +       state->task = task;
> +       state->bp = get_frame_pointer(task, regs);
> +
> +       get_stack_info(state->bp, state->task, &state->stack_info,
> +                      &state->stack_mask);
> +       update_stack_state(state, state->bp, FRAME_HEADER_SIZE);
> +
> +       /* unwind to the first frame after the specified stack pointer */
> +       while (state->bp < sp && !unwind_done(state))
> +               unwind_next_frame(state);

Do we unwind all the frames here?  It seems strange to me that in a
function named __unwind_start(), we unwind all the frames.

> +}
> +EXPORT_SYMBOL_GPL(__unwind_start);
> diff --git a/arch/x86/kernel/unwind_guess.c b/arch/x86/kernel/unwind_guess.c
> new file mode 100644
> index 0000000..e03df5a
> --- /dev/null
> +++ b/arch/x86/kernel/unwind_guess.c
> @@ -0,0 +1,40 @@
> +#include <linux/sched.h>
> +#include <linux/ftrace.h>
> +#include <asm/ptrace.h>
> +#include <asm/bitops.h>
> +#include <asm/stacktrace.h>
> +#include <asm/unwind.h>
> +
> +bool unwind_next_frame(struct unwind_state *state)
> +{
> +       struct stack_info *info = &state->stack_info;
> +
> +       if (info->type == STACK_TYPE_UNKNOWN)
> +               return false;
> +
> +       do {
> +               for (state->sp++; state->sp < info->end; state->sp++)
> +                       if (__kernel_text_address(*state->sp))
> +                               return true;
> +
> +               state->sp = info->next_sp;
> +
> +       } while (!get_stack_info(state->sp, state->task, info,
> +                                &state->stack_mask));
> +
> +       return false;
> +}
> +
> +void __unwind_start(struct unwind_state *state, struct task_struct *task,
> +                   struct pt_regs *regs, unsigned long *sp)
> +{
> +       memset(state, 0, sizeof(*state));
> +
> +       state->task = task;
> +       state->sp   = sp;
> +
> +       get_stack_info(sp, state->task, &state->stack_info, &state->stack_mask);
> +
> +       if (!__kernel_text_address(*sp))
> +               unwind_next_frame(state);
> +}
> --
> 2.7.4
>

Why is it that you need to export symbols in unwind_frame.c but not in
unwind_guess.c.  As per the Makefile, we would be compiling either of
those two files.  Should not EXPORT_SYMBOL_GPL(__unwind_start) appear
in both files?

--
Nilay

[toc] | [prev] | [next] | [standalone]

#1459174 — Re: [PATCH v2 30/44] x86/unwind: add new unwind interface and implementations

On Tue, Aug 09, 2016 at 06:17:41PM -0500, Nilay Vaish wrote:
> On 4 August 2016 at 17:22, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > diff --git a/arch/x86/kernel/unwind_frame.c b/arch/x86/kernel/unwind_frame.c
> > new file mode 100644
> > index 0000000..f28f1b5
> > --- /dev/null
> > +++ b/arch/x86/kernel/unwind_frame.c
> > @@ -0,0 +1,84 @@
> > +#include <linux/sched.h>
> > +#include <asm/ptrace.h>
> > +#include <asm/bitops.h>
> > +#include <asm/stacktrace.h>
> > +#include <asm/unwind.h>
> > +
> > +#define FRAME_HEADER_SIZE (sizeof(long) * 2)
> > +
> > +unsigned long unwind_get_return_address(struct unwind_state *state)
> > +{
> > +       unsigned long *addr_p = unwind_get_return_address_ptr(state);
> > +       unsigned long addr;
> > +
> > +       if (state->stack_info.type == STACK_TYPE_UNKNOWN)
> > +               return 0;
> > +
> > +       addr = ftrace_graph_ret_addr(state->task, &state->graph_idx, *addr_p,
> > +                                    addr_p);
> > +
> > +       return __kernel_text_address(addr) ? addr : 0;
> > +}
> > +EXPORT_SYMBOL_GPL(unwind_get_return_address);
> > +
> > +static bool update_stack_state(struct unwind_state *state, void *addr,
> > +                              size_t len)
> > +{
> > +       struct stack_info *info = &state->stack_info;
> > +
> > +       if (on_stack(info, addr, len))
> > +               return true;
> > +
> > +       if (get_stack_info(info->next_sp, state->task, info,
> > +                          &state->stack_mask))
> > +               goto unknown;
> > +
> > +       if (!on_stack(info, addr, len))
> > +               goto unknown;
> > +
> > +       return true;
> > +
> > +unknown:
> > +       info->type = STACK_TYPE_UNKNOWN;
> > +       return false;
> > +}
> > +
> > +bool unwind_next_frame(struct unwind_state *state)
> > +{
> > +       unsigned long *next_bp;
> > +
> > +       if (unwind_done(state))
> > +               return false;
> > +
> > +       next_bp = (unsigned long *)*state->bp;
> > +
> > +       /*
> > +        * Make sure the next frame is on a valid stack and can be accessed
> > +        * safely.
> > +        */
> > +       if (!update_stack_state(state, next_bp, FRAME_HEADER_SIZE))
> > +               return false;
> > +
> > +       /* move to the next frame */
> > +       state->bp = next_bp;
> > +       return true;
> > +}
> > +EXPORT_SYMBOL_GPL(unwind_next_frame);
> > +
> > +void __unwind_start(struct unwind_state *state, struct task_struct *task,
> > +                   struct pt_regs *regs, unsigned long *sp)
> > +{
> > +       memset(state, 0, sizeof(*state));
> > +
> > +       state->task = task;
> > +       state->bp = get_frame_pointer(task, regs);
> > +
> > +       get_stack_info(state->bp, state->task, &state->stack_info,
> > +                      &state->stack_mask);
> > +       update_stack_state(state, state->bp, FRAME_HEADER_SIZE);
> > +
> > +       /* unwind to the first frame after the specified stack pointer */
> > +       while (state->bp < sp && !unwind_done(state))
> > +               unwind_next_frame(state);
> 
> Do we unwind all the frames here?  It seems strange to me that in a
> function named __unwind_start(), we unwind all the frames.

It just skips any stack frames before the specified "sp" pointer.
Several callers use this, for example, to start at regs->sp instead of
the current stack frame.  I'll try to make the comment clearer.

> > +}
> > +EXPORT_SYMBOL_GPL(__unwind_start);
> > diff --git a/arch/x86/kernel/unwind_guess.c b/arch/x86/kernel/unwind_guess.c
> > new file mode 100644
> > index 0000000..e03df5a
> > --- /dev/null
> > +++ b/arch/x86/kernel/unwind_guess.c
> > @@ -0,0 +1,40 @@
> > +#include <linux/sched.h>
> > +#include <linux/ftrace.h>
> > +#include <asm/ptrace.h>
> > +#include <asm/bitops.h>
> > +#include <asm/stacktrace.h>
> > +#include <asm/unwind.h>
> > +
> > +bool unwind_next_frame(struct unwind_state *state)
> > +{
> > +       struct stack_info *info = &state->stack_info;
> > +
> > +       if (info->type == STACK_TYPE_UNKNOWN)
> > +               return false;
> > +
> > +       do {
> > +               for (state->sp++; state->sp < info->end; state->sp++)
> > +                       if (__kernel_text_address(*state->sp))
> > +                               return true;
> > +
> > +               state->sp = info->next_sp;
> > +
> > +       } while (!get_stack_info(state->sp, state->task, info,
> > +                                &state->stack_mask));
> > +
> > +       return false;
> > +}
> > +
> > +void __unwind_start(struct unwind_state *state, struct task_struct *task,
> > +                   struct pt_regs *regs, unsigned long *sp)
> > +{
> > +       memset(state, 0, sizeof(*state));
> > +
> > +       state->task = task;
> > +       state->sp   = sp;
> > +
> > +       get_stack_info(sp, state->task, &state->stack_info, &state->stack_mask);
> > +
> > +       if (!__kernel_text_address(*sp))
> > +               unwind_next_frame(state);
> > +}
> > --
> > 2.7.4
> >
> 
> Why is it that you need to export symbols in unwind_frame.c but not in
> unwind_guess.c.  As per the Makefile, we would be compiling either of
> those two files.  Should not EXPORT_SYMBOL_GPL(__unwind_start) appear
> in both files?

Yeah, good catch.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1459601

On Aug 10, 2016 2:27 AM, "Josh Poimboeuf" <jpoimboe@redhat.com> wrote:
>
> On Tue, Aug 09, 2016 at 06:17:41PM -0500, Nilay Vaish wrote:
> > On 4 August 2016 at 17:22, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > > diff --git a/arch/x86/kernel/unwind_frame.c b/arch/x86/kernel/unwind_frame.c
> > > new file mode 100644
> > > index 0000000..f28f1b5
> > > --- /dev/null
> > > +++ b/arch/x86/kernel/unwind_frame.c
> > > @@ -0,0 +1,84 @@
> > > +#include <linux/sched.h>
> > > +#include <asm/ptrace.h>
> > > +#include <asm/bitops.h>
> > > +#include <asm/stacktrace.h>
> > > +#include <asm/unwind.h>
> > > +
> > > +#define FRAME_HEADER_SIZE (sizeof(long) * 2)
> > > +
> > > +unsigned long unwind_get_return_address(struct unwind_state *state)
> > > +{
> > > +       unsigned long *addr_p = unwind_get_return_address_ptr(state);
> > > +       unsigned long addr;
> > > +
> > > +       if (state->stack_info.type == STACK_TYPE_UNKNOWN)
> > > +               return 0;
> > > +
> > > +       addr = ftrace_graph_ret_addr(state->task, &state->graph_idx, *addr_p,
> > > +                                    addr_p);
> > > +
> > > +       return __kernel_text_address(addr) ? addr : 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(unwind_get_return_address);
> > > +
> > > +static bool update_stack_state(struct unwind_state *state, void *addr,
> > > +                              size_t len)
> > > +{
> > > +       struct stack_info *info = &state->stack_info;
> > > +
> > > +       if (on_stack(info, addr, len))
> > > +               return true;
> > > +
> > > +       if (get_stack_info(info->next_sp, state->task, info,
> > > +                          &state->stack_mask))
> > > +               goto unknown;
> > > +
> > > +       if (!on_stack(info, addr, len))
> > > +               goto unknown;
> > > +
> > > +       return true;
> > > +
> > > +unknown:
> > > +       info->type = STACK_TYPE_UNKNOWN;
> > > +       return false;
> > > +}
> > > +
> > > +bool unwind_next_frame(struct unwind_state *state)
> > > +{
> > > +       unsigned long *next_bp;
> > > +
> > > +       if (unwind_done(state))
> > > +               return false;
> > > +
> > > +       next_bp = (unsigned long *)*state->bp;
> > > +
> > > +       /*
> > > +        * Make sure the next frame is on a valid stack and can be accessed
> > > +        * safely.
> > > +        */
> > > +       if (!update_stack_state(state, next_bp, FRAME_HEADER_SIZE))
> > > +               return false;
> > > +
> > > +       /* move to the next frame */
> > > +       state->bp = next_bp;
> > > +       return true;
> > > +}
> > > +EXPORT_SYMBOL_GPL(unwind_next_frame);
> > > +
> > > +void __unwind_start(struct unwind_state *state, struct task_struct *task,
> > > +                   struct pt_regs *regs, unsigned long *sp)
> > > +{
> > > +       memset(state, 0, sizeof(*state));
> > > +
> > > +       state->task = task;
> > > +       state->bp = get_frame_pointer(task, regs);
> > > +
> > > +       get_stack_info(state->bp, state->task, &state->stack_info,
> > > +                      &state->stack_mask);
> > > +       update_stack_state(state, state->bp, FRAME_HEADER_SIZE);
> > > +
> > > +       /* unwind to the first frame after the specified stack pointer */
> > > +       while (state->bp < sp && !unwind_done(state))
> > > +               unwind_next_frame(state);
> >
> > Do we unwind all the frames here?  It seems strange to me that in a
> > function named __unwind_start(), we unwind all the frames.
>
> It just skips any stack frames before the specified "sp" pointer.
> Several callers use this, for example, to start at regs->sp instead of
> the current stack frame.  I'll try to make the comment clearer.
>

Are you checking the right condition?  Shouldn't this check that sp is
in bounds for the current stack if a stack switch happened?

I admit I don't fully understand the use case.  If someone wants to
start a trace in the middle, shouldn't they just pass regs in?

[toc] | [prev] | [next] | [standalone]

#1459794 — Re: [PATCH v2 30/44] x86/unwind: add new unwind interface and implementations

On Wed, Aug 10, 2016 at 12:25:11AM -0700, Andy Lutomirski wrote:
> On Aug 10, 2016 2:27 AM, "Josh Poimboeuf" <jpoimboe@redhat.com> wrote:
> >
> > On Tue, Aug 09, 2016 at 06:17:41PM -0500, Nilay Vaish wrote:
> > > On 4 August 2016 at 17:22, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > > > diff --git a/arch/x86/kernel/unwind_frame.c b/arch/x86/kernel/unwind_frame.c
> > > > new file mode 100644
> > > > index 0000000..f28f1b5
> > > > --- /dev/null
> > > > +++ b/arch/x86/kernel/unwind_frame.c
> > > > @@ -0,0 +1,84 @@
> > > > +#include <linux/sched.h>
> > > > +#include <asm/ptrace.h>
> > > > +#include <asm/bitops.h>
> > > > +#include <asm/stacktrace.h>
> > > > +#include <asm/unwind.h>
> > > > +
> > > > +#define FRAME_HEADER_SIZE (sizeof(long) * 2)
> > > > +
> > > > +unsigned long unwind_get_return_address(struct unwind_state *state)
> > > > +{
> > > > +       unsigned long *addr_p = unwind_get_return_address_ptr(state);
> > > > +       unsigned long addr;
> > > > +
> > > > +       if (state->stack_info.type == STACK_TYPE_UNKNOWN)
> > > > +               return 0;
> > > > +
> > > > +       addr = ftrace_graph_ret_addr(state->task, &state->graph_idx, *addr_p,
> > > > +                                    addr_p);
> > > > +
> > > > +       return __kernel_text_address(addr) ? addr : 0;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(unwind_get_return_address);
> > > > +
> > > > +static bool update_stack_state(struct unwind_state *state, void *addr,
> > > > +                              size_t len)
> > > > +{
> > > > +       struct stack_info *info = &state->stack_info;
> > > > +
> > > > +       if (on_stack(info, addr, len))
> > > > +               return true;
> > > > +
> > > > +       if (get_stack_info(info->next_sp, state->task, info,
> > > > +                          &state->stack_mask))
> > > > +               goto unknown;
> > > > +
> > > > +       if (!on_stack(info, addr, len))
> > > > +               goto unknown;
> > > > +
> > > > +       return true;
> > > > +
> > > > +unknown:
> > > > +       info->type = STACK_TYPE_UNKNOWN;
> > > > +       return false;
> > > > +}
> > > > +
> > > > +bool unwind_next_frame(struct unwind_state *state)
> > > > +{
> > > > +       unsigned long *next_bp;
> > > > +
> > > > +       if (unwind_done(state))
> > > > +               return false;
> > > > +
> > > > +       next_bp = (unsigned long *)*state->bp;
> > > > +
> > > > +       /*
> > > > +        * Make sure the next frame is on a valid stack and can be accessed
> > > > +        * safely.
> > > > +        */
> > > > +       if (!update_stack_state(state, next_bp, FRAME_HEADER_SIZE))
> > > > +               return false;
> > > > +
> > > > +       /* move to the next frame */
> > > > +       state->bp = next_bp;
> > > > +       return true;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(unwind_next_frame);
> > > > +
> > > > +void __unwind_start(struct unwind_state *state, struct task_struct *task,
> > > > +                   struct pt_regs *regs, unsigned long *sp)
> > > > +{
> > > > +       memset(state, 0, sizeof(*state));
> > > > +
> > > > +       state->task = task;
> > > > +       state->bp = get_frame_pointer(task, regs);
> > > > +
> > > > +       get_stack_info(state->bp, state->task, &state->stack_info,
> > > > +                      &state->stack_mask);
> > > > +       update_stack_state(state, state->bp, FRAME_HEADER_SIZE);
> > > > +
> > > > +       /* unwind to the first frame after the specified stack pointer */
> > > > +       while (state->bp < sp && !unwind_done(state))
> > > > +               unwind_next_frame(state);
> > >
> > > Do we unwind all the frames here?  It seems strange to me that in a
> > > function named __unwind_start(), we unwind all the frames.
> >
> > It just skips any stack frames before the specified "sp" pointer.
> > Several callers use this, for example, to start at regs->sp instead of
> > the current stack frame.  I'll try to make the comment clearer.
> >
> 
> Are you checking the right condition?  Shouldn't this check that sp is
> in bounds for the current stack if a stack switch happened?

You're right.

> I admit I don't fully understand the use case.  If someone wants to
> start a trace in the middle, shouldn't they just pass regs in?

The regs aren't always available.  Some callers just want to skip the
first few frames so the stack dump code itself isn't traced.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1460217

On Aug 10, 2016 5:16 PM, "Josh Poimboeuf" <jpoimboe@redhat.com> wrote:
>
> On Wed, Aug 10, 2016 at 12:25:11AM -0700, Andy Lutomirski wrote:
> > On Aug 10, 2016 2:27 AM, "Josh Poimboeuf" <jpoimboe@redhat.com> wrote:
> > >
> > > On Tue, Aug 09, 2016 at 06:17:41PM -0500, Nilay Vaish wrote:
> > > > On 4 August 2016 at 17:22, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > > > > diff --git a/arch/x86/kernel/unwind_frame.c b/arch/x86/kernel/unwind_frame.c
> > > > > new file mode 100644
> > > > > index 0000000..f28f1b5
> > > > > --- /dev/null
> > > > > +++ b/arch/x86/kernel/unwind_frame.c
> > > > > @@ -0,0 +1,84 @@
> > > > > +#include <linux/sched.h>
> > > > > +#include <asm/ptrace.h>
> > > > > +#include <asm/bitops.h>
> > > > > +#include <asm/stacktrace.h>
> > > > > +#include <asm/unwind.h>
> > > > > +
> > > > > +#define FRAME_HEADER_SIZE (sizeof(long) * 2)
> > > > > +
> > > > > +unsigned long unwind_get_return_address(struct unwind_state *state)
> > > > > +{
> > > > > +       unsigned long *addr_p = unwind_get_return_address_ptr(state);
> > > > > +       unsigned long addr;
> > > > > +
> > > > > +       if (state->stack_info.type == STACK_TYPE_UNKNOWN)
> > > > > +               return 0;
> > > > > +
> > > > > +       addr = ftrace_graph_ret_addr(state->task, &state->graph_idx, *addr_p,
> > > > > +                                    addr_p);
> > > > > +
> > > > > +       return __kernel_text_address(addr) ? addr : 0;
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(unwind_get_return_address);
> > > > > +
> > > > > +static bool update_stack_state(struct unwind_state *state, void *addr,
> > > > > +                              size_t len)
> > > > > +{
> > > > > +       struct stack_info *info = &state->stack_info;
> > > > > +
> > > > > +       if (on_stack(info, addr, len))
> > > > > +               return true;
> > > > > +
> > > > > +       if (get_stack_info(info->next_sp, state->task, info,
> > > > > +                          &state->stack_mask))
> > > > > +               goto unknown;
> > > > > +
> > > > > +       if (!on_stack(info, addr, len))
> > > > > +               goto unknown;
> > > > > +
> > > > > +       return true;
> > > > > +
> > > > > +unknown:
> > > > > +       info->type = STACK_TYPE_UNKNOWN;
> > > > > +       return false;
> > > > > +}
> > > > > +
> > > > > +bool unwind_next_frame(struct unwind_state *state)
> > > > > +{
> > > > > +       unsigned long *next_bp;
> > > > > +
> > > > > +       if (unwind_done(state))
> > > > > +               return false;
> > > > > +
> > > > > +       next_bp = (unsigned long *)*state->bp;
> > > > > +
> > > > > +       /*
> > > > > +        * Make sure the next frame is on a valid stack and can be accessed
> > > > > +        * safely.
> > > > > +        */
> > > > > +       if (!update_stack_state(state, next_bp, FRAME_HEADER_SIZE))
> > > > > +               return false;
> > > > > +
> > > > > +       /* move to the next frame */
> > > > > +       state->bp = next_bp;
> > > > > +       return true;
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(unwind_next_frame);
> > > > > +
> > > > > +void __unwind_start(struct unwind_state *state, struct task_struct *task,
> > > > > +                   struct pt_regs *regs, unsigned long *sp)
> > > > > +{
> > > > > +       memset(state, 0, sizeof(*state));
> > > > > +
> > > > > +       state->task = task;
> > > > > +       state->bp = get_frame_pointer(task, regs);
> > > > > +
> > > > > +       get_stack_info(state->bp, state->task, &state->stack_info,
> > > > > +                      &state->stack_mask);
> > > > > +       update_stack_state(state, state->bp, FRAME_HEADER_SIZE);
> > > > > +
> > > > > +       /* unwind to the first frame after the specified stack pointer */
> > > > > +       while (state->bp < sp && !unwind_done(state))
> > > > > +               unwind_next_frame(state);
> > > >
> > > > Do we unwind all the frames here?  It seems strange to me that in a
> > > > function named __unwind_start(), we unwind all the frames.
> > >
> > > It just skips any stack frames before the specified "sp" pointer.
> > > Several callers use this, for example, to start at regs->sp instead of
> > > the current stack frame.  I'll try to make the comment clearer.
> > >
> >
> > Are you checking the right condition?  Shouldn't this check that sp is
> > in bounds for the current stack if a stack switch happened?
>
> You're right.
>
> > I admit I don't fully understand the use case.  If someone wants to
> > start a trace in the middle, shouldn't they just pass regs in?
>
> The regs aren't always available.  Some callers just want to skip the
> first few frames so the stack dump code itself isn't traced.

I suspect that all users are okay with your algorithm simply because
they don't switch stacks.  Maybe the thing to do is to stop advancing
when sp is passed or if the stack switches at all.

Could you point me at a user that passes anything other than regs->sp
here?  On brief inspection, I haven't found any at all.

--Andy

[toc] | [prev] | [next] | [standalone]

#1460572 — Re: [PATCH v2 30/44] x86/unwind: add new unwind interface and implementations

On Thu, Aug 11, 2016 at 12:18:54AM -0700, Andy Lutomirski wrote:
> On Aug 10, 2016 5:16 PM, "Josh Poimboeuf" <jpoimboe@redhat.com> wrote:
> >
> > On Wed, Aug 10, 2016 at 12:25:11AM -0700, Andy Lutomirski wrote:
> > > On Aug 10, 2016 2:27 AM, "Josh Poimboeuf" <jpoimboe@redhat.com> wrote:
> > > >
> > > > On Tue, Aug 09, 2016 at 06:17:41PM -0500, Nilay Vaish wrote:
> > > > > On 4 August 2016 at 17:22, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > > > > > diff --git a/arch/x86/kernel/unwind_frame.c b/arch/x86/kernel/unwind_frame.c
> > > > > > new file mode 100644
> > > > > > index 0000000..f28f1b5
> > > > > > --- /dev/null
> > > > > > +++ b/arch/x86/kernel/unwind_frame.c
> > > > > > @@ -0,0 +1,84 @@
> > > > > > +#include <linux/sched.h>
> > > > > > +#include <asm/ptrace.h>
> > > > > > +#include <asm/bitops.h>
> > > > > > +#include <asm/stacktrace.h>
> > > > > > +#include <asm/unwind.h>
> > > > > > +
> > > > > > +#define FRAME_HEADER_SIZE (sizeof(long) * 2)
> > > > > > +
> > > > > > +unsigned long unwind_get_return_address(struct unwind_state *state)
> > > > > > +{
> > > > > > +       unsigned long *addr_p = unwind_get_return_address_ptr(state);
> > > > > > +       unsigned long addr;
> > > > > > +
> > > > > > +       if (state->stack_info.type == STACK_TYPE_UNKNOWN)
> > > > > > +               return 0;
> > > > > > +
> > > > > > +       addr = ftrace_graph_ret_addr(state->task, &state->graph_idx, *addr_p,
> > > > > > +                                    addr_p);
> > > > > > +
> > > > > > +       return __kernel_text_address(addr) ? addr : 0;
> > > > > > +}
> > > > > > +EXPORT_SYMBOL_GPL(unwind_get_return_address);
> > > > > > +
> > > > > > +static bool update_stack_state(struct unwind_state *state, void *addr,
> > > > > > +                              size_t len)
> > > > > > +{
> > > > > > +       struct stack_info *info = &state->stack_info;
> > > > > > +
> > > > > > +       if (on_stack(info, addr, len))
> > > > > > +               return true;
> > > > > > +
> > > > > > +       if (get_stack_info(info->next_sp, state->task, info,
> > > > > > +                          &state->stack_mask))
> > > > > > +               goto unknown;
> > > > > > +
> > > > > > +       if (!on_stack(info, addr, len))
> > > > > > +               goto unknown;
> > > > > > +
> > > > > > +       return true;
> > > > > > +
> > > > > > +unknown:
> > > > > > +       info->type = STACK_TYPE_UNKNOWN;
> > > > > > +       return false;
> > > > > > +}
> > > > > > +
> > > > > > +bool unwind_next_frame(struct unwind_state *state)
> > > > > > +{
> > > > > > +       unsigned long *next_bp;
> > > > > > +
> > > > > > +       if (unwind_done(state))
> > > > > > +               return false;
> > > > > > +
> > > > > > +       next_bp = (unsigned long *)*state->bp;
> > > > > > +
> > > > > > +       /*
> > > > > > +        * Make sure the next frame is on a valid stack and can be accessed
> > > > > > +        * safely.
> > > > > > +        */
> > > > > > +       if (!update_stack_state(state, next_bp, FRAME_HEADER_SIZE))
> > > > > > +               return false;
> > > > > > +
> > > > > > +       /* move to the next frame */
> > > > > > +       state->bp = next_bp;
> > > > > > +       return true;
> > > > > > +}
> > > > > > +EXPORT_SYMBOL_GPL(unwind_next_frame);
> > > > > > +
> > > > > > +void __unwind_start(struct unwind_state *state, struct task_struct *task,
> > > > > > +                   struct pt_regs *regs, unsigned long *sp)
> > > > > > +{
> > > > > > +       memset(state, 0, sizeof(*state));
> > > > > > +
> > > > > > +       state->task = task;
> > > > > > +       state->bp = get_frame_pointer(task, regs);
> > > > > > +
> > > > > > +       get_stack_info(state->bp, state->task, &state->stack_info,
> > > > > > +                      &state->stack_mask);
> > > > > > +       update_stack_state(state, state->bp, FRAME_HEADER_SIZE);
> > > > > > +
> > > > > > +       /* unwind to the first frame after the specified stack pointer */
> > > > > > +       while (state->bp < sp && !unwind_done(state))
> > > > > > +               unwind_next_frame(state);
> > > > >
> > > > > Do we unwind all the frames here?  It seems strange to me that in a
> > > > > function named __unwind_start(), we unwind all the frames.
> > > >
> > > > It just skips any stack frames before the specified "sp" pointer.
> > > > Several callers use this, for example, to start at regs->sp instead of
> > > > the current stack frame.  I'll try to make the comment clearer.
> > > >
> > >
> > > Are you checking the right condition?  Shouldn't this check that sp is
> > > in bounds for the current stack if a stack switch happened?
> >
> > You're right.
> >
> > > I admit I don't fully understand the use case.  If someone wants to
> > > start a trace in the middle, shouldn't they just pass regs in?
> >
> > The regs aren't always available.  Some callers just want to skip the
> > first few frames so the stack dump code itself isn't traced.
> 
> I suspect that all users are okay with your algorithm simply because
> they don't switch stacks.  Maybe the thing to do is to stop advancing
> when sp is passed or if the stack switches at all.

There are actually some cases which could be broken by the sloppy "while
state->bp < sp" check.  When starting with sp from 'regs->sp', sp often
points to a different stack than the current one.  It seemed to work in
my testing, but I guess I got lucky, with irq percpu stack addresses
being smaller than the thread stack addresses.

> Could you point me at a user that passes anything other than regs->sp
> here?  On brief inspection, I haven't found any at all.

For example, see show_stack().

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1460590

On Thu, Aug 11, 2016 at 7:28 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Thu, Aug 11, 2016 at 12:18:54AM -0700, Andy Lutomirski wrote:
>> > > I admit I don't fully understand the use case.  If someone wants to
>> > > start a trace in the middle, shouldn't they just pass regs in?
>> >
>> > The regs aren't always available.  Some callers just want to skip the
>> > first few frames so the stack dump code itself isn't traced.
>>
>> I suspect that all users are okay with your algorithm simply because
>> they don't switch stacks.  Maybe the thing to do is to stop advancing
>> when sp is passed or if the stack switches at all.
>
> There are actually some cases which could be broken by the sloppy "while
> state->bp < sp" check.  When starting with sp from 'regs->sp', sp often
> points to a different stack than the current one.  It seemed to work in
> my testing, but I guess I got lucky, with irq percpu stack addresses
> being smaller than the thread stack addresses.
>
>> Could you point me at a user that passes anything other than regs->sp
>> here?  On brief inspection, I haven't found any at all.
>
> For example, see show_stack().

Is that a non-trivial case?  show_stack() is generating sp *and* bp.
Why not just pass both of them all the way in to show_trace_log_lvl
(which the existing code already does) and then pass it into
unwind_start?

Alternatively, since that risks causing a bit of loss if you implement
DWARF, you could add an unwind_start_here() function that captures the
state in the calling function and pass the result all the way through.
Or you could write a silly asm helper that literally fills in a struct
pt_regs for the current context (although that could get a bit awkward
for 32-bit, since pt_regs doesn't really contain sp there).

If there are genuinely zero non-trivial cases, then this should fully
solve the problem, no?

Aside: the current code looks a bit silly to me:

    unsigned long stack;
    ...
        sp = &stack;
        bp = stack_frame(current, NULL);

Why not just force a frame pointer and read out sp and bp using asm?

--Andy

[toc] | [prev] | [next] | [standalone]

#1460645 — Re: [PATCH v2 30/44] x86/unwind: add new unwind interface and implementations

On Thu, Aug 11, 2016 at 07:58:34AM -0700, Andy Lutomirski wrote:
> On Thu, Aug 11, 2016 at 7:28 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > On Thu, Aug 11, 2016 at 12:18:54AM -0700, Andy Lutomirski wrote:
> >> > > I admit I don't fully understand the use case.  If someone wants to
> >> > > start a trace in the middle, shouldn't they just pass regs in?
> >> >
> >> > The regs aren't always available.  Some callers just want to skip the
> >> > first few frames so the stack dump code itself isn't traced.
> >>
> >> I suspect that all users are okay with your algorithm simply because
> >> they don't switch stacks.  Maybe the thing to do is to stop advancing
> >> when sp is passed or if the stack switches at all.
> >
> > There are actually some cases which could be broken by the sloppy "while
> > state->bp < sp" check.  When starting with sp from 'regs->sp', sp often
> > points to a different stack than the current one.  It seemed to work in
> > my testing, but I guess I got lucky, with irq percpu stack addresses
> > being smaller than the thread stack addresses.
> >
> >> Could you point me at a user that passes anything other than regs->sp
> >> here?  On brief inspection, I haven't found any at all.
> >
> > For example, see show_stack().
> 
> Is that a non-trivial case?  show_stack() is generating sp *and* bp.
> Why not just pass both of them all the way in to show_trace_log_lvl
> (which the existing code already does) and then pass it into
> unwind_start?
> 
> Alternatively, since that risks causing a bit of loss if you implement
> DWARF, you could add an unwind_start_here() function that captures the
> state in the calling function and pass the result all the way through.
> Or you could write a silly asm helper that literally fills in a struct
> pt_regs for the current context (although that could get a bit awkward
> for 32-bit, since pt_regs doesn't really contain sp there).
> 
> If there are genuinely zero non-trivial cases, then this should fully
> solve the problem, no?

Hm, what do you mean by non-trivial cases?

As far as I can tell, they're all trivial, and we don't need the caller
to provide bp.  Just start with the current frame's bp and unwind until
bp and sp are on the same stack and bp > sp.

> Aside: the current code looks a bit silly to me:
> 
>     unsigned long stack;
>     ...
>         sp = &stack;
>         bp = stack_frame(current, NULL);
> 
> Why not just force a frame pointer and read out sp and bp using asm?

Due to the above-discussed algorithm, we no longer need to get bp in
show_stack() (though I forgot to remove the get_frame_pointer() call in
my patches).

And I changed the 'sp = &stack' to a call to get_stack_pointer().

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1460742

On Aug 11, 2016 7:09 PM, "Josh Poimboeuf" <jpoimboe@redhat.com> wrote:
>
> On Thu, Aug 11, 2016 at 07:58:34AM -0700, Andy Lutomirski wrote:
> > On Thu, Aug 11, 2016 at 7:28 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > > On Thu, Aug 11, 2016 at 12:18:54AM -0700, Andy Lutomirski wrote:
> > >> > > I admit I don't fully understand the use case.  If someone wants to
> > >> > > start a trace in the middle, shouldn't they just pass regs in?
> > >> >
> > >> > The regs aren't always available.  Some callers just want to skip the
> > >> > first few frames so the stack dump code itself isn't traced.
> > >>
> > >> I suspect that all users are okay with your algorithm simply because
> > >> they don't switch stacks.  Maybe the thing to do is to stop advancing
> > >> when sp is passed or if the stack switches at all.
> > >
> > > There are actually some cases which could be broken by the sloppy "while
> > > state->bp < sp" check.  When starting with sp from 'regs->sp', sp often
> > > points to a different stack than the current one.  It seemed to work in
> > > my testing, but I guess I got lucky, with irq percpu stack addresses
> > > being smaller than the thread stack addresses.
> > >
> > >> Could you point me at a user that passes anything other than regs->sp
> > >> here?  On brief inspection, I haven't found any at all.
> > >
> > > For example, see show_stack().
> >
> > Is that a non-trivial case?  show_stack() is generating sp *and* bp.
> > Why not just pass both of them all the way in to show_trace_log_lvl
> > (which the existing code already does) and then pass it into
> > unwind_start?
> >
> > Alternatively, since that risks causing a bit of loss if you implement
> > DWARF, you could add an unwind_start_here() function that captures the
> > state in the calling function and pass the result all the way through.
> > Or you could write a silly asm helper that literally fills in a struct
> > pt_regs for the current context (although that could get a bit awkward
> > for 32-bit, since pt_regs doesn't really contain sp there).
> >
> > If there are genuinely zero non-trivial cases, then this should fully
> > solve the problem, no?
>
> Hm, what do you mean by non-trivial cases?
>
> As far as I can tell, they're all trivial, and we don't need the caller
> to provide bp.  Just start with the current frame's bp and unwind until
> bp and sp are on the same stack and bp > sp.

Given that your unwind-till-we-get-there algorithm isn't quite
correct, wouldn't it be easier to just pass all the needed information
through?  Especially since that information is there (in at least one
case) on current kernels, so the diff would be smaller.  It seems
overcomplicated to me to regenerate the state that was already in the
registers by unwinding to it rather than just passing it in to the
unwinder directly.

--Andy

[toc] | [prev] | [next] | [standalone]

#1460745 — Re: [PATCH v2 30/44] x86/unwind: add new unwind interface and implementations

On Thu, Aug 11, 2016 at 11:58:13AM -0700, Andy Lutomirski wrote:
> On Aug 11, 2016 7:09 PM, "Josh Poimboeuf" <jpoimboe@redhat.com> wrote:
> >
> > On Thu, Aug 11, 2016 at 07:58:34AM -0700, Andy Lutomirski wrote:
> > > On Thu, Aug 11, 2016 at 7:28 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > > > On Thu, Aug 11, 2016 at 12:18:54AM -0700, Andy Lutomirski wrote:
> > > >> > > I admit I don't fully understand the use case.  If someone wants to
> > > >> > > start a trace in the middle, shouldn't they just pass regs in?
> > > >> >
> > > >> > The regs aren't always available.  Some callers just want to skip the
> > > >> > first few frames so the stack dump code itself isn't traced.
> > > >>
> > > >> I suspect that all users are okay with your algorithm simply because
> > > >> they don't switch stacks.  Maybe the thing to do is to stop advancing
> > > >> when sp is passed or if the stack switches at all.
> > > >
> > > > There are actually some cases which could be broken by the sloppy "while
> > > > state->bp < sp" check.  When starting with sp from 'regs->sp', sp often
> > > > points to a different stack than the current one.  It seemed to work in
> > > > my testing, but I guess I got lucky, with irq percpu stack addresses
> > > > being smaller than the thread stack addresses.
> > > >
> > > >> Could you point me at a user that passes anything other than regs->sp
> > > >> here?  On brief inspection, I haven't found any at all.
> > > >
> > > > For example, see show_stack().
> > >
> > > Is that a non-trivial case?  show_stack() is generating sp *and* bp.
> > > Why not just pass both of them all the way in to show_trace_log_lvl
> > > (which the existing code already does) and then pass it into
> > > unwind_start?
> > >
> > > Alternatively, since that risks causing a bit of loss if you implement
> > > DWARF, you could add an unwind_start_here() function that captures the
> > > state in the calling function and pass the result all the way through.
> > > Or you could write a silly asm helper that literally fills in a struct
> > > pt_regs for the current context (although that could get a bit awkward
> > > for 32-bit, since pt_regs doesn't really contain sp there).
> > >
> > > If there are genuinely zero non-trivial cases, then this should fully
> > > solve the problem, no?
> >
> > Hm, what do you mean by non-trivial cases?
> >
> > As far as I can tell, they're all trivial, and we don't need the caller
> > to provide bp.  Just start with the current frame's bp and unwind until
> > bp and sp are on the same stack and bp > sp.
> 
> Given that your unwind-till-we-get-there algorithm isn't quite
> correct

But it *will* be correct in v3, with a minor change.  Sneak preview:

	/*
	 * The caller can optionally provide a stack pointer directly
	 * (first_sp) or indirectly (regs->sp), which indicates which stack
	 * frame to start unwinding at.  Skip ahead until we reach that frame.
	 */
	while (!unwind_done(state) &&
	       (!on_stack(&state->stack_info, first_sp, sizeof(*first_sp) ||
		state->bp < first_sp)))
		unwind_next_frame(state);

> wouldn't it be easier to just pass all the needed information
> through?  Especially since that information is there (in at least one
> case) on current kernels, so the diff would be smaller.

Why focus on diff size?  Isn't it the resulting code that's most
important?  It's not like removing the bp argument causes a big diff
anyway.

> It seems overcomplicated to me to regenerate the state that was
> already in the registers by unwinding to it rather than just passing
> it in to the unwinder directly.

Passing in the frame pointer when it isn't needed makes the interface
more complex.  It creates more opportunity for the caller to mess things
up and more edge cases to consider in the unwinder.

And as you mentioned, it's not compatible with DWARF.  Adding
unwind_start_here() would add even more interface complexity.

-- 
Josh

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web