lkml.org 
[lkml]   [2015]   [Jan]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] ftrace: don't allow IPMODIFY without proper compiler support (was Re: Re: livepatching tree for linux-next)
    (2015/01/14 7:47), Jiri Kosina wrote:
    > On Mon, 12 Jan 2015, Masami Hiramatsu wrote:
    >
    >>> In any case, Masami, I really think you would like to do something
    >>> like that for IPMODIFY as well ... or are you deliberately defering
    >>> the responsibility to handle the possible mcount fallout to the
    >>> ftrace_ops owner?
    >>
    >> Ah, good point. I just tried to use ftrace and WARN if not possible
    >> to use it. I'll see it tomorrow. Anyway, I'd prefer to have this
    >> kind of checking functionality in ftrace.
    >
    > Okay, so how about something like this, for example ... ?

    Thanks! Could you read my comments?

    > From: Jiri Kosina <jkosina@suse.cz>
    > Subject: [PATCH] ftrace: don't allow IPMODIFY without proper compiler support
    >
    > Using IPMODIFY needs to be allowed only with compilers which are
    > guaranteed to generate function prologues compatible with function
    > redirection through changing instruction pointer in saved regs.
    >
    > For example changing regs->ip on x86_64 in cases when gcc is using mcount
    > (and not fentry) is not allowed, because at the time mcount call is
    > issued, the original function's prologue has already been executed, which
    > leads to all kinds of inconsistent havoc.
    >
    > There is currently no way to express dependency on gcc features in
    > Kconfig, (CC_USING_FENTRY is defined only during build, so it's not
    > possible for Kconfig symbol to depend on it) so this needs to be checked
    > in runtime.
    >
    > Mark x86_64 with fentry supported for now. Other archs can be added
    > gradually.
    >
    > Signed-off-by: Jiri Kosina <jkosina@suse.cz>
    > ---
    > arch/x86/include/asm/ftrace.h | 2 ++
    > include/linux/ftrace.h | 4 ++++
    > kernel/trace/ftrace.c | 5 +++++
    > 3 files changed, 11 insertions(+)
    >
    > diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
    > index f45acad..29fa417 100644
    > --- a/arch/x86/include/asm/ftrace.h
    > +++ b/arch/x86/include/asm/ftrace.h
    > @@ -4,8 +4,10 @@
    > #ifdef CONFIG_FUNCTION_TRACER
    > #ifdef CC_USING_FENTRY
    > # define MCOUNT_ADDR ((long)(__fentry__))
    > +# define arch_ftrace_ipmodify_compiler_support(void) ({ 1; })
    > #else
    > # define MCOUNT_ADDR ((long)(mcount))
    > +#define arch_ftrace_ipmodify_compiler_support(void) ({ 0; })

    Hmm, can we just define ARCH_FTRACE_SUPPORT_IPMODIFY here?

    > #endif
    > #define MCOUNT_INSN_SIZE 5 /* sizeof mcount call */
    >
    > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
    > index 1da6029..655ba99 100644
    > --- a/include/linux/ftrace.h
    > +++ b/include/linux/ftrace.h
    > @@ -244,6 +244,10 @@ static inline int ftrace_function_local_disabled(struct ftrace_ops *ops)
    > extern void ftrace_stub(unsigned long a0, unsigned long a1,
    > struct ftrace_ops *op, struct pt_regs *regs);
    >
    > +#ifndef arch_ftrace_ipmodify_compiler_support
    > +/* let's not make any implicit assumptions about profiling call placement */
    > +# define arch_ftrace_ipmodify_compiler_support() { 0; }
    > +#endif
    > #else /* !CONFIG_FUNCTION_TRACER */
    > /*
    > * (un)register_ftrace_function must be a macro since the ops parameter
    > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
    > index 929a733..11370fd 100644
    > --- a/kernel/trace/ftrace.c
    > +++ b/kernel/trace/ftrace.c
    > @@ -1809,6 +1809,11 @@ static int __ftrace_hash_update_ipmodify(struct ftrace_ops *ops,
    > if (!(ops->flags & FTRACE_OPS_FL_IPMODIFY))
    > return 0;
    >
    > + if (!arch_ftrace_ipmodify_compiler_support()) {
    > + WARN(1, "Your compiler doesn't support features necessary for IPMODIFY");
    > + return 0;
    > + }

    Actually, if ftrace doesn't support IPMODIFY, I would like to just drop
    the entire code for CONFIG_KPROBES_ON_FTRACE(this is a hidden config),
    instead of checking at runtime.

    So, there are 2 ifdefs of code in kernel/kprobes.c for
    CONFIG_KPROBES_ON_FTRACE, those should also check
    ARCH_FTRACE_SUPPORT_IPMODIFY too.

    Thank you,

    > +
    > /*
    > * Since the IPMODIFY is a very address sensitive action, we do not
    > * allow ftrace_ops to set all functions to new hash.
    >


    --
    Masami HIRAMATSU
    Software Platform Research Dept. Linux Technology Research Center
    Hitachi, Ltd., Yokohama Research Laboratory
    E-mail: masami.hiramatsu.pt@hitachi.com




    \
     
     \ /
      Last update: 2015-01-14 03:41    [W:4.295 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site