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
Do not take into account outdated headers unrelated to the build of the
current architecture.
Reviewed-by: Miroslav Benes <[email protected]>
Signed-off-by: Julien Thierry <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Julien Thierry <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Julien Thierry <[email protected]>
---
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
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?
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 <[email protected]>
> Signed-off-by: Julien Thierry <[email protected]>
> ---
> tools/objtool/arch/x86/arch_special.c | 13 +++++++++++++
The "arch" in "arch_special.c" is redundant, how about special.c?
--
Josh
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
>> Signed-off-by: Julien Thierry <[email protected]>
>> ---
>> 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
> 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 <[email protected]>
M
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
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
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
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
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
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
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