lkml.org 
[lkml]   [2009]   [Nov]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectBUG: function graph tracer function frame assumptions
B1;2005;0cOn Thu, 19 Nov 2009, Thomas Gleixner wrote:
> On Thu, 19 Nov 2009, Ingo Molnar wrote:
> > i've bisected back to it:
> >
> > | 887a29f59b93cf54e21814869a4ab6e80b6fa623 is the first bad commit
> > | commit 887a29f59b93cf54e21814869a4ab6e80b6fa623
> > | Author: Feng Tang <feng.tang@intel.com>
> > | Date: Thu Sep 3 16:32:53 2009 +0800
> > |
> > | hrtimer: Fix /proc/timer_list regression
> >
> > Config attached.
> >
> > I've removed it from timers/urgent for now.
>
> Come on, this is a patently wrong conclusion.
>
> We call timer_stats_update_stats() which returns on top of the
> function due to:
>
> if (likely(!timer_stats_active))
> return;
>
> timer_stats_active is 0 during boot and you can only activate it by
> writing to /proc/timer_stats which you certainly did not at this
> point.
>
> Can you please explain how a call to a function which returns right
> away can cause that problem ?
>
> That patch unearthed some other bug and your revert is just papering
> over that fact.

Looked deeper into that and found the root cause.

The function graph tracer expects that the first code sequence in a
function is:

function_start:
push %ebp
mov %esp, %ebp
....
call mcount
and on the return side:

pop %ebp
ret

which is the case for most functions except, but not for all

timer_stats_update_stats() and a few others have:

function_start:
push %edi
lea 0x8(%esp),%edi
and $0xfffffff0,%esp
pushl -0x4(%edi)
push %ebp
mov %esp,%ebp
...
call mcount
and the return does:

pop %ebp
lea -0x8(%edi),%esp
pop %edi
ret

mcount calls prepare_ftrace_return() with the following calling sequence:

pushl %eax
pushl %ecx
pushl %edx
movl 0xc(%esp), %edx
lea 0x4(%ebp), %eax

Here is where the shit hits the fan.

For the usual function frames this is correct:

ebp points to the stack location where ebp was pushed and ebp + 4
points to the return address.

For the timer_stats_update_stats case points to the stack location
where ebp was pushed _BUT_ ebp + 4 is pointing to a stack entry
_BELOW_ the return address.

movl (%ebp), %ecx
subl $MCOUNT_INSN_SIZE, %edx
call prepare_ftrace_return
popl %edx
popl %ecx
popl %eax
ret

prepare_ftrace_return does:

void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,
unsigned long frame_pointer)
{
unsigned long old;
int faulted;
unsigned long return_hooker = (unsigned long)
&return_to_handler;
asm volatile(
"1: " _ASM_MOV " (%[parent]), %[old]\n"
"2: " _ASM_MOV " %[return_hooker], (%[parent])\n"
" movl $0, %[faulted]\n"
"3:\n"

So here we modify the "return" address on the stack where parent is
supposed to point to. That works for the standard function frames, but
not for the ones which look like timer_stats_update_stats().

In the timer_stats_update_stats() case we do not call the
return_to_handler when the function returns, because we did not modify
the return address. So the next return in the calling function will
trip over the sanity checking and panic.

I'm not yet sure whether this is a compiler problem (using gcc 4.4.1)
or just the stupid assumption that function frames always start with

push %ebp
mov %esp, %ebp

Thanks,

tglx


\
 
 \ /
  Last update: 2009-11-19 15:33    [from the cache]
©2003-2011 Jasper Spaans