Re: [PATCH v3 2/4] objtool: Add support for C jump tables

[Josh Poimboeuf <jpoimboe@redhat.com>]

On Wed, Jun 26, 2019 at 07:54:08PM -0700, Alexei Starovoitov wrote:
> On Wed, Jun 26, 2019 at 7:47 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >
> > On Wed, Jun 26, 2019 at 06:42:40PM -0700, Alexei Starovoitov wrote:
> > > > @@ -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 <ast@kernel.org>
> > >
> > > 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:
> >
> > -       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.
> 
> 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.

Completely untested swag:

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 8aaf7cd026b0..84212bcb5015 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -116,9 +116,14 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
 	".pushsection .discard.unreachable\n\t"				\
 	".long 999b - .\n\t"						\
 	".popsection\n\t"
+
+/* Annotate a C jump table to enable objtool to follow the code flow */
+#define __annotate_jump_table __section(".jump_table.rodata")
+
 #else
 #define annotate_reachable()
 #define annotate_unreachable()
+#define __annotate_jump_table
 #endif
 
 #ifndef ASM_UNREACHABLE
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 080e2bb644cc..e67977e22967 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -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 __annotate_jump_table *jumptable[256] = {
 		[0 ... 255] = &&default_label,
 		/* Now overwrite non-defaults ... */
 		BPF_INSN_MAP(BPF_INSN_2_LBL, BPF_INSN_3_LBL),
@@ -1558,7 +1558,6 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
 		BUG_ON(1);
 		return 0;
 }
-STACK_FRAME_NON_STANDARD(___bpf_prog_run); /* jump table */
 
 #define PROG_NAME(stack_size) __bpf_prog_run##stack_size
 #define DEFINE_BPF_PROG_RUN(stack_size) \
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 8341c2fff14f..d6d7dd65a195 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -18,7 +18,7 @@
 
 #define FAKE_JUMP_OFFSET -1
 
-#define JUMP_TABLE_SYM_PREFIX "jump_table."
+#define C_JUMP_TABLE_SECTION ".jump_table.rodata"
 
 struct alternative {
 	struct list_head list;
@@ -999,7 +999,6 @@ 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
@@ -1041,15 +1040,12 @@ static struct rela *find_switch_table(struct objtool_file *file,
 		 * 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.  Note: GCC will add a numbered suffix to the
-		 * ELF symbol name, like "jump_table.12345", which it does for
-		 * all static local variables.
+		 * switch jump tables.  For objtool to recognize them, they
+		 * need to be placed in the C_JUMP_TABLE_SECTION section.  They
+		 * have symbols associated with them.
 		 */
-		sym = find_symbol_containing(rodata_sec, table_offset);
-		if (sym && strncmp(sym->name, JUMP_TABLE_SYM_PREFIX,
-				   strlen(JUMP_TABLE_SYM_PREFIX)))
+		if (find_symbol_containing(rodata_sec, table_offset) &&
+		    strcmp(rodata_sec->name, C_JUMP_TABLE_SECTION))
 			continue;
 
 		rodata_rela = find_rela_by_dest(rodata_sec, table_offset);
@@ -1289,13 +1285,19 @@ static void mark_rodata(struct objtool_file *file)
 	bool found = false;
 
 	/*
-	 * This searches for the .rodata section or multiple .rodata.func_name
-	 * sections if -fdata-sections is being used. The .str.1.1 and .str.1.8
-	 * rodata sections are ignored as they don't contain jump tables.
+	 * This searches for the following rodata sections, each of which can
+	 * potentially contain jump tables:
+	 *
+	 * - .rodata - can contain GCC switch tables
+	 * - .rodata.func_name - if -fdata-sections * is being used
+	 * - .jump_table.rodata - contains C annotated jump tables
+	 *
+	 * The .rodata.str.* sections are ignored because they don't contain
+	 * jump tables.
 	 */
 	for_each_sec(file, sec) {
-		if (!strncmp(sec->name, ".rodata", 7) &&
-		    !strstr(sec->name, ".str1.")) {
+		if ((!strncmp(sec->name, ".rodata", 7) && !strstr(sec->name, ".str1.")) ||
+		    !strcmp(sec->name, C_JUMP_TABLE_SECTION)) {
 			sec->rodata = true;
 			found = true;
 		}