On Wed, Oct 10, 2018 at 10:52 AM Josh Poimboeuf <jpoim...@redhat.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 <jpoim...@redhat.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 :)
