x86: Fix perf deadlock caused by dumpstack cleanup

Linus,

Here are two patches that fix the deadlock that you discovered.

The first patch is the real culprit, but as I was looking at the code I realized that the irq stack part was a bit off too. That part didn't cause the lock up, but needs to be fixed regardless.

Note, I'm hoping to use these clean ups to make x86_64 and i386 code a bit closer to each other, so I hope the original change does not get reverted.

-- Steve

Steven Rostedt (Red Hat) (2):
x86: Fix dumpstack_64 to keep state of "used" variable in loop
x86: Fix dumpstack_64 irq stack handling

----

 arch/x86/kernel/dumpstack_64.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

From: "Steven Rostedt (Red Hat)" <[email protected]>

Commit 2223f6f6eeaa "x86: Clean up dumpstack_64.c code" moved the used variable to a local within the loop, but the in_exception_stack() depended on being non-volatile with the ability to change it.

By always re-initializing the "used" variable to zero, it would cause the in_exception_stack() to return the same thing each time, and cause the dump_stack loop to go into an infinite loop.

---
 arch/x86/kernel/dumpstack_64.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

index 346b1df..74c262a 100644
@@ -115,19 +115,18 @@ enum stack_type {
 };
 
 static enum stack_type
-analyze_stack(int cpu, struct task_struct *task,
-	      unsigned long *stack, unsigned long **stack_end, char **id)
+analyze_stack(int cpu, struct task_struct *task, unsigned long *stack,
+	      unsigned long **stack_end, unsigned *used, char **id)
 {
 	unsigned long *irq_stack;
 	unsigned long addr;
-	unsigned used = 0;
 
 	addr = ((unsigned long)stack & (~(THREAD_SIZE - 1)));
 	if ((unsigned long)task_stack_page(task) == addr)
 		return STACK_IS_NORMAL;
 
 	*stack_end = in_exception_stack(cpu, (unsigned long)stack,
-					 &used, id);
+					used, id);
 	if (*stack_end)
 		return STACK_IS_EXCEPTION;
 

@@ -158,6 +157,7 @@ void dump_trace(struct task_struct *task, struct pt_regs *regs,
 	struct thread_info *tinfo;
 	unsigned long *irq_stack;
 	unsigned long dummy;
+	unsigned used = 0;
 	int graph = 0;
 	int done = 0;
 

@@ -186,7 +186,7 @@ void dump_trace(struct task_struct *task, struct pt_regs *regs,
 		enum stack_type stype;
 		char *id;
 
-		stype = analyze_stack(cpu, task, stack, &stack_end, &id);
+		stype = analyze_stack(cpu, task, stack, &stack_end, &used, &id);
 
 		/* Default finish unless specified to continue */
 		done = 1;

1.8.5.3

From: "Steven Rostedt (Red Hat)" <[email protected]>

Commit 2223f6f6eeaa "x86: Clean up dumpstack_64.c code" changed the irq_stack processing a little from what it was before. The irq_stack_end variable needed to be cleared after its first use. By setting irq_stack to the per cpu irq_stack and passing that to analyze_stack(), and then clearing it after it is processed, we can get back the original behavior.

---
 arch/x86/kernel/dumpstack_64.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

index 74c262a..1abcb50 100644
@@ -116,9 +116,9 @@ enum stack_type {
 
 static enum stack_type
 analyze_stack(int cpu, struct task_struct *task, unsigned long *stack,
-	      unsigned long **stack_end, unsigned *used, char **id)
+	      unsigned long **stack_end, unsigned long *irq_stack,
+	      unsigned *used, char **id)
 {
-	unsigned long *irq_stack;
 	unsigned long addr;
 
 	addr = ((unsigned long)stack & (~(THREAD_SIZE - 1)));

@@ -130,11 +130,11 @@ analyze_stack(int cpu, struct task_struct *task, unsigned long *stack,
 	if (*stack_end)
 		return STACK_IS_EXCEPTION;
 
-	*stack_end = (unsigned long *)per_cpu(irq_stack_ptr, cpu);
-	if (!*stack_end)
-		return STACK_IS_UNKNOWN;
+	if (!irq_stack)
+		return STACK_IS_NORMAL;
 
-	irq_stack = *stack_end - irq_stack_size;
+	*stack_end = irq_stack;
+	irq_stack = irq_stack - irq_stack_size;
 
 	if (in_irq_stack(stack, irq_stack, *stack_end))
 		return STACK_IS_IRQ;

@@ -155,7 +155,7 @@ void dump_trace(struct task_struct *task, struct pt_regs *regs,
 {
 	const unsigned cpu = get_cpu();
 	struct thread_info *tinfo;
-	unsigned long *irq_stack;
+	unsigned long *irq_stack = (unsigned long *)per_cpu(irq_stack_ptr, cpu);
 	unsigned long dummy;
 	unsigned used = 0;
 	int graph = 0;

@@ -186,7 +186,8 @@ void dump_trace(struct task_struct *task, struct pt_regs *regs,
 		enum stack_type stype;
 		char *id;
 
-		stype = analyze_stack(cpu, task, stack, &stack_end, &used, &id);
+		stype = analyze_stack(cpu, task, stack, &stack_end,
+				      irq_stack, &used, &id);
 
 		/* Default finish unless specified to continue */
 		done = 1;

@@ -226,7 +227,7 @@ void dump_trace(struct task_struct *task, struct pt_regs *regs,
 			 * pointer (index -1 to end) in the IRQ stack:
 			 */
 			stack = (unsigned long *) (stack_end[-1]);
-			irq_stack = stack_end - irq_stack_size;
+			irq_stack = NULL;
 			ops->stack(data, "EOI");
 			done = 0;
 			break;

1.8.5.3

I'll apply them.

That said, I'm not *AT*ALL* convinced that a "cleanup" that involves having helper functions with multiple bugs and now seven (7!) arguments passed into it is a "cleanup" at all.

The cleanup was claimed to help improve readability. Really? Somebody needs to reconsider their goals in life if they think that a 7-argument helper functions where the caller needs to pass in addresses to variables because the helper function will change them is a good idea. Just the *call* is spread out over two lines because calling that function is so complex.

I suspect that maybe creating some kind of "struct stack_information" structure might help somewhat. Because right now I'm very doubtful about the whole "this clarifies/simplifies" argument. The very fact that the "cleaned-up" function was a buggy pile of sh*t should make you question the cleanup itself.

Linus

Yeah, I didn't like it after the second patch :-(

The helper can be merged back to the main function as it doesn't seem to be much of a helper anymore.

Well, actually, it proves that there's a clean up needed, as the reason for the bugs was due to the subtle interactions of the original code. That said, I agree, the cleanup itself isn't much of a cleanup. But I intend on continuing to cleanup this code, and remove all the subtle interactions that is going on here.

This is an ongoing project and what you see here is not the end result. I am quite ashamed of the bugs though.

-- Steve