x86/dumpstack: rewrite x86 stack dump code

There are a bewildering array of options for dumping the stack. Simplify things a little by removing show_trace(), which is unused.

The various functions involved in dumping the stack all do similar things with regard to getting the stack pointer and the frame pointer based on the regs and task arguments. Create helper functions to do that instead.

When calling show_stack_log_lvl() or dump_trace() with a regs argument, providing a stack pointer or frame pointer is redundant.

arch/x86/kernel/dumpstack_32.c +0 -0

index 358fe1c..c533b8b 100644

Quoted 9 lines · Josh Poimboeuf

When calling show_stack_log_lvl() or dump_trace() with a regs argument, providing a stack pointer or frame pointer is redundant.

arch/x86/kernel/dumpstack_32.c +0 -0

index 358fe1c..c533b8b 100644

arch/x86/kernel/dumpstack_32.c void show_regs(struct pt_regs *regs) · line 125

Quoted 5 lines

arch/x86/kernel/dumpstack_32.c +1 -1

@@ -122,7 +122,7 @@ void show_regs(struct pt_regs *regs)
                u8 *ip;
 
                pr_emerg("Stack:\n");
-               show_stack_log_lvl(NULL, regs, &regs->sp, 0, KERN_EMERG);
+               show_stack_log_lvl(NULL, regs, NULL, 0, KERN_EMERG);

This is weird -- note the &. You're at some risk of exposing a bug in x86_32's kernel_stack_pointer() function, which is a mess. (I don't see why it's written the way it is -- the actual return stack pointer given a pt_regs is quite well defined -- if regs->cs & 3 != 0, then it's regs->sp, else it's &regs->sp.)

That being said, this isn't a big deal, so:

Reviewed-by Andy Lutomirski <[email protected]>

If you want to make this all a bit more reliably on x86_32, you could fix kernel_stack_pointer().

Quoted 24 lines · Andy Lutomirski

Quoted 9 lines · Josh Poimboeuf

When calling show_stack_log_lvl() or dump_trace() with a regs argument, providing a stack pointer or frame pointer is redundant.

arch/x86/kernel/dumpstack_32.c +0 -0

index 358fe1c..c533b8b 100644

arch/x86/kernel/dumpstack_32.c void show_regs(struct pt_regs *regs) · line 125

Quoted 5 lines

arch/x86/kernel/dumpstack_32.c +1 -1

@@ -122,7 +122,7 @@ void show_regs(struct pt_regs *regs)
                u8 *ip;
 
                pr_emerg("Stack:\n");
-               show_stack_log_lvl(NULL, regs, &regs->sp, 0, KERN_EMERG);
+               show_stack_log_lvl(NULL, regs, NULL, 0, KERN_EMERG);

This is weird -- note the &. You're at some risk of exposing a bug in x86_32's kernel_stack_pointer() function, which is a mess. (I don't see why it's written the way it is -- the actual return stack pointer given a pt_regs is quite well defined -- if regs->cs & 3 != 0, then it's regs->sp, else it's &regs->sp.)

That being said, this isn't a big deal, so:

Quoted 4 lines

Reviewed-by Andy Lutomirski <[email protected]>

If you want to make this all a bit more reliably on x86_32, you could fix kernel_stack_pointer().

Ok. The whole '&regs->sp' thing threw me for a loop. I have no idea what kernel_stack_pointer() is trying to do. I just assumed it was correct. I'll take a look at it and try to fix it in another patch.

Quoted 24 lines · Andy Lutomirski

Quoted 9 lines · Josh Poimboeuf

When calling show_stack_log_lvl() or dump_trace() with a regs argument, providing a stack pointer or frame pointer is redundant.

arch/x86/kernel/dumpstack_32.c +0 -0

index 358fe1c..c533b8b 100644

arch/x86/kernel/dumpstack_32.c void show_regs(struct pt_regs *regs) · line 125

Quoted 5 lines

arch/x86/kernel/dumpstack_32.c +1 -1

@@ -122,7 +122,7 @@ void show_regs(struct pt_regs *regs)
                u8 *ip;
 
                pr_emerg("Stack:\n");
-               show_stack_log_lvl(NULL, regs, &regs->sp, 0, KERN_EMERG);
+               show_stack_log_lvl(NULL, regs, NULL, 0, KERN_EMERG);

This is weird -- note the &. You're at some risk of exposing a bug in x86_32's kernel_stack_pointer() function, which is a mess. (I don't see why it's written the way it is -- the actual return stack pointer given a pt_regs is quite well defined -- if regs->cs & 3 != 0, then it's regs->sp, else it's &regs->sp.)

That being said, this isn't a big deal, so:

Quoted 4 lines

Reviewed-by Andy Lutomirski <[email protected]>

If you want to make this all a bit more reliably on x86_32, you could fix kernel_stack_pointer().

Ok. The whole '&regs->sp' thing threw me for a loop. I have no idea what kernel_stack_pointer() is trying to do. I just assumed it was correct. I'll take a look at it and try to fix it in another patch.

When function graph tracing is enabled for a function, its return address on the stack is replaced with the address of an ftrace handler (return_to_handler). When dumping the stack of a task with graph tracing enabled, there are some subtle bugs:

- The fake return_to_handler() address can be reported as reliable. Instead, because it's not the real caller, it should be considered unreliable.

- In print_context_stack(), the real caller's return address is always reported as reliable, even if the return_to_handler() address wasn't referred to by a frame pointer.

In addition to fixing these bugs, convert print_ftrace_graph_addr() to a more generic function which can be used outside of dump_trace() callbacks.

Signed-off-by Josh Poimboeuf <[email protected]>

---
 arch/x86/include/asm/stacktrace.h | 13 ++++++++++
 arch/x86/kernel/dumpstack.c       | 50 +++++++++++++++++----------------------
 2 files changed, 35 insertions(+), 28 deletions(-)

arch/x86/include/asm/stacktrace.h +13 -0

index 6f65995..5d3d258 100644
@@ -14,6 +14,19 @@ extern int kstack_depth_to_print;
 struct thread_info;
 struct stacktrace_ops;
 
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+
+unsigned long
+ftrace_graph_ret_addr(struct task_struct *task, int *idx, unsigned long addr);
+
+#else
+static inline unsigned long
+ftrace_graph_ret_addr(struct task_struct *task, int *idx, unsigned long addr)
+{
+	return addr;
+}
+#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
+
 typedef unsigned long (*walk_stack_t)(struct task_struct *task,
 				      unsigned long *stack,
 				      unsigned long bp,

arch/x86/kernel/dumpstack.c +16 -24

index 692eecae..0a8694b 100644
@@ -40,36 +40,25 @@ void printk_address(unsigned long address)
 }
 
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
-static void
-print_ftrace_graph_addr(unsigned long addr, void *data,
-			const struct stacktrace_ops *ops,
-			struct task_struct *task, int *graph)
+unsigned long
+ftrace_graph_ret_addr(struct task_struct *task, int *idx, unsigned long addr)
 {
-	unsigned long ret_addr;
-	int index;
+	int task_idx;
 
 	if (addr != (unsigned long)return_to_handler)
-		return;
+		return addr;
 
-	index = task->curr_ret_stack;
+	task_idx = task->curr_ret_stack;
 
-	if (!task->ret_stack || index < *graph)
-		return;
+	if (!task->ret_stack || task_idx < *idx)
+		return addr;
 
-	index -= *graph;
-	ret_addr = task->ret_stack[index].ret;
+	task_idx -= *idx;
+	(*idx)++;
 
-	ops->address(data, ret_addr, 1);
-
-	(*graph)++;
+	return task->ret_stack[task_idx].ret;
 }
-#else
-static inline void
-print_ftrace_graph_addr(unsigned long addr, void *data,
-			const struct stacktrace_ops *ops,
-			struct task_struct *task, int *graph)
-{ }
-#endif
+#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
 
 /*
  * x86-64 can have up to three kernel stacks:

@@ -108,18 +97,23 @@ print_context_stack(struct task_struct *task,
 		stack = (unsigned long *)task_stack_page(task);
 
 	while (valid_stack_ptr(task, stack, sizeof(*stack), end)) {
-		unsigned long addr;
+		unsigned long addr = *stack;
 
 		addr = *stack;
 		if (__kernel_text_address(addr)) {
+			int reliable = 0;
+			unsigned long real_addr;
+
 			if ((unsigned long) stack == bp + sizeof(long)) {
-				ops->address(data, addr, 1);
+				reliable = 1;

arch/x86/kernel/dumpstack.c +1 -0

+			ops->address(data, real_addr, reliable);
 		}
 		stack++;
 	}

Quoted 8 lines · On Thu, 21 Jul 2016 16:21:42 -0500 Josh Poimboeuf <[email protected]> wrote:

When function graph tracing is enabled for a function, its return address on the stack is replaced with the address of an ftrace handler (return_to_handler). When dumping the stack of a task with graph tracing enabled, there are some subtle bugs:

- The fake return_to_handler() address can be reported as reliable. Instead, because it's not the real caller, it should be considered unreliable.

I have some mixed emotions about this. First, it's not "fake", the function *is* going to return to it, but you are right, that's not the function that was called.

I do like to see these in the trace, because sometimes these functions are an issue. But I guess I can live with them being marked as "unreliable".

Quoted 3 lines

- In print_context_stack(), the real caller's return address is always reported as reliable, even if the return_to_handler() address wasn't referred to by a frame pointer.

Hmm, if CONFIG_FRAME_POINTER is enabled, perhaps we should only call the look up of ftrace_graph_ret_addr(). Hmm, playing with this, yeah, we definitely should. It can report the wrong reliability.

Without doing the reliability check we can get out of sync with the ret_stack. I have a patch to go on top of this patch below (hmm, it may not apply fully, because I was using a different base tree than you).

BTW, it would be really nice if ftrace_graph_ret_addr() were idempotent so we could get the "real" return address without having to pass in a state variable.

For example we could add an "unsigned long *retp" pointer to ftrace_ret_stack, which points to the return address on the stack. Then we could get rid of the index state variable in ftrace_graph_ret_addr, and also then there would never be a chance of the stack dump getting out of sync with the ret_stack.

What do you think?

Quoted 10 lines · On Fri, 29 Jul 2016 19:50:59 -0500 Josh Poimboeuf <[email protected]> wrote:

BTW, it would be really nice if ftrace_graph_ret_addr() were idempotent so we could get the "real" return address without having to pass in a state variable.

For example we could add an "unsigned long *retp" pointer to ftrace_ret_stack, which points to the return address on the stack. Then we could get rid of the index state variable in ftrace_graph_ret_addr, and also then there would never be a chance of the stack dump getting out of sync with the ret_stack.

Quoted 2 lines

What do you think?

I don't want to extend ret_stack as that is allocated 50 of these structures for every task. That said, we have the "fp" field that's used to check for frame pointer corruption when mcount is used. With CC_USING_FENTRY, that field is ignored. Perhaps we could overload that field for this.

Quoted 13 lines · Steven Rostedt

Quoted 10 lines · On Fri, 29 Jul 2016 19:50:59 -0500 Josh Poimboeuf <[email protected]> wrote:

BTW, it would be really nice if ftrace_graph_ret_addr() were idempotent so we could get the "real" return address without having to pass in a state variable.

For example we could add an "unsigned long *retp" pointer to ftrace_ret_stack, which points to the return address on the stack. Then we could get rid of the index state variable in ftrace_graph_ret_addr, and also then there would never be a chance of the stack dump getting out of sync with the ret_stack.

Quoted 8 lines

Quoted 2 lines

What do you think?

I don't want to extend ret_stack as that is allocated 50 of these structures for every task. That said, we have the "fp" field that's used to check for frame pointer corruption when mcount is used. With CC_USING_FENTRY, that field is ignored. Perhaps we could overload that field for this.

In that case, I guess we would need two versions of ftrace_graph_ret_addr(), with the current implementation still needed for mcount+HAVE_FUNCTION_GRAPH_FP_TEST.

Or would you want to get rid of HAVE_FUNCTION_GRAPH_FP_TEST for x86?

BTW, on a different note, should I put ftrace_graph_ret_addr() to kernel/trace/trace_functions_graph.c so other arches can use it?

Quoted 23 lines · On Sat, 30 Jul 2016 08:51:25 -0500 Josh Poimboeuf <[email protected]> wrote:

Quoted 21 lines · Steven Rostedt

Quoted 4 lines · On Fri, 29 Jul 2016 19:50:59 -0500 Josh Poimboeuf <[email protected]> wrote:

BTW, it would be really nice if ftrace_graph_ret_addr() were idempotent so we could get the "real" return address without having to pass in a state variable.

Quoted 8 lines

For example we could add an "unsigned long *retp" pointer to ftrace_ret_stack, which points to the return address on the stack. Then we could get rid of the index state variable in ftrace_graph_ret_addr, and also then there would never be a chance of the stack dump getting out of sync with the ret_stack.

What do you think?

I don't want to extend ret_stack as that is allocated 50 of these structures for every task. That said, we have the "fp" field that's used to check for frame pointer corruption when mcount is used. With CC_USING_FENTRY, that field is ignored. Perhaps we could overload that field for this.

Quoted 3 lines

In that case, I guess we would need two versions of ftrace_graph_ret_addr(), with the current implementation still needed for mcount+HAVE_FUNCTION_GRAPH_FP_TEST.

How hard would it be in that case?

Quoted 1 line

Or would you want to get rid of HAVE_FUNCTION_GRAPH_FP_TEST for x86?

No, because there's gcc versions that we still support that mess up mcount, and could still cause issues with function graph.

Quoted 1 line

Quoted 3 lines

BTW, on a different note, should I put ftrace_graph_ret_addr() to kernel/trace/trace_functions_graph.c so other arches can use it?

I guess you could. There doesn't seem to be any x86 specific code in that right?

Quoted 26 lines · Steven Rostedt

Quoted 23 lines · On Sat, 30 Jul 2016 08:51:25 -0500 Josh Poimboeuf <[email protected]> wrote:

Quoted 21 lines · Steven Rostedt

Quoted 4 lines · On Fri, 29 Jul 2016 19:50:59 -0500 Josh Poimboeuf <[email protected]> wrote:

BTW, it would be really nice if ftrace_graph_ret_addr() were idempotent so we could get the "real" return address without having to pass in a state variable.

Quoted 8 lines

For example we could add an "unsigned long *retp" pointer to ftrace_ret_stack, which points to the return address on the stack. Then we could get rid of the index state variable in ftrace_graph_ret_addr, and also then there would never be a chance of the stack dump getting out of sync with the ret_stack.

What do you think?

I don't want to extend ret_stack as that is allocated 50 of these structures for every task. That said, we have the "fp" field that's used to check for frame pointer corruption when mcount is used. With CC_USING_FENTRY, that field is ignored. Perhaps we could overload that field for this.

Quoted 5 lines

Quoted 3 lines

In that case, I guess we would need two versions of ftrace_graph_ret_addr(), with the current implementation still needed for mcount+HAVE_FUNCTION_GRAPH_FP_TEST.

How hard would it be in that case?

Well, it would be easy enough, but then the caller would still need to pass in the state variable. So maybe it's not worth the trouble.

The ret_stack is out of sync with the stack dump because the stack dump was started with the regs from the NMI, instead of being started from the current frame.

So I guess there are a couple of ways to fix it:

a) keep track of the return address pointer like we discussed above;

or

b) have the unwinder count the # of skipped frames which refer to 'return_to_handler', and pass that as the initial index value to ftrace_graph_ret_addr().

Option a) would be much cleaner. But to fix it for both mcount and fentry, we couldn't override 'fp' so I guess we'd need to add a new field to ftrace_ret_stack.

Option b) is uglier, but I could probably make it work with the new unwinder.

On Tue, 2 Aug 2016 16:00:11 -0500
Josh Poimboeuf <[email protected]> wrote:

   [<ffffffff81061d8b>] nmi_raise_cpu_backtrace+0x1b/0x20

Quoted 11 lines

The ret_stack is out of sync with the stack dump because the stack dump was started with the regs from the NMI, instead of being started from the current frame.

So I guess there are a couple of ways to fix it:

a) keep track of the return address pointer like we discussed above;

or

Quoted 7 lines

b) have the unwinder count the # of skipped frames which refer to 'return_to_handler', and pass that as the initial index value to ftrace_graph_ret_addr().

Option a) would be much cleaner. But to fix it for both mcount and fentry, we couldn't override 'fp' so I guess we'd need to add a new field to ftrace_ret_stack.

Actually, what about calling ftrace_graph_ret_addr() to figure out the next stack conversion only if reliable or CONFIG_FRAME_POINTER is not enabled?

	unsigned long real_addr = addr;

	[...]

	if (!IS_ENABLED(CONFIG_FRAME_POINTER) || reliable)
		real_addr = ftrace_graph_ret_addr(task, graph, addr);
	if (addr != real_addr)
		ops->address(data, addr, 0);
	ops->address(data, real_addr, reliable);

Then we only need the fp use case when FRAME_POINTER is not set. As mcount forces FRAME_POINTER, we only need to worry about the fentry case.

Quoted 3 lines

Then we only need the fp use case when FRAME_POINTER is not set. As mcount forces FRAME_POINTER, we only need to worry about the fentry case.

Hm, I'm confused. First, I don't see where mcount forces FRAME_POINTER.

Second, I don't see why that even matters. If mcount and frame pointers are enabled, then the 'fp' field of ftrace_ret_stack is needed for the gcc sanity check, right? So we couldn't override 'fp', and the old "stateful index" version of ftrace_graph_ret_addr() would have to be used in the code above for reliable addresses, and we'd still have the same out-of-sync bug.

Or am I missing something?

Quoted 5 lines · On Tue, 2 Aug 2016 17:13:59 -0500 Josh Poimboeuf <[email protected]> wrote:

Quoted 3 lines

Then we only need the fp use case when FRAME_POINTER is not set. As mcount forces FRAME_POINTER, we only need to worry about the fentry case.

Hm, I'm confused. First, I don't see where mcount forces FRAME_POINTER.

Hmm, we should probably force it generally, as gcc itself requires mcount to be used with framepointers. -mcount can't be used without them.

Quoted 10 lines

Second, I don't see why that even matters. If mcount and frame pointers are enabled, then the 'fp' field of ftrace_ret_stack is needed for the gcc sanity check, right? So we couldn't override 'fp', and the old "stateful index" version of ftrace_graph_ret_addr() would have to be used in the code above for reliable addresses, and we'd still have the same out-of-sync bug.

Or am I missing something?

Or I missed something. How did we get out of sync? If we have frame pointers, shouldn't the "return_to_handler" be seen as reliable by the code (not that we save it as such)? That is, if the frame pointer shows that the next function is return_to_handler, then we increment the index into ret_stack, otherwise we simply record the return_to_handler as a normal "unreliable" function, without any processing of it.

I guess I don't actually understand how the NMI screwed it up, as function graph doesn't trace "do_nmi()" itself nor anything before that. I'm guessing it really got out of sync because there's a "return_to_handler" in the stack that wasn't really called (not a frame pointer). The ftrace_graph_ret_addr() will shift the index currently regardless if the return_to_handler found is part of a stack frame, or just left over in the stack. THAT is why I think it got out of sync.

It's not specific to NMIs. The problem is that dump_trace() is starting from the frame pointed to by a pt_regs, rather than the current frame. Instead of starting with the current frame, the first 10 functions on the stack are skipped by the unwinder, but they're *not* skipped on the ret_stack. So it starts out out-of-sync.

Quoted 5 lines · On Tue, 2 Aug 2016 20:56:56 -0500 Josh Poimboeuf <[email protected]> wrote:

It's not specific to NMIs. The problem is that dump_trace() is starting from the frame pointed to by a pt_regs, rather than the current frame. Instead of starting with the current frame, the first 10 functions on the stack are skipped by the unwinder, but they're *not* skipped on the ret_stack. So it starts out out-of-sync.

OK, I see what you mean. If we do a dumpstack from interrupt passing in the pt_regs of the kernel thread that was interrupted, even though functions up to the interrupt was called and traced, which will show up in the dump stack that shouldn't.

OK, you convinced me. Add the extra pointer, then we will have 4 longs and 2 long longs in ftrace_ret_stack. That's not that bad.

include/linux/ftrace.h +1 -0

 	unsigned long fp;
+#endif
 };
 
 /*

kernel/trace/trace_functions_graph.c +2 -0

index 9caa9b2..86b2719 100644
@@ -171,7 +171,9 @@ ftrace_push_return_trace(unsigned long ret, unsigned long func, int *depth,
 	current->ret_stack[index].func = func;
 	current->ret_stack[index].calltime = calltime;
 	current->ret_stack[index].subtime = 0;
+#if defined(CONFIG_HAVE_FUNCTION_GRAPH_FP_TEST) && !defined(CC_USING_FENTRY)
 	current->ret_stack[index].fp = frame_pointer;
+#endif
 	*depth = current->curr_ret_stack;
 
 	return 0;

include/linux/ftrace.h struct ftrace_ret_stack { · line 798

Quoted 9 lines · On Tue, 2 Aug 2016 21:50:12 -0500 Josh Poimboeuf <[email protected]> wrote:

include/linux/ftrace.h +1 -0

index 1e814ae..fc508a7 100644
@@ -795,7 +795,9 @@ struct ftrace_ret_stack {
 	unsigned long func;
 	unsigned long long calltime;
 	unsigned long long subtime;
+#if defined(CONFIG_HAVE_FUNCTION_GRAPH_FP_TEST) && !defined(CC_USING_FENTRY)

We need to make a new defined in ftrace.h:

#if defined(CONFIG_HAVE_FUNCTION_GRAPH_FP_TEST) && !defined(CC_USING_FENTRY)

# define HAVE_FUNCTION_GRAPH_FP_TEST
#endif

And use that instead of this && complexity.

Or better yet, get rid of the CONFIG_HAVE_FUNCTION_GRAPH_FP_TEST define and only have HAVE_FUNCTION_GRAPH_FP_TEST defined in the asm/ftrace.h in each arch. Then, x86 could just do;

#ifndef CC_USING_FENTRY

# define HAVE_FUNCTION_GRAPH_FP_TEST
#endif

Sounds good. I was thinking I could also add a similar define to indicate whether an arch passes the return address stack pointer to ftrace_push_return_trace(). HAVE_FUNCTION_GRAPH_RET_ADDR_PTR?

On Tue, 2 Aug 2016 22:12:33 -0500
Josh Poimboeuf <[email protected]> wrote:

Quoted 4 lines

Sounds good. I was thinking I could also add a similar define to indicate whether an arch passes the return address stack pointer to ftrace_push_return_trace(). HAVE_FUNCTION_GRAPH_RET_ADDR_PTR?

If you are making this function global, might as well make all pass that pointer when you do the conversion. I don't think we need a define to differentiate it.

Quoted 4 lines · On Tue, 2 Aug 2016 23:18:57 -0400 Steven Rostedt <[email protected]> wrote:

On Tue, 2 Aug 2016 22:12:33 -0500
Josh Poimboeuf <[email protected]> wrote:

Quoted 9 lines

Quoted 4 lines

Sounds good. I was thinking I could also add a similar define to indicate whether an arch passes the return address stack pointer to ftrace_push_return_trace(). HAVE_FUNCTION_GRAPH_RET_ADDR_PTR?

If you are making this function global, might as well make all pass that pointer when you do the conversion. I don't think we need a define to differentiate it.

Bah, I was thinking of your ftrace_graph_ret_addr() function. /me needs to go to bed.

Anyway, if we have to add a parameter, we probably need to update all the callers anyway. We do need to add a parameter for this, right?

Yeah, we do need to add a parameter to ftrace_push_return_trace(). But callers which don't implement it could just pass zero like they do with 'fp'.

Quoted 4 lines · On Tue, 2 Aug 2016 22:31:25 -0500 Josh Poimboeuf <[email protected]> wrote:

Yeah, we do need to add a parameter to ftrace_push_return_trace(). But callers which don't implement it could just pass zero like they do with 'fp'.

Right, if zero is passed in, then just ignore it.

On Tue, 2 Aug 2016 22:12:33 -0500
Josh Poimboeuf <[email protected]> wrote:

Quoted 4 lines

Sounds good. I was thinking I could also add a similar define to indicate whether an arch passes the return address stack pointer to ftrace_push_return_trace(). HAVE_FUNCTION_GRAPH_RET_ADDR_PTR?

If you are making this function global, might as well make all pass that pointer when you do the conversion. I don't think we need a define to differentiate it.

Here's my patch that should be applied on top.

Maybe add a Signed-off-by: Steven Rostedt <[email protected]> along with your SOB. But you should remain Author.

arch/x86/kernel/dumpstack.c +1 -0

@@ -123,13 +124,16 @@ print_context_stack_bp(struct task_struc
 
 	while (valid_stack_ptr(task, ret_addr, sizeof(*ret_addr), end)) {
 		unsigned long addr = *ret_addr;
+		unsigned long real_addr;
 
 		if (!__kernel_text_address(addr))
 			break;
 

Quoted 4 lines · Steven Rostedt

Here's my patch that should be applied on top.

Maybe add a Signed-off-by: Steven Rostedt <[email protected]> along with your SOB. But you should remain Author.

[...]

Quoted 9 lines

arch/x86/kernel/dumpstack.c +1 -0

@@ -123,13 +124,16 @@ print_context_stack_bp(struct task_struc
 
 	while (valid_stack_ptr(task, ret_addr, sizeof(*ret_addr), end)) {
 		unsigned long addr = *ret_addr;
+		unsigned long real_addr;
 
 		if (!__kernel_text_address(addr))
 			break;
 

arch/x86/kernel/dumpstack.c print_context_stack_bp(struct task_struc · line 139

Quoted 10 lines

arch/x86/kernel/dumpstack.c +4 -2

-		addr = ftrace_graph_ret_addr(task, graph, addr);
-		if (ops->address(data, addr, 1))
+		real_addr = ftrace_graph_ret_addr(task, graph, addr);
+		if (ops->address(data, real_addr, 1))
 			break;
+		if (real_addr != addr)
+			ops->address(data, addr, 0);
 		frame = frame->next_frame;
 		ret_addr = &frame->return_address;
 	}  

Actually this hunk isn't needed because all users of print_context_stack_bp() only care about "reliable" addresses. With frame pointers enabled, the only place "unreliable" addresses are used is in show_trace_log_lvl() -- and it uses the print_context_stack() callback.

I rely on that fact in the new frame pointer unwind code: it only reports reliable addresses.

Quoted 34 lines · On Mon, 1 Aug 2016 10:59:03 -0500 Josh Poimboeuf <[email protected]> wrote:

Quoted 4 lines · Steven Rostedt

Here's my patch that should be applied on top.

Maybe add a Signed-off-by: Steven Rostedt <[email protected]> along with your SOB. But you should remain Author.

[...]

Quoted 9 lines

arch/x86/kernel/dumpstack.c +1 -0

@@ -123,13 +124,16 @@ print_context_stack_bp(struct task_struc
 
 	while (valid_stack_ptr(task, ret_addr, sizeof(*ret_addr), end)) {
 		unsigned long addr = *ret_addr;
+		unsigned long real_addr;
 
 		if (!__kernel_text_address(addr))
 			break;
 

arch/x86/kernel/dumpstack.c print_context_stack_bp(struct task_struc · line 139

Quoted 10 lines

arch/x86/kernel/dumpstack.c +4 -2

-		addr = ftrace_graph_ret_addr(task, graph, addr);
-		if (ops->address(data, addr, 1))
+		real_addr = ftrace_graph_ret_addr(task, graph, addr);
+		if (ops->address(data, real_addr, 1))
 			break;
+		if (real_addr != addr)
+			ops->address(data, addr, 0);
 		frame = frame->next_frame;
 		ret_addr = &frame->return_address;
 	}  

Actually this hunk isn't needed because all users of print_context_stack_bp() only care about "reliable" addresses. With frame pointers enabled, the only place "unreliable" addresses are used is in show_trace_log_lvl() -- and it uses the print_context_stack() callback.

Quoted 3 lines

I rely on that fact in the new frame pointer unwind code: it only reports reliable addresses.

Can you make this a separate patch then. Before this one, and explain why it isn't needed in the change log. I rather have the current patch not make such a change in logic.

Quoted 21 lines

(quoted diff) +5 -5

@@ -108,18 +97,23 @@ print_context_stack(struct task_struct *task,
 		stack = (unsigned long *)task_stack_page(task);
 
 	while (valid_stack_ptr(task, stack, sizeof(*stack), end)) {
-		unsigned long addr;
+		unsigned long addr = *stack;
 
 		addr = *stack;
 		if (__kernel_text_address(addr)) {
+			int reliable = 0;
+			unsigned long real_addr;
+
 			if ((unsigned long) stack == bp + sizeof(long)) {
-				ops->address(data, addr, 1);
+				reliable = 1;
 				frame = frame->next_frame;
 				bp = (unsigned long) frame;
-			} else {
-				ops->address(data, addr, 0);
 			}
-			print_ftrace_graph_addr(addr, data, ops, task, graph);

Review print_context_stack(struct task_struct *task, · line 116

Quoted 5 lines

(quoted diff) +5 -0

+
+			real_addr = ftrace_graph_ret_addr(task, graph, addr);
+			if (addr != real_addr)
+				ops->address(data, addr, 0);
+			ops->address(data, real_addr, reliable);

Note this changes behavior, as the original code had the ret_to_handler first. This makes it second. (I fixed this below).

Quoted 9 lines

Review

Quoted 6 lines

(quoted diff) +5 -1

-			print_ftrace_graph_addr(addr, data, ops, task, graph);
+
+			real_addr = ftrace_graph_ret_addr(task, graph, addr);
+			if (addr != real_addr)
+				ops->address(data, addr, 0);
+			ops->address(data, real_addr, reliable);  

Note this changes behavior, as the original code had the ret_to_handler first. This makes it second. (I fixed this below).

Hm, as far as I can tell this actually keeps the original behavior. The "unreliable" ret_to_handler is still printed first, no?

For reasons unknown, the x86_64 irq stack starts at an offset 64 bytes from the end of the page. At least make that explicit.

FIXME: Can we just remove the 64 byte gap? If not, at least document why.

Quoted 2 lines · Josh Poimboeuf

For reasons unknown, the x86_64 irq stack starts at an offset 64 bytes from the end of the page. At least make that explicit.

This is a change in behavior -- see below. Please mention this in the changelog.

Quoted 4 lines

FIXME: Can we just remove the 64 byte gap? If not, at least document why.

I have no clue.

Quoted 1 line

arch/x86/kernel/dumpstack_64.c show_stack_log_lvl(struct task_struct *task, struct pt_regs *regs, · line 247

Quoted 4 lines

        irq_stack_end   = (unsigned long *)(per_cpu(irq_stack_ptr, cpu));

arch/x86/kernel/dumpstack_64.c +2 -1

-       irq_stack       = (unsigned long *)(per_cpu(irq_stack_ptr, cpu) - IRQ_STACK_SIZE);
+       irq_stack       = (unsigned long *)(per_cpu(irq_stack_ptr, cpu) -
+                         IRQ_USABLE_STACK_SIZE);

This is different.

Quoted 6 lines

arch/x86/kernel/dumpstack_64.c show_stack_log_lvl(struct task_struct *task, struct pt_regs *regs, · line 247

Quoted 4 lines

        irq_stack_end   = (unsigned long *)(per_cpu(irq_stack_ptr, cpu));

arch/x86/kernel/dumpstack_64.c +2 -1

-       irq_stack       = (unsigned long *)(per_cpu(irq_stack_ptr, cpu) - IRQ_STACK_SIZE);
+       irq_stack       = (unsigned long *)(per_cpu(irq_stack_ptr, cpu) -
+                         IRQ_USABLE_STACK_SIZE);

This is different.

If nobody knows the reason for it, I may just remove it. It doesn't seem to blow anything up on my system. I tried digging through the git history but it's been there since the beginning of git time.

in_exception_stack() does some bad, bad things just so the unwinder can print different values for different areas of the debug exception stack.

There's no need to clarify where exactly on the stack it is. Just print "#DB" and be done with it.

valid_stack_ptr() is buggy: it assumes that all stacks are of size THREAD_SIZE, which is not true for exception stacks. So the walk_stack() callbacks will need to know the location of the beginning of the stack as well as the end.

Another issue is that in general the various features of a stack (type, size, next stack pointer, description string) are scattered around in various places throughout the stack dump code.

arch/x86/kernel/dumpstack_32.c +6 -0

+static bool in_hardirq_stack(unsigned long *stack, struct stack_info *info,
+                            unsigned long *visit_mask)
+{
+       unsigned long *begin = (unsigned long *)this_cpu_read(hardirq_stack);
+       unsigned long *end   = begin + (THREAD_SIZE / sizeof(long));
+

arch/x86/kernel/dumpstack_64.c +2 -1

+static bool in_exception_stack(unsigned long *s, struct stack_info *info,
+                              unsigned long *visit_mask)
 {
        unsigned long stack = (unsigned long)s;
        unsigned long begin, end;

@@ -44,55 +63,62 @@ static unsigned long *in_exception_stack(unsigned long *s, char **name,
                if (stack < begin || stack >= end)
                        continue;
 
-               if (test_and_set_bit(k, visit_mask))

Quoted 5 lines · Josh Poimboeuf

valid_stack_ptr() is buggy: it assumes that all stacks are of size THREAD_SIZE, which is not true for exception stacks. So the walk_stack() callbacks will need to know the location of the beginning of the stack as well as the end.

Quoted 3 lines

Another issue is that in general the various features of a stack (type, size, next stack pointer, description string) are scattered around in various places throughout the stack dump code.

I finally figured out what visit_info is. But would it make more sense to track it in the unwind state so that the unwinder can directly make sure it doesn't start looping?

And please remove test_and_set_bit() -- it's pointlessly slow.

Quoted 6 lines

arch/x86/kernel/dumpstack_32.c +6 -0

+static bool in_hardirq_stack(unsigned long *stack, struct stack_info *info,
+                            unsigned long *visit_mask)
+{
+       unsigned long *begin = (unsigned long *)this_cpu_read(hardirq_stack);
+       unsigned long *end   = begin + (THREAD_SIZE / sizeof(long));
+

arch/x86/kernel/dumpstack_32.c line 48

Quoted 10 lines

arch/x86/kernel/dumpstack_32.c +10 -0

+       if (stack < begin || stack >= end)
+               return false;
+
+       if (visit_mask && test_and_set_bit(STACK_TYPE_IRQ, visit_mask))
+               return false;
+
+       info->type      = STACK_TYPE_IRQ;
+       info->begin     = begin;
+       info->end       = end;
+       info->next      = (unsigned long *)*begin;

This works, but it's a bit magic. I don't suppose we could get rid of this ->next thing entirely and teach show_stack_log_lvl(), etc. to move from stack to stack by querying the stack type of whatever the frame base address is if the frame base address ends up being out of bounds for the current stack? Or maybe the unwinder could even do this by itself.

Quoted 10 lines

arch/x86/kernel/dumpstack_64.c +2 -1

+static bool in_exception_stack(unsigned long *s, struct stack_info *info,
+                              unsigned long *visit_mask)
 {
        unsigned long stack = (unsigned long)s;
        unsigned long begin, end;

@@ -44,55 +63,62 @@ static unsigned long *in_exception_stack(unsigned long *s, char **name,
                if (stack < begin || stack >= end)
                        continue;
 
-               if (test_and_set_bit(k, visit_mask))

arch/x86/kernel/dumpstack_64.c static unsigned long *in_exception_stack(unsigned long *s, char **name, · line 73

Quoted 10 lines

arch/x86/kernel/dumpstack_64.c +6 -2

+               if (visit_mask &&
+                   test_and_set_bit(STACK_TYPE_EXCEPTION + k, visit_mask))
                        return false;
 
-               *name = exception_stack_names[k];
-               return (unsigned long *)end;
+               info->type      = STACK_TYPE_EXCEPTION + k;
+               info->begin     = (unsigned long *)begin;
+               info->end       = (unsigned long *)end;
+               info->next      = (unsigned long *)info->end[-2];

This is so magical that I don't immediately see why it's correct. Presumably it's because the thing two slots down from the top of the stack is regs->sp? If so, that needs a comment.

But again, couldn't we use the fact that we now know how to decode pt_regs to avoid needing this? I can imagine it being useful as a fallback in the event that the unwinder fails, but this is just a fallback. Also, NMI is weird and I'm wondering whether this works at all when trying to unwind from a looped NMI.

Fixing this up could be a followup after this series is in, I think -- you're preserving existing behavior AFAICS. I just don't particularly like the existing behavior.

FWIW, I think this code needs to be explicitly tested for the 32-bit double fault case. It's highly magical.

Quoted 6 lines · Andy Lutomirski

Quoted 5 lines · Josh Poimboeuf

valid_stack_ptr() is buggy: it assumes that all stacks are of size THREAD_SIZE, which is not true for exception stacks. So the walk_stack() callbacks will need to know the location of the beginning of the stack as well as the end.

Quoted 8 lines

Quoted 3 lines

Another issue is that in general the various features of a stack (type, size, next stack pointer, description string) are scattered around in various places throughout the stack dump code.

I finally figured out what visit_info is. But would it make more sense to track it in the unwind state so that the unwinder can directly make sure it doesn't start looping?

I just realized that it *is* in the unwind state. But maybe this code in update_stack_state:

    sp = info->next;
    if (!sp).
        goto unknown;

    if (get_stack_info(sp, state->task, info, &state->stack_mask))
        goto unknown;

    if (!on_stack(info, addr, len))
        goto unknown;

should do something like:

if (get_stack_info(addr, ...))
goto unknown.

sp = info->end;

instead. Alternatively, maybe it would make sense to keep sp as is (have update_stack_state return bool instead of returning a pointer) so that a frame that switches stacks still shows the actual sp at the time that the frame called whatever the it called.

I'm really quite confused by what state->sp means in your current code. In the non-stack-switching case (everything is on the thread stack), it appears to always match state->bp. Am I missing something?
If I'm understanding this correctly, when you're pointing at a call
frame, state->bp is that frame's base address (the top of the stack frame), unwind_get_return_address() returns the address to which that frame would return, and, in the future, unwind_get_gpr(UNWIND_DI) or whatever it ends up looking like will return RDI at the time that the frame called whatever function it called, if known. By that logic, shouldn't state->sp be sp on entry to the call instruction? (Or could sp just be removed? Does it do anything?)

I guess the reason I'm still not 100% comfortable with the idea that pt_regs frames don't exist a real frames is that I keep waffling as to how I should think about the regs associated with a frame in the future DWARF world. I think I imagine them being the regs at the time that the frame did it's call to the next frame, which, by an admittedly weak analogy, means that the pt_regs state would be the regs at the time that the exception or interrupt happened. That offers a third silly option for dealing with the annoying '?': emit two frames for a pt_regs, but have the frame in the entry code show NULL for its return address because it's not a normal return.

Quoted 5 lines · Josh Poimboeuf

valid_stack_ptr() is buggy: it assumes that all stacks are of size THREAD_SIZE, which is not true for exception stacks. So the walk_stack() callbacks will need to know the location of the beginning of the stack as well as the end.

Quoted 3 lines

Another issue is that in general the various features of a stack (type, size, next stack pointer, description string) are scattered around in various places throughout the stack dump code.

I finally figured out what visit_info is. But would it make more sense to track it in the unwind state so that the unwinder can directly make sure it doesn't start looping?

And please remove test_and_set_bit() -- it's pointlessly slow.

Quoted 16 lines

arch/x86/kernel/dumpstack_32.c +15 -0

+static bool in_hardirq_stack(unsigned long *stack, struct stack_info *info,
+                            unsigned long *visit_mask)
+{
+       unsigned long *begin = (unsigned long *)this_cpu_read(hardirq_stack);
+       unsigned long *end   = begin + (THREAD_SIZE / sizeof(long));
+
+       if (stack < begin || stack >= end)
+               return false;
+
+       if (visit_mask && test_and_set_bit(STACK_TYPE_IRQ, visit_mask))
+               return false;
+
+       info->type      = STACK_TYPE_IRQ;
+       info->begin     = begin;
+       info->end       = end;

Quoted 1 line

+ info->next = (unsigned long *)*begin;

This works, but it's a bit magic. I don't suppose we could get rid of this ->next thing entirely and teach show_stack_log_lvl(), etc. to move from stack to stack by querying the stack type of whatever the frame base address is if the frame base address ends up being out of bounds for the current stack? Or maybe the unwinder could even do this by itself.

Quoted 15 lines

arch/x86/kernel/dumpstack_64.c +4 -1

+static bool in_exception_stack(unsigned long *s, struct stack_info *info,
+                              unsigned long *visit_mask)
 {
        unsigned long stack = (unsigned long)s;
        unsigned long begin, end;

@@ -44,55 +63,62 @@ static unsigned long *in_exception_stack(unsigned long *s, char **name,
                if (stack < begin || stack >= end)
                        continue;
 
-               if (test_and_set_bit(k, visit_mask))
+               if (visit_mask &&
+                   test_and_set_bit(STACK_TYPE_EXCEPTION + k, visit_mask))
                        return false;
 

arch/x86/kernel/dumpstack_64.c static unsigned long *in_exception_stack(unsigned long *s, char **name, · line 73

Quoted 6 lines

arch/x86/kernel/dumpstack_64.c +4 -2

-               *name = exception_stack_names[k];
-               return (unsigned long *)end;
+               info->type      = STACK_TYPE_EXCEPTION + k;
+               info->begin     = (unsigned long *)begin;
+               info->end       = (unsigned long *)end;
+               info->next      = (unsigned long *)info->end[-2];

This is so magical that I don't immediately see why it's correct. Presumably it's because the thing two slots down from the top of the stack is regs->sp? If so, that needs a comment.

But again, couldn't we use the fact that we now know how to decode pt_regs to avoid needing this? I can imagine it being useful as a fallback in the event that the unwinder fails, but this is just a fallback.

Also, NMI is weird and I'm wondering whether this works at all when trying to unwind from a looped NMI.

Fixing this up could be a followup after this series is in, I think -- you're preserving existing behavior AFAICS. I just don't particularly like the existing behavior.

FWIW, I think this code needs to be explicitly tested for the 32-bit double fault case. It's highly magical.

Quoted 15 lines

Quoted 15 lines

arch/x86/kernel/dumpstack_32.c +15 -0

+static bool in_hardirq_stack(unsigned long *stack, struct stack_info *info,
+                            unsigned long *visit_mask)
+{
+       unsigned long *begin = (unsigned long *)this_cpu_read(hardirq_stack);
+       unsigned long *end   = begin + (THREAD_SIZE / sizeof(long));
+
+       if (stack < begin || stack >= end)
+               return false;
+
+       if (visit_mask && test_and_set_bit(STACK_TYPE_IRQ, visit_mask))
+               return false;
+
+       info->type      = STACK_TYPE_IRQ;
+       info->begin     = begin;
+       info->end       = end;

Quoted 8 lines

Quoted 1 line

+ info->next = (unsigned long *)*begin;

This works, but it's a bit magic. I don't suppose we could get rid of this ->next thing entirely and teach show_stack_log_lvl(), etc. to move from stack to stack by querying the stack type of whatever the frame base address is if the frame base address ends up being out of bounds for the current stack? Or maybe the unwinder could even do this by itself.

I'm not quite sure what you mean here. The ->next stack pointer is quite useful and it abstracts that ugliness away from the callers of get_stack_info(). I'm open to any specific suggestions.

Quoted 15 lines

Quoted 15 lines

arch/x86/kernel/dumpstack_64.c +4 -1

+static bool in_exception_stack(unsigned long *s, struct stack_info *info,
+                              unsigned long *visit_mask)
 {
        unsigned long stack = (unsigned long)s;
        unsigned long begin, end;

@@ -44,55 +63,62 @@ static unsigned long *in_exception_stack(unsigned long *s, char **name,
                if (stack < begin || stack >= end)
                        continue;
 
-               if (test_and_set_bit(k, visit_mask))
+               if (visit_mask &&
+                   test_and_set_bit(STACK_TYPE_EXCEPTION + k, visit_mask))
                        return false;
 

Quoted 10 lines

arch/x86/kernel/dumpstack_64.c static unsigned long *in_exception_stack(unsigned long *s, char **name, · line 73

Quoted 6 lines

arch/x86/kernel/dumpstack_64.c +4 -2

-               *name = exception_stack_names[k];
-               return (unsigned long *)end;
+               info->type      = STACK_TYPE_EXCEPTION + k;
+               info->begin     = (unsigned long *)begin;
+               info->end       = (unsigned long *)end;
+               info->next      = (unsigned long *)info->end[-2];

This is so magical that I don't immediately see why it's correct. Presumably it's because the thing two slots down from the top of the stack is regs->sp? If so, that needs a comment.

Heck if I know, I just stole it from dump_trace() ;-)

I'll figure it out and add a comment.

Quoted 5 lines

But again, couldn't we use the fact that we now know how to decode pt_regs to avoid needing this? I can imagine it being useful as a fallback in the event that the unwinder fails, but this is just a fallback.

Yeah, this is needed as a fallback. But I wouldn't call it "just" a
fallback: the stack dump code *needs* to be able to still traverse the stacks if frame pointers fail.

Quoted 2 lines

Also, NMI is weird and I'm wondering whether this works at all when trying to unwind from a looped NMI.

Unless I'm missing something, I think it should be fine for nested NMIs, since they're all on the same stack. I can try to test it. What in particular are you worried about?

Quoted 24 lines · Josh Poimboeuf

Quoted 15 lines

Quoted 15 lines

arch/x86/kernel/dumpstack_32.c +15 -0

+static bool in_hardirq_stack(unsigned long *stack, struct stack_info *info,
+                            unsigned long *visit_mask)
+{
+       unsigned long *begin = (unsigned long *)this_cpu_read(hardirq_stack);
+       unsigned long *end   = begin + (THREAD_SIZE / sizeof(long));
+
+       if (stack < begin || stack >= end)
+               return false;
+
+       if (visit_mask && test_and_set_bit(STACK_TYPE_IRQ, visit_mask))
+               return false;
+
+       info->type      = STACK_TYPE_IRQ;
+       info->begin     = begin;
+       info->end       = end;

Quoted 8 lines

Quoted 1 line

+ info->next = (unsigned long *)*begin;

This works, but it's a bit magic. I don't suppose we could get rid of this ->next thing entirely and teach show_stack_log_lvl(), etc. to move from stack to stack by querying the stack type of whatever the frame base address is if the frame base address ends up being out of bounds for the current stack? Or maybe the unwinder could even do this by itself.

Quoted 3 lines

I'm not quite sure what you mean here. The ->next stack pointer is quite useful and it abstracts that ugliness away from the callers of get_stack_info(). I'm open to any specific suggestions.

So far I've found two users of this thing. One is show_stack_log_lvl(), and it makes sense there, but maybe info->heuristic_next_stack would be a better name. The other is the unwinder itself, and I think that walking from stack to stack using this heuristic is the wrong approach there, at least in the long term. I'd rather we just followed the bp chain wherever it leads us, as long as it leads us to a valid stack that we haven't visited before.

As a concrete example of what I think is wrong with the current approach, ISTM it would be totally valid to implement stack switching like this:

some_func:
push %rbp
mov %rsp, %rbp
...
mov [next stack], %rsp
call some_other_func
mov %rbp, %rsp
pop %rbp
ret

With the current approach, you can't unwind out of that function, because there's no way to populate info->next. I'm not actually suggesting that the kernel should ever do such a thing on x86, and my proposed rewrite of the IRQ stack code [1] will be fully compatible with your approach, but it seems odd to me that the unwinder should depend on idea that the stacks in use are chained together in a way that can be decoded without . (But maybe some of the Go compilers do work this way -- I've never looked at their precise stack layout.)

Also, if you ever intend to port this thing to other architectures, I think there are architectures that have separate exception stacks and that track the next available slot on those stacks dynamically. I think that x86_32 is an example of this if task gates are used in a back-and-forth manner, although Linux doesn't do that. (x86_64 should have done this for IST, but it didn't.) On those architectures, you can have two separate switches onto the same stack live at the same time, and your current approach won't work. (Even if you make the change I'm suggesting, visit_mask will break, too, but fixing that would be a much less invasive change.)

Am I making any sense? This is a suggestion for making it better, not something I see as a requirement for getting the x86 code upstream.

Quoted 27 lines

Quoted 14 lines

Quoted 14 lines

arch/x86/kernel/dumpstack_64.c +4 -1

+static bool in_exception_stack(unsigned long *s, struct stack_info *info,
+                              unsigned long *visit_mask)
 {
        unsigned long stack = (unsigned long)s;
        unsigned long begin, end;

@@ -44,55 +63,62 @@ static unsigned long *in_exception_stack(unsigned long *s, char **name,
                if (stack < begin || stack >= end)
                        continue;
 
-               if (test_and_set_bit(k, visit_mask))
+               if (visit_mask &&
+                   test_and_set_bit(STACK_TYPE_EXCEPTION + k, visit_mask))
                        return false;
 

Quoted 10 lines

arch/x86/kernel/dumpstack_64.c static unsigned long *in_exception_stack(unsigned long *s, char **name, · line 73

Quoted 6 lines

arch/x86/kernel/dumpstack_64.c +4 -2

-               *name = exception_stack_names[k];
-               return (unsigned long *)end;
+               info->type      = STACK_TYPE_EXCEPTION + k;
+               info->begin     = (unsigned long *)begin;
+               info->end       = (unsigned long *)end;
+               info->next      = (unsigned long *)info->end[-2];

This is so magical that I don't immediately see why it's correct. Presumably it's because the thing two slots down from the top of the stack is regs->sp? If so, that needs a comment.

Heck if I know, I just stole it from dump_trace() ;-)

Quoted 1 line

I'll figure it out and add a comment.

If you can write it as:

struct pt_regs *regs = (struct pt_regs *)end - 1;
info->next = regs->sp;

and it still works, then no comment required :)

Quoted 9 lines

Quoted 4 lines

But again, couldn't we use the fact that we now know how to decode pt_regs to avoid needing this? I can imagine it being useful as a fallback in the event that the unwinder fails, but this is just a fallback.

Yeah, this is needed as a fallback. But I wouldn't call it "just" a
fallback: the stack dump code *needs* to be able to still traverse the stacks if frame pointers fail.

Quoted 7 lines

Quoted 2 lines

Also, NMI is weird and I'm wondering whether this works at all when trying to unwind from a looped NMI.

Unless I'm missing something, I think it should be fine for nested NMIs, since they're all on the same stack. I can try to test it. What in particular are you worried about?

The top of the NMI frame contains no less than *three* (SS, SP, FLAGS, CS, IP) records. Off the top of my head, the record that matters is the third one, so it should be regs[-15]. If an MCE hits while this mess is being set up, good luck unwinding *that*. If you really want to know, take a deep breath, read the long comment in entry_64.S after .Lnmi_from_kernel, then give up on x86 and start hacking on some other architecture.

Quoted 51 lines · Andy Lutomirski

Quoted 24 lines · Josh Poimboeuf

Quoted 15 lines

Quoted 15 lines

arch/x86/kernel/dumpstack_32.c +15 -0

+static bool in_hardirq_stack(unsigned long *stack, struct stack_info *info,
+                            unsigned long *visit_mask)
+{
+       unsigned long *begin = (unsigned long *)this_cpu_read(hardirq_stack);
+       unsigned long *end   = begin + (THREAD_SIZE / sizeof(long));
+
+       if (stack < begin || stack >= end)
+               return false;
+
+       if (visit_mask && test_and_set_bit(STACK_TYPE_IRQ, visit_mask))
+               return false;
+
+       info->type      = STACK_TYPE_IRQ;
+       info->begin     = begin;
+       info->end       = end;

Quoted 8 lines

Quoted 1 line

+ info->next = (unsigned long *)*begin;

This works, but it's a bit magic. I don't suppose we could get rid of this ->next thing entirely and teach show_stack_log_lvl(), etc. to move from stack to stack by querying the stack type of whatever the frame base address is if the frame base address ends up being out of bounds for the current stack? Or maybe the unwinder could even do this by itself.

Quoted 3 lines

I'm not quite sure what you mean here. The ->next stack pointer is quite useful and it abstracts that ugliness away from the callers of get_stack_info(). I'm open to any specific suggestions.

So far I've found two users of this thing. One is show_stack_log_lvl(), and it makes sense there, but maybe info->heuristic_next_stack would be a better name. The other is the unwinder itself, and I think that walking from stack to stack using this heuristic is the wrong approach there, at least in the long term. I'd rather we just followed the bp chain wherever it leads us, as long as it leads us to a valid stack that we haven't visited before.

As a concrete example of what I think is wrong with the current approach, ISTM it would be totally valid to implement stack switching like this:

some_func:
push %rbp
mov %rsp, %rbp
...
mov [next stack], %rsp
call some_other_func
mov %rbp, %rsp
pop %rbp
ret

Quoted 8 lines

With the current approach, you can't unwind out of that function, because there's no way to populate info->next. I'm not actually suggesting that the kernel should ever do such a thing on x86, and my proposed rewrite of the IRQ stack code [1] will be fully compatible with your approach, but it seems odd to me that the unwinder should depend on idea that the stacks in use are chained together in a way that can be decoded without . (But maybe some of the Go compilers do work this way -- I've never looked at their precise stack layout.)

I don't think relying on frame pointers to switch between stacks is necessarily a good idea:

- It requires CONFIG_FRAME_POINTER, which makes it unwinder-specific. The current approach is unwinder-agnostic.

- Instead of relying on a single correct "next stack" pointer, it requires relying on potentially dozens of correct frame pointers, across multiple stacks. So a lot of things have to go right, instead of just one. And then show_trace_log_lvl() becomes more dependent on the unwinder not screwing things up.

Quoted 7 lines

If you can write it as:

struct pt_regs *regs = (struct pt_regs *)end - 1;
info->next = regs->sp;

and it still works, then no comment required :)

Yeah. in_irq_stack() does something similar, though it uses end[-1]. And its regs are actually stored on the thread stack. So something doesn't quite add up for irqs. I still need to do some homework there.

Quoted 6 lines

Quoted 5 lines

Unless I'm missing something, I think it should be fine for nested NMIs, since they're all on the same stack. I can try to test it. What in particular are you worried about?

Quoted 7 lines

The top of the NMI frame contains no less than *three* (SS, SP, FLAGS, CS, IP) records. Off the top of my head, the record that matters is the third one, so it should be regs[-15]. If an MCE hits while this mess is being set up, good luck unwinding *that*. If you really want to know, take a deep breath, read the long comment in entry_64.S after .Lnmi_from_kernel, then give up on x86 and start hacking on some other architecture.

I did read that comment. Fortunately there's a big difference between reading and understanding so I can go on being an ignorant x86 hacker!

For nested NMIs, it does look like the stack of the exception which interrupted the first NMI would get skipped by the stack dump. (But that's a general problem, not specific to my patch set.)

Am I correct in understanding that there can only be one level of NMI nesting at any given time? If so, could we make it easier on the unwinder by putting the nested NMI on a separate software stack, so the "next stack" pointers are always in the same place? Or am I just being naive?

Quoted 16 lines · Josh Poimboeuf

Quoted 5 lines

Quoted 4 lines

Unless I'm missing something, I think it should be fine for nested NMIs, since they're all on the same stack. I can try to test it. What in particular are you worried about?

Quoted 7 lines

The top of the NMI frame contains no less than *three* (SS, SP, FLAGS, CS, IP) records. Off the top of my head, the record that matters is the third one, so it should be regs[-15]. If an MCE hits while this mess is being set up, good luck unwinding *that*. If you really want to know, take a deep breath, read the long comment in entry_64.S after .Lnmi_from_kernel, then give up on x86 and start hacking on some other architecture.

I did read that comment. Fortunately there's a big difference between reading and understanding so I can go on being an ignorant x86 hacker!

Quoted 3 lines

For nested NMIs, it does look like the stack of the exception which interrupted the first NMI would get skipped by the stack dump. (But that's a general problem, not specific to my patch set.)

If we end up with task -> IST -> NMI -> same IST, we're doomed and we're going to crash, so it doesn't matter much whether the unwinder works. Is that what you mean?

Quoted 5 lines

Am I correct in understanding that there can only be one level of NMI nesting at any given time? If so, could we make it easier on the unwinder by putting the nested NMI on a separate software stack, so the "next stack" pointers are always in the same place? Or am I just being naive?

I think you're being naive :)

But we don't really need the unwinder to be 100% faithful here. If we have:

task stack
NMI
nested NMI

then the nested NMI code won't call into C and thus it should be impossible to ever invoke your unwinder on that state. Instead the nested NMI code will fiddle with the saved regs and return in such a way that the outer NMI will be forced to loop through again. So it *should* (assuming I'm remembering all this crap correctly) be sufficient to find the "outermost" pt_regs, which is sitting at (struct pt_regs *)(end - 12) - 1 or thereabouts and look at it's ->sp value. This ought to be the same thing that the frame-based unwinder would naturally try to do. But if you make this change, ISTM you should make it separately because it does change behavior and Linus is understandably leery about that.

Hmm. I wonder if it would make sense to decode this thing both ways and display it. So show_trace_log_lvl() could print something like:

<#DB (0xffffwhatever000-0xffffwhateverfff), next frame is at 0xffffsomething>

and, in the case where the frame unwinder disagrees, it'll at least be visible in that 0xffffsomething won't be between 0xffffwhatever000 and 0xffffwhateverfff.

Then Linus is happy because the unwinder works just like it did before but people like me are also happy because it's clearer what's going on. FWIW, I've personally debugged crashes in the NMI code where the current unwinder falls apart completely and it's not fun -- having a display indicating that the unwinder got confused would be nice.

Ok, I think that makes sense to me now. As I understand it, the "outermost" RIP is the authoritative one, because it was written by the original NMI. Any nested NMIs will update the original and/or iret RIPs, which will only ever point to NMI entry code, and so they should be ignored.

But I think there's a case where this wouldn't work:

task stack
NMI
IST
stack dump

If the IST interrupt hits before the NMI has a chance to update the outermost regs, the authoritative RIP would be the original one written by HW, right?

Quoted 5 lines · On Tue, 26 Jul 2016 11:26:42 -0500 Josh Poimboeuf <[email protected]> wrote:

Ok, I think that makes sense to me now. As I understand it, the "outermost" RIP is the authoritative one, because it was written by the original NMI. Any nested NMIs will update the original and/or iret RIPs, which will only ever point to NMI entry code, and so they should be ignored.

Just to confirm:

-- top-of-stack --
[ hardware written stack ] <- what the NMI hardware mechanism wrote [ internal variables ] <- you don't need to know what this is [ where to go next ] <- the stack to use to return on current NMI [ original copy of hardware stack ] <- the stack of the first NMI

IIRC, the original version had the "where to go next" stack last, but to keep pt_regs in line with the stack, it made sense to have the original NMI stack at the bottom, just above pt_regs, like a real interrupt would.

Quoted 8 lines

But I think there's a case where this wouldn't work:

task stack
NMI
IST
stack dump

Quoted 3 lines

If the IST interrupt hits before the NMI has a chance to update the outermost regs, the authoritative RIP would be the original one written by HW, right?

The only IST interrupt that would hit there is MCE and it would probably be a critical error. Do we really need to worry about such an unlikely scenario? The system is probably doomed anyway.

Quoted 17 lines · Andy Lutomirski

Quoted 16 lines · Josh Poimboeuf

Quoted 5 lines

Quoted 4 lines

Unless I'm missing something, I think it should be fine for nested NMIs, since they're all on the same stack. I can try to test it. What in particular are you worried about?

Quoted 7 lines

The top of the NMI frame contains no less than *three* (SS, SP, FLAGS, CS, IP) records. Off the top of my head, the record that matters is the third one, so it should be regs[-15]. If an MCE hits while this mess is being set up, good luck unwinding *that*. If you really want to know, take a deep breath, read the long comment in entry_64.S after .Lnmi_from_kernel, then give up on x86 and start hacking on some other architecture.

I did read that comment. Fortunately there's a big difference between reading and understanding so I can go on being an ignorant x86 hacker!

Quoted 7 lines

Quoted 3 lines

For nested NMIs, it does look like the stack of the exception which interrupted the first NMI would get skipped by the stack dump. (But that's a general problem, not specific to my patch set.)

If we end up with task -> IST -> NMI -> same IST, we're doomed and we're going to crash, so it doesn't matter much whether the unwinder works. Is that what you mean?

I read the NMI entry code again, and now I realize my comment was completely misinformed, so never mind.

Is "IST -> NMI -> same IST" even possible, since the other IST's are higher priority than NMI?

Quoted 15 lines

Quoted 6 lines

Am I correct in understanding that there can only be one level of NMI nesting at any given time? If so, could we make it easier on the unwinder by putting the nested NMI on a separate software stack, so the "next stack" pointers are always in the same place? Or am I just being naive?

I think you're being naive :)

But we don't really need the unwinder to be 100% faithful here. If we have:

task stack
NMI
nested NMI

Quoted 11 lines

then the nested NMI code won't call into C and thus it should be impossible to ever invoke your unwinder on that state. Instead the nested NMI code will fiddle with the saved regs and return in such a way that the outer NMI will be forced to loop through again. So it *should* (assuming I'm remembering all this crap correctly) be sufficient to find the "outermost" pt_regs, which is sitting at (struct pt_regs *)(end - 12) - 1 or thereabouts and look at it's ->sp value. This ought to be the same thing that the frame-based unwinder would naturally try to do. But if you make this change, ISTM you should make it separately because it does change behavior and Linus is understandably leery about that.

Ok, I think that makes sense to me now. As I understand it, the "outermost" RIP is the authoritative one, because it was written by the original NMI. Any nested NMIs will update the original and/or iret RIPs, which will only ever point to NMI entry code, and so they should be ignored.

But I think there's a case where this wouldn't work:

task stack
NMI
IST
stack dump

If the IST interrupt hits before the NMI has a chance to update the outermost regs, the authoritative RIP would be the original one written by HW, right?

Quoted 10 lines

Hmm. I wonder if it would make sense to decode this thing both ways and display it. So show_trace_log_lvl() could print something like:

<#DB (0xffffwhatever000-0xffffwhateverfff), next frame is at 0xffffsomething>

and, in the case where the frame unwinder disagrees, it'll at least be visible in that 0xffffsomething won't be between 0xffffwhatever000 and 0xffffwhateverfff.

Quoted 5 lines

Then Linus is happy because the unwinder works just like it did before but people like me are also happy because it's clearer what's going on. FWIW, I've personally debugged crashes in the NMI code where the current unwinder falls apart completely and it's not fun -- having a display indicating that the unwinder got confused would be nice.

Hm, maybe. Another idea would be to have the unwinder print some kind of warning if it goes off the rails. It should be able to detect that, because every stack trace should end at a user pt_regs.

Quoted 29 lines · Josh Poimboeuf

Quoted 17 lines · Andy Lutomirski

Quoted 16 lines · Josh Poimboeuf

Quoted 5 lines

Quoted 4 lines

Unless I'm missing something, I think it should be fine for nested NMIs, since they're all on the same stack. I can try to test it. What in particular are you worried about?

Quoted 7 lines

The top of the NMI frame contains no less than *three* (SS, SP, FLAGS, CS, IP) records. Off the top of my head, the record that matters is the third one, so it should be regs[-15]. If an MCE hits while this mess is being set up, good luck unwinding *that*. If you really want to know, take a deep breath, read the long comment in entry_64.S after .Lnmi_from_kernel, then give up on x86 and start hacking on some other architecture.

I did read that comment. Fortunately there's a big difference between reading and understanding so I can go on being an ignorant x86 hacker!

Quoted 7 lines

Quoted 3 lines

For nested NMIs, it does look like the stack of the exception which interrupted the first NMI would get skipped by the stack dump. (But that's a general problem, not specific to my patch set.)

If we end up with task -> IST -> NMI -> same IST, we're doomed and we're going to crash, so it doesn't matter much whether the unwinder works. Is that what you mean?

I read the NMI entry code again, and now I realize my comment was completely misinformed, so never mind.

Quoted 2 lines

Is "IST -> NMI -> same IST" even possible, since the other IST's are higher priority than NMI?

Priority only matters for events that happen concurrently. Synchronous things like #DB will always fire if the conditions that trigger them are hit,

Quoted 39 lines

Quoted 14 lines

Quoted 5 lines

Am I correct in understanding that there can only be one level of NMI nesting at any given time? If so, could we make it easier on the unwinder by putting the nested NMI on a separate software stack, so the "next stack" pointers are always in the same place? Or am I just being naive?

I think you're being naive :)

But we don't really need the unwinder to be 100% faithful here. If we have:

task stack
NMI
nested NMI

Quoted 11 lines

then the nested NMI code won't call into C and thus it should be impossible to ever invoke your unwinder on that state. Instead the nested NMI code will fiddle with the saved regs and return in such a way that the outer NMI will be forced to loop through again. So it *should* (assuming I'm remembering all this crap correctly) be sufficient to find the "outermost" pt_regs, which is sitting at (struct pt_regs *)(end - 12) - 1 or thereabouts and look at it's ->sp value. This ought to be the same thing that the frame-based unwinder would naturally try to do. But if you make this change, ISTM you should make it separately because it does change behavior and Linus is understandably leery about that.

Ok, I think that makes sense to me now. As I understand it, the "outermost" RIP is the authoritative one, because it was written by the original NMI. Any nested NMIs will update the original and/or iret RIPs, which will only ever point to NMI entry code, and so they should be ignored.

But I think there's a case where this wouldn't work:

task stack
NMI
IST
stack dump

Quoted 3 lines

If the IST interrupt hits before the NMI has a chance to update the outermost regs, the authoritative RIP would be the original one written by HW, right?

This should be impossible unless that last entry is MCE. If we actually fire an event that isn't MCE early in NMI entry, something already went very wrong.

For NMI -> MCE -> stack dump, the frame-based unwinder will do better than get_stack_info() unless get_stack_info() learns to use the top-of-stack hardware copy if the actual RSP it finds is too high (above the "outermost" frame).

Quoted 15 lines

Quoted 9 lines

Hmm. I wonder if it would make sense to decode this thing both ways and display it. So show_trace_log_lvl() could print something like:

<#DB (0xffffwhatever000-0xffffwhateverfff), next frame is at 0xffffsomething>

and, in the case where the frame unwinder disagrees, it'll at least be visible in that 0xffffsomething won't be between 0xffffwhatever000 and 0xffffwhateverfff.

Quoted 5 lines

Then Linus is happy because the unwinder works just like it did before but people like me are also happy because it's clearer what's going on. FWIW, I've personally debugged crashes in the NMI code where the current unwinder falls apart completely and it's not fun -- having a display indicating that the unwinder got confused would be nice.

Quoted 3 lines

Hm, maybe. Another idea would be to have the unwinder print some kind of warning if it goes off the rails. It should be able to detect that, because every stack trace should end at a user pt_regs.

I like that.

Be careful, though: kernel threads might not have a "user" pt_regs in the "user_mode" returns true sense. Checking that it's either user_mode() or at task_pt_regs() might be a good condition to check.

Quoted 3 lines

This should be impossible unless that last entry is MCE. If we actually fire an event that isn't MCE early in NMI entry, something already went very wrong.

So we don't need to support breakpoints in the early NMI entry code?

Quoted 33 lines · Andy Lutomirski

Quoted 31 lines · Josh Poimboeuf

Quoted 17 lines · Andy Lutomirski

Quoted 16 lines · Josh Poimboeuf

Quoted 5 lines

Quoted 4 lines

Unless I'm missing something, I think it should be fine for nested NMIs, since they're all on the same stack. I can try to test it. What in particular are you worried about?

Quoted 7 lines

The top of the NMI frame contains no less than *three* (SS, SP, FLAGS, CS, IP) records. Off the top of my head, the record that matters is the third one, so it should be regs[-15]. If an MCE hits while this mess is being set up, good luck unwinding *that*. If you really want to know, take a deep breath, read the long comment in entry_64.S after .Lnmi_from_kernel, then give up on x86 and start hacking on some other architecture.

I did read that comment. Fortunately there's a big difference between reading and understanding so I can go on being an ignorant x86 hacker!

Quoted 7 lines

Quoted 3 lines

For nested NMIs, it does look like the stack of the exception which interrupted the first NMI would get skipped by the stack dump. (But that's a general problem, not specific to my patch set.)

If we end up with task -> IST -> NMI -> same IST, we're doomed and we're going to crash, so it doesn't matter much whether the unwinder works. Is that what you mean?

I read the NMI entry code again, and now I realize my comment was completely misinformed, so never mind.

Is "IST -> NMI -> same IST" even possible, since the other IST's are higher priority than NMI?

Quoted 3 lines

Priority only matters for events that happen concurrently. Synchronous things like #DB will always fire if the conditions that trigger them are hit,

So just to clarify, are you saying a lower priority exception like NMI can interrupt a higher priority exception handler like #DB? I'm getting a different conclusion from reading section 6.9 of the Intel System Programming Guide.

Quoted 44 lines

Quoted 43 lines

Quoted 15 lines

Quoted 6 lines

Am I correct in understanding that there can only be one level of NMI nesting at any given time? If so, could we make it easier on the unwinder by putting the nested NMI on a separate software stack, so the "next stack" pointers are always in the same place? Or am I just being naive?

I think you're being naive :)

But we don't really need the unwinder to be 100% faithful here. If we have:

task stack
NMI
nested NMI

Quoted 11 lines

then the nested NMI code won't call into C and thus it should be impossible to ever invoke your unwinder on that state. Instead the nested NMI code will fiddle with the saved regs and return in such a way that the outer NMI will be forced to loop through again. So it *should* (assuming I'm remembering all this crap correctly) be sufficient to find the "outermost" pt_regs, which is sitting at (struct pt_regs *)(end - 12) - 1 or thereabouts and look at it's ->sp value. This ought to be the same thing that the frame-based unwinder would naturally try to do. But if you make this change, ISTM you should make it separately because it does change behavior and Linus is understandably leery about that.

Ok, I think that makes sense to me now. As I understand it, the "outermost" RIP is the authoritative one, because it was written by the original NMI. Any nested NMIs will update the original and/or iret RIPs, which will only ever point to NMI entry code, and so they should be ignored.

But I think there's a case where this wouldn't work:

task stack
NMI
IST
stack dump

If the IST interrupt hits before the NMI has a chance to update the outermost regs, the authoritative RIP would be the original one written by HW, right?

Quoted 3 lines

This should be impossible unless that last entry is MCE. If we actually fire an event that isn't MCE early in NMI entry, something already went very wrong.

So we don't need to support breakpoints in the early NMI entry code?

Quoted 4 lines

Be careful, though: kernel threads might not have a "user" pt_regs in the "user_mode" returns true sense. Checking that it's either user_mode() or at task_pt_regs() might be a good condition to check.

Yeah. I guess there are two distinct cases of "going off the rails":

1) The unwinder doesn't get to the end of the stack (user regs for user tasks, or whatever the end is for kthreads).

2) The unwinder strays away from the current stack's "previous stack" pointer.

We could warn on either case (though there's probably overlap between the two).

--
Josh

Quoted 5 lines · Josh Poimboeuf

Am I correct in understanding that there can only be one level of NMI nesting at any given time? If so, could we make it easier on the unwinder by putting the nested NMI on a separate software stack, so the "next stack" pointers are always in the same place? Or am I just being naive?

I think you're being naive :)

Quoted 8 lines · Andy Lutomirski

Quoted 5 lines · Josh Poimboeuf

Am I correct in understanding that there can only be one level of NMI nesting at any given time? If so, could we make it easier on the unwinder by putting the nested NMI on a separate software stack, so the "next stack" pointers are always in the same place? Or am I just being naive?

I think you're being naive :)

Another dumb question: since NMIs are reentrant, have you considered removing the NMI IST entry, and instead just have NMIs keep using the current stack?

The first NMI could then be switched to an NMI software stack, like IRQs (assuming there's a way to do that atomically!). And then determining the context of subsequent NMIs would be straightforward, and we'd no longer need to jump through all those horrible hoops in the entry code to deal with NMI nesting.

Now you can tell me what else I'm missing...

Quoted 20 lines · Josh Poimboeuf

Quoted 8 lines · Andy Lutomirski

Quoted 5 lines · Josh Poimboeuf

Am I correct in understanding that there can only be one level of NMI nesting at any given time? If so, could we make it easier on the unwinder by putting the nested NMI on a separate software stack, so the "next stack" pointers are always in the same place? Or am I just being naive?

I think you're being naive :)

Another dumb question: since NMIs are reentrant, have you considered removing the NMI IST entry, and instead just have NMIs keep using the current stack?

The first NMI could then be switched to an NMI software stack, like IRQs (assuming there's a way to do that atomically!). And then determining the context of subsequent NMIs would be straightforward, and we'd no longer need to jump through all those horrible hoops in the entry code to deal with NMI nesting.

Quoted 1 line

Now you can tell me what else I'm missing...

There are several places (most notably SYSCALL entry) where the kernel stack pointer is unsafe/user controlled for a brief time. Since an NMI can interrupt anywhere in the kernel, you have to use an IST to protect against that case.