x86: bpf unwinder fixes

Objtool doesn't know how to read C jump tables, so it has to whitelist functions which use them, causing missing ORC unwinder data for such functions, e.g. ___bpf_prog_run().

C jump tables are very similar to GCC switch jump tables, which objtool already knows how to read. So adding support for C jump tables is easy. It just needs to be able to find the tables and distinguish them from other data.

To allow the jump tables to be found, create a standard: objtool will automatically recognize any static local jump table named "jump_table".

Signed-off-by Josh Poimboeuf <[email protected]>

Acked-by Peter Zijlstra (Intel) <[email protected]>

---
 tools/objtool/check.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

tools/objtool/check.c +3 -0

index 172f99195726..8341c2fff14f 100644
@@ -18,6 +18,8 @@
 
 #define FAKE_JUMP_OFFSET -1
 
+#define JUMP_TABLE_SYM_PREFIX "jump_table."
+
 struct alternative {
        struct list_head list;
        struct instruction *insn;

@@ -997,6 +999,7 @@ static struct rela *find_switch_table(struct objtool_file *file,
        struct instruction *orig_insn = insn;
        struct section *rodata_sec;
        unsigned long table_offset;
+       struct symbol *sym;
 
        /*
         * Backward search using the @first_jump_src links, these help avoid

tools/objtool/check.c static struct rela *find_switch_table(struct objtool_file *file, · line 1046

Quoted 11 lines

tools/objtool/check.c +6 -1

@@ -1035,9 +1038,18 @@ static struct rela *find_switch_table(struct objtool_file *file,
 
                /*
                 * Make sure the .rodata address isn't associated with a
-                * symbol.  gcc jump tables are anonymous data.
+                * symbol.  GCC jump tables are anonymous data.
+                *
+                * Also support C jump tables which are in the same format as
+                * switch jump tables.  Each jump table should be a static
+                * local const array named "jump_table" for objtool to
+                * recognize it.

Nacked-by Alexei Starovoitov <[email protected]>

It's not acceptable for objtool to dictate kernel naming convention.

Quoted 14 lines · Alexei Starovoitov

tools/objtool/check.c static struct rela *find_switch_table(struct objtool_file *file, · line 1046

Quoted 11 lines

tools/objtool/check.c +6 -1

@@ -1035,9 +1038,18 @@ static struct rela *find_switch_table(struct objtool_file *file,
 
                /*
                 * Make sure the .rodata address isn't associated with a
-                * symbol.  gcc jump tables are anonymous data.
+                * symbol.  GCC jump tables are anonymous data.
+                *
+                * Also support C jump tables which are in the same format as
+                * switch jump tables.  Each jump table should be a static
+                * local const array named "jump_table" for objtool to
+                * recognize it.

Nacked-by Alexei Starovoitov <[email protected]>

Quoted 1 line

It's not acceptable for objtool to dictate kernel naming convention.

Abrasive nack notwithstanding, I agree it's not ideal.

How about the following approach instead? This is the only other way I can think of to annotate a jump table so that objtool can distinguish it:

#define __annotate_jump_table __section(".jump_table.rodata")

Then bpf would just need the following:

(quoted diff) +1 -1

-       static const void *jumptable[256] = {
+       static const void __annotate_jump_table *jumptable[256] = {

This would be less magical and fragile than my original approach.

I think the jump table would still be placed with all the other rodata, like before, because the vmlinux linker script recognizes the section ".rodata" suffix and bundles them all together.

Quoted 31 lines · Josh Poimboeuf

Quoted 14 lines · Alexei Starovoitov

Review static struct rela *find_switch_table(struct objtool_file *file, · line 1046

Quoted 11 lines

(quoted diff) +6 -1

@@ -1035,9 +1038,18 @@ static struct rela *find_switch_table(struct objtool_file *file,
 
                /*
                 * Make sure the .rodata address isn't associated with a
-                * symbol.  gcc jump tables are anonymous data.
+                * symbol.  GCC jump tables are anonymous data.
+                *
+                * Also support C jump tables which are in the same format as
+                * switch jump tables.  Each jump table should be a static
+                * local const array named "jump_table" for objtool to
+                * recognize it.

Nacked-by Alexei Starovoitov <[email protected]>

Quoted 1 line

It's not acceptable for objtool to dictate kernel naming convention.

Abrasive nack notwithstanding, I agree it's not ideal.

How about the following approach instead? This is the only other way I can think of to annotate a jump table so that objtool can distinguish it:

#define __annotate_jump_table __section(".jump_table.rodata")

Then bpf would just need the following:

(quoted diff) +1 -1

-       static const void *jumptable[256] = {
+       static const void __annotate_jump_table *jumptable[256] = {

Quoted 5 lines

This would be less magical and fragile than my original approach.

I think the jump table would still be placed with all the other rodata, like before, because the vmlinux linker script recognizes the section ".rodata" suffix and bundles them all together.

I like it if that works :)
Definitely cleaner.
May be a bit more linker script magic would be necessary, but hopefully not.
And no need to rely on gcc style of mangling static vars.

Quoted 17 lines

Quoted 16 lines

How about the following approach instead? This is the only other way I can think of to annotate a jump table so that objtool can distinguish it:

#define __annotate_jump_table __section(".jump_table.rodata")

Then bpf would just need the following:

(quoted diff) +1 -1

-       static const void *jumptable[256] = {
+       static const void __annotate_jump_table *jumptable[256] = {

This would be less magical and fragile than my original approach.

I think the jump table would still be placed with all the other rodata, like before, because the vmlinux linker script recognizes the section ".rodata" suffix and bundles them all together.

Quoted 5 lines

I like it if that works :)
Definitely cleaner.
May be a bit more linker script magic would be necessary, but hopefully not.
And no need to rely on gcc style of mangling static vars.

The last patch was based weird, this one's based on upstream. Will test tomorrow.

Quoted 14 lines · Alexei Starovoitov

tools/objtool/check.c static struct rela *find_switch_table(struct objtool_file *file, · line 1046

Quoted 11 lines

tools/objtool/check.c +6 -1

@@ -1035,9 +1038,18 @@ static struct rela *find_switch_table(struct objtool_file *file,
 
                /*
                 * Make sure the .rodata address isn't associated with a
-                * symbol.  gcc jump tables are anonymous data.
+                * symbol.  GCC jump tables are anonymous data.
+                *
+                * Also support C jump tables which are in the same format as
+                * switch jump tables.  Each jump table should be a static
+                * local const array named "jump_table" for objtool to
+                * recognize it.  

Nacked-by Alexei Starovoitov <[email protected]>

Quoted 1 line

It's not acceptable for objtool to dictate kernel naming convention.

Abrasive nack notwithstanding, I agree it's not ideal.

How about the following approach instead? This is the only other way I can think of to annotate a jump table so that objtool can distinguish it:

#define __annotate_jump_table __section(".jump_table.rodata")

Then bpf would just need the following:

(quoted diff) +1 -1

-	static const void *jumptable[256] = {
+	static const void __annotate_jump_table *jumptable[256] = {

This would be less magical and fragile than my original approach.

I think the jump table would still be placed with all the other rodata, like before, because the vmlinux linker script recognizes the section ".rodata" suffix and bundles them all together.

Objtool previously ignored ___bpf_prog_run() because it didn't understand the jump table. This resulted in the ORC unwinder not being able to unwind through non-JIT BPF code.

Now that objtool knows how to read jump tables, remove the whitelist and rename the variable to "jump_table" so objtool can recognize it.

Fixes d15d356887e7 ("perf/x86: Make perf callchains work without CONFIG_FRAME_POINTER")

Reported-by Song Liu <[email protected]>

Signed-off-by Josh Poimboeuf <[email protected]>

---
 kernel/bpf/core.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

kernel/bpf/core.c +1 -1

index 080e2bb644cc..ff66294882f8 100644
@@ -1299,7 +1299,7 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
 {
 #define BPF_INSN_2_LBL(x, y)    [BPF_##x | BPF_##y] = &&x##_##y
 #define BPF_INSN_3_LBL(x, y, z) [BPF_##x | BPF_##y | BPF_##z] = &&x##_##y##_##z
-       static const void *jumptable[256] = {
+       static const void *jump_table[256] = {
                [0 ... 255] = &&default_label,
                /* Now overwrite non-defaults ... */
                BPF_INSN_MAP(BPF_INSN_2_LBL, BPF_INSN_3_LBL),

Quoted 31 lines · Josh Poimboeuf

Objtool previously ignored ___bpf_prog_run() because it didn't understand the jump table. This resulted in the ORC unwinder not being able to unwind through non-JIT BPF code.

Now that objtool knows how to read jump tables, remove the whitelist and rename the variable to "jump_table" so objtool can recognize it.

Fixes d15d356887e7 ("perf/x86: Make perf callchains work without CONFIG_FRAME_POINTER")

Reported-by Song Liu <[email protected]>

Signed-off-by Josh Poimboeuf <[email protected]>

---
 kernel/bpf/core.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

kernel/bpf/core.c +1 -1

index 080e2bb644cc..ff66294882f8 100644
@@ -1299,7 +1299,7 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
 {
 #define BPF_INSN_2_LBL(x, y)    [BPF_##x | BPF_##y] = &&x##_##y
 #define BPF_INSN_3_LBL(x, y, z) [BPF_##x | BPF_##y | BPF_##z] = &&x##_##y##_##z
-       static const void *jumptable[256] = {
+       static const void *jump_table[256] = {
                [0 ... 255] = &&default_label,
                /* Now overwrite non-defaults ... */
                BPF_INSN_MAP(BPF_INSN_2_LBL, BPF_INSN_3_LBL),

@@ -1315,7 +1315,7 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
 #define CONT_JMP ({ insn++; goto select_insn; })
 

kernel/bpf/core.c static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack) · line 1318

Quoted 3 lines

kernel/bpf/core.c +1 -1

 select_insn:
-       goto *jumptable[insn->code];
+       goto *jump_table[insn->code];

I thought we were clear that it is a nack?
Either live it alone or rename to something like jump_table_bpf_interpreter or bpf_interpreter_jump_table.

Quoted 32 lines · Alexei Starovoitov

Quoted 31 lines · Josh Poimboeuf

Objtool previously ignored ___bpf_prog_run() because it didn't understand the jump table. This resulted in the ORC unwinder not being able to unwind through non-JIT BPF code.

Now that objtool knows how to read jump tables, remove the whitelist and rename the variable to "jump_table" so objtool can recognize it.

Fixes d15d356887e7 ("perf/x86: Make perf callchains work without CONFIG_FRAME_POINTER")

Reported-by Song Liu <[email protected]>

Signed-off-by Josh Poimboeuf <[email protected]>

---
 kernel/bpf/core.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

kernel/bpf/core.c +1 -1

index 080e2bb644cc..ff66294882f8 100644
@@ -1299,7 +1299,7 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
 {
 #define BPF_INSN_2_LBL(x, y)    [BPF_##x | BPF_##y] = &&x##_##y
 #define BPF_INSN_3_LBL(x, y, z) [BPF_##x | BPF_##y | BPF_##z] = &&x##_##y##_##z
-       static const void *jumptable[256] = {
+       static const void *jump_table[256] = {
                [0 ... 255] = &&default_label,
                /* Now overwrite non-defaults ... */
                BPF_INSN_MAP(BPF_INSN_2_LBL, BPF_INSN_3_LBL),

@@ -1315,7 +1315,7 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
 #define CONT_JMP ({ insn++; goto select_insn; })
 

Quoted 7 lines

kernel/bpf/core.c static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack) · line 1318

Quoted 3 lines

kernel/bpf/core.c +1 -1

 select_insn:
-       goto *jumptable[insn->code];
+       goto *jump_table[insn->code];

I thought we were clear that it is a nack?
Either live it alone or rename to something like jump_table_bpf_interpreter or bpf_interpreter_jump_table.

As I have said many times:

The jump table detection is a generic objtool feature. It makes no sense to give a bpf-specific name to a generic objtool feature which can be used by other components.

Quoted 42 lines · Josh Poimboeuf

Quoted 39 lines · Alexei Starovoitov

Quoted 29 lines · Josh Poimboeuf

Objtool previously ignored ___bpf_prog_run() because it didn't understand the jump table. This resulted in the ORC unwinder not being able to unwind through non-JIT BPF code.

Now that objtool knows how to read jump tables, remove the whitelist and rename the variable to "jump_table" so objtool can recognize it.

Fixes d15d356887e7 ("perf/x86: Make perf callchains work without CONFIG_FRAME_POINTER")

Reported-by Song Liu <[email protected]>

Signed-off-by Josh Poimboeuf <[email protected]>

---
 kernel/bpf/core.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

kernel/bpf/core.c +1 -1

index 080e2bb644cc..ff66294882f8 100644
@@ -1299,7 +1299,7 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
 {
 #define BPF_INSN_2_LBL(x, y)    [BPF_##x | BPF_##y] = &&x##_##y
 #define BPF_INSN_3_LBL(x, y, z) [BPF_##x | BPF_##y | BPF_##z] = &&x##_##y##_##z
-       static const void *jumptable[256] = {
+       static const void *jump_table[256] = {
                [0 ... 255] = &&default_label,
                /* Now overwrite non-defaults ... */
                BPF_INSN_MAP(BPF_INSN_2_LBL, BPF_INSN_3_LBL),

kernel/bpf/core.c static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack) · line 1318

Quoted 5 lines

kernel/bpf/core.c +1 -1

@@ -1315,7 +1315,7 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
 #define CONT_JMP ({ insn++; goto select_insn; })
 
 select_insn:
-       goto *jumptable[insn->code];
+       goto *jump_table[insn->code];

I thought we were clear that it is a nack?
Either live it alone or rename to something like jump_table_bpf_interpreter or bpf_interpreter_jump_table.

Quoted 5 lines

As I have said many times:

The jump table detection is a generic objtool feature. It makes no sense to give a bpf-specific name to a generic objtool feature which can be used by other components.

Nacked-by Alexei Starovoitov <[email protected]>