Date:   Wed, 10 Oct 2018 11:03:43 -0700
From:   Andy Lutomirski <luto@...capital.net>
To:     Josh Poimboeuf <jpoimboe@...hat.com>
Cc:     Steven Rostedt <rostedt@...dmis.org>,
        Peter Zijlstra <peterz@...radead.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Ingo Molnar <mingo@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        Matthew Helsley <mhelsley@...are.com>,
        "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
        David Woodhouse <dwmw2@...radead.org>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Jason Baron <jbaron@...mai.com>, Jiri Kosina <jkosina@...e.cz>,
        Ard Biesheuvel <ard.biesheuvel@...aro.org>,
        Andrew Lutomirski <luto@...nel.org>
Subject: Re: [POC][RFC][PATCH 1/2] jump_function: Addition of new feature "jump_function"

On Wed, Oct 10, 2018 at 10:52 AM Josh Poimboeuf <jpoimboe@...hat.com> wrote:
>
> On Mon, Oct 08, 2018 at 11:57:50PM -0400, Steven Rostedt wrote:
> > On Mon, 8 Oct 2018 21:17:10 -0500
> > Josh Poimboeuf <jpoimboe@...hat.com> wrote:
> >
> > > I'm not really convinced we need objtool for this, maybe I'll try
> > > whipping up a POC.
> >
> > Awesome!
> >
> > I wasn't thinking of actually having objtool itself perform this task,
> > but instead breaking the internals of objtool up into more of a generic
> > infrastructure, that recordmcount.c, objtool, and whatever this does
> > can use.
>
> So I had been thinking that we could find the call sites at runtime, by
> looking at the relocations.  But I managed to forget that vmlinux
> relocations are resolved during linking.  So yeah, some kind of tooling
> magic would be needed.
>
> I worked up a POC using objtool.  It doesn't *have* to be done with
> objtool, but since it's already reading/writing all the ELF stuff
> anyway, it was pretty easy to add this on.
>
> This patch has at least a few issues:
>
> - No module support.
>
> - For some reason, the sync_cores in text_poke_bp() don't always seem to
>   be working as expected.  Running this patch on my VM, the test code in
>   cmdline_proc_show() works *most* of the time, but it occasionally
>   branches off into the weeds.  I have no idea what the problem is yet.
>
> diff --git a/arch/Kconfig b/arch/Kconfig
> index 9d329608913e..20ff5624dad7 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -865,6 +865,9 @@ config HAVE_ARCH_PREL32_RELOCATIONS
>           architectures, and don't require runtime relocation on relocatable
>           kernels.
>
> +config HAVE_ARCH_STATIC_CALL
> +       bool
> +
>  source "kernel/gcov/Kconfig"
>
>  source "scripts/gcc-plugins/Kconfig"
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 5136a1281870..1a14c8f87876 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -128,6 +128,7 @@ config X86
>         select HAVE_ARCH_COMPAT_MMAP_BASES      if MMU && COMPAT
>         select HAVE_ARCH_PREL32_RELOCATIONS
>         select HAVE_ARCH_SECCOMP_FILTER
> +       select HAVE_ARCH_STATIC_CALL            if X86_64
>         select HAVE_ARCH_THREAD_STRUCT_WHITELIST
>         select HAVE_ARCH_TRACEHOOK
>         select HAVE_ARCH_TRANSPARENT_HUGEPAGE
> diff --git a/arch/x86/include/asm/static_call.h b/arch/x86/include/asm/static_call.h
> new file mode 100644
> index 000000000000..40fec631b760
> --- /dev/null
> +++ b/arch/x86/include/asm/static_call.h
> @@ -0,0 +1,35 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_STATIC_CALL_H
> +#define _ASM_STATIC_CALL_H
> +
> +#ifdef CONFIG_X86_64
> +
> +#include <linux/frame.h>
> +
> +void static_call_init(void);
> +extern void __static_call_update(void *tramp, void *func);
> +
> +#define DECLARE_STATIC_CALL(tramp, func)                               \
> +       extern typeof(func) tramp;                                      \
> +       static void __used __section(.discard.static_call_tramps)       \
> +               *__static_call_tramp_##tramp = tramp
> +

Confused.  What's the __static_call_tramp_##tramp variable for?  And
why is a DECLARE_ macro defining a variable?

> +#define DEFINE_STATIC_CALL(tramp, func)                                        \
> +       DECLARE_STATIC_CALL(tramp, func);                               \
> +       asm(".pushsection .text, \"ax\"                         \n"     \
> +           ".align 4                                           \n"     \
> +           ".globl " #tramp "                                  \n"     \
> +           ".type " #tramp ", @function                        \n"     \
> +           #tramp ":                                           \n"     \
> +           "jmp " #func "                                      \n"     \

I think this would be nicer as an indirect call that gets patched to a
direct call so that the update mechanism works even before it's
initialized.  (Currently static_branch blows up horribly if you try to
update one too early, and that's rather annoying IMO.)

Also, I think you're looking for "jmp.d32", which is available in
newer toolchains.  For older toolchains, you could use .byte 0xe9 or
you could use a different section (I think) to force a real 32-bit
jump.

> +void __init static_call_init(void)
> +{
> +       struct static_call_entry *entry;
> +       unsigned long insn, tramp, func;
> +       unsigned char insn_opcode, tramp_opcode;
> +       s32 call_dest;
> +
> +       for (entry = __start_static_call_table;
> +            entry < __stop_static_call_table; entry++) {
> +
> +               insn = (long)entry->insn + (unsigned long)&entry->insn;
> +               tramp = (long)entry->tramp + (unsigned long)&entry->tramp;
> +
> +               insn_opcode = *(unsigned char *)insn;
> +               if (insn_opcode != 0xe8 && insn_opcode != 0xe9) {
> +                       WARN_ONCE(1, "unexpected static call insn opcode %x at %pS",
> +                                 insn_opcode, (void *)insn);
> +                       continue;
> +               }
> +
> +               tramp_opcode = *(unsigned char *)tramp;
> +               if (tramp_opcode != 0xeb && tramp_opcode != 0xe9) {
> +                       WARN_ONCE(1, "unexpected trampoline jump opcode %x at %ps",
> +                                tramp_opcode, (void *)tramp);
> +                       continue;
> +               }
> +
> +               if (tramp_opcode == 0xeb)
> +                       func = *(s8 *)(tramp + 1) + (tramp + 2);

I realize you expect some instances of 0xeb due to the assembler
messing you up (see above), but this seems a bit too permissive, since
a 0xeb without the appropriate set of NOPs is going to explode.  And:

> +               else
> +                       func = *(s32 *)(tramp + 1) + (tramp + 5);
> +
> +               call_dest = (long)(func) - (long)(insn + 5);
> +
> +               printk("static_call_init: poking %lx at %lx\n", (unsigned long)call_dest, (insn+1));
> +
> +               text_poke_early((void *)(insn + 1), &call_dest, 4);

If you really are going to rewrite an 8-bit jump to a 32-bit jump, I
think you need to actually patch the opcode byte :)

Powered by blists - more mailing lists