[PATCH] Interrupt entry CONFIG_FRAME_POINTER fix

[PATCH] Interrupt entry CONFIG_FRAME_POINTER fix

[Tejun Heo]

[-- Attachment #1: Type: text/plain, Size: 276 bytes --]

 On x86_64, rbp isn't saved on entering interrupt handler even when
CONFIG_FRAME_POINTER is turned on.  This breaks profile_pc()
(resulting in oops) which uses regs->rbp to track back to the original
stack.  Save full stack when CONFIG_FRAME_POINTER is specified.

-- 
tejun


[-- Attachment #2: x86_64_frame_pointer.patch --]
[-- Type: text/plain, Size: 1634 bytes --]

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2004/09/12 16:38:06+09:00 tj@htj.dyndns.org 
#   On x86_64, rbp isn't saved on entering interrupt handler even when
#   CONFIG_FRAME_POINTER is turned on.  This breaks profile_pc() which
#   uses regs->rbp to track back to the original stack.  Save full stack
#   when CONFIG_FRAME_POINTER is specified.
# 
# arch/x86_64/kernel/entry.S
#   2004/09/12 16:37:55+09:00 tj@htj.dyndns.org +10 -4
#   On x86_64, rbp isn't saved on entering interrupt handler even when
#   CONFIG_FRAME_POINTER is turned on.  This breaks profile_pc() which
#   uses regs->rbp to track back to the original stack.  Save full stack
#   when CONFIG_FRAME_POINTER is specified.
# 
diff -Nru a/arch/x86_64/kernel/entry.S b/arch/x86_64/kernel/entry.S
--- a/arch/x86_64/kernel/entry.S	2004-09-12 18:05:16 +09:00
+++ b/arch/x86_64/kernel/entry.S	2004-09-12 18:05:16 +09:00
@@ -410,8 +410,8 @@
 	CFI_REL_OFFSET	rsp,(RSP-ORIG_RAX)
 	CFI_REL_OFFSET	rip,(RIP-ORIG_RAX)
 	cld
-#ifdef CONFIG_DEBUG_INFO
-	SAVE_ALL	
+#if defined(CONFIG_DEBUG_INFO)
+	SAVE_ALL
 	movq %rsp,%rdi
 	/*
 	 * Setup a stack frame pointer.  This allows gdb to trace
@@ -419,10 +419,16 @@
 	 */
 	movq %rsp,%rbp
 	CFI_DEF_CFA_REGISTER	rbp
-#else		
+#elif defined(CONFIG_FRAME_POINTER)
+	/* Save full stack frame such that interrupt handlers can trace
+	 * back to the original stack using regs->rbp.  Currently,
+	 * profile_pc() uses it when CONFIG_SMP is also turned on. */
+	SAVE_ALL
+	movq %rsp,%rdi
+#else
 	SAVE_ARGS
 	leaq -ARGOFFSET(%rsp),%rdi	# arg1 for handler
-#endif	
+#endif
 	testl $3,CS(%rdi)
 	je 1f
 	swapgs	

Re: [PATCH] Interrupt entry CONFIG_FRAME_POINTER fix

[Andi Kleen]

On Sun, 12 Sep 2004 18:16:28 +0900
Tejun Heo <tj@home-tj.org> wrote:

>  On x86_64, rbp isn't saved on entering interrupt handler even when
> CONFIG_FRAME_POINTER is turned on.  This breaks profile_pc()
> (resulting in oops) which uses regs->rbp to track back to the original
> stack.  Save full stack when CONFIG_FRAME_POINTER is specified.


I don't think your patch is correct, you don't restore rbp ever and it gets corrupted.

I think the correct change is to fix profile_pc() to not reference rbp, but just hardcode
the rsp offset for the FP and non FP cases (8 and 0) 

-Andi

Re: [PATCH] Interrupt entry CONFIG_FRAME_POINTER fix

[Tejun Heo]

Hi,

On Sun, Sep 12, 2004 at 01:24:54PM +0200, Andi Kleen wrote:
> I don't think your patch is correct, you don't restore rbp ever and
> it gets corrupted.

 R15 R14 R13 R12 RBP RBX are callee-saved and they aren't
saved/restored during kernel entry unless they are explicitly needed
(ptrace, etc..).  interrupt macro which I modified is used only for
IRQ handling and apic interrupts, both of which aren't supposed to
modify any of above registers.

 So, in the original code, when CONFIG_DEBUG is off only caller-saved
registers are saved and restored, and when the option is on, SAVE_ALL
is used but it's saved only to link back to the original stack such
that the debugger can track back into the previous frame.  The current
code contains a line to restore rbp in ret_from_interrupt (other
calle-saved registers are not restored), but I don't think that line
is necessary.  Unless somebody explicitly changes regs->rbp, the value
would be always the same, and if somebody is allowed to modify the
contents of regs when CONFIG_DEBUG is turned on, we should be
restoring all the callee-saved registers not just rbp.

> I think the correct change is to fix profile_pc() to not reference
> rbp,

 I thought about that, but CONFIG_FRAME_POINTER is a debug feature
anyway, and it seemed reasonable to allow frame linking on interrupts
when the option turned on (x86 also supports back linking when
CONFIG_FRAME_POINTER is turned on).

> but just hardcode the rsp offset for the FP and non FP cases (8
> and 0)

 You lost me here.  regs->rbp is used in profile_pc() if the interrupt
occurs while the kernel is running spinlock codes.  To attribute ticks
spent during spinlock operations to the locking/unlocking function,
profile_pc() reads the return value of the frame which was running
before the interrupt occurs.  Are you saying that we can track back
into the previous frame without saving rbp?  We can read the next
frame's(do_IRQ's) rbp storage area, but that doesn't seem to be a very
good idea to me.

-- 
tejun

Re: [PATCH] Interrupt entry CONFIG_FRAME_POINTER fix

[Zwane Mwaikambo]

On Sun, 12 Sep 2004, Andi Kleen wrote:

> On Sun, 12 Sep 2004 18:16:28 +0900
> Tejun Heo <tj@home-tj.org> wrote:
> 
> >  On x86_64, rbp isn't saved on entering interrupt handler even when
> > CONFIG_FRAME_POINTER is turned on.  This breaks profile_pc()
> > (resulting in oops) which uses regs->rbp to track back to the original
> > stack.  Save full stack when CONFIG_FRAME_POINTER is specified.
> 
> 
> I don't think your patch is correct, you don't restore rbp ever and it gets corrupted.
> 
> I think the correct change is to fix profile_pc() to not reference rbp, but just hardcode
> the rsp offset for the FP and non FP cases (8 and 0) 

Yep, i botched up the patch, after looking at the disassembly on 
x86_64 without CONFIG_FRAME_POINTER again it's definitely incorrect. In 
fact there are still a few users such as _spin_lock_irqsave which push 
flags onto the stack and the stack pointer isn't consistent across all 
functions in that text section. I'm going to have to try Andi's previous 
suggestions.

Thanks,
	Zwane

Re: [PATCH] Interrupt entry CONFIG_FRAME_POINTER fix

[Tejun Heo]

On Sun, Sep 12, 2004 at 01:10:26PM -0400, Zwane Mwaikambo wrote:
> On Sun, 12 Sep 2004, Andi Kleen wrote:
> > On Sun, 12 Sep 2004 18:16:28 +0900
> > Tejun Heo <tj@home-tj.org> wrote:
> > 
> > >  On x86_64, rbp isn't saved on entering interrupt handler even when
> > > CONFIG_FRAME_POINTER is turned on.  This breaks profile_pc()
> > > (resulting in oops) which uses regs->rbp to track back to the original
> > > stack.  Save full stack when CONFIG_FRAME_POINTER is specified.
> > 
> > 
> > I don't think your patch is correct, you don't restore rbp ever and it gets corrupted.
> > 
> > I think the correct change is to fix profile_pc() to not reference rbp, but just hardcode
> > the rsp offset for the FP and non FP cases (8 and 0) 
> 
> Yep, i botched up the patch, after looking at the disassembly on 
> x86_64 without CONFIG_FRAME_POINTER again it's definitely incorrect. In 
> fact there are still a few users such as _spin_lock_irqsave which push 
> flags onto the stack and the stack pointer isn't consistent across all 
> functions in that text section. I'm going to have to try Andi's previous 
> suggestions.

 I'm sorry but I guess I'm slow today. :-( Can you please be kind
enough to lighten me up on how things get corrupted?  I've read the
assembly source and disassembly of the output but I don't really see
how it'll get corrupted.

-- 
tejun