Messages in this thread | | | From | Andy Lutomirski <> | Date | Wed, 10 Oct 2018 11:03:43 -0700 | 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@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 <jpoimboe@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 :)
| |