PROOF OF CONCEPT: Dynamic Functions (jump functions)

From: "Steven Rostedt (VMware)" <[email protected]>

Signed-off-by Steven Rostedt (VMware) <[email protected]>

---
 include/asm-generic/vmlinux.lds.h |   4 +
 include/linux/jump_function.h     |  93 ++++++++
 kernel/Makefile                   |   2 +-
 kernel/jump_function.c            | 368 ++++++++++++++++++++++++++++++
 4 files changed, 466 insertions(+), 1 deletion(-)
 create mode 100644 include/linux/jump_function.h
 create mode 100644 kernel/jump_function.c

include/asm-generic/vmlinux.lds.h +0 -0

index 7b75ff6e2fce..0e205069ff36 100644

include/linux/jump_function.h line 31

Quoted 8 lines · On Fri, 05 Oct 2018 21:51:11 -0400 Steven Rostedt <[email protected]> wrote:

include/linux/jump_function.h new file +8 -0

+#ifndef PARAMS
+#define PARAMS(x...) x
+#endif
+
+#ifndef ARGS
+#define ARGS(x...) x
+#endif
+  

This is also leftover from the first attempt and can be nuked.

include/linux/jump_function.h line 18

Quoted 7 lines · Steven Rostedt

include/linux/jump_function.h new file +7 -0

+#define arch_dynfunc_trampoline(name, def)	\
+	asm volatile (				\
+	".globl dynfunc_" #name "; \n\t"	\
+	"dynfunc_" #name ": \n\t"		\
+	"jmp " #def " \n\t"			\
+	".balign 8 \n \t"			\
+	: : : "memory" )  

Bah, what is it with you people and trampolines. Why can't we, just like jump_label, patch the call directly?

The whole call+jmp thing is silly, don't do that. It just wrecks I$ and is slower for no real reason afaict.

Steve, also see:

https://lkml.kernel.org/r/[email protected]

Quoted 9 lines · On Sat, 6 Oct 2018 14:12:11 +0200 Peter Zijlstra <[email protected]> wrote:

include/linux/jump_function.h line 18

Quoted 8 lines · Steven Rostedt

include/linux/jump_function.h new file +7 -0

+#define arch_dynfunc_trampoline(name, def)    \
+    asm volatile (                \
+    ".globl dynfunc_" #name "; \n\t"    \
+    "dynfunc_" #name ": \n\t"        \
+    "jmp " #def " \n\t"            \
+    ".balign 8 \n \t"            \
+    : : : "memory" )  

Quoted 5 lines

Bah, what is it with you people and trampolines. Why can't we, just like jump_label, patch the call directly?

The whole call+jmp thing is silly, don't do that. It just wrecks I$ and is slower for no real reason afaict.

My first attempt was to do just that. But to add a label at the call site required handling all the parameters too. See my branch:
ftrace/jump_function-v1 for how ugly it got (and it didn't work).

Quoted 4 lines

Steve, also see:

https://lkml.kernel.org/r/[email protected]

Interesting. I don't have time to look at it at the moment to see what was done, but will do so in the near future.

Remember, this was a proof of concept and even with the trampolines, it showed a great level of improvement. One thought was to do a "recordmcount.c" type of action to find where the calls were and patch them directly at boot up. I tried to keep the API the same where this could actually be done as an improvement later.

Perhaps a gcc plugin might work too.

I'll have to see what Ard did to handle the function parameters.

-- Steve

Quoted 2 lines

Perhaps a gcc plugin might work too.

My suggestion was to have objtool do the dirty work. Josh said something suspiciously like “sounds fun” on IRC :)

Quoted 12 lines · On Sat, 6 Oct 2018 14:12:11 +0200 Peter Zijlstra <[email protected]> wrote:

include/linux/jump_function.h line 18

Quoted 7 lines · Steven Rostedt

include/linux/jump_function.h new file +7 -0

+#define arch_dynfunc_trampoline(name, def)	\
+	asm volatile (				\
+	".globl dynfunc_" #name "; \n\t"	\
+	"dynfunc_" #name ": \n\t"		\
+	"jmp " #def " \n\t"			\
+	".balign 8 \n \t"			\
+	: : : "memory" )  

Bah, what is it with you people and trampolines. Why can't we, just like jump_label, patch the call directly?

Quoted 2 lines

The whole call+jmp thing is silly, don't do that. It just wrecks I$ and is slower for no real reason afaict.

My first attempt was to do just that. But to add a label at the call site required handling all the parameters too. See my branch:
ftrace/jump_function-v1 for how ugly it got (and it didn't work).

Quoted 20 lines · Steven Rostedt

Quoted 14 lines · On Sat, 6 Oct 2018 14:12:11 +0200 Peter Zijlstra <[email protected]> wrote:

include/linux/jump_function.h line 18

Quoted 8 lines · Steven Rostedt

include/linux/jump_function.h new file +7 -0

+#define arch_dynfunc_trampoline(name, def)    \
+    asm volatile (                \
+    ".globl dynfunc_" #name "; \n\t"    \
+    "dynfunc_" #name ": \n\t"        \
+    "jmp " #def " \n\t"            \
+    ".balign 8 \n \t"            \
+    : : : "memory" )  

Bah, what is it with you people and trampolines. Why can't we, just like jump_label, patch the call directly?

The whole call+jmp thing is silly, don't do that. It just wrecks I$ and is slower for no real reason afaict.

Quoted 3 lines

My first attempt was to do just that. But to add a label at the call site required handling all the parameters too. See my branch:
ftrace/jump_function-v1 for how ugly it got (and it didn't work).

Can't we hijack the relocation records for these functions before they get thrown out in the (final) link pass or something?

Quoted 2 lines

Can't we hijack the relocation records for these functions before they get thrown out in the (final) link pass or something?

I could be talking out my arse here, but I thought we could do this, too, then changed my mind. The relocation records give us the location of the call or jump operand, but they don’t give the address of the beginning of the instruction.

Quoted 7 lines · Andy Lutomirski

Quoted 2 lines

Can't we hijack the relocation records for these functions before they get thrown out in the (final) link pass or something?

I could be talking out my arse here, but I thought we could do this, too, then changed my mind. The relocation records give us the location of the call or jump operand, but they don’t give the address of the beginning of the instruction.

But that's like 1 byte before the operand, right? We could even double check this by reading back that byte and ensuring it is in fact 0xE8 (CALL).

AFAICT there is only the _1_ CALL encoding, and that is the 5 byte: E8 <PLT32>, so if we have the PLT32 location, we also have the instruction location. Or am I missing something?

Quoted 14 lines · Peter Zijlstra

Quoted 7 lines · Andy Lutomirski

Quoted 2 lines

Can't we hijack the relocation records for these functions before they get thrown out in the (final) link pass or something?

I could be talking out my arse here, but I thought we could do this, too, then changed my mind. The relocation records give us the location of the call or jump operand, but they don’t give the address of the beginning of the instruction.

But that's like 1 byte before the operand, right? We could even double check this by reading back that byte and ensuring it is in fact 0xE8 (CALL).

Quoted 3 lines

AFAICT there is only the _1_ CALL encoding, and that is the 5 byte: E8 <PLT32>, so if we have the PLT32 location, we also have the instruction location. Or am I missing something?

There’s also JMP and Jcc, any of which can be used for rail calls, but those are also one byte. I suppose GCC is unlikely to emit a prefixed form of any of these. So maybe we really can assume they’re all one byte.

But there is a nasty potential special case: anything that takes the function’s address. This includes jump tables, computed gotos, and plain old function pointers. And I suspect that any of these could have one of the rather large number of CALL/JMP/Jcc bytes before the relocation by coincidence.

Quoted 16 lines · Peter Zijlstra

Quoted 7 lines · Andy Lutomirski

Quoted 2 lines

Can't we hijack the relocation records for these functions before they get thrown out in the (final) link pass or something?

I could be talking out my arse here, but I thought we could do this, too, then changed my mind. The relocation records give us the location of the call or jump operand, but they don’t give the address of the beginning of the instruction.

But that's like 1 byte before the operand, right? We could even double check this by reading back that byte and ensuring it is in fact 0xE8 (CALL).

Quoted 3 lines

AFAICT there is only the _1_ CALL encoding, and that is the 5 byte: E8 <PLT32>, so if we have the PLT32 location, we also have the instruction location. Or am I missing something?

There’s also JMP and Jcc, any of which can be used for rail calls, but those are also one byte. I suppose GCC is unlikely to emit a prefixed form of any of these. So maybe we really can assume they’re all one byte.

But there is a nasty potential special case: anything that takes the function’s address. This includes jump tables, computed gotos, and plain old function pointers. And I suspect that any of these could have one of the rather large number of CALL/JMP/Jcc bytes before the relocation by coincidence.

Quoted 16 lines · Andy Lutomirski

Quoted 16 lines · Peter Zijlstra

Quoted 7 lines · Andy Lutomirski

Quoted 2 lines

Can't we hijack the relocation records for these functions before they get thrown out in the (final) link pass or something?

I could be talking out my arse here, but I thought we could do this, too, then changed my mind. The relocation records give us the location of the call or jump operand, but they don’t give the address of the beginning of the instruction.

But that's like 1 byte before the operand, right? We could even double check this by reading back that byte and ensuring it is in fact 0xE8 (CALL).

Quoted 8 lines

Quoted 3 lines

AFAICT there is only the _1_ CALL encoding, and that is the 5 byte: E8 <PLT32>, so if we have the PLT32 location, we also have the instruction location. Or am I missing something?

There’s also JMP and Jcc, any of which can be used for rail calls, but those are also one byte. I suppose GCC is unlikely to emit a prefixed form of any of these. So maybe we really can assume they’re all one byte.

Oh, I had not considered tail calls..

Quoted 5 lines

But there is a nasty potential special case: anything that takes the function’s address. This includes jump tables, computed gotos, and plain old function pointers. And I suspect that any of these could have one of the rather large number of CALL/JMP/Jcc bytes before the relocation by coincidence.

We can have objtool verify the CALL/JMP/Jcc only condition. So if someone tries to take the address of a patchable function, it will error out.

Quoted 35 lines · Peter Zijlstra

Quoted 16 lines · Andy Lutomirski

Quoted 16 lines · Peter Zijlstra

Quoted 7 lines · Andy Lutomirski

Quoted 2 lines

Can't we hijack the relocation records for these functions before they get thrown out in the (final) link pass or something?

I could be talking out my arse here, but I thought we could do this, too, then changed my mind. The relocation records give us the location of the call or jump operand, but they don’t give the address of the beginning of the instruction.

But that's like 1 byte before the operand, right? We could even double check this by reading back that byte and ensuring it is in fact 0xE8 (CALL).

Quoted 8 lines

Quoted 3 lines

AFAICT there is only the _1_ CALL encoding, and that is the 5 byte: E8 <PLT32>, so if we have the PLT32 location, we also have the instruction location. Or am I missing something?

There’s also JMP and Jcc, any of which can be used for rail calls, but those are also one byte. I suppose GCC is unlikely to emit a prefixed form of any of these. So maybe we really can assume they’re all one byte.

Oh, I had not considered tail calls..

Quoted 5 lines

But there is a nasty potential special case: anything that takes the function’s address. This includes jump tables, computed gotos, and plain old function pointers. And I suspect that any of these could have one of the rather large number of CALL/JMP/Jcc bytes before the relocation by coincidence.

Quoted 3 lines

We can have objtool verify the CALL/JMP/Jcc only condition. So if someone tries to take the address of a patchable function, it will error out.

I think we should just ignore the sites that take the address and maybe issue a warning. After all, GCC can create them all by itself. We'll always have a plain wrapper function, and I think we should just not patch code that takes its address. So we do, roughly:

void default_foo(void);

GLOBAL(foo)
jmp *current_foo(%rip)
ENDPROC(foo)

And code that does:

foo();

as a call, a tail call, a conditional tail call, etc, gets discovered by objtool + relocation processing or whatever and gets patched. (And foo() itself gets patched, too, as a special case. But we patch foo itself at some point during boot to turn it into a direct JMP. Doing it this way means that the whole mechanism works from very early boot.)

And anything awful like:

switch(whatever) {
case 0:
foo();
};

that gets translated to a jump table and gets optimized such that it jumps straight to foo just gets left alone, since it still works. It's just a bit suboptimial. Similarly, code that does:

void (*ptr)(void);
ptr = foo;

gets a bona fide pointer to foo(), and any calls through the pointer land on foo() and jump to the current selected foo with only a single indirect branch / retpoline.

Does this seem reasonable? Is there a reason we should make it more restrictive?

Quoted 53 lines · Andy Lutomirski

Quoted 35 lines · Peter Zijlstra

Quoted 16 lines · Andy Lutomirski

Quoted 16 lines · Peter Zijlstra

Quoted 7 lines · Andy Lutomirski

Quoted 2 lines

Can't we hijack the relocation records for these functions before they get thrown out in the (final) link pass or something?

I could be talking out my arse here, but I thought we could do this, too, then changed my mind. The relocation records give us the location of the call or jump operand, but they don’t give the address of the beginning of the instruction.

But that's like 1 byte before the operand, right? We could even double check this by reading back that byte and ensuring it is in fact 0xE8 (CALL).

Quoted 8 lines

Quoted 3 lines

AFAICT there is only the _1_ CALL encoding, and that is the 5 byte: E8 <PLT32>, so if we have the PLT32 location, we also have the instruction location. Or am I missing something?

There’s also JMP and Jcc, any of which can be used for rail calls, but those are also one byte. I suppose GCC is unlikely to emit a prefixed form of any of these. So maybe we really can assume they’re all one byte.

Oh, I had not considered tail calls..

Quoted 5 lines

But there is a nasty potential special case: anything that takes the function’s address. This includes jump tables, computed gotos, and plain old function pointers. And I suspect that any of these could have one of the rather large number of CALL/JMP/Jcc bytes before the relocation by coincidence.

Quoted 3 lines

We can have objtool verify the CALL/JMP/Jcc only condition. So if someone tries to take the address of a patchable function, it will error out.

I think we should just ignore the sites that take the address and maybe issue a warning. After all, GCC can create them all by itself. We'll always have a plain wrapper function, and I think we should just not patch code that takes its address. So we do, roughly:

void default_foo(void);

GLOBAL(foo)
jmp *current_foo(%rip)
ENDPROC(foo)

And code that does:

Quoted 8 lines

foo();

as a call, a tail call, a conditional tail call, etc, gets discovered by objtool + relocation processing or whatever and gets patched. (And foo() itself gets patched, too, as a special case. But we patch foo itself at some point during boot to turn it into a direct JMP. Doing it this way means that the whole mechanism works from very early boot.)

Does that mean that architectures could opt out of doing the whole objtool + relocation processing thing, and instead take the hit of going through the trampoline for all calls?

Does that mean that architectures could opt out of doing the whole objtool + relocation processing thing, and instead take the hit of going through the trampoline for all calls?

Quoted 3 lines · Ard Biesheuvel

Does that mean that architectures could opt out of doing the whole objtool + relocation processing thing, and instead take the hit of going through the trampoline for all calls?

There are architectures that aren't [currently] supported by objtool at all anyway.

Quoted 3 lines · Ard Biesheuvel

Does that mean that architectures could opt out of doing the whole objtool + relocation processing thing, and instead take the hit of going through the trampoline for all calls?

There are architectures that aren't [currently] supported by objtool at all anyway.

Quoted 16 lines · Peter Zijlstra

Quoted 7 lines · Andy Lutomirski

Quoted 2 lines

Can't we hijack the relocation records for these functions before they get thrown out in the (final) link pass or something?

I could be talking out my arse here, but I thought we could do this, too, then changed my mind. The relocation records give us the location of the call or jump operand, but they don’t give the address of the beginning of the instruction.

But that's like 1 byte before the operand, right? We could even double check this by reading back that byte and ensuring it is in fact 0xE8 (CALL).

Quoted 3 lines

AFAICT there is only the _1_ CALL encoding, and that is the 5 byte: E8 <PLT32>, so if we have the PLT32 location, we also have the instruction location. Or am I missing something?

There’s also JMP and Jcc, any of which can be used for rail calls, but those are also one byte. I suppose GCC is unlikely to emit a prefixed form of any of these. So maybe we really can assume they’re all one byte.

But there is a nasty potential special case: anything that takes the function’s address. This includes jump tables, computed gotos, and plain old function pointers. And I suspect that any of these could have one of the rather large number of CALL/JMP/Jcc bytes before the relocation by coincidence.

I'm not really convinced we need objtool for this, maybe I'll try whipping up a POC.

Quoted 2 lines · On Mon, 8 Oct 2018 21:17:10 -0500 Josh Poimboeuf <[email protected]> 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.

Quoted 6 lines · Steven Rostedt

Quoted 2 lines · On Mon, 8 Oct 2018 21:17:10 -0500 Josh Poimboeuf <[email protected]> wrote:

I'm not really convinced we need objtool for this, maybe I'll try whipping up a POC.

Quoted 6 lines

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.

arch/Kconfig +3 -0

index 9d329608913e..20ff5624dad7 100644
@@ -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"

arch/x86/Kconfig +1 -0

index 5136a1281870..1a14c8f87876 100644
@@ -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

arch/x86/include/asm/static_call.h new file +6 -0

new file mode 100644
index 000000000000..40fec631b760
@@ -0,0 +1,35 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_STATIC_CALL_H
+#define _ASM_STATIC_CALL_H
+
+#ifdef CONFIG_X86_64
+

arch/x86/include/asm/static_call.h new file +19 -0

+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;
+               }

arch/x86/include/asm/static_call.h line 16

Quoted 5 lines

arch/x86/include/asm/static_call.h new file +5 -0

+#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?

arch/x86/include/asm/static_call.h line 24

Quoted 8 lines

arch/x86/include/asm/static_call.h new file +8 -0

+#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.

Quoted 25 lines

arch/x86/kernel/static_call.c new file +25 -0

+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;

arch/x86/kernel/static_call.c line 49

Quoted 4 lines

arch/x86/kernel/static_call.c new file +4 -0

+               }
+
+               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:

arch/x86/kernel/static_call.c line 57

Quoted 8 lines

arch/x86/kernel/static_call.c new file +8 -0

+               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 :)

Quoted 8 lines · Andy Lutomirski

arch/x86/include/asm/static_call.h line 16

Quoted 5 lines

arch/x86/include/asm/static_call.h new file +5 -0

+#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?

This is the magic needed for objtool to find all the call sites.

The variable itself isn't needed, but the .discard.static_call_tramps entry is. Objtool reads that section to find out which function call sites are targeted to a static call trampoline.

Quoted 12 lines · Josh Poimboeuf

Quoted 8 lines · Andy Lutomirski

arch/x86/include/asm/static_call.h line 16

Quoted 5 lines

arch/x86/include/asm/static_call.h new file +5 -0

+#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?

This is the magic needed for objtool to find all the call sites.

Quoted 3 lines

The variable itself isn't needed, but the .discard.static_call_tramps entry is. Objtool reads that section to find out which function call sites are targeted to a static call trampoline.

To clarify: objtool reads that section to find out which functions are really static call trampolines. Then it annotates all the instructions which call/jmp to those trampolines. Those annotations are then read by the kernel.

Quoted 14 lines · Josh Poimboeuf

Quoted 12 lines · Josh Poimboeuf

Quoted 8 lines · Andy Lutomirski

Review

Quoted 5 lines

(quoted diff) +5 -0

+#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?

This is the magic needed for objtool to find all the call sites.

Quoted 9 lines

Quoted 3 lines

The variable itself isn't needed, but the .discard.static_call_tramps entry is. Objtool reads that section to find out which function call sites are targeted to a static call trampoline.

To clarify: objtool reads that section to find out which functions are really static call trampolines. Then it annotates all the instructions which call/jmp to those trampolines. Those annotations are then read by the kernel.

Ah, right, and objtool runs on a per-object basis so it has no other way to know what symbols are actually static calls.

There's another way to skin this cat, though:

extern typeof(func) __static_call_trampoline_##tramp; #define tramp __static_call_trampoline_##tramp

And objtool could recognize it by name. But, of course, you can't put a #define in a macro. But maybe there's a way to hack it up with a static inline?

Anyway, your way is probably fine with a few caveats:

- It won't really work if the call comes from a .S file.

- There should probably be a comment to help de-confuse future people
like me :)

Quoted 28 lines · Andy Lutomirski

Quoted 14 lines · Josh Poimboeuf

Quoted 12 lines · Josh Poimboeuf

Quoted 8 lines · Andy Lutomirski

arch/x86/include/asm/static_call.h line 25

Quoted 5 lines

arch/x86/include/asm/static_call.h new file +5 -0

+#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?

This is the magic needed for objtool to find all the call sites.

Quoted 9 lines

Quoted 3 lines

The variable itself isn't needed, but the .discard.static_call_tramps entry is. Objtool reads that section to find out which function call sites are targeted to a static call trampoline.

To clarify: objtool reads that section to find out which functions are really static call trampolines. Then it annotates all the instructions which call/jmp to those trampolines. Those annotations are then read by the kernel.

Ah, right, and objtool runs on a per-object basis so it has no other way to know what symbols are actually static calls.

Quoted 8 lines

There's another way to skin this cat, though:

extern typeof(func) __static_call_trampoline_##tramp; #define tramp __static_call_trampoline_##tramp

And objtool could recognize it by name. But, of course, you can't put a #define in a macro. But maybe there's a way to hack it up with a static inline?

Quoted 31 lines · Josh Poimboeuf

Quoted 29 lines · Andy Lutomirski

Quoted 15 lines · Josh Poimboeuf

Quoted 14 lines · Josh Poimboeuf

Quoted 8 lines · Andy Lutomirski

arch/x86/include/asm/static_call.h line 25

Quoted 5 lines

arch/x86/include/asm/static_call.h new file +5 -0

+#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?

This is the magic needed for objtool to find all the call sites.

Quoted 9 lines

Quoted 3 lines

The variable itself isn't needed, but the .discard.static_call_tramps entry is. Objtool reads that section to find out which function call sites are targeted to a static call trampoline.

To clarify: objtool reads that section to find out which functions are really static call trampolines. Then it annotates all the instructions which call/jmp to those trampolines. Those annotations are then read by the kernel.

Ah, right, and objtool runs on a per-object basis so it has no other way to know what symbols are actually static calls.

Quoted 8 lines

Quoted 8 lines

There's another way to skin this cat, though:

extern typeof(func) __static_call_trampoline_##tramp; #define tramp __static_call_trampoline_##tramp

And objtool could recognize it by name. But, of course, you can't put a #define in a macro. But maybe there's a way to hack it up with a static inline?

I went with something similar in the latest version. It's less surprising in a couple of ways:

- DECLARE_STATIC_CALL doesn't have the magic objtool definition.

- Call sites use the static_call() wrapper, which makes static calls clearly visible.

Quoted 9 lines

arch/x86/include/asm/static_call.h line 24

Quoted 8 lines

arch/x86/include/asm/static_call.h new file +8 -0

+#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"     \

Quoted 4 lines

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.)

Yeah, that would be better. It would also allow trampoline function pointers to work, which I think you mentioned elsewhere. And then I shouldn't trample this code in __static_call_update() -- that was already kind of nasty anyway.

Re-reading your suggestion, I may have misunderstood what you're suggesting here, but I'm thinking about doing something like what you proposed earlier:

GLOBAL(tramp)
jmp *current_func(%rip)
ENDPROC(tramp)

That is, doing an indirect jump instead of the above direct jump, so that any previous references to the trampoline would still work (and it would also work during early boot).

Though it should probably be a retpoline instead of an indirect jump.

Quoted 12 lines · On Wed, 10 Oct 2018 13:33:30 -0500 Josh Poimboeuf <[email protected]> wrote:

Re-reading your suggestion, I may have misunderstood what you're suggesting here, but I'm thinking about doing something like what you proposed earlier:

GLOBAL(tramp)
jmp *current_func(%rip)
ENDPROC(tramp)

That is, doing an indirect jump instead of the above direct jump, so that any previous references to the trampoline would still work (and it would also work during early boot).

Quoted 1 line

Though it should probably be a retpoline instead of an indirect jump.

But do we care, as it only takes place during text_poke_bp() right?

I don't think we need to worry about training trampoline branch prediction that can only be hit when something enables the jump.

Quoted 17 lines · Steven Rostedt

Quoted 13 lines · On Wed, 10 Oct 2018 13:33:30 -0500 Josh Poimboeuf <[email protected]> wrote:

Re-reading your suggestion, I may have misunderstood what you're suggesting here, but I'm thinking about doing something like what you proposed earlier:

GLOBAL(tramp)
jmp *current_func(%rip)
ENDPROC(tramp)

That is, doing an indirect jump instead of the above direct jump, so that any previous references to the trampoline would still work (and it would also work during early boot).

Though it should probably be a retpoline instead of an indirect jump.

Quoted 4 lines

But do we care, as it only takes place during text_poke_bp() right?

I don't think we need to worry about training trampoline branch prediction that can only be hit when something enables the jump.

Yeah, I guess it depends on if we'd expect anybody (or gcc) to get a function pointer to the trampoline itself. I can just create a warning for that in objtool.

Quoted 7 lines · Andy Lutomirski

Quoted 2 lines

Can't we hijack the relocation records for these functions before they get thrown out in the (final) link pass or something?

I could be talking out my arse here, but I thought we could do this, too, then changed my mind. The relocation records give us the location of the call or jump operand, but they don’t give the address of the beginning of the instruction.

But that's like 1 byte before the operand, right? We could even double check this by reading back that byte and ensuring it is in fact 0xE8 (CALL).

AFAICT there is only the _1_ CALL encoding, and that is the 5 byte: E8 <PLT32>, so if we have the PLT32 location, we also have the instruction location. Or am I missing something?

Quoted 9 lines · On Sat, 6 Oct 2018 14:12:11 +0200 Peter Zijlstra <[email protected]> wrote:

include/linux/jump_function.h line 18

Quoted 7 lines · Steven Rostedt

include/linux/jump_function.h new file +7 -0

+#define arch_dynfunc_trampoline(name, def) \
+   asm volatile (                          \
+   ".globl dynfunc_" #name "; \n\t"        \
+   "dynfunc_" #name ": \n\t"               \
+   "jmp " #def " \n\t"                     \
+   ".balign 8 \n \t"                       \
+   : : : "memory" )

Quoted 5 lines

Bah, what is it with you people and trampolines. Why can't we, just like jump_label, patch the call directly?

The whole call+jmp thing is silly, don't do that. It just wrecks I$ and is slower for no real reason afaict.

My first attempt was to do just that. But to add a label at the call site required handling all the parameters too. See my branch:
ftrace/jump_function-v1 for how ugly it got (and it didn't work).

Quoted 4 lines

Steve, also see:

https://lkml.kernel.org/r/[email protected]

Interesting. I don't have time to look at it at the moment to see what was done, but will do so in the near future.

Remember, this was a proof of concept and even with the trampolines, it showed a great level of improvement. One thought was to do a "recordmcount.c" type of action to find where the calls were and patch them directly at boot up. I tried to keep the API the same where this could actually be done as an improvement later.

Perhaps a gcc plugin might work too.

I'll have to see what Ard did to handle the function parameters.

include/linux/jump_function.h line 19

Quoted 12 lines · On Fri, 05 Oct 2018 21:51:11 -0400 Steven Rostedt <[email protected]> wrote:

include/linux/jump_function.h new file +12 -0

+typedef long dynfunc_t;
+
+struct dynfunc_struct;
+
+#define arch_dynfunc_trampoline(name, def)	\
+	asm volatile (				\
+	".globl dynfunc_" #name "; \n\t"	\
+	"dynfunc_" #name ": \n\t"		\
+	"jmp " #def " \n\t"			\
+	".balign 8 \n \t"			\
+	: : : "memory" )
+  

I have just a question, what is this different from livepatch? :)

I think we can replace the first 5 bytes of the default function to jmp instruction (to alternative function) instead of making this trampoline.

IOW, as far as I can see, this is changing

----
call %reg (or retpoline_reg)
----

to

----
call dynfunc_A

dynfunc_A:
jmp func_A or altered_func_A
----

If so, why don't we put the jmp on default func_A directly?
----
call func_A

func_A:
"jmp altered_func" or "original sequence"
----
(this is idealy same as jprobes did)

Of course we have to arbitrate it with ftrace (fentry) but it may not so hard (simplest way is just adding "notrace" on the default function)

BTW, I think "dynamic_function" may not correct name, it may be "alternative_function" or something like that, because this function must be replaced system-wide and this means we can not use this for generic function pointer usage which depends on thread context (like file_operations). But good for something pluggable code (LSM?).

Quoted 17 lines · On Tue, 9 Oct 2018 12:44:01 +0900 Masami Hiramatsu <[email protected]> wrote:

include/linux/jump_function.h line 19

Quoted 12 lines · On Fri, 05 Oct 2018 21:51:11 -0400 Steven Rostedt <[email protected]> wrote:

include/linux/jump_function.h new file +12 -0

+typedef long dynfunc_t;
+
+struct dynfunc_struct;
+
+#define arch_dynfunc_trampoline(name, def)	\
+	asm volatile (				\
+	".globl dynfunc_" #name "; \n\t"	\
+	"dynfunc_" #name ": \n\t"		\
+	"jmp " #def " \n\t"			\
+	".balign 8 \n \t"			\
+	: : : "memory" )
+  

I have just a question, what is this different from livepatch? :)

I actually thought about this a bit, but decided against it.

I didn't want to hook another infrastructure into the fentry nop. It's already complex enough with kprobes, live patching and ftrace.

The ideal solution is what Peter suggested, and that's to patch the call sites, and I think that is attainable with objtool modifications.

Quoted 30 lines

I think we can replace the first 5 bytes of the default function to jmp instruction (to alternative function) instead of making this trampoline.

IOW, as far as I can see, this is changing

----
call %reg (or retpoline_reg)
----

to

----
call dynfunc_A

dynfunc_A:
jmp func_A or altered_func_A
----

If so, why don't we put the jmp on default func_A directly?
----
call func_A

func_A:
"jmp altered_func" or "original sequence"
----
(this is idealy same as jprobes did)

Quoted 3 lines

Of course we have to arbitrate it with ftrace (fentry) but it may not so hard (simplest way is just adding "notrace" on the default function)

Then we lose the 5 byte nop.

Quoted 6 lines

BTW, I think "dynamic_function" may not correct name, it may be "alternative_function" or something like that, because this function must be replaced system-wide and this means we can not use this for generic function pointer usage which depends on thread context (like file_operations). But good for something pluggable code (LSM?).

I don't like the name alternative, as that's usually a one shot deal (SMP vs UP).

It is dynamic, as it's a function that changes dynamically. Yes its global, but that's not mutually exclusive to dynamic.

The use case I want this for is for tracing. But it can be useful for KVM and power management governors. Basically anything that has a global function pointer (hmm, even the idle call can use this).

-- Steve

Sent: 09 October 2018 04:44

I think we can replace the first 5 bytes of the default function to jmp instruction (to alternative function) instead of making this trampoline.