lkml.org 
[lkml]   [2019]   [Apr]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC PATCH 1/1] x86/ftrace: make ftrace_int3_handler() not to skip fops invocation
On Tue, 23 Apr 2019 20:15:49 +0200
Nicolai Stange <nstange@suse.de> wrote:

> > This can be made much simpler by making a hardcoded ftrace_int3_tramp
> > that does the following:
> >
> > ftrace_int3_tramp
> > push %r11
> > jmp ftrace_caller
>
>
> But wouldn't this mean that ftrace_caller could nest if the breakpoint
> in question happened to be placed at ftrace_call? Infinite recursion set
> aside, the ip value determined from inner calls based on the on-stack
> return address would be wrong, AFAICS. Or am I missing something here?

I had a reply here, but I think you explained what I just explained
(and then deleted) below ;-)

>
>
> > The ftrace_caller will either call a single ftrace callback, if there's
> > only a single ops registered, or it will call the loop function that
> > iterates over all the ftrace_ops registered and will call each function
> > that matches the ftrace_ops hashes.
> >
> > In either case, it's what we want.
>
> Ok, under the assumption that my above point is valid, this patch could
> still get simplified a lot by having two trampolines:
>
> 1.) Your ftrace_int3_tramp from above, to be used if the patched insn is
> some mcount call site. The live patching fops will need
> ftrace_regs_caller though. So,
>
> ftrace_int3_tramp_regs_caller:
> push %r11
> jmp ftrace_regs_caller
>
> 2.) Another one redirecting control flow to ftrace_ops_list_func(). It's
> going to be used when the int3 is found at ftrace_call or
> ftrace_regs_call resp. their counterparts in some ftrace_ops'
> trampoline.
>
> ftrace_int3_tramp_list_func:
> push %r11
> jmp ftrace_ops_list_func

Yes, I wrote a reply basically stating something similar, but then
deleted it after reading what you wrote here!

>
> ftrace_int3_handler() would then distinguish the following cases:
> 1.) ip == ftrace_graph_call => ignore, i.e. skip the insn

OK, because this transition would be from "call function graph" to
"nop" or the other way. Either case, one would always be a nop.

> 2.) is_ftrace_caller(ip) => ftrace_int3_tramp_list_func
> 3.) ftrace_location(ip) => ftrace_int3_tramp_regs_caller
>
> ftrace_location() is getting invoked from ftrace_int3_handler() already,
> so there wouldn't be any additional cost.
>
> If that makes sense to you, I'd prepare a patch...

Yes it does.

>
>
> > The ftrace_int3_tramp will simply simulate the call ftrace_caller that
> > would be there as the default caller if more than one function is set
> > to it.
> >
> > For 32 bit, we could add 4 variables on the thread_info and make 4
> > trampolines, one for each context (normal, softirq, irq and NMI), and
> > have them use the variable stored in the thread_info for that location:
> >
> > ftrace_int3_tramp_normal
> > push %eax # just for space
> > push %eax
> > mov whatever_to_get_thread_info, %eax
> > mov normal(%eax), %eax
> > mov %eax, 4(%esp)
> > pop %eax
> > jmp ftrace_caller
> >
> > ftrace_int3_tramp_softiqr
> > push %eax # just for space
> > push %eax
> > mov whatever_to_get_thread_info, %eax
> > mov softirq(%eax), %eax
> > mov %eax, 4(%esp)
> > pop %eax
> > jmp ftrace_caller
> >
> > etc..
> >
> >
> > A bit crazier for 32 bit but it can still be done. ;-)
>
> Ok, but currently CONFIG_HAVE_LIVEPATCH=n for x86 && !x86_64,
> so I'd rather not invest too much energy into screwing 32 bit up ;)
>

Probably not something you care about, but something that I do. Which
means it will have to go on my TODO list. I care about missed functions
being called. This means, if you have something tracing a function, and
then enable something else to trace that same function, you might miss
the first one.

-- Steve

\
 
 \ /
  Last update: 2019-04-24 01:51    [W:0.086 / U:5.312 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site