[PATCH 0/2] x86: Fix perf deadlock caused by dumpstack cleanup

[PATCH 0/2] x86: Fix perf deadlock caused by dumpstack cleanup

[Steven Rostedt]

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(-)

[PATCH 1/2] x86: Fix dumpstack_64 to keep state of "used" variable in loop

[Steven Rostedt]

[-- Attachment #1: 0001-x86-Fix-dumpstack_64-to-keep-state-of-used-variable-.patch --]
[-- Type: text/plain, Size: 2080 bytes --]

From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>

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.

Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 arch/x86/kernel/dumpstack_64.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/dumpstack_64.c b/arch/x86/kernel/dumpstack_64.c
index 346b1df..74c262a 100644
--- a/arch/x86/kernel/dumpstack_64.c
+++ b/arch/x86/kernel/dumpstack_64.c
@@ -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

[PATCH 2/2] x86: Fix dumpstack_64 irq stack handling

[Steven Rostedt]

[-- Attachment #1: 0002-x86-Fix-dumpstack_64-irq-stack-handling.patch --]
[-- Type: text/plain, Size: 2639 bytes --]

From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>

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.

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 arch/x86/kernel/dumpstack_64.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kernel/dumpstack_64.c b/arch/x86/kernel/dumpstack_64.c
index 74c262a..1abcb50 100644
--- a/arch/x86/kernel/dumpstack_64.c
+++ b/arch/x86/kernel/dumpstack_64.c
@@ -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

Re: [PATCH 0/2] x86: Fix perf deadlock caused by dumpstack cleanup

[Linus Torvalds]

On Wed, Apr 2, 2014 at 10:26 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> 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.

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

Re: [PATCH 0/2] x86: Fix perf deadlock caused by dumpstack cleanup

[Steven Rostedt]

On Wed, 2 Apr 2014 11:46:38 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Wed, Apr 2, 2014 at 10:26 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > 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.
> 
> 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.
> 

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

> 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.

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

> 
> 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.

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