[PATCH v2 0/9] Make check implementation arch agnostic

[PATCH v2 0/9] Make check implementation arch agnostic

[Julien Thierry]

Hi,

The current implementation of the check subcommand has various x86 bits
here and there. In order to prepare objtool to provide check for other
architectures, add some abstraction over the x86 specific bits, relying
on objtool arch specific code to provide some necessary operations.

This is part of the effort to implement check for arm64, initiated [1]
by Raphael. The series is based on top of the separation of check & orc
subcommands series[2].

I've push both series base on top of tip/objtool/core at [3].

- The first two patches make it simpler for new arches to provide their
list of kernel headers, without worrying about modifications in the x86
headers.
- Patch 3 Moves arch specific macros to more suitable location
- Patches 4 and 5 add abstraction to handle alternatives
- Patch 6 adds abstraction to handle jump table
- Patches 7-9 abstracts the use of unwind hints, so some definitions
  can be shared across architectures while keeping arch specific
  semantics

Changes since v1 [4]:
- Rebased on recent tip/objtool/core
- Split the unwind hint rework into multiple patches as suggested by
  Miroslav

[1] https://lkml.org/lkml/2019/8/16/400
[2] https://lkml.org/lkml/2020/7/30/415
[3] https://github.com/julien-thierry/linux/tree/arch-independent-check
[4] https://lkml.org/lkml/2020/6/8/535

Cheers,

Julien

-->

Julien Thierry (8):
  objtool: Group headers to check in a single list
  objtool: Make sync-check consider the target architecture
  objtool: Move macros describing structures to arch-dependent code
  objtool: Abstract alternative special case handling
  objtool: Make relocation in alternative handling arch dependent
  frame: Only include valid definitions depending on source file type
  frame: Make unwind hints definitions available to other architectures
  objtool: Abstract unwind hint reading

Raphael Gault (1):
  objtool: Refactor switch-tables code to support other architectures

 arch/x86/include/asm/orc_types.h              |  13 --
 arch/x86/include/asm/unwind_hints.h           |  44 +----
 include/linux/frame.h                         |  78 ++++++++
 tools/arch/x86/include/asm/orc_types.h        |  13 --
 tools/include/linux/frame.h                   | 113 ++++++++++++
 tools/objtool/Makefile                        |   2 +-
 tools/objtool/arch.h                          |   5 +-
 tools/objtool/arch/x86/Build                  |   1 +
 tools/objtool/arch/x86/arch_special.c         | 145 +++++++++++++++
 tools/objtool/arch/x86/decode.c               |  54 ++++++
 tools/objtool/arch/x86/include/arch_special.h |  20 +++
 tools/objtool/cfi.h                           |   3 +-
 tools/objtool/check.c                         | 166 ++----------------
 tools/objtool/check.h                         |   7 +-
 tools/objtool/objtool.h                       |   2 +
 tools/objtool/orc_gen.c                       |   4 +-
 tools/objtool/special.c                       |  48 +----
 tools/objtool/special.h                       |  10 ++
 tools/objtool/sync-check.sh                   |  27 ++-
 tools/objtool/weak.c                          |   2 -
 20 files changed, 486 insertions(+), 271 deletions(-)
 create mode 100644 tools/include/linux/frame.h
 create mode 100644 tools/objtool/arch/x86/arch_special.c
 create mode 100644 tools/objtool/arch/x86/include/arch_special.h

--
2.21.3

[PATCH v2 1/9] objtool: Group headers to check in a single list

[Julien Thierry]

In order to support multiple architectures and potentially different
sets of header to compare against their kernel equivalent, it is simpler
to have all headers to check in a single list.

Reviewed-by: Miroslav Benes <mbenes@suse.cz>
Signed-off-by: Julien Thierry <jthierry@redhat.com>
---
 tools/objtool/sync-check.sh | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/tools/objtool/sync-check.sh b/tools/objtool/sync-check.sh
index 2a1261bfbb62..13e4fca28015 100755
--- a/tools/objtool/sync-check.sh
+++ b/tools/objtool/sync-check.sh
@@ -1,13 +1,17 @@
 #!/bin/sh
 # SPDX-License-Identifier: GPL-2.0
 
-FILES='
+FILES="
 arch/x86/include/asm/inat_types.h
 arch/x86/include/asm/orc_types.h
 arch/x86/include/asm/emulate_prefix.h
 arch/x86/lib/x86-opcode-map.txt
 arch/x86/tools/gen-insn-attr-x86.awk
-'
+arch/x86/include/asm/inat.h     -I '^#include [\"<]\(asm/\)*inat_types.h[\">]'
+arch/x86/include/asm/insn.h     -I '^#include [\"<]\(asm/\)*inat.h[\">]'
+arch/x86/lib/inat.c             -I '^#include [\"<]\(../include/\)*asm/insn.h[\">]'
+arch/x86/lib/insn.c             -I '^#include [\"<]\(../include/\)*asm/in\(at\|sn\).h[\">]' -I '^#include [\"<]\(../include/\)*asm/emulate_prefix.h[\">]'
+"
 
 check_2 () {
   file1=$1
@@ -40,11 +44,10 @@ fi
 
 cd ../..
 
-for i in $FILES; do
-  check $i
-done
+while read -r file_entry; do
+    if [ -z "$file_entry" ]; then
+	continue
+    fi
 
-check arch/x86/include/asm/inat.h     '-I "^#include [\"<]\(asm/\)*inat_types.h[\">]"'
-check arch/x86/include/asm/insn.h     '-I "^#include [\"<]\(asm/\)*inat.h[\">]"'
-check arch/x86/lib/inat.c             '-I "^#include [\"<]\(../include/\)*asm/insn.h[\">]"'
-check arch/x86/lib/insn.c             '-I "^#include [\"<]\(../include/\)*asm/in\(at\|sn\).h[\">]" -I "^#include [\"<]\(../include/\)*asm/emulate_prefix.h[\">]"'
+    check $file_entry
+done <<< $FILES
-- 
2.21.3

[PATCH v2 2/9] objtool: Make sync-check consider the target architecture

[Julien Thierry]

Do not take into account outdated headers unrelated to the build of the
current architecture.

Reviewed-by: Miroslav Benes <mbenes@suse.cz>
Signed-off-by: Julien Thierry <jthierry@redhat.com>
---
 tools/objtool/Makefile      | 2 +-
 tools/objtool/sync-check.sh | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/tools/objtool/Makefile b/tools/objtool/Makefile
index 7770edcda3a0..614b87278260 100644
--- a/tools/objtool/Makefile
+++ b/tools/objtool/Makefile
@@ -60,7 +60,7 @@ export srctree OUTPUT CFLAGS SRCARCH AWK
 include $(srctree)/tools/build/Makefile.include
 
 $(OBJTOOL_IN): fixdep FORCE
-	@$(CONFIG_SHELL) ./sync-check.sh
+	@$(CONFIG_SHELL) ./sync-check.sh $(SRCARCH)
 	@$(MAKE) $(build)=objtool
 
 $(OBJTOOL): $(LIBSUBCMD) $(OBJTOOL_IN)
diff --git a/tools/objtool/sync-check.sh b/tools/objtool/sync-check.sh
index 13e4fca28015..f01b5a4d12ac 100755
--- a/tools/objtool/sync-check.sh
+++ b/tools/objtool/sync-check.sh
@@ -1,6 +1,9 @@
 #!/bin/sh
 # SPDX-License-Identifier: GPL-2.0
 
+TARGET_ARCH=$1
+
+if [ "$TARGET_ARCH" == "x86" ]; then
 FILES="
 arch/x86/include/asm/inat_types.h
 arch/x86/include/asm/orc_types.h
@@ -12,6 +15,7 @@ arch/x86/include/asm/insn.h     -I '^#include [\"<]\(asm/\)*inat.h[\">]'
 arch/x86/lib/inat.c             -I '^#include [\"<]\(../include/\)*asm/insn.h[\">]'
 arch/x86/lib/insn.c             -I '^#include [\"<]\(../include/\)*asm/in\(at\|sn\).h[\">]' -I '^#include [\"<]\(../include/\)*asm/emulate_prefix.h[\">]'
 "
+fi
 
 check_2 () {
   file1=$1
-- 
2.21.3

[PATCH v2 3/9] objtool: Move macros describing structures to arch-dependent code

[Julien Thierry]

Some macros are defined to describe the size and layout of structures
exception_table_entry, jump_entry and alt_instr. These values can vary
from one architecture to another.

Have the values be defined by arch specific code.

Suggested-by: Raphael Gault <raphael.gault@arm.com>
Reviewed-by: Miroslav Benes <mbenes@suse.cz>
Signed-off-by: Julien Thierry <jthierry@redhat.com>
---
 tools/objtool/arch/x86/include/arch_special.h | 20 +++++++++++++++++++
 tools/objtool/special.c                       | 16 +--------------
 2 files changed, 21 insertions(+), 15 deletions(-)
 create mode 100644 tools/objtool/arch/x86/include/arch_special.h

diff --git a/tools/objtool/arch/x86/include/arch_special.h b/tools/objtool/arch/x86/include/arch_special.h
new file mode 100644
index 000000000000..d818b2bffa02
--- /dev/null
+++ b/tools/objtool/arch/x86/include/arch_special.h
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+#ifndef _X86_ARCH_SPECIAL_H
+#define _X86_ARCH_SPECIAL_H
+
+#define EX_ENTRY_SIZE		12
+#define EX_ORIG_OFFSET		0
+#define EX_NEW_OFFSET		4
+
+#define JUMP_ENTRY_SIZE		16
+#define JUMP_ORIG_OFFSET	0
+#define JUMP_NEW_OFFSET		4
+
+#define ALT_ENTRY_SIZE		13
+#define ALT_ORIG_OFFSET		0
+#define ALT_NEW_OFFSET		4
+#define ALT_FEATURE_OFFSET	8
+#define ALT_ORIG_LEN_OFFSET	10
+#define ALT_NEW_LEN_OFFSET	11
+
+#endif /* _X86_ARCH_SPECIAL_H */
diff --git a/tools/objtool/special.c b/tools/objtool/special.c
index e893f1e48e44..b04f395de5de 100644
--- a/tools/objtool/special.c
+++ b/tools/objtool/special.c
@@ -14,21 +14,7 @@
 #include "builtin.h"
 #include "special.h"
 #include "warn.h"
-
-#define EX_ENTRY_SIZE		12
-#define EX_ORIG_OFFSET		0
-#define EX_NEW_OFFSET		4
-
-#define JUMP_ENTRY_SIZE		16
-#define JUMP_ORIG_OFFSET	0
-#define JUMP_NEW_OFFSET		4
-
-#define ALT_ENTRY_SIZE		13
-#define ALT_ORIG_OFFSET		0
-#define ALT_NEW_OFFSET		4
-#define ALT_FEATURE_OFFSET	8
-#define ALT_ORIG_LEN_OFFSET	10
-#define ALT_NEW_LEN_OFFSET	11
+#include "arch_special.h"
 
 #define X86_FEATURE_POPCNT (4*32+23)
 #define X86_FEATURE_SMAP   (9*32+20)
-- 
2.21.3

[PATCH v2 4/9] objtool: Abstract alternative special case handling

[Julien Thierry]

Some alternatives associated with a specific feature need to be treated
in a special way. Since the features and how to treat them vary from one
architecture to another, move the special case handling to arch specific
code.

Reviewed-by: Miroslav Benes <mbenes@suse.cz>
Signed-off-by: Julien Thierry <jthierry@redhat.com>
---
 tools/objtool/arch/x86/Build          |  1 +
 tools/objtool/arch/x86/arch_special.c | 37 +++++++++++++++++++++++++++
 tools/objtool/objtool.h               |  2 ++
 tools/objtool/special.c               | 32 ++++-------------------
 tools/objtool/special.h               |  2 ++
 tools/objtool/weak.c                  |  2 --
 6 files changed, 47 insertions(+), 29 deletions(-)
 create mode 100644 tools/objtool/arch/x86/arch_special.c

diff --git a/tools/objtool/arch/x86/Build b/tools/objtool/arch/x86/Build
index 7c5004008e97..2c3ac13cfa8b 100644
--- a/tools/objtool/arch/x86/Build
+++ b/tools/objtool/arch/x86/Build
@@ -1,3 +1,4 @@
+objtool-y += arch_special.o
 objtool-y += decode.o
 
 inat_tables_script = ../arch/x86/tools/gen-insn-attr-x86.awk
diff --git a/tools/objtool/arch/x86/arch_special.c b/tools/objtool/arch/x86/arch_special.c
new file mode 100644
index 000000000000..823561e4015c
--- /dev/null
+++ b/tools/objtool/arch/x86/arch_special.c
@@ -0,0 +1,37 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+#include "../../special.h"
+#include "../../builtin.h"
+
+#define X86_FEATURE_POPCNT (4 * 32 + 23)
+#define X86_FEATURE_SMAP   (9 * 32 + 20)
+
+void arch_handle_alternative(unsigned short feature, struct special_alt *alt)
+{
+	switch (feature) {
+	case X86_FEATURE_SMAP:
+		/*
+		 * If UACCESS validation is enabled; force that alternative;
+		 * otherwise force it the other way.
+		 *
+		 * What we want to avoid is having both the original and the
+		 * alternative code flow at the same time, in that case we can
+		 * find paths that see the STAC but take the NOP instead of
+		 * CLAC and the other way around.
+		 */
+		if (uaccess)
+			alt->skip_orig = true;
+		else
+			alt->skip_alt = true;
+		break;
+	case X86_FEATURE_POPCNT:
+		/*
+		 * It has been requested that we don't validate the !POPCNT
+		 * feature path which is a "very very small percentage of
+		 * machines".
+		 */
+		alt->skip_orig = true;
+		break;
+	default:
+		break;
+	}
+}
diff --git a/tools/objtool/objtool.h b/tools/objtool/objtool.h
index e61b486d4c05..1f85409a57f9 100644
--- a/tools/objtool/objtool.h
+++ b/tools/objtool/objtool.h
@@ -12,6 +12,8 @@
 
 #include "elf.h"
 
+#define __weak __attribute__((weak))
+
 struct objtool_file {
 	struct elf *elf;
 	struct list_head insn_list;
diff --git a/tools/objtool/special.c b/tools/objtool/special.c
index b04f395de5de..1a2420febd08 100644
--- a/tools/objtool/special.c
+++ b/tools/objtool/special.c
@@ -16,9 +16,6 @@
 #include "warn.h"
 #include "arch_special.h"
 
-#define X86_FEATURE_POPCNT (4*32+23)
-#define X86_FEATURE_SMAP   (9*32+20)
-
 struct special_entry {
 	const char *sec;
 	bool group, jump_or_nop;
@@ -54,6 +51,10 @@ struct special_entry entries[] = {
 	{},
 };
 
+void __weak arch_handle_alternative(unsigned short feature, struct special_alt *alt)
+{
+}
+
 static int get_alt_entry(struct elf *elf, struct special_entry *entry,
 			 struct section *sec, int idx,
 			 struct special_alt *alt)
@@ -78,30 +79,7 @@ static int get_alt_entry(struct elf *elf, struct special_entry *entry,
 
 		feature = *(unsigned short *)(sec->data->d_buf + offset +
 					      entry->feature);
-
-		/*
-		 * It has been requested that we don't validate the !POPCNT
-		 * feature path which is a "very very small percentage of
-		 * machines".
-		 */
-		if (feature == X86_FEATURE_POPCNT)
-			alt->skip_orig = true;
-
-		/*
-		 * If UACCESS validation is enabled; force that alternative;
-		 * otherwise force it the other way.
-		 *
-		 * What we want to avoid is having both the original and the
-		 * alternative code flow at the same time, in that case we can
-		 * find paths that see the STAC but take the NOP instead of
-		 * CLAC and the other way around.
-		 */
-		if (feature == X86_FEATURE_SMAP) {
-			if (uaccess)
-				alt->skip_orig = true;
-			else
-				alt->skip_alt = true;
-		}
+		arch_handle_alternative(feature, alt);
 	}
 
 	orig_reloc = find_reloc_by_dest(elf, sec, offset + entry->orig);
diff --git a/tools/objtool/special.h b/tools/objtool/special.h
index 35061530e46e..44da89afeda2 100644
--- a/tools/objtool/special.h
+++ b/tools/objtool/special.h
@@ -28,4 +28,6 @@ struct special_alt {
 
 int special_get_alts(struct elf *elf, struct list_head *alts);
 
+void arch_handle_alternative(unsigned short feature, struct special_alt *alt);
+
 #endif /* _SPECIAL_H */
diff --git a/tools/objtool/weak.c b/tools/objtool/weak.c
index 29180d599b08..7843e9a7a72f 100644
--- a/tools/objtool/weak.c
+++ b/tools/objtool/weak.c
@@ -9,8 +9,6 @@
 #include <errno.h>
 #include "objtool.h"
 
-#define __weak __attribute__((weak))
-
 #define UNSUPPORTED(name)						\
 ({									\
 	fprintf(stderr, "error: objtool: " name " not implemented\n");	\
-- 
2.21.3

[PATCH v2 5/9] objtool: Make relocation in alternative handling arch dependent

[Julien Thierry]

As pointed out by the comment in handle_group_alt(), support of
relocation for instructions in an alternative group depends on whether
arch specific kernel code handles it.

So, let objtool arch specific code decide whether a relocation for
the alternative section should be accepted.

Reviewed-by: Miroslav Benes <mbenes@suse.cz>
Signed-off-by: Julien Thierry <jthierry@redhat.com>
---
 tools/objtool/arch/x86/arch_special.c | 13 +++++++++++++
 tools/objtool/check.c                 | 19 ++++++-------------
 tools/objtool/check.h                 |  6 ++++++
 tools/objtool/special.h               |  4 ++++
 4 files changed, 29 insertions(+), 13 deletions(-)

diff --git a/tools/objtool/arch/x86/arch_special.c b/tools/objtool/arch/x86/arch_special.c
index 823561e4015c..34e0e162e6fd 100644
--- a/tools/objtool/arch/x86/arch_special.c
+++ b/tools/objtool/arch/x86/arch_special.c
@@ -35,3 +35,16 @@ void arch_handle_alternative(unsigned short feature, struct special_alt *alt)
 		break;
 	}
 }
+
+bool arch_support_alt_relocation(struct special_alt *special_alt,
+				 struct instruction *insn,
+				 struct reloc *reloc)
+{
+	/*
+	 * The x86 alternatives code adjusts the offsets only when it
+	 * encounters a branch instruction at the very beginning of the
+	 * replacement group.
+	 */
+	return insn->offset == special_alt->new_off &&
+	       (insn->type == INSN_CALL || is_static_jump(insn));
+}
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index bb19e4c79e46..18ef9c64719f 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -109,12 +109,6 @@ static struct instruction *prev_insn_same_sym(struct objtool_file *file,
 	for (insn = next_insn_same_sec(file, insn); insn;		\
 	     insn = next_insn_same_sec(file, insn))
 
-static bool is_static_jump(struct instruction *insn)
-{
-	return insn->type == INSN_JUMP_CONDITIONAL ||
-	       insn->type == INSN_JUMP_UNCONDITIONAL;
-}
-
 static bool is_sibling_call(struct instruction *insn)
 {
 	/* An indirect jump is either a sibling call or a jump to a table. */
@@ -866,6 +860,8 @@ static int handle_group_alt(struct objtool_file *file,
 	alt_group = alt_group_next_index++;
 	insn = *new_insn;
 	sec_for_each_insn_from(file, insn) {
+		struct reloc *alt_reloc;
+
 		if (insn->offset >= special_alt->new_off + special_alt->new_len)
 			break;
 
@@ -882,14 +878,11 @@ static int handle_group_alt(struct objtool_file *file,
 		 * .altinstr_replacement section, unless the arch's
 		 * alternatives code can adjust the relative offsets
 		 * accordingly.
-		 *
-		 * The x86 alternatives code adjusts the offsets only when it
-		 * encounters a branch instruction at the very beginning of the
-		 * replacement group.
 		 */
-		if ((insn->offset != special_alt->new_off ||
-		    (insn->type != INSN_CALL && !is_static_jump(insn))) &&
-		    find_reloc_by_dest_range(file->elf, insn->sec, insn->offset, insn->len)) {
+		alt_reloc = find_reloc_by_dest_range(file->elf, insn->sec,
+						   insn->offset, insn->len);
+		if (alt_reloc &&
+		    !arch_support_alt_relocation(special_alt, insn, alt_reloc)) {
 
 			WARN_FUNC("unsupported relocation in alternatives section",
 				  insn->sec, insn->offset);
diff --git a/tools/objtool/check.h b/tools/objtool/check.h
index 059c43bfeb18..c08ca6cd9a89 100644
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -44,6 +44,12 @@ struct instruction {
 	struct cfi_state cfi;
 };
 
+static inline bool is_static_jump(struct instruction *insn)
+{
+	return insn->type == INSN_JUMP_CONDITIONAL ||
+	       insn->type == INSN_JUMP_UNCONDITIONAL;
+}
+
 struct instruction *find_insn(struct objtool_file *file,
 			      struct section *sec, unsigned long offset);
 
diff --git a/tools/objtool/special.h b/tools/objtool/special.h
index 44da89afeda2..1dc1bb3e74c6 100644
--- a/tools/objtool/special.h
+++ b/tools/objtool/special.h
@@ -7,6 +7,7 @@
 #define _SPECIAL_H
 
 #include <stdbool.h>
+#include "check.h"
 #include "elf.h"
 
 struct special_alt {
@@ -30,4 +31,7 @@ int special_get_alts(struct elf *elf, struct list_head *alts);
 
 void arch_handle_alternative(unsigned short feature, struct special_alt *alt);
 
+bool arch_support_alt_relocation(struct special_alt *special_alt,
+				 struct instruction *insn,
+				 struct reloc *reloc);
 #endif /* _SPECIAL_H */
-- 
2.21.3

[PATCH v2 6/9] objtool: Refactor switch-tables code to support other architectures

[Julien Thierry]

From: Raphael Gault <raphael.gault@arm.com>

The way to identify switch-tables and retrieves all the data necessary
to handle the different execution branches is not the same on all
architecture. In order to be able to add other architecture support,
define an arch-dependent function to process jump-tables.

Reviewed-by: Miroslav Benes <mbenes@suse.cz>
Signed-off-by: Raphael Gault <raphael.gault@arm.com>
[J.T.: Move arm64 bits out of this patch,
       Have only one function to find the start of the jump table,
       for now assume that the jump table format will be the same as
       x86]
Signed-off-by: Julien Thierry <jthierry@redhat.com>
---
 tools/objtool/arch/x86/arch_special.c | 95 +++++++++++++++++++++++++++
 tools/objtool/check.c                 | 88 +------------------------
 tools/objtool/check.h                 |  1 -
 tools/objtool/special.h               |  4 ++
 4 files changed, 102 insertions(+), 86 deletions(-)

diff --git a/tools/objtool/arch/x86/arch_special.c b/tools/objtool/arch/x86/arch_special.c
index 34e0e162e6fd..fd4af88c0ea5 100644
--- a/tools/objtool/arch/x86/arch_special.c
+++ b/tools/objtool/arch/x86/arch_special.c
@@ -1,4 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
+#include <string.h>
+
 #include "../../special.h"
 #include "../../builtin.h"
 
@@ -48,3 +50,96 @@ bool arch_support_alt_relocation(struct special_alt *special_alt,
 	return insn->offset == special_alt->new_off &&
 	       (insn->type == INSN_CALL || is_static_jump(insn));
 }
+
+/*
+ * There are 3 basic jump table patterns:
+ *
+ * 1. jmpq *[rodata addr](,%reg,8)
+ *
+ *    This is the most common case by far.  It jumps to an address in a simple
+ *    jump table which is stored in .rodata.
+ *
+ * 2. jmpq *[rodata addr](%rip)
+ *
+ *    This is caused by a rare GCC quirk, currently only seen in three driver
+ *    functions in the kernel, only with certain obscure non-distro configs.
+ *
+ *    As part of an optimization, GCC makes a copy of an existing switch jump
+ *    table, modifies it, and then hard-codes the jump (albeit with an indirect
+ *    jump) to use a single entry in the table.  The rest of the jump table and
+ *    some of its jump targets remain as dead code.
+ *
+ *    In such a case we can just crudely ignore all unreachable instruction
+ *    warnings for the entire object file.  Ideally we would just ignore them
+ *    for the function, but that would require redesigning the code quite a
+ *    bit.  And honestly that's just not worth doing: unreachable instruction
+ *    warnings are of questionable value anyway, and this is such a rare issue.
+ *
+ * 3. mov [rodata addr],%reg1
+ *    ... some instructions ...
+ *    jmpq *(%reg1,%reg2,8)
+ *
+ *    This is a fairly uncommon pattern which is new for GCC 6.  As of this
+ *    writing, there are 11 occurrences of it in the allmodconfig kernel.
+ *
+ *    As of GCC 7 there are quite a few more of these and the 'in between' code
+ *    is significant. Esp. with KASAN enabled some of the code between the mov
+ *    and jmpq uses .rodata itself, which can confuse things.
+ *
+ *    TODO: Once we have DWARF CFI and smarter instruction decoding logic,
+ *    ensure the same register is used in the mov and jump instructions.
+ *
+ *    NOTE: RETPOLINE made it harder still to decode dynamic jumps.
+ */
+struct reloc *arch_find_switch_table(struct objtool_file *file,
+				    struct instruction *insn)
+{
+	struct reloc  *text_reloc, *rodata_reloc;
+	struct section *table_sec;
+	unsigned long table_offset;
+
+	/* look for a relocation which references .rodata */
+	text_reloc = find_reloc_by_dest_range(file->elf, insn->sec,
+					      insn->offset, insn->len);
+	if (!text_reloc || text_reloc->sym->type != STT_SECTION ||
+	    !text_reloc->sym->sec->rodata)
+		return NULL;
+
+	table_offset = text_reloc->addend;
+	table_sec = text_reloc->sym->sec;
+
+	if (text_reloc->type == R_X86_64_PC32)
+		table_offset += 4;
+
+	/*
+	 * Make sure the .rodata address isn't associated with a
+	 * symbol.  GCC jump tables are anonymous data.
+	 *
+	 * Also support C jump tables which are in the same format as
+	 * 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.
+	 */
+	if (find_symbol_containing(table_sec, table_offset) &&
+	    strcmp(table_sec->name, C_JUMP_TABLE_SECTION))
+		return NULL;
+
+	/*
+	 * Each table entry has a rela associated with it.  The rela
+	 * should reference text in the same function as the original
+	 * instruction.
+	 */
+	rodata_reloc = find_reloc_by_dest(file->elf, table_sec, table_offset);
+	if (!rodata_reloc)
+		return NULL;
+
+	/*
+	 * Use of RIP-relative switch jumps is quite rare, and
+	 * indicates a rare GCC quirk/bug which can leave dead
+	 * code behind.
+	 */
+	if (text_reloc->type == R_X86_64_PC32)
+		file->ignore_unreachables = true;
+
+	return rodata_reloc;
+}
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 18ef9c64719f..60f5be2accf6 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -19,8 +19,6 @@
 
 #define FAKE_JUMP_OFFSET -1
 
-#define C_JUMP_TABLE_SECTION ".rodata..c_jump_table"
-
 struct alternative {
 	struct list_head list;
 	struct instruction *insn;
@@ -1085,55 +1083,14 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn,
 
 /*
  * find_jump_table() - Given a dynamic jump, find the switch jump table in
- * .rodata associated with it.
- *
- * There are 3 basic patterns:
- *
- * 1. jmpq *[rodata addr](,%reg,8)
- *
- *    This is the most common case by far.  It jumps to an address in a simple
- *    jump table which is stored in .rodata.
- *
- * 2. jmpq *[rodata addr](%rip)
- *
- *    This is caused by a rare GCC quirk, currently only seen in three driver
- *    functions in the kernel, only with certain obscure non-distro configs.
- *
- *    As part of an optimization, GCC makes a copy of an existing switch jump
- *    table, modifies it, and then hard-codes the jump (albeit with an indirect
- *    jump) to use a single entry in the table.  The rest of the jump table and
- *    some of its jump targets remain as dead code.
- *
- *    In such a case we can just crudely ignore all unreachable instruction
- *    warnings for the entire object file.  Ideally we would just ignore them
- *    for the function, but that would require redesigning the code quite a
- *    bit.  And honestly that's just not worth doing: unreachable instruction
- *    warnings are of questionable value anyway, and this is such a rare issue.
- *
- * 3. mov [rodata addr],%reg1
- *    ... some instructions ...
- *    jmpq *(%reg1,%reg2,8)
- *
- *    This is a fairly uncommon pattern which is new for GCC 6.  As of this
- *    writing, there are 11 occurrences of it in the allmodconfig kernel.
- *
- *    As of GCC 7 there are quite a few more of these and the 'in between' code
- *    is significant. Esp. with KASAN enabled some of the code between the mov
- *    and jmpq uses .rodata itself, which can confuse things.
- *
- *    TODO: Once we have DWARF CFI and smarter instruction decoding logic,
- *    ensure the same register is used in the mov and jump instructions.
- *
- *    NOTE: RETPOLINE made it harder still to decode dynamic jumps.
+ * associated with it.
  */
 static struct reloc *find_jump_table(struct objtool_file *file,
 				      struct symbol *func,
 				      struct instruction *insn)
 {
-	struct reloc *text_reloc, *table_reloc;
+	struct reloc *table_reloc;
 	struct instruction *dest_insn, *orig_insn = insn;
-	struct section *table_sec;
-	unsigned long table_offset;
 
 	/*
 	 * Backward search using the @first_jump_src links, these help avoid
@@ -1154,52 +1111,13 @@ static struct reloc *find_jump_table(struct objtool_file *file,
 		     insn->jump_dest->offset > orig_insn->offset))
 		    break;
 
-		/* look for a relocation which references .rodata */
-		text_reloc = find_reloc_by_dest_range(file->elf, insn->sec,
-						    insn->offset, insn->len);
-		if (!text_reloc || text_reloc->sym->type != STT_SECTION ||
-		    !text_reloc->sym->sec->rodata)
-			continue;
-
-		table_offset = text_reloc->addend;
-		table_sec = text_reloc->sym->sec;
-
-		if (text_reloc->type == R_X86_64_PC32)
-			table_offset += 4;
-
-		/*
-		 * Make sure the .rodata address isn't associated with a
-		 * symbol.  GCC jump tables are anonymous data.
-		 *
-		 * Also support C jump tables which are in the same format as
-		 * 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.
-		 */
-		if (find_symbol_containing(table_sec, table_offset) &&
-		    strcmp(table_sec->name, C_JUMP_TABLE_SECTION))
-			continue;
-
-		/*
-		 * Each table entry has a reloc associated with it.  The reloc
-		 * should reference text in the same function as the original
-		 * instruction.
-		 */
-		table_reloc = find_reloc_by_dest(file->elf, table_sec, table_offset);
+		table_reloc = arch_find_switch_table(file, insn);
 		if (!table_reloc)
 			continue;
 		dest_insn = find_insn(file, table_reloc->sym->sec, table_reloc->addend);
 		if (!dest_insn || !dest_insn->func || dest_insn->func->pfunc != func)
 			continue;
 
-		/*
-		 * Use of RIP-relative switch jumps is quite rare, and
-		 * indicates a rare GCC quirk/bug which can leave dead code
-		 * behind.
-		 */
-		if (text_reloc->type == R_X86_64_PC32)
-			file->ignore_unreachables = true;
-
 		return table_reloc;
 	}
 
diff --git a/tools/objtool/check.h b/tools/objtool/check.h
index c08ca6cd9a89..8a1a49407cac 100644
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -62,5 +62,4 @@ struct instruction *find_insn(struct objtool_file *file,
 			insn->sec == sec;				\
 	     insn = list_next_entry(insn, list))
 
-
 #endif /* _CHECK_H */
diff --git a/tools/objtool/special.h b/tools/objtool/special.h
index 1dc1bb3e74c6..abddf38ef334 100644
--- a/tools/objtool/special.h
+++ b/tools/objtool/special.h
@@ -10,6 +10,8 @@
 #include "check.h"
 #include "elf.h"
 
+#define C_JUMP_TABLE_SECTION ".rodata..c_jump_table"
+
 struct special_alt {
 	struct list_head list;
 
@@ -34,4 +36,6 @@ void arch_handle_alternative(unsigned short feature, struct special_alt *alt);
 bool arch_support_alt_relocation(struct special_alt *special_alt,
 				 struct instruction *insn,
 				 struct reloc *reloc);
+struct reloc *arch_find_switch_table(struct objtool_file *file,
+				    struct instruction *insn);
 #endif /* _SPECIAL_H */
-- 
2.21.3

[PATCH v2 7/9] frame: Only include valid definitions depending on source file type

[Julien Thierry]

Header include/linux/frame.h contains both C and assembly definition that
are visible regardless of the file including them.

Place definition under conditional __ASSEMBLY__.

Signed-off-by: Julien Thierry <jthierry@redhat.com>
---
 include/linux/frame.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/frame.h b/include/linux/frame.h
index 303cda600e56..d946adb5de17 100644
--- a/include/linux/frame.h
+++ b/include/linux/frame.h
@@ -3,6 +3,8 @@
 #define _LINUX_FRAME_H
 
 #ifdef CONFIG_STACK_VALIDATION
+
+#ifndef __ASSEMBLY__
 /*
  * This macro marks the given function's stack frame as "non-standard", which
  * tells objtool to ignore the function when doing stack metadata validation.
@@ -15,6 +17,8 @@
 	static void __used __section(.discard.func_stack_frame_non_standard) \
 		*__func_stack_frame_non_standard_##func = func
 
+#else /* __ASSEMBLY__ */
+
 /*
  * This macro indicates that the following intra-function call is valid.
  * Any non-annotated intra-function call will cause objtool to issue a warning.
@@ -25,6 +29,8 @@
 	.long 999b;						\
 	.popsection;
 
+#endif /* __ASSEMBLY__ */
+
 #else /* !CONFIG_STACK_VALIDATION */
 
 #define STACK_FRAME_NON_STANDARD(func)
-- 
2.21.3

[PATCH v2 8/9] frame: Make unwind hints definitions available to other architectures

[Julien Thierry]

Unwind hints are useful to provide objtool with information about stack
states in non-standard functions/code.
While the type of information being provided might be very arch
specific, the mechanism to provide the information can be useful for
other architectures.

Move the relevant unwint hint definitions for all architectures to
see.

Signed-off-by: Julien Thierry <jthierry@redhat.com>
---
 arch/x86/include/asm/orc_types.h       |  13 ---
 arch/x86/include/asm/unwind_hints.h    |  44 ++--------
 include/linux/frame.h                  |  72 ++++++++++++++++
 tools/arch/x86/include/asm/orc_types.h |  13 ---
 tools/include/linux/frame.h            | 113 +++++++++++++++++++++++++
 tools/objtool/check.c                  |   1 +
 tools/objtool/sync-check.sh            |   4 +-
 7 files changed, 194 insertions(+), 66 deletions(-)
 create mode 100644 tools/include/linux/frame.h

diff --git a/arch/x86/include/asm/orc_types.h b/arch/x86/include/asm/orc_types.h
index d25534940bde..00408d30b13e 100644
--- a/arch/x86/include/asm/orc_types.h
+++ b/arch/x86/include/asm/orc_types.h
@@ -78,19 +78,6 @@ struct orc_entry {
 	unsigned	end:1;
 } __packed;
 
-/*
- * This struct is used by asm and inline asm code to manually annotate the
- * location of registers on the stack for the ORC unwinder.
- *
- * Type can be either ORC_TYPE_* or UNWIND_HINT_TYPE_*.
- */
-struct unwind_hint {
-	u32		ip;
-	s16		sp_offset;
-	u8		sp_reg;
-	u8		type;
-	u8		end;
-};
 #endif /* __ASSEMBLY__ */
 
 #endif /* _ORC_TYPES_H */
diff --git a/arch/x86/include/asm/unwind_hints.h b/arch/x86/include/asm/unwind_hints.h
index 7d903fdb3f43..e81a18bb114f 100644
--- a/arch/x86/include/asm/unwind_hints.h
+++ b/arch/x86/include/asm/unwind_hints.h
@@ -1,48 +1,14 @@
 #ifndef _ASM_X86_UNWIND_HINTS_H
 #define _ASM_X86_UNWIND_HINTS_H
 
+#include <linux/frame.h>
+
 #include "orc_types.h"
 
 #ifdef __ASSEMBLY__
 
-/*
- * In asm, there are two kinds of code: normal C-type callable functions and
- * the rest.  The normal callable functions can be called by other code, and
- * don't do anything unusual with the stack.  Such normal callable functions
- * are annotated with the ENTRY/ENDPROC macros.  Most asm code falls in this
- * category.  In this case, no special debugging annotations are needed because
- * objtool can automatically generate the ORC data for the ORC unwinder to read
- * at runtime.
- *
- * Anything which doesn't fall into the above category, such as syscall and
- * interrupt handlers, tends to not be called directly by other functions, and
- * often does unusual non-C-function-type things with the stack pointer.  Such
- * code needs to be annotated such that objtool can understand it.  The
- * following CFI hint macros are for this type of code.
- *
- * These macros provide hints to objtool about the state of the stack at each
- * instruction.  Objtool starts from the hints and follows the code flow,
- * making automatic CFI adjustments when it sees pushes and pops, filling out
- * the debuginfo as necessary.  It will also warn if it sees any
- * inconsistencies.
- */
-.macro UNWIND_HINT sp_reg=ORC_REG_SP sp_offset=0 type=ORC_TYPE_CALL end=0
-#ifdef CONFIG_STACK_VALIDATION
-.Lunwind_hint_ip_\@:
-	.pushsection .discard.unwind_hints
-		/* struct unwind_hint */
-		.long .Lunwind_hint_ip_\@ - .
-		.short \sp_offset
-		.byte \sp_reg
-		.byte \type
-		.byte \end
-		.balign 4
-	.popsection
-#endif
-.endm
-
 .macro UNWIND_HINT_EMPTY
-	UNWIND_HINT sp_reg=ORC_REG_UNDEFINED end=1
+	UNWIND_HINT sp_reg=ORC_REG_UNDEFINED type=ORC_TYPE_CALL end=1
 .endm
 
 .macro UNWIND_HINT_REGS base=%rsp offset=0 indirect=0 extra=1 iret=0
@@ -83,7 +49,7 @@
 .endm
 
 .macro UNWIND_HINT_FUNC sp_offset=8
-	UNWIND_HINT sp_offset=\sp_offset
+	UNWIND_HINT sp_reg=ORC_REG_SP sp_offset=\sp_offset type=ORC_TYPE_CALL
 .endm
 
 /*
@@ -92,7 +58,7 @@
  * initial_func_cfi.
  */
 .macro UNWIND_HINT_RET_OFFSET sp_offset=8
-	UNWIND_HINT type=UNWIND_HINT_TYPE_RET_OFFSET sp_offset=\sp_offset
+	UNWIND_HINT sp_reg=ORC_REG_SP type=UNWIND_HINT_TYPE_RET_OFFSET sp_offset=\sp_offset
 .endm
 
 #endif /* __ASSEMBLY__ */
diff --git a/include/linux/frame.h b/include/linux/frame.h
index d946adb5de17..fa06f2b99625 100644
--- a/include/linux/frame.h
+++ b/include/linux/frame.h
@@ -2,9 +2,39 @@
 #ifndef _LINUX_FRAME_H
 #define _LINUX_FRAME_H
 
+#ifndef __ASSEMBLY__
+
+#include <linux/types.h>
+
+/*
+ * This struct is used by asm and inline asm code to manually annotate the
+ * location of registers on the stack.
+ */
+struct unwind_hint {
+	u32		ip;
+	s16		sp_offset;
+	u8		sp_reg;
+	u8		type;
+	u8		end;
+};
+#endif
+
 #ifdef CONFIG_STACK_VALIDATION
 
 #ifndef __ASSEMBLY__
+
+#define UNWIND_HINT(sp_reg, sp_offset, type, end)		\
+	"987: \n\t"						\
+	".pushsection .discard.unwind_hints\n\t"		\
+	/* struct unwind_hint */				\
+	".long 987b - .\n\t"					\
+	".short " __stringify(sp_offset) "\n\t"			\
+	".byte " __stringify(sp_reg) "\n\t"			\
+	".byte " __stringify(type) "\n\t"			\
+	".byte " __stringify(end) "\n\t"			\
+	".balign 4 \n\t"					\
+	".popsection\n\t"
+
 /*
  * This macro marks the given function's stack frame as "non-standard", which
  * tells objtool to ignore the function when doing stack metadata validation.
@@ -29,12 +59,54 @@
 	.long 999b;						\
 	.popsection;
 
+/*
+ * In asm, there are two kinds of code: normal C-type callable functions and
+ * the rest.  The normal callable functions can be called by other code, and
+ * don't do anything unusual with the stack.  Such normal callable functions
+ * are annotated with the ENTRY/ENDPROC macros.  Most asm code falls in this
+ * category.  In this case, no special debugging annotations are needed because
+ * objtool can automatically generate the ORC data for the ORC unwinder to read
+ * at runtime.
+ *
+ * Anything which doesn't fall into the above category, such as syscall and
+ * interrupt handlers, tends to not be called directly by other functions, and
+ * often does unusual non-C-function-type things with the stack pointer.  Such
+ * code needs to be annotated such that objtool can understand it.  The
+ * following CFI hint macros are for this type of code.
+ *
+ * These macros provide hints to objtool about the state of the stack at each
+ * instruction.  Objtool starts from the hints and follows the code flow,
+ * making automatic CFI adjustments when it sees pushes and pops, filling out
+ * the debuginfo as necessary.  It will also warn if it sees any
+ * inconsistencies.
+ */
+.macro UNWIND_HINT sp_reg:req sp_offset=0 type:req end=0
+.Lunwind_hint_ip_\@:
+	.pushsection .discard.unwind_hints
+		/* struct unwind_hint */
+		.long .Lunwind_hint_ip_\@ - .
+		.short \sp_offset
+		.byte \sp_reg
+		.byte \type
+		.byte \end
+		.balign 4
+	.popsection
+.endm
+
 #endif /* __ASSEMBLY__ */
 
 #else /* !CONFIG_STACK_VALIDATION */
 
+#ifndef __ASSEMBLY__
+
+#define UNWIND_HINT(sp_reg, sp_offset, type, end)	\
+	"\n\t"
 #define STACK_FRAME_NON_STANDARD(func)
+#else
 #define ANNOTATE_INTRA_FUNCTION_CALL
+.macro UNWIND_HINT sp_reg:req sp_offset=0 type:req end=0
+.endm
+#endif
 
 #endif /* CONFIG_STACK_VALIDATION */
 
diff --git a/tools/arch/x86/include/asm/orc_types.h b/tools/arch/x86/include/asm/orc_types.h
index d25534940bde..00408d30b13e 100644
--- a/tools/arch/x86/include/asm/orc_types.h
+++ b/tools/arch/x86/include/asm/orc_types.h
@@ -78,19 +78,6 @@ struct orc_entry {
 	unsigned	end:1;
 } __packed;
 
-/*
- * This struct is used by asm and inline asm code to manually annotate the
- * location of registers on the stack for the ORC unwinder.
- *
- * Type can be either ORC_TYPE_* or UNWIND_HINT_TYPE_*.
- */
-struct unwind_hint {
-	u32		ip;
-	s16		sp_offset;
-	u8		sp_reg;
-	u8		type;
-	u8		end;
-};
 #endif /* __ASSEMBLY__ */
 
 #endif /* _ORC_TYPES_H */
diff --git a/tools/include/linux/frame.h b/tools/include/linux/frame.h
new file mode 100644
index 000000000000..fa06f2b99625
--- /dev/null
+++ b/tools/include/linux/frame.h
@@ -0,0 +1,113 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_FRAME_H
+#define _LINUX_FRAME_H
+
+#ifndef __ASSEMBLY__
+
+#include <linux/types.h>
+
+/*
+ * This struct is used by asm and inline asm code to manually annotate the
+ * location of registers on the stack.
+ */
+struct unwind_hint {
+	u32		ip;
+	s16		sp_offset;
+	u8		sp_reg;
+	u8		type;
+	u8		end;
+};
+#endif
+
+#ifdef CONFIG_STACK_VALIDATION
+
+#ifndef __ASSEMBLY__
+
+#define UNWIND_HINT(sp_reg, sp_offset, type, end)		\
+	"987: \n\t"						\
+	".pushsection .discard.unwind_hints\n\t"		\
+	/* struct unwind_hint */				\
+	".long 987b - .\n\t"					\
+	".short " __stringify(sp_offset) "\n\t"			\
+	".byte " __stringify(sp_reg) "\n\t"			\
+	".byte " __stringify(type) "\n\t"			\
+	".byte " __stringify(end) "\n\t"			\
+	".balign 4 \n\t"					\
+	".popsection\n\t"
+
+/*
+ * This macro marks the given function's stack frame as "non-standard", which
+ * tells objtool to ignore the function when doing stack metadata validation.
+ * It should only be used in special cases where you're 100% sure it won't
+ * affect the reliability of frame pointers and kernel stack traces.
+ *
+ * For more information, see tools/objtool/Documentation/stack-validation.txt.
+ */
+#define STACK_FRAME_NON_STANDARD(func) \
+	static void __used __section(.discard.func_stack_frame_non_standard) \
+		*__func_stack_frame_non_standard_##func = func
+
+#else /* __ASSEMBLY__ */
+
+/*
+ * This macro indicates that the following intra-function call is valid.
+ * Any non-annotated intra-function call will cause objtool to issue a warning.
+ */
+#define ANNOTATE_INTRA_FUNCTION_CALL				\
+	999:							\
+	.pushsection .discard.intra_function_calls;		\
+	.long 999b;						\
+	.popsection;
+
+/*
+ * In asm, there are two kinds of code: normal C-type callable functions and
+ * the rest.  The normal callable functions can be called by other code, and
+ * don't do anything unusual with the stack.  Such normal callable functions
+ * are annotated with the ENTRY/ENDPROC macros.  Most asm code falls in this
+ * category.  In this case, no special debugging annotations are needed because
+ * objtool can automatically generate the ORC data for the ORC unwinder to read
+ * at runtime.
+ *
+ * Anything which doesn't fall into the above category, such as syscall and
+ * interrupt handlers, tends to not be called directly by other functions, and
+ * often does unusual non-C-function-type things with the stack pointer.  Such
+ * code needs to be annotated such that objtool can understand it.  The
+ * following CFI hint macros are for this type of code.
+ *
+ * These macros provide hints to objtool about the state of the stack at each
+ * instruction.  Objtool starts from the hints and follows the code flow,
+ * making automatic CFI adjustments when it sees pushes and pops, filling out
+ * the debuginfo as necessary.  It will also warn if it sees any
+ * inconsistencies.
+ */
+.macro UNWIND_HINT sp_reg:req sp_offset=0 type:req end=0
+.Lunwind_hint_ip_\@:
+	.pushsection .discard.unwind_hints
+		/* struct unwind_hint */
+		.long .Lunwind_hint_ip_\@ - .
+		.short \sp_offset
+		.byte \sp_reg
+		.byte \type
+		.byte \end
+		.balign 4
+	.popsection
+.endm
+
+#endif /* __ASSEMBLY__ */
+
+#else /* !CONFIG_STACK_VALIDATION */
+
+#ifndef __ASSEMBLY__
+
+#define UNWIND_HINT(sp_reg, sp_offset, type, end)	\
+	"\n\t"
+#define STACK_FRAME_NON_STANDARD(func)
+#else
+#define ANNOTATE_INTRA_FUNCTION_CALL
+.macro UNWIND_HINT sp_reg:req sp_offset=0 type:req end=0
+.endm
+#endif
+
+#endif /* CONFIG_STACK_VALIDATION */
+
+#endif /* _LINUX_FRAME_H */
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 60f5be2accf6..d6731e88259d 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -14,6 +14,7 @@
 #include "warn.h"
 #include "arch_elf.h"
 
+#include <linux/frame.h>
 #include <linux/hashtable.h>
 #include <linux/kernel.h>
 
diff --git a/tools/objtool/sync-check.sh b/tools/objtool/sync-check.sh
index f01b5a4d12ac..dc14ade100aa 100755
--- a/tools/objtool/sync-check.sh
+++ b/tools/objtool/sync-check.sh
@@ -3,8 +3,10 @@
 
 TARGET_ARCH=$1
 
+FILES="include/linux/frame.h"
+
 if [ "$TARGET_ARCH" == "x86" ]; then
-FILES="
+FILES="$FILES
 arch/x86/include/asm/inat_types.h
 arch/x86/include/asm/orc_types.h
 arch/x86/include/asm/emulate_prefix.h
-- 
2.21.3

[PATCH v2 9/9] objtool: Abstract unwind hint reading

[Julien Thierry]

The type of unwind hints and the semantics associated with them depend
on the architecture. Let arch specific code convert unwind hints into
objtool stack state descriptions.

Signed-off-by: Julien Thierry <jthierry@redhat.com>
---
 tools/objtool/arch.h            |  5 +--
 tools/objtool/arch/x86/decode.c | 54 ++++++++++++++++++++++++++++++
 tools/objtool/cfi.h             |  3 +-
 tools/objtool/check.c           | 58 +++++----------------------------
 tools/objtool/orc_gen.c         |  4 ++-
 5 files changed, 71 insertions(+), 53 deletions(-)

diff --git a/tools/objtool/arch.h b/tools/objtool/arch.h
index 2e2ce089b0e9..44107e9aab71 100644
--- a/tools/objtool/arch.h
+++ b/tools/objtool/arch.h
@@ -7,12 +7,11 @@
 #define _ARCH_H
 
 #include <stdbool.h>
+#include <linux/frame.h>
 #include <linux/list.h>
 #include "objtool.h"
 #include "cfi.h"
 
-#include <asm/orc_types.h>
-
 enum insn_type {
 	INSN_JUMP_CONDITIONAL,
 	INSN_JUMP_UNCONDITIONAL,
@@ -86,4 +85,6 @@ unsigned long arch_dest_reloc_offset(int addend);
 
 const char *arch_nop_insn(int len);
 
+int arch_decode_insn_hint(struct instruction *insn, struct unwind_hint *hint);
+
 #endif /* _ARCH_H */
diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
index 1967370440b3..2099809925af 100644
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -6,6 +6,8 @@
 #include <stdio.h>
 #include <stdlib.h>
 
+#include <linux/frame.h>
+
 #define unlikely(cond) (cond)
 #include <asm/insn.h>
 #include "../../../arch/x86/lib/inat.c"
@@ -15,6 +17,7 @@
 #include "../../elf.h"
 #include "../../arch.h"
 #include "../../warn.h"
+#include <asm/orc_types.h>
 
 static unsigned char op_to_cfi_reg[][2] = {
 	{CFI_AX, CFI_R8},
@@ -583,3 +586,54 @@ const char *arch_nop_insn(int len)
 
 	return nops[len-1];
 }
+
+int arch_decode_insn_hint(struct instruction *insn, struct unwind_hint *hint)
+{
+	struct cfi_reg *cfa = &insn->cfi.cfa;
+
+	if (hint->type == UNWIND_HINT_TYPE_RET_OFFSET) {
+		insn->ret_offset = hint->sp_offset;
+		return 0;
+	}
+
+	insn->hint = true;
+
+	switch (hint->sp_reg) {
+	case ORC_REG_UNDEFINED:
+		cfa->base = CFI_UNDEFINED;
+		break;
+	case ORC_REG_SP:
+		cfa->base = CFI_SP;
+		break;
+	case ORC_REG_BP:
+		cfa->base = CFI_BP;
+		break;
+	case ORC_REG_SP_INDIRECT:
+		cfa->base = CFI_SP_INDIRECT;
+		break;
+	case ORC_REG_R10:
+		cfa->base = CFI_R10;
+		break;
+	case ORC_REG_R13:
+		cfa->base = CFI_R13;
+		break;
+	case ORC_REG_DI:
+		cfa->base = CFI_DI;
+		break;
+	case ORC_REG_DX:
+		cfa->base = CFI_DX;
+		break;
+	default:
+		WARN_FUNC("unsupported unwind_hint sp base reg %d",
+			  insn->sec, insn->offset, hint->sp_reg);
+		return -1;
+	}
+
+	cfa->offset = hint->sp_offset;
+	insn->cfi.hint_type = hint->type;
+	insn->cfi.end = hint->end;
+
+	insn->cfi.sp_only = hint->type == ORC_TYPE_REGS || hint->type == ORC_TYPE_REGS_IRET;
+
+	return 0;
+}
diff --git a/tools/objtool/cfi.h b/tools/objtool/cfi.h
index c7c59c6a44ee..f5aeca023133 100644
--- a/tools/objtool/cfi.h
+++ b/tools/objtool/cfi.h
@@ -29,10 +29,11 @@ struct cfi_state {
 	struct cfi_reg cfa;
 	int stack_size;
 	int drap_reg, drap_offset;
-	unsigned char type;
+	unsigned char hint_type;
 	bool bp_scratch;
 	bool drap;
 	bool end;
+	bool sp_only;
 };
 
 #endif /* _OBJTOOL_CFI_H */
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index d6731e88259d..d856580369dd 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1217,7 +1217,6 @@ static int read_unwind_hints(struct objtool_file *file)
 	struct reloc *reloc;
 	struct unwind_hint *hint;
 	struct instruction *insn;
-	struct cfi_reg *cfa;
 	int i;
 
 	sec = find_section_by_name(file->elf, ".discard.unwind_hints");
@@ -1252,49 +1251,10 @@ static int read_unwind_hints(struct objtool_file *file)
 			return -1;
 		}
 
-		cfa = &insn->cfi.cfa;
-
-		if (hint->type == UNWIND_HINT_TYPE_RET_OFFSET) {
-			insn->ret_offset = hint->sp_offset;
-			continue;
-		}
-
-		insn->hint = true;
-
-		switch (hint->sp_reg) {
-		case ORC_REG_UNDEFINED:
-			cfa->base = CFI_UNDEFINED;
-			break;
-		case ORC_REG_SP:
-			cfa->base = CFI_SP;
-			break;
-		case ORC_REG_BP:
-			cfa->base = CFI_BP;
-			break;
-		case ORC_REG_SP_INDIRECT:
-			cfa->base = CFI_SP_INDIRECT;
-			break;
-		case ORC_REG_R10:
-			cfa->base = CFI_R10;
-			break;
-		case ORC_REG_R13:
-			cfa->base = CFI_R13;
-			break;
-		case ORC_REG_DI:
-			cfa->base = CFI_DI;
-			break;
-		case ORC_REG_DX:
-			cfa->base = CFI_DX;
-			break;
-		default:
-			WARN_FUNC("unsupported unwind_hint sp base reg %d",
-				  insn->sec, insn->offset, hint->sp_reg);
+		if (arch_decode_insn_hint(insn, hint)) {
+			WARN_FUNC("Bad unwind hint", insn->sec, insn->offset);
 			return -1;
 		}
-
-		cfa->offset = hint->sp_offset;
-		insn->cfi.type = hint->type;
-		insn->cfi.end = hint->end;
 	}
 
 	return 0;
@@ -1571,9 +1531,9 @@ static bool has_valid_stack_frame(struct insn_state *state)
 	return false;
 }
 
-static int update_cfi_state_regs(struct instruction *insn,
-				  struct cfi_state *cfi,
-				  struct stack_op *op)
+static int update_sp_only_cfi_state(struct instruction *insn,
+				    struct cfi_state *cfi,
+				    struct stack_op *op)
 {
 	struct cfi_reg *cfa = &cfi->cfa;
 
@@ -1679,8 +1639,8 @@ static int update_cfi_state(struct instruction *insn, struct cfi_state *cfi,
 		return 0;
 	}
 
-	if (cfi->type == ORC_TYPE_REGS || cfi->type == ORC_TYPE_REGS_IRET)
-		return update_cfi_state_regs(insn, cfi, op);
+	if (cfi->sp_only)
+		return update_sp_only_cfi_state(insn, cfi, op);
 
 	switch (op->dest.type) {
 
@@ -2084,10 +2044,10 @@ static bool insn_cfi_match(struct instruction *insn, struct cfi_state *cfi2)
 			break;
 		}
 
-	} else if (cfi1->type != cfi2->type) {
+	} else if (cfi1->hint_type != cfi2->hint_type) {
 
 		WARN_FUNC("stack state mismatch: type1=%d type2=%d",
-			  insn->sec, insn->offset, cfi1->type, cfi2->type);
+			  insn->sec, insn->offset, cfi1->hint_type, cfi2->hint_type);
 
 	} else if (cfi1->drap != cfi2->drap ||
 		   (cfi1->drap && cfi1->drap_reg != cfi2->drap_reg) ||
diff --git a/tools/objtool/orc_gen.c b/tools/objtool/orc_gen.c
index 00f1efd05653..a36aee98bed0 100644
--- a/tools/objtool/orc_gen.c
+++ b/tools/objtool/orc_gen.c
@@ -9,6 +9,8 @@
 #include "check.h"
 #include "warn.h"
 
+#include <asm/orc_types.h>
+
 struct orc_data {
 	struct list_head list;
 	struct instruction *insn;
@@ -92,7 +94,7 @@ int create_orc(struct objtool_file *file)
 
 		orc->sp_offset = cfa->offset;
 		orc->bp_offset = bp->offset;
-		orc->type = insn->cfi.type;
+		orc->type = insn->cfi.hint_type;
 	}
 
 	return 0;
-- 
2.21.3

Re: [PATCH v2 0/9] Make check implementation arch agnostic

[peterz]

On Thu, Jul 30, 2020 at 10:46:43AM +0100, Julien Thierry wrote:

> Julien Thierry (8):
>   objtool: Group headers to check in a single list
>   objtool: Make sync-check consider the target architecture
>   objtool: Move macros describing structures to arch-dependent code
>   objtool: Abstract alternative special case handling
>   objtool: Make relocation in alternative handling arch dependent
>   frame: Only include valid definitions depending on source file type
>   frame: Make unwind hints definitions available to other architectures
>   objtool: Abstract unwind hint reading
> 
> Raphael Gault (1):
>   objtool: Refactor switch-tables code to support other architectures

These look OK to me, Josh?

Re: [PATCH v2 5/9] objtool: Make relocation in alternative handling arch dependent

[Josh Poimboeuf]

On Thu, Jul 30, 2020 at 10:46:48AM +0100, Julien Thierry wrote:
> As pointed out by the comment in handle_group_alt(), support of
> relocation for instructions in an alternative group depends on whether
> arch specific kernel code handles it.
> 
> So, let objtool arch specific code decide whether a relocation for
> the alternative section should be accepted.
> 
> Reviewed-by: Miroslav Benes <mbenes@suse.cz>
> Signed-off-by: Julien Thierry <jthierry@redhat.com>
> ---
>  tools/objtool/arch/x86/arch_special.c | 13 +++++++++++++

The "arch" in "arch_special.c" is redundant, how about special.c?

-- 
Josh

Re: [PATCH v2 8/9] frame: Make unwind hints definitions available to other architectures

[Josh Poimboeuf]

On Thu, Jul 30, 2020 at 10:46:51AM +0100, Julien Thierry wrote:
> Unwind hints are useful to provide objtool with information about stack
> states in non-standard functions/code.
> While the type of information being provided might be very arch
> specific, the mechanism to provide the information can be useful for
> other architectures.
> 
> Move the relevant unwint hint definitions for all architectures to
> see.

The scope of include/linux/frame.h has been creeping, it's no longer
just about frame pointers.  Maybe we should rename it to objtool.h.

-- 
Josh

Re: [PATCH v2 9/9] objtool: Abstract unwind hint reading

[Josh Poimboeuf]

On Thu, Jul 30, 2020 at 10:46:52AM +0100, Julien Thierry wrote:
> The type of unwind hints and the semantics associated with them depend
> on the architecture. Let arch specific code convert unwind hints into
> objtool stack state descriptions.
> 
> Signed-off-by: Julien Thierry <jthierry@redhat.com>
> ---
>  tools/objtool/arch.h            |  5 +--
>  tools/objtool/arch/x86/decode.c | 54 ++++++++++++++++++++++++++++++
>  tools/objtool/cfi.h             |  3 +-
>  tools/objtool/check.c           | 58 +++++----------------------------
>  tools/objtool/orc_gen.c         |  4 ++-
>  5 files changed, 71 insertions(+), 53 deletions(-)
> 
> diff --git a/tools/objtool/arch.h b/tools/objtool/arch.h
> index 2e2ce089b0e9..44107e9aab71 100644
> --- a/tools/objtool/arch.h
> +++ b/tools/objtool/arch.h
> @@ -7,12 +7,11 @@
>  #define _ARCH_H
>  
>  #include <stdbool.h>
> +#include <linux/frame.h>
>  #include <linux/list.h>
>  #include "objtool.h"
>  #include "cfi.h"
>  
> -#include <asm/orc_types.h>
> -
>  enum insn_type {
>  	INSN_JUMP_CONDITIONAL,
>  	INSN_JUMP_UNCONDITIONAL,
> @@ -86,4 +85,6 @@ unsigned long arch_dest_reloc_offset(int addend);
>  
>  const char *arch_nop_insn(int len);
>  
> +int arch_decode_insn_hint(struct instruction *insn, struct unwind_hint *hint);
> +
>  #endif /* _ARCH_H */
> diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
> index 1967370440b3..2099809925af 100644
> --- a/tools/objtool/arch/x86/decode.c
> +++ b/tools/objtool/arch/x86/decode.c
> @@ -6,6 +6,8 @@
>  #include <stdio.h>
>  #include <stdlib.h>
>  
> +#include <linux/frame.h>
> +
>  #define unlikely(cond) (cond)
>  #include <asm/insn.h>
>  #include "../../../arch/x86/lib/inat.c"
> @@ -15,6 +17,7 @@
>  #include "../../elf.h"
>  #include "../../arch.h"
>  #include "../../warn.h"
> +#include <asm/orc_types.h>
>  
>  static unsigned char op_to_cfi_reg[][2] = {
>  	{CFI_AX, CFI_R8},
> @@ -583,3 +586,54 @@ const char *arch_nop_insn(int len)
>  
>  	return nops[len-1];
>  }
> +
> +int arch_decode_insn_hint(struct instruction *insn, struct unwind_hint *hint)
> +{
> +	struct cfi_reg *cfa = &insn->cfi.cfa;
> +
> +	if (hint->type == UNWIND_HINT_TYPE_RET_OFFSET) {
> +		insn->ret_offset = hint->sp_offset;
> +		return 0;
> +	}
> +
> +	insn->hint = true;
> +
> +	switch (hint->sp_reg) {
> +	case ORC_REG_UNDEFINED:
> +		cfa->base = CFI_UNDEFINED;
> +		break;
> +	case ORC_REG_SP:
> +		cfa->base = CFI_SP;
> +		break;
> +	case ORC_REG_BP:
> +		cfa->base = CFI_BP;
> +		break;
> +	case ORC_REG_SP_INDIRECT:
> +		cfa->base = CFI_SP_INDIRECT;
> +		break;
> +	case ORC_REG_R10:
> +		cfa->base = CFI_R10;
> +		break;
> +	case ORC_REG_R13:
> +		cfa->base = CFI_R13;
> +		break;
> +	case ORC_REG_DI:
> +		cfa->base = CFI_DI;
> +		break;
> +	case ORC_REG_DX:
> +		cfa->base = CFI_DX;
> +		break;
> +	default:
> +		WARN_FUNC("unsupported unwind_hint sp base reg %d",
> +			  insn->sec, insn->offset, hint->sp_reg);
> +		return -1;
> +	}
> +
> +	cfa->offset = hint->sp_offset;
> +	insn->cfi.hint_type = hint->type;
> +	insn->cfi.end = hint->end;
> +
> +	insn->cfi.sp_only = hint->type == ORC_TYPE_REGS || hint->type == ORC_TYPE_REGS_IRET;

What does "sp" mean here in sp_only?

> +		if (arch_decode_insn_hint(insn, hint)) {
> +			WARN_FUNC("Bad unwind hint", insn->sec, insn->offset);

No need for a warning here, since the arch-specific function already
prints one.

-- 
Josh

Re: [PATCH v2 9/9] objtool: Abstract unwind hint reading

[Julien Thierry]

On 7/30/20 4:03 PM, Josh Poimboeuf wrote:
> On Thu, Jul 30, 2020 at 10:46:52AM +0100, Julien Thierry wrote:
>> The type of unwind hints and the semantics associated with them depend
>> on the architecture. Let arch specific code convert unwind hints into
>> objtool stack state descriptions.
>>
>> Signed-off-by: Julien Thierry <jthierry@redhat.com>
>> ---
>>   tools/objtool/arch.h            |  5 +--
>>   tools/objtool/arch/x86/decode.c | 54 ++++++++++++++++++++++++++++++
>>   tools/objtool/cfi.h             |  3 +-
>>   tools/objtool/check.c           | 58 +++++----------------------------
>>   tools/objtool/orc_gen.c         |  4 ++-
>>   5 files changed, 71 insertions(+), 53 deletions(-)
>>
>> diff --git a/tools/objtool/arch.h b/tools/objtool/arch.h
>> index 2e2ce089b0e9..44107e9aab71 100644
>> --- a/tools/objtool/arch.h
>> +++ b/tools/objtool/arch.h
>> @@ -7,12 +7,11 @@
>>   #define _ARCH_H
>>   
>>   #include <stdbool.h>
>> +#include <linux/frame.h>
>>   #include <linux/list.h>
>>   #include "objtool.h"
>>   #include "cfi.h"
>>   
>> -#include <asm/orc_types.h>
>> -
>>   enum insn_type {
>>   	INSN_JUMP_CONDITIONAL,
>>   	INSN_JUMP_UNCONDITIONAL,
>> @@ -86,4 +85,6 @@ unsigned long arch_dest_reloc_offset(int addend);
>>   
>>   const char *arch_nop_insn(int len);
>>   
>> +int arch_decode_insn_hint(struct instruction *insn, struct unwind_hint *hint);
>> +
>>   #endif /* _ARCH_H */
>> diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
>> index 1967370440b3..2099809925af 100644
>> --- a/tools/objtool/arch/x86/decode.c
>> +++ b/tools/objtool/arch/x86/decode.c
>> @@ -6,6 +6,8 @@
>>   #include <stdio.h>
>>   #include <stdlib.h>
>>   
>> +#include <linux/frame.h>
>> +
>>   #define unlikely(cond) (cond)
>>   #include <asm/insn.h>
>>   #include "../../../arch/x86/lib/inat.c"
>> @@ -15,6 +17,7 @@
>>   #include "../../elf.h"
>>   #include "../../arch.h"
>>   #include "../../warn.h"
>> +#include <asm/orc_types.h>
>>   
>>   static unsigned char op_to_cfi_reg[][2] = {
>>   	{CFI_AX, CFI_R8},
>> @@ -583,3 +586,54 @@ const char *arch_nop_insn(int len)
>>   
>>   	return nops[len-1];
>>   }
>> +
>> +int arch_decode_insn_hint(struct instruction *insn, struct unwind_hint *hint)
>> +{
>> +	struct cfi_reg *cfa = &insn->cfi.cfa;
>> +
>> +	if (hint->type == UNWIND_HINT_TYPE_RET_OFFSET) {
>> +		insn->ret_offset = hint->sp_offset;
>> +		return 0;
>> +	}
>> +
>> +	insn->hint = true;
>> +
>> +	switch (hint->sp_reg) {
>> +	case ORC_REG_UNDEFINED:
>> +		cfa->base = CFI_UNDEFINED;
>> +		break;
>> +	case ORC_REG_SP:
>> +		cfa->base = CFI_SP;
>> +		break;
>> +	case ORC_REG_BP:
>> +		cfa->base = CFI_BP;
>> +		break;
>> +	case ORC_REG_SP_INDIRECT:
>> +		cfa->base = CFI_SP_INDIRECT;
>> +		break;
>> +	case ORC_REG_R10:
>> +		cfa->base = CFI_R10;
>> +		break;
>> +	case ORC_REG_R13:
>> +		cfa->base = CFI_R13;
>> +		break;
>> +	case ORC_REG_DI:
>> +		cfa->base = CFI_DI;
>> +		break;
>> +	case ORC_REG_DX:
>> +		cfa->base = CFI_DX;
>> +		break;
>> +	default:
>> +		WARN_FUNC("unsupported unwind_hint sp base reg %d",
>> +			  insn->sec, insn->offset, hint->sp_reg);
>> +		return -1;
>> +	}
>> +
>> +	cfa->offset = hint->sp_offset;
>> +	insn->cfi.hint_type = hint->type;
>> +	insn->cfi.end = hint->end;
>> +
>> +	insn->cfi.sp_only = hint->type == ORC_TYPE_REGS || hint->type == ORC_TYPE_REGS_IRET;
> 
> What does "sp" mean here in sp_only?
>

Stack pointer, like in CFI_SP. When objtool encounters one of these 
hints, it starts to only track the stack frame with the stack pointer 
(no BP, no drap register, no move to temporary registers). Just trying 
to make some sense of this corner case.

>> +		if (arch_decode_insn_hint(insn, hint)) {
>> +			WARN_FUNC("Bad unwind hint", insn->sec, insn->offset);
> 
> No need for a warning here, since the arch-specific function already
> prints one.
> 

Right.

Thanks,

-- 
Julien Thierry

Re: [PATCH v2 5/9] objtool: Make relocation in alternative handling arch dependent

[Julien Thierry]

On 7/30/20 3:42 PM, Josh Poimboeuf wrote:
> On Thu, Jul 30, 2020 at 10:46:48AM +0100, Julien Thierry wrote:
>> As pointed out by the comment in handle_group_alt(), support of
>> relocation for instructions in an alternative group depends on whether
>> arch specific kernel code handles it.
>>
>> So, let objtool arch specific code decide whether a relocation for
>> the alternative section should be accepted.
>>
>> Reviewed-by: Miroslav Benes <mbenes@suse.cz>
>> Signed-off-by: Julien Thierry <jthierry@redhat.com>
>> ---
>>   tools/objtool/arch/x86/arch_special.c | 13 +++++++++++++
> 
> The "arch" in "arch_special.c" is redundant, how about special.c?
> 

Yes, that makes sense.

-- 
Julien Thierry

Re: [PATCH v2 0/9] Make check implementation arch agnostic

[Miroslav Benes]

> I've push both series base on top of tip/objtool/core at [3].
> 
> - The first two patches make it simpler for new arches to provide their
> list of kernel headers, without worrying about modifications in the x86
> headers.
> - Patch 3 Moves arch specific macros to more suitable location
> - Patches 4 and 5 add abstraction to handle alternatives
> - Patch 6 adds abstraction to handle jump table
> - Patches 7-9 abstracts the use of unwind hints, so some definitions
>   can be shared across architectures while keeping arch specific
>   semantics
> 
> Changes since v1 [4]:
> - Rebased on recent tip/objtool/core
> - Split the unwind hint rework into multiple patches as suggested by
>   Miroslav

For remaining patches 7-9

Reviewed-by: Miroslav Benes <mbenes@suse.cz>

M

Re: [PATCH v2 9/9] objtool: Abstract unwind hint reading

[Josh Poimboeuf]

On Fri, Jul 31, 2020 at 08:00:58AM +0100, Julien Thierry wrote:
> > > +	cfa->offset = hint->sp_offset;
> > > +	insn->cfi.hint_type = hint->type;
> > > +	insn->cfi.end = hint->end;
> > > +
> > > +	insn->cfi.sp_only = hint->type == ORC_TYPE_REGS || hint->type == ORC_TYPE_REGS_IRET;
> > 
> > What does "sp" mean here in sp_only?
> > 
> 
> Stack pointer, like in CFI_SP. When objtool encounters one of these hints,
> it starts to only track the stack frame with the stack pointer (no BP, no
> drap register, no move to temporary registers). Just trying to make some
> sense of this corner case.

I think that's not quite right, because ORC_TYPE_CALL could also be
"sp_only" in some cases, by that definition.

The call to update_cfi_state_regs() is really regs-specific, not
sp-specific.

-- 
Josh

Re: [PATCH v2 9/9] objtool: Abstract unwind hint reading

[Julien Thierry]

On 7/31/20 3:04 PM, Josh Poimboeuf wrote:
> On Fri, Jul 31, 2020 at 08:00:58AM +0100, Julien Thierry wrote:
>>>> +	cfa->offset = hint->sp_offset;
>>>> +	insn->cfi.hint_type = hint->type;
>>>> +	insn->cfi.end = hint->end;
>>>> +
>>>> +	insn->cfi.sp_only = hint->type == ORC_TYPE_REGS || hint->type == ORC_TYPE_REGS_IRET;
>>>
>>> What does "sp" mean here in sp_only?
>>>
>>
>> Stack pointer, like in CFI_SP. When objtool encounters one of these hints,
>> it starts to only track the stack frame with the stack pointer (no BP, no
>> drap register, no move to temporary registers). Just trying to make some
>> sense of this corner case.
> 
> I think that's not quite right, because ORC_TYPE_CALL could also be
> "sp_only" in some cases, by that definition.
> 

But in that case the code will still track when/if the CFI becomes 
pointed to by BP.

> The call to update_cfi_state_regs() is really regs-specific, not
> sp-specific.
> 

I must admit I don't really understand what "regs" is and why exactly 
such an exception in stack state tracking is made where only operations 
to SP are taken into account.

-- 
Julien Thierry

Re: [PATCH v2 9/9] objtool: Abstract unwind hint reading

[Josh Poimboeuf]

On Mon, Aug 03, 2020 at 01:13:14PM +0100, Julien Thierry wrote:
> 
> 
> On 7/31/20 3:04 PM, Josh Poimboeuf wrote:
> > On Fri, Jul 31, 2020 at 08:00:58AM +0100, Julien Thierry wrote:
> > > > > +	cfa->offset = hint->sp_offset;
> > > > > +	insn->cfi.hint_type = hint->type;
> > > > > +	insn->cfi.end = hint->end;
> > > > > +
> > > > > +	insn->cfi.sp_only = hint->type == ORC_TYPE_REGS || hint->type == ORC_TYPE_REGS_IRET;
> > > > 
> > > > What does "sp" mean here in sp_only?
> > > > 
> > > 
> > > Stack pointer, like in CFI_SP. When objtool encounters one of these hints,
> > > it starts to only track the stack frame with the stack pointer (no BP, no
> > > drap register, no move to temporary registers). Just trying to make some
> > > sense of this corner case.
> > 
> > I think that's not quite right, because ORC_TYPE_CALL could also be
> > "sp_only" in some cases, by that definition.
> > 
> 
> But in that case the code will still track when/if the CFI becomes pointed
> to by BP.
> 
> > The call to update_cfi_state_regs() is really regs-specific, not
> > sp-specific.
> > 
> 
> I must admit I don't really understand what "regs" is and why exactly such
> an exception in stack state tracking is made where only operations to SP are
> taken into account.

"regs" is a special type of stack frame, usually for asm entry code,
where the frame is actually an instance of 'struct pt_regs'.  So if
there's a variable associated it with it, maybe it should have "regs" in
the name.

Though I think non-x86 arches will also have regs frames, so would it
make sense to just make the unwind hint types a global multiarch thing?
They could be renamed to UNWIND_HINT_TYPE_REGS{_PARTIAL}.  Then there
wouldn't really be a need for the "sp_only" thing.

-- 
Josh

Re: [PATCH v2 8/9] frame: Make unwind hints definitions available to other architectures

[Julien Thierry]

On 7/30/20 3:56 PM, Josh Poimboeuf wrote:
> On Thu, Jul 30, 2020 at 10:46:51AM +0100, Julien Thierry wrote:
>> Unwind hints are useful to provide objtool with information about stack
>> states in non-standard functions/code.
>> While the type of information being provided might be very arch
>> specific, the mechanism to provide the information can be useful for
>> other architectures.
>>
>> Move the relevant unwint hint definitions for all architectures to
>> see.
> 
> The scope of include/linux/frame.h has been creeping, it's no longer
> just about frame pointers.  Maybe we should rename it to objtool.h.
> 

I missed this comment until now, sorry.

The name "objtool.h" might conflict with tools/objtool/objtool.h. What 
about "objtool_utils.h" or "objtool_defs.h" ?

-- 
Julien Thierry

Re: [PATCH v2 9/9] objtool: Abstract unwind hint reading

[Julien Thierry]

On 8/3/20 10:35 PM, Josh Poimboeuf wrote:
> On Mon, Aug 03, 2020 at 01:13:14PM +0100, Julien Thierry wrote:
>>
>>
>> On 7/31/20 3:04 PM, Josh Poimboeuf wrote:
>>> On Fri, Jul 31, 2020 at 08:00:58AM +0100, Julien Thierry wrote:
>>>>>> +	cfa->offset = hint->sp_offset;
>>>>>> +	insn->cfi.hint_type = hint->type;
>>>>>> +	insn->cfi.end = hint->end;
>>>>>> +
>>>>>> +	insn->cfi.sp_only = hint->type == ORC_TYPE_REGS || hint->type == ORC_TYPE_REGS_IRET;
>>>>>
>>>>> What does "sp" mean here in sp_only?
>>>>>
>>>>
>>>> Stack pointer, like in CFI_SP. When objtool encounters one of these hints,
>>>> it starts to only track the stack frame with the stack pointer (no BP, no
>>>> drap register, no move to temporary registers). Just trying to make some
>>>> sense of this corner case.
>>>
>>> I think that's not quite right, because ORC_TYPE_CALL could also be
>>> "sp_only" in some cases, by that definition.
>>>
>>
>> But in that case the code will still track when/if the CFI becomes pointed
>> to by BP.
>>
>>> The call to update_cfi_state_regs() is really regs-specific, not
>>> sp-specific.
>>>
>>
>> I must admit I don't really understand what "regs" is and why exactly such
>> an exception in stack state tracking is made where only operations to SP are
>> taken into account.
> 
> "regs" is a special type of stack frame, usually for asm entry code,
> where the frame is actually an instance of 'struct pt_regs'.  So if
> there's a variable associated it with it, maybe it should have "regs" in
> the name.
> 
> Though I think non-x86 arches will also have regs frames, so would it
> make sense to just make the unwind hint types a global multiarch thing?
> They could be renamed to UNWIND_HINT_TYPE_REGS{_PARTIAL}.  Then there
> wouldn't really be a need for the "sp_only" thing.
> 

If having regs frame means having a pt_regs on the stack when procedure 
calls/return, then yes this will probably be the case on most archs (it 
is for arm64 at least.

However in that case, arm64 still builds a stack frame and sets the 
frame pointer, so only handling SP operations doesn't make much sense 
for arm64.

Also, things like ORC_TYPE_REGS_IRET don't have a use for arm64 (but 
maybe for other non-x86 arches it does?)

In the end that's why I left the unwind hint types as arch defined. It 
seems like every arch will have their specific semantics they might want 
to let objtool know about.

-- 
Julien Thierry

Re: [PATCH v2 8/9] frame: Make unwind hints definitions available to other architectures

[Josh Poimboeuf]

On Tue, Aug 25, 2020 at 01:12:04PM +0100, Julien Thierry wrote:
> 
> 
> On 7/30/20 3:56 PM, Josh Poimboeuf wrote:
> > On Thu, Jul 30, 2020 at 10:46:51AM +0100, Julien Thierry wrote:
> > > Unwind hints are useful to provide objtool with information about stack
> > > states in non-standard functions/code.
> > > While the type of information being provided might be very arch
> > > specific, the mechanism to provide the information can be useful for
> > > other architectures.
> > > 
> > > Move the relevant unwint hint definitions for all architectures to
> > > see.
> > 
> > The scope of include/linux/frame.h has been creeping, it's no longer
> > just about frame pointers.  Maybe we should rename it to objtool.h.
> > 
> 
> I missed this comment until now, sorry.
> 
> The name "objtool.h" might conflict with tools/objtool/objtool.h. What about
> "objtool_utils.h" or "objtool_defs.h" ?

There shouldn't be a conflict: objtool doesn't include kernel headers
directly; and even if it did need a copy (in tools/include/linux), it
would be referenced as <linux/objtool.h> instead of "objtool.h".

-- 
Josh

Re: [PATCH v2 9/9] objtool: Abstract unwind hint reading

[Josh Poimboeuf]

On Tue, Aug 25, 2020 at 01:31:46PM +0100, Julien Thierry wrote:
> > > > The call to update_cfi_state_regs() is really regs-specific, not
> > > > sp-specific.
> > > > 
> > > 
> > > I must admit I don't really understand what "regs" is and why exactly such
> > > an exception in stack state tracking is made where only operations to SP are
> > > taken into account.
> > 
> > "regs" is a special type of stack frame, usually for asm entry code,
> > where the frame is actually an instance of 'struct pt_regs'.  So if
> > there's a variable associated it with it, maybe it should have "regs" in
> > the name.
> > 
> > Though I think non-x86 arches will also have regs frames, so would it
> > make sense to just make the unwind hint types a global multiarch thing?
> > They could be renamed to UNWIND_HINT_TYPE_REGS{_PARTIAL}.  Then there
> > wouldn't really be a need for the "sp_only" thing.
> > 
> 
> If having regs frame means having a pt_regs on the stack when procedure
> calls/return, then yes this will probably be the case on most archs (it is
> for arm64 at least.

The regs frames aren't for function/procedure calls, but rather
exceptions/interrupts.

> However in that case, arm64 still builds a stack frame and sets the frame
> pointer, so only handling SP operations doesn't make much sense for arm64.

If for example arm64 were to switch to ORC, it could have regs-only
frames in entry code just like x86.

> Also, things like ORC_TYPE_REGS_IRET don't have a use for arm64 (but maybe
> for other non-x86 arches it does?)

If we called it TYPE_REGS_PARTIAL, at least the name would be generic
enough, even if it's not used by all arches.

> In the end that's why I left the unwind hint types as arch defined. It seems
> like every arch will have their specific semantics they might want to let
> objtool know about.

Ok, I guess I'd need to look at the code to have an opinion.  If there
turn out to be commonalities between the different arch unwind hints
then we could always unify them later.

-- 
Josh