[PATCH v2 00/10] Objtool updates for easier portability

[PATCH v2 00/10] Objtool updates for easier portability

[Julien Thierry]

Hi,

This patchset includes some of the least controversial changes that
were needed as part of the arm64 port [1].

I'm resending these rebase on top of linux-tip/core/objtool, following
the addition of Peter's patches [2]

It consist mostly of small fixes or lifting some limitations to make it
easier to support a new architecture in objtool. Of course, these will
not be the only required changes, but these are the ones I hope make
enough sense to be merged separately from the rest of arm64 port series.

Changes since v1[3]:
- Really just rebased things

[1] https://lkml.org/lkml/2020/1/9/643
[2] https://lkml.org/lkml/2020/3/25/807
[3] https://www.spinics.net/lists/kernel/msg3453718.html

Thanks,

Julien

-->

Julien Thierry (9):
  objtool: Move header sync-check ealier in build
  objtool: check: Remove redundant checks on operand type
  objtool: check: Clean instruction state before each function
    validation
  objtool: check: Ignore empty alternative groups
  objtool: check: Remove check preventing branches within alternative
  objtool: check: Use arch specific values in restore_reg()
  objtool: check: Allow save/restore hint in non standard function
    symbols
  objtool: Split generic and arch specific CFI definitions
  objtool: Support multiple stack_op per instruction

Raphael Gault (1):
  objtool: Add abstraction for computation of symbols offsets

 tools/objtool/Makefile                    |   5 +-
 tools/objtool/arch.h                      |  10 +-
 tools/objtool/arch/x86/decode.c           |  24 +++-
 tools/objtool/arch/x86/include/cfi_regs.h |  25 ++++
 tools/objtool/cfi.h                       |  21 +--
 tools/objtool/check.c                     | 152 +++++++++++++---------
 tools/objtool/check.h                     |   3 +-
 7 files changed, 154 insertions(+), 86 deletions(-)
 create mode 100644 tools/objtool/arch/x86/include/cfi_regs.h

--
2.21.1

[PATCH v2 01/10] objtool: Move header sync-check ealier in build

[Julien Thierry]

Currently, the check of tools files against kernel equivalent is only
done after every object file has been built. This means one might fix
build issues against outdated headers without seeing a warning about
this.

Check headers before any object is built. Also, make it part of a
FORCE'd recipe so every attempt to build objtool will report the
outdated headers (if any).

Signed-off-by: Julien Thierry <jthierry@redhat.com>
---
 tools/objtool/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/objtool/Makefile b/tools/objtool/Makefile
index ee08aeff30a1..519af6ec4eee 100644
--- a/tools/objtool/Makefile
+++ b/tools/objtool/Makefile
@@ -43,10 +43,10 @@ export srctree OUTPUT CFLAGS SRCARCH AWK
 include $(srctree)/tools/build/Makefile.include
 
 $(OBJTOOL_IN): fixdep FORCE
+	@$(CONFIG_SHELL) ./sync-check.sh
 	@$(MAKE) $(build)=objtool
 
 $(OBJTOOL): $(LIBSUBCMD) $(OBJTOOL_IN)
-	@$(CONFIG_SHELL) ./sync-check.sh
 	$(QUIET_LINK)$(CC) $(OBJTOOL_IN) $(LDFLAGS) -o $@
 
 
-- 
2.21.1

[PATCH v2 02/10] objtool: check: Remove redundant checks on operand type

[Julien Thierry]

POP operations are already in code path where destination operand
is OP_DEST_REG. There is no need to check the operand type again.

Signed-off-by: Julien Thierry <jthierry@redhat.com>
---
 tools/objtool/check.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 58f4c0525ae8..3ab87e3aa750 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1717,15 +1717,13 @@ static int update_insn_state(struct instruction *insn, struct insn_state *state)
 
 		case OP_SRC_POP:
 		case OP_SRC_POPF:
-			if (!state->drap && op->dest.type == OP_DEST_REG &&
-			    op->dest.reg == cfa->base) {
+			if (!state->drap && op->dest.reg == cfa->base) {
 
 				/* pop %rbp */
 				cfa->base = CFI_SP;
 			}
 
 			if (state->drap && cfa->base == CFI_BP_INDIRECT &&
-			    op->dest.type == OP_DEST_REG &&
 			    op->dest.reg == state->drap_reg &&
 			    state->drap_offset == -state->stack_size) {
 
-- 
2.21.1

[PATCH v2 03/10] objtool: check: Clean instruction state before each function validation

[Julien Thierry]

When a function fails its validation, it might leave a stale state
that will be used for the validation of other functions. That would
cause false warnings on potentially valid functions.

Reset the instruction state before the validation of each individual
function.

Signed-off-by: Julien Thierry <jthierry@redhat.com>
---
 tools/objtool/check.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 3ab87e3aa750..74353b2c39ce 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2400,13 +2400,6 @@ static int validate_section(struct objtool_file *file, struct section *sec)
 	struct insn_state state;
 	int ret, warnings = 0;
 
-	clear_insn_state(&state);
-
-	state.cfa = initial_func_cfi.cfa;
-	memcpy(&state.regs, &initial_func_cfi.regs,
-	       CFI_NUM_REGS * sizeof(struct cfi_reg));
-	state.stack_size = initial_func_cfi.cfa.offset;
-
 	list_for_each_entry(func, &sec->symbol_list, list) {
 		if (func->type != STT_FUNC)
 			continue;
@@ -2424,6 +2417,12 @@ static int validate_section(struct objtool_file *file, struct section *sec)
 		if (!insn || insn->ignore || insn->visited)
 			continue;
 
+		clear_insn_state(&state);
+		state.cfa = initial_func_cfi.cfa;
+		memcpy(&state.regs, &initial_func_cfi.regs,
+		       CFI_NUM_REGS * sizeof(struct cfi_reg));
+		state.stack_size = initial_func_cfi.cfa.offset;
+
 		state.uaccess = func->uaccess_safe;
 
 		ret = validate_branch(file, func, insn, state);
-- 
2.21.1

[PATCH v2 04/10] objtool: check: Ignore empty alternative groups

[Julien Thierry]

Atlernative section can contain entries for alternatives with no
instructions. Objtool will currently crash when handling such an entry.

Just skip that entry, but still give a warning to discourage useless
entries.

Signed-off-by: Julien Thierry <jthierry@redhat.com>
---
 tools/objtool/check.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 74353b2c39ce..5c03460f1f07 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -904,6 +904,12 @@ static int add_special_section_alts(struct objtool_file *file)
 		}
 
 		if (special_alt->group) {
+			if (!special_alt->orig_len) {
+				WARN_FUNC("empty alternative entry",
+					  orig_insn->sec, orig_insn->offset);
+				continue;
+			}
+
 			ret = handle_group_alt(file, special_alt, orig_insn,
 					       &new_insn);
 			if (ret)
-- 
2.21.1

[PATCH v2 05/10] objtool: check: Remove check preventing branches within alternative

[Julien Thierry]

While jumping from outside an alternative region to the middle of an
alternative region is very likely wrong, jumping from an alternative
region into the same region is valid. It is a common pattern on arm64.

The first pattern is unlikely to happen in practice and checking only
for this adds a lot of complexity.

Just remove the current check.

Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Julien Thierry <jthierry@redhat.com>
---
 tools/objtool/check.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 5c03460f1f07..6af14a55490d 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2037,12 +2037,6 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
 	insn = first;
 	sec = insn->sec;
 
-	if (insn->alt_group && list_empty(&insn->alts)) {
-		WARN_FUNC("don't know how to handle branch to middle of alternative instruction group",
-			  sec, insn->offset);
-		return 1;
-	}
-
 	while (1) {
 		next_insn = next_insn_same_sec(file, insn);
 
-- 
2.21.1

[PATCH v2 06/10] objtool: check: Use arch specific values in restore_reg()

[Julien Thierry]

Initial register state is set up by arch specific code. Use the value
the arch code has set when restoring registers from the stack.

Suggested-by: Raphael Gault <raphael.gault@arm.com>
Signed-off-by: Julien Thierry <jthierry@redhat.com>
---
 tools/objtool/check.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 6af14a55490d..65809c74f6a8 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1488,8 +1488,8 @@ static void save_reg(struct insn_state *state, unsigned char reg, int base,
 
 static void restore_reg(struct insn_state *state, unsigned char reg)
 {
-	state->regs[reg].base = CFI_UNDEFINED;
-	state->regs[reg].offset = 0;
+	state->regs[reg].base = initial_func_cfi.regs[reg].base;
+	state->regs[reg].offset = initial_func_cfi.regs[reg].offset;
 }
 
 /*
-- 
2.21.1

[PATCH v2 07/10] objtool: check: Allow save/restore hint in non standard function symbols

[Julien Thierry]

The kernel code base provides CODE_SYM_START/END to declare assembly
code sequences that don't follow function standard calling conventions.

As non-C/non-standard code, these sequences can very much benefit from
unwind hints. However, when a restore unwind_hint is used in a
non-function code sequence, objtool will crash when looking for the
corresponding save hint.

Record the code symbol an instruction belongs to and look for save hints
belonging to the same code symbol as the restore hint.

Signed-off-by: Julien Thierry <jthierry@redhat.com>
---
 tools/objtool/check.c | 27 +++++++++++++++++++--------
 tools/objtool/check.h |  1 +
 2 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 65809c74f6a8..fb1ddde66b48 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -236,7 +236,7 @@ static void clear_insn_state(struct insn_state *state)
 static int decode_instructions(struct objtool_file *file)
 {
 	struct section *sec;
-	struct symbol *func;
+	struct symbol *sym;
 	unsigned long offset;
 	struct instruction *insn;
 	unsigned long nr_insns = 0;
@@ -278,18 +278,22 @@ static int decode_instructions(struct objtool_file *file)
 			nr_insns++;
 		}
 
-		list_for_each_entry(func, &sec->symbol_list, list) {
-			if (func->type != STT_FUNC || func->alias != func)
+		list_for_each_entry(sym, &sec->symbol_list, list) {
+			if ((sym->type != STT_FUNC && sym->type != STT_NOTYPE) ||
+			     sym->alias != sym)
 				continue;
 
-			if (!find_insn(file, sec, func->offset)) {
+			if (!find_insn(file, sec, sym->offset)) {
 				WARN("%s(): can't find starting instruction",
-				     func->name);
+				     sym->name);
 				return -1;
 			}
 
-			sym_for_each_insn(file, func, insn)
-				insn->func = func;
+			sym_for_each_insn(file, sym, insn) {
+				insn->code_sym = sym;
+				if (sym->type == STT_FUNC)
+					insn->func = sym;
+			}
 		}
 	}
 
@@ -758,6 +762,7 @@ static int handle_group_alt(struct objtool_file *file,
 		fake_jump->type = INSN_JUMP_UNCONDITIONAL;
 		fake_jump->jump_dest = list_next_entry(last_orig_insn, list);
 		fake_jump->func = orig_insn->func;
+		fake_jump->code_sym = orig_insn->code_sym;
 	}
 
 	if (!special_alt->new_len) {
@@ -2065,9 +2070,15 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
 			if (insn->restore) {
 				struct instruction *save_insn, *i;
 
+				if (!insn->code_sym) {
+					WARN_FUNC("restore instruction doesn't belong to any symbol",
+						  insn->sec, insn->offset);
+					return 1;
+				}
+
 				i = insn;
 				save_insn = NULL;
-				sym_for_each_insn_continue_reverse(file, func, i) {
+				sym_for_each_insn_continue_reverse(file, insn->code_sym, i) {
 					if (i->save) {
 						save_insn = i;
 						break;
diff --git a/tools/objtool/check.h b/tools/objtool/check.h
index f0ce8ffe7135..d1b55961474c 100644
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -42,6 +42,7 @@ struct instruction {
 	struct rela *jump_table;
 	struct list_head alts;
 	struct symbol *func;
+	struct symbol *code_sym;
 	struct stack_op stack_op;
 	struct insn_state state;
 	struct orc_entry orc;
-- 
2.21.1

[PATCH v2 08/10] objtool: Add abstraction for computation of symbols offsets

[Julien Thierry]

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

The jump destination and relocation offset used previously are only
reliable on x86_64 architecture. We abstract these computations by calling
arch-dependent implementations.

Signed-off-by: Raphael Gault <raphael.gault@arm.com>
[J.T. Remove superfluous comment, replace other addend offsets
      with arch_dest_rela_offset]
Signed-off-by: Julien Thierry <jthierry@redhat.com>
---
 tools/objtool/arch.h            |  6 ++++++
 tools/objtool/arch/x86/decode.c | 11 +++++++++++
 tools/objtool/check.c           | 18 ++++++++++--------
 3 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/tools/objtool/arch.h b/tools/objtool/arch.h
index ced3765c4f44..a9a50a25ca66 100644
--- a/tools/objtool/arch.h
+++ b/tools/objtool/arch.h
@@ -66,6 +66,8 @@ struct stack_op {
 	struct op_src src;
 };
 
+struct instruction;
+
 void arch_initial_func_cfi_state(struct cfi_state *state);
 
 int arch_decode_instruction(struct elf *elf, struct section *sec,
@@ -75,4 +77,8 @@ int arch_decode_instruction(struct elf *elf, struct section *sec,
 
 bool arch_callee_saved_reg(unsigned char reg);
 
+unsigned long arch_jump_destination(struct instruction *insn);
+
+unsigned long arch_dest_rela_offset(int addend);
+
 #endif /* _ARCH_H */
diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
index a62e032863a8..7ce8650cf085 100644
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -11,6 +11,7 @@
 #include "../../../arch/x86/lib/inat.c"
 #include "../../../arch/x86/lib/insn.c"
 
+#include "../../check.h"
 #include "../../elf.h"
 #include "../../arch.h"
 #include "../../warn.h"
@@ -66,6 +67,16 @@ bool arch_callee_saved_reg(unsigned char reg)
 	}
 }
 
+unsigned long arch_dest_rela_offset(int addend)
+{
+	return addend + 4;
+}
+
+unsigned long arch_jump_destination(struct instruction *insn)
+{
+	return insn->offset + insn->len + insn->immediate;
+}
+
 int arch_decode_instruction(struct elf *elf, struct section *sec,
 			    unsigned long offset, unsigned int maxlen,
 			    unsigned int *len, enum insn_type *type,
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index fb1ddde66b48..dbfd1f5d06f7 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -577,13 +577,14 @@ static int add_jump_destinations(struct objtool_file *file)
 					       insn->offset, insn->len);
 		if (!rela) {
 			dest_sec = insn->sec;
-			dest_off = insn->offset + insn->len + insn->immediate;
+			dest_off = arch_jump_destination(insn);
 		} else if (rela->sym->type == STT_SECTION) {
 			dest_sec = rela->sym->sec;
-			dest_off = rela->addend + 4;
+			dest_off = arch_dest_rela_offset(rela->addend);
 		} else if (rela->sym->sec->idx) {
 			dest_sec = rela->sym->sec;
-			dest_off = rela->sym->sym.st_value + rela->addend + 4;
+			dest_off = rela->sym->sym.st_value +
+				   arch_dest_rela_offset(rela->addend);
 		} else if (strstr(rela->sym->name, "_indirect_thunk_")) {
 			/*
 			 * Retpoline jumps are really dynamic jumps in
@@ -673,7 +674,7 @@ static int add_call_destinations(struct objtool_file *file)
 		rela = find_rela_by_dest_range(file->elf, insn->sec,
 					       insn->offset, insn->len);
 		if (!rela) {
-			dest_off = insn->offset + insn->len + insn->immediate;
+			dest_off = arch_jump_destination(insn);
 			insn->call_dest = find_func_by_offset(insn->sec, dest_off);
 			if (!insn->call_dest)
 				insn->call_dest = find_symbol_by_offset(insn->sec, dest_off);
@@ -696,13 +697,14 @@ static int add_call_destinations(struct objtool_file *file)
 			}
 
 		} else if (rela->sym->type == STT_SECTION) {
+			dest_off = arch_dest_rela_offset(rela->addend);
 			insn->call_dest = find_func_by_offset(rela->sym->sec,
-							      rela->addend+4);
+							      dest_off);
 			if (!insn->call_dest) {
-				WARN_FUNC("can't find call dest symbol at %s+0x%x",
+				WARN_FUNC("can't find call dest symbol at %s+0x%lx",
 					  insn->sec, insn->offset,
 					  rela->sym->sec->name,
-					  rela->addend + 4);
+					  dest_off);
 				return -1;
 			}
 		} else
@@ -814,7 +816,7 @@ static int handle_group_alt(struct objtool_file *file,
 		if (!insn->immediate)
 			continue;
 
-		dest_off = insn->offset + insn->len + insn->immediate;
+		dest_off = arch_jump_destination(insn);
 		if (dest_off == special_alt->new_off + special_alt->new_len) {
 			if (!fake_jump) {
 				WARN("%s: alternative jump to end of section",
-- 
2.21.1

[PATCH v2 09/10] objtool: Split generic and arch specific CFI definitions

[Julien Thierry]

Some CFI definitions used by generic objtool code have no reason to vary
from one architecture to another. Move those definition to generic code
and keep a separate per arch header to provide architecture specific CFI
definitions.

Signed-off-by: Julien Thierry <jthierry@redhat.com>
---
 tools/objtool/Makefile                    |  3 ++-
 tools/objtool/arch/x86/include/cfi_regs.h | 25 +++++++++++++++++++++++
 tools/objtool/cfi.h                       | 21 ++-----------------
 3 files changed, 29 insertions(+), 20 deletions(-)
 create mode 100644 tools/objtool/arch/x86/include/cfi_regs.h

diff --git a/tools/objtool/Makefile b/tools/objtool/Makefile
index 519af6ec4eee..11b7dc5d18fe 100644
--- a/tools/objtool/Makefile
+++ b/tools/objtool/Makefile
@@ -29,7 +29,8 @@ all: $(OBJTOOL)
 
 INCLUDES := -I$(srctree)/tools/include \
 	    -I$(srctree)/tools/arch/$(HOSTARCH)/include/uapi \
-	    -I$(srctree)/tools/arch/$(SRCARCH)/include
+	    -I$(srctree)/tools/arch/$(SRCARCH)/include	\
+	    -I$(srctree)/tools/objtool/arch/$(SRCARCH)/include
 WARNINGS := $(EXTRA_WARNINGS) -Wno-switch-default -Wno-switch-enum -Wno-packed
 CFLAGS   := -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -g $(INCLUDES) $(LIBELF_FLAGS)
 LDFLAGS  += $(LIBELF_LIBS) $(LIBSUBCMD) $(KBUILD_HOSTLDFLAGS)
diff --git a/tools/objtool/arch/x86/include/cfi_regs.h b/tools/objtool/arch/x86/include/cfi_regs.h
new file mode 100644
index 000000000000..79bc517efba8
--- /dev/null
+++ b/tools/objtool/arch/x86/include/cfi_regs.h
@@ -0,0 +1,25 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#ifndef _OBJTOOL_CFI_REGS_H
+#define _OBJTOOL_CFI_REGS_H
+
+#define CFI_AX			0
+#define CFI_DX			1
+#define CFI_CX			2
+#define CFI_BX			3
+#define CFI_SI			4
+#define CFI_DI			5
+#define CFI_BP			6
+#define CFI_SP			7
+#define CFI_R8			8
+#define CFI_R9			9
+#define CFI_R10			10
+#define CFI_R11			11
+#define CFI_R12			12
+#define CFI_R13			13
+#define CFI_R14			14
+#define CFI_R15			15
+#define CFI_RA			16
+#define CFI_NUM_REGS		17
+
+#endif /* _OBJTOOL_CFI_REGS_H */
diff --git a/tools/objtool/cfi.h b/tools/objtool/cfi.h
index 4427bf8ed686..1a3e7b807994 100644
--- a/tools/objtool/cfi.h
+++ b/tools/objtool/cfi.h
@@ -6,30 +6,13 @@
 #ifndef _OBJTOOL_CFI_H
 #define _OBJTOOL_CFI_H
 
+#include "cfi_regs.h"
+
 #define CFI_UNDEFINED		-1
 #define CFI_CFA			-2
 #define CFI_SP_INDIRECT		-3
 #define CFI_BP_INDIRECT		-4
 
-#define CFI_AX			0
-#define CFI_DX			1
-#define CFI_CX			2
-#define CFI_BX			3
-#define CFI_SI			4
-#define CFI_DI			5
-#define CFI_BP			6
-#define CFI_SP			7
-#define CFI_R8			8
-#define CFI_R9			9
-#define CFI_R10			10
-#define CFI_R11			11
-#define CFI_R12			12
-#define CFI_R13			13
-#define CFI_R14			14
-#define CFI_R15			15
-#define CFI_RA			16
-#define CFI_NUM_REGS		17
-
 struct cfi_reg {
 	int base;
 	int offset;
-- 
2.21.1

[PATCH v2 10/10] objtool: Support multiple stack_op per instruction

[Julien Thierry]

Instruction sets can include more or less complex operations which might
not fit the currently defined set of stack_ops.

Combining more than one stack_op provides more flexibility to describe
the behaviour of an instruction. This also reduces the need to define
new stack_ops specific to a single instruction set.

Allow instruction decoders to generate multiple stack_op per
instruction.

Signed-off-by: Julien Thierry <jthierry@redhat.com>
---
 tools/objtool/arch.h            |  4 +-
 tools/objtool/arch/x86/decode.c | 13 +++++-
 tools/objtool/check.c           | 74 ++++++++++++++++++++-------------
 tools/objtool/check.h           |  2 +-
 4 files changed, 62 insertions(+), 31 deletions(-)

diff --git a/tools/objtool/arch.h b/tools/objtool/arch.h
index a9a50a25ca66..f9883c431949 100644
--- a/tools/objtool/arch.h
+++ b/tools/objtool/arch.h
@@ -64,6 +64,7 @@ struct op_src {
 struct stack_op {
 	struct op_dest dest;
 	struct op_src src;
+	struct list_head list;
 };
 
 struct instruction;
@@ -73,7 +74,8 @@ void arch_initial_func_cfi_state(struct cfi_state *state);
 int arch_decode_instruction(struct elf *elf, struct section *sec,
 			    unsigned long offset, unsigned int maxlen,
 			    unsigned int *len, enum insn_type *type,
-			    unsigned long *immediate, struct stack_op *op);
+			    unsigned long *immediate,
+			    struct list_head *ops_list);
 
 bool arch_callee_saved_reg(unsigned char reg);
 
diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
index 7ce8650cf085..199b4084a13c 100644
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -80,13 +80,15 @@ unsigned long arch_jump_destination(struct instruction *insn)
 int arch_decode_instruction(struct elf *elf, struct section *sec,
 			    unsigned long offset, unsigned int maxlen,
 			    unsigned int *len, enum insn_type *type,
-			    unsigned long *immediate, struct stack_op *op)
+			    unsigned long *immediate,
+			    struct list_head *ops_list)
 {
 	struct insn insn;
 	int x86_64, sign;
 	unsigned char op1, op2, rex = 0, rex_b = 0, rex_r = 0, rex_w = 0,
 		      rex_x = 0, modrm = 0, modrm_mod = 0, modrm_rm = 0,
 		      modrm_reg = 0, sib = 0;
+	struct stack_op *op;
 
 	x86_64 = is_x86_64(elf);
 	if (x86_64 == -1)
@@ -127,6 +129,10 @@ int arch_decode_instruction(struct elf *elf, struct section *sec,
 	if (insn.sib.nbytes)
 		sib = insn.sib.bytes[0];
 
+	op = calloc(1, sizeof(*op));
+	if (!op)
+		return -1;
+
 	switch (op1) {
 
 	case 0x1:
@@ -488,6 +494,11 @@ int arch_decode_instruction(struct elf *elf, struct section *sec,
 
 	*immediate = insn.immediate.nbytes ? insn.immediate.value : 0;
 
+	if (*type == INSN_STACK)
+		list_add_tail(&op->list, ops_list);
+	else
+		free(op);
+
 	return 0;
 }
 
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index dbfd1f5d06f7..30d916dcc2a8 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -260,6 +260,7 @@ static int decode_instructions(struct objtool_file *file)
 			}
 			memset(insn, 0, sizeof(*insn));
 			INIT_LIST_HEAD(&insn->alts);
+			INIT_LIST_HEAD(&insn->stack_ops);
 			clear_insn_state(&insn->state);
 
 			insn->sec = sec;
@@ -269,7 +270,7 @@ static int decode_instructions(struct objtool_file *file)
 						      sec->len - offset,
 						      &insn->len, &insn->type,
 						      &insn->immediate,
-						      &insn->stack_op);
+						      &insn->stack_ops);
 			if (ret)
 				goto err;
 
@@ -757,6 +758,7 @@ static int handle_group_alt(struct objtool_file *file,
 		}
 		memset(fake_jump, 0, sizeof(*fake_jump));
 		INIT_LIST_HEAD(&fake_jump->alts);
+		INIT_LIST_HEAD(&fake_jump->stack_ops);
 		clear_insn_state(&fake_jump->state);
 
 		fake_jump->sec = special_alt->new_sec;
@@ -1459,10 +1461,11 @@ static bool has_valid_stack_frame(struct insn_state *state)
 	return false;
 }
 
-static int update_insn_state_regs(struct instruction *insn, struct insn_state *state)
+static int update_insn_state_regs(struct instruction *insn,
+				  struct insn_state *state,
+				  struct stack_op *op)
 {
 	struct cfi_reg *cfa = &state->cfa;
-	struct stack_op *op = &insn->stack_op;
 
 	if (cfa->base != CFI_SP)
 		return 0;
@@ -1552,9 +1555,9 @@ static void restore_reg(struct insn_state *state, unsigned char reg)
  *   41 5d			pop    %r13
  *   c3				retq
  */
-static int update_insn_state(struct instruction *insn, struct insn_state *state)
+static int update_insn_state(struct instruction *insn, struct insn_state *state,
+			     struct stack_op *op)
 {
-	struct stack_op *op = &insn->stack_op;
 	struct cfi_reg *cfa = &state->cfa;
 	struct cfi_reg *regs = state->regs;
 
@@ -1568,7 +1571,7 @@ static int update_insn_state(struct instruction *insn, struct insn_state *state)
 	}
 
 	if (state->type == ORC_TYPE_REGS || state->type == ORC_TYPE_REGS_IRET)
-		return update_insn_state_regs(insn, state);
+		return update_insn_state_regs(insn, state, op);
 
 	switch (op->dest.type) {
 
@@ -1905,6 +1908,42 @@ static int update_insn_state(struct instruction *insn, struct insn_state *state)
 	return 0;
 }
 
+static int handle_insn_ops(struct instruction *insn, struct insn_state *state)
+{
+	struct stack_op *op;
+
+	list_for_each_entry(op, &insn->stack_ops, list) {
+		int res;
+
+		res = update_insn_state(insn, state, op);
+		if (res)
+			return res;
+
+		if (op->dest.type == OP_DEST_PUSHF) {
+			if (!state->uaccess_stack) {
+				state->uaccess_stack = 1;
+			} else if (state->uaccess_stack >> 31) {
+				WARN_FUNC("PUSHF stack exhausted",
+					  insn->sec, insn->offset);
+				return 1;
+			}
+			state->uaccess_stack <<= 1;
+			state->uaccess_stack  |= state->uaccess;
+		}
+
+		if (op->src.type == OP_SRC_POPF) {
+			if (state->uaccess_stack) {
+				state->uaccess = state->uaccess_stack & 1;
+				state->uaccess_stack >>= 1;
+				if (state->uaccess_stack == 1)
+					state->uaccess_stack = 0;
+			}
+		}
+	}
+
+	return 0;
+}
+
 static bool insn_state_match(struct instruction *insn, struct insn_state *state)
 {
 	struct insn_state *state1 = &insn->state, *state2 = state;
@@ -2205,29 +2244,8 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
 			return 0;
 
 		case INSN_STACK:
-			if (update_insn_state(insn, &state))
+			if (handle_insn_ops(insn, &state))
 				return 1;
-
-			if (insn->stack_op.dest.type == OP_DEST_PUSHF) {
-				if (!state.uaccess_stack) {
-					state.uaccess_stack = 1;
-				} else if (state.uaccess_stack >> 31) {
-					WARN_FUNC("PUSHF stack exhausted", sec, insn->offset);
-					return 1;
-				}
-				state.uaccess_stack <<= 1;
-				state.uaccess_stack  |= state.uaccess;
-			}
-
-			if (insn->stack_op.src.type == OP_SRC_POPF) {
-				if (state.uaccess_stack) {
-					state.uaccess = state.uaccess_stack & 1;
-					state.uaccess_stack >>= 1;
-					if (state.uaccess_stack == 1)
-						state.uaccess_stack = 0;
-				}
-			}
-
 			break;
 
 		case INSN_STAC:
diff --git a/tools/objtool/check.h b/tools/objtool/check.h
index d1b55961474c..1a089fc0bd23 100644
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -43,7 +43,7 @@ struct instruction {
 	struct list_head alts;
 	struct symbol *func;
 	struct symbol *code_sym;
-	struct stack_op stack_op;
+	struct list_head stack_ops;
 	struct insn_state state;
 	struct orc_entry orc;
 };
-- 
2.21.1

Re: [PATCH v2 01/10] objtool: Move header sync-check ealier in build

[Miroslav Benes]

On Fri, 27 Mar 2020, Julien Thierry wrote:

> Currently, the check of tools files against kernel equivalent is only
> done after every object file has been built. 

> This means one might fix
> build issues against outdated headers without seeing a warning about
> this.

Could you explain the above in more detail, please?

Otherwise it looks good.

Miroslav

> Check headers before any object is built. Also, make it part of a
> FORCE'd recipe so every attempt to build objtool will report the
> outdated headers (if any).
> 
> Signed-off-by: Julien Thierry <jthierry@redhat.com>
> ---
>  tools/objtool/Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/objtool/Makefile b/tools/objtool/Makefile
> index ee08aeff30a1..519af6ec4eee 100644
> --- a/tools/objtool/Makefile
> +++ b/tools/objtool/Makefile
> @@ -43,10 +43,10 @@ export srctree OUTPUT CFLAGS SRCARCH AWK
>  include $(srctree)/tools/build/Makefile.include
>  
>  $(OBJTOOL_IN): fixdep FORCE
> +	@$(CONFIG_SHELL) ./sync-check.sh
>  	@$(MAKE) $(build)=objtool
>  
>  $(OBJTOOL): $(LIBSUBCMD) $(OBJTOOL_IN)
> -	@$(CONFIG_SHELL) ./sync-check.sh
>  	$(QUIET_LINK)$(CC) $(OBJTOOL_IN) $(LDFLAGS) -o $@
>  
>  
> -- 
> 2.21.1
> 

Re: [PATCH v2 01/10] objtool: Move header sync-check ealier in build

[Julien Thierry]

On 4/1/20 1:32 PM, Miroslav Benes wrote:
> On Fri, 27 Mar 2020, Julien Thierry wrote:
> 
>> Currently, the check of tools files against kernel equivalent is only
>> done after every object file has been built.
> 
>> This means one might fix
>> build issues against outdated headers without seeing a warning about
>> this.
> 
> Could you explain the above in more detail, please?
> 

I must admit that this patch is more fixing the issues I've faced while 
working on the arm64 support and sharing some kernel headers from the 
arch/arm64 tree.

The annoying part was:
- Have build errors in objtool
- Fix them
- Objtool build succeeds, but have warning about outdated headers
- Update headers
- New errors come up, potentially making obsolete the ealier fixes

So it's not really a "must have" change. But it's nice to have when 
bringing new kernel headers to objtool.

I hope this makes things clearer.

Cheers,

-- 
Julien Thierry

Re: [PATCH v2 04/10] objtool: check: Ignore empty alternative groups

[Miroslav Benes]

On Fri, 27 Mar 2020, Julien Thierry wrote:

> Atlernative section can contain entries for alternatives with no
> instructions. Objtool will currently crash when handling such an entry.
> 
> Just skip that entry, but still give a warning to discourage useless
> entries.
> 
> Signed-off-by: Julien Thierry <jthierry@redhat.com>
> ---
>  tools/objtool/check.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index 74353b2c39ce..5c03460f1f07 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -904,6 +904,12 @@ static int add_special_section_alts(struct objtool_file *file)
>  		}
>  
>  		if (special_alt->group) {
> +			if (!special_alt->orig_len) {
> +				WARN_FUNC("empty alternative entry",
> +					  orig_insn->sec, orig_insn->offset);
> +				continue;
> +			}
> +
>  			ret = handle_group_alt(file, special_alt, orig_insn,
>  					       &new_insn);
>  			if (ret)

Probably the first time I am looking at alternatives handling in objtool, 
so I must be missing something, but is this even possible now? I mean 
get_alt_entry() in special.c sets alt->orig_len when alt->group is true 
(which means .alternatives section) to something which cannot be zero.

Is this a preparatory patch for arm64, where this could happen? If yes, it 
would be better to mention it in the changelog.

Miroslav

Re: [PATCH v2 01/10] objtool: Move header sync-check ealier in build

[Miroslav Benes]

On Wed, 1 Apr 2020, Julien Thierry wrote:

> 
> 
> On 4/1/20 1:32 PM, Miroslav Benes wrote:
> > On Fri, 27 Mar 2020, Julien Thierry wrote:
> > 
> >> Currently, the check of tools files against kernel equivalent is only
> >> done after every object file has been built.
> > 
> >> This means one might fix
> >> build issues against outdated headers without seeing a warning about
> >> this.
> > 
> > Could you explain the above in more detail, please?
> > 
> 
> I must admit that this patch is more fixing the issues I've faced while
> working on the arm64 support and sharing some kernel headers from the
> arch/arm64 tree.
> 
> The annoying part was:
> - Have build errors in objtool
> - Fix them
> - Objtool build succeeds, but have warning about outdated headers
> - Update headers
> - New errors come up, potentially making obsolete the ealier fixes

Ah right, yes.
 
> So it's not really a "must have" change. But it's nice to have when bringing
> new kernel headers to objtool.

No, I think it is useful.

> I hope this makes things clearer.

Yes, it does.

Thanks
Miroslav

Re: [PATCH v2 04/10] objtool: check: Ignore empty alternative groups

[Julien Thierry]

On 4/1/20 1:53 PM, Miroslav Benes wrote:
> On Fri, 27 Mar 2020, Julien Thierry wrote:
> 
>> Atlernative section can contain entries for alternatives with no
>> instructions. Objtool will currently crash when handling such an entry.
>>
>> Just skip that entry, but still give a warning to discourage useless
>> entries.
>>
>> Signed-off-by: Julien Thierry <jthierry@redhat.com>
>> ---
>>   tools/objtool/check.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
>> index 74353b2c39ce..5c03460f1f07 100644
>> --- a/tools/objtool/check.c
>> +++ b/tools/objtool/check.c
>> @@ -904,6 +904,12 @@ static int add_special_section_alts(struct objtool_file *file)
>>   		}
>>   
>>   		if (special_alt->group) {
>> +			if (!special_alt->orig_len) {
>> +				WARN_FUNC("empty alternative entry",
>> +					  orig_insn->sec, orig_insn->offset);
>> +				continue;
>> +			}
>> +
>>   			ret = handle_group_alt(file, special_alt, orig_insn,
>>   					       &new_insn);
>>   			if (ret)
> 
> Probably the first time I am looking at alternatives handling in objtool,
> so I must be missing something, but is this even possible now? I mean
> get_alt_entry() in special.c sets alt->orig_len when alt->group is true
> (which means .alternatives section) to something which cannot be zero.
> 

What I see is:

	if (alt->group) {
		alt->orig_len = *(unsigned char *)(sec->data->d_buf + offset +
						   entry->orig_len);
		alt->new_len = *(unsigned char *)(sec->data->d_buf + offset +
                                                   entry->new_len);
	}


And as far as I can tell, "alt->orig_len" can be 0 if the entry in the 
.altinstructions section of the .o file has the length set to 0.

I don't know how the alternative section generation works on x86, but on 
arm64 it's just a computed assembly offset which can be 0.

> Is this a preparatory patch for arm64, where this could happen? If yes, it
> would be better to mention it in the changelog.
> 

It used to happen on arm64, but the fix [1] was picked.

I can add that link to the commit if necessary.

[1] https://lkml.org/lkml/2020/1/9/708

Cheers,

-- 
Julien Thierry

Re: [PATCH v2 04/10] objtool: check: Ignore empty alternative groups

[Miroslav Benes]

On Wed, 1 Apr 2020, Julien Thierry wrote:

> 
> 
> On 4/1/20 1:53 PM, Miroslav Benes wrote:
> > On Fri, 27 Mar 2020, Julien Thierry wrote:
> > 
> >> Atlernative section can contain entries for alternatives with no
> >> instructions. Objtool will currently crash when handling such an entry.
> >>
> >> Just skip that entry, but still give a warning to discourage useless
> >> entries.
> >>
> >> Signed-off-by: Julien Thierry <jthierry@redhat.com>
> >> ---
> >>   tools/objtool/check.c | 6 ++++++
> >>   1 file changed, 6 insertions(+)
> >>
> >> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> >> index 74353b2c39ce..5c03460f1f07 100644
> >> --- a/tools/objtool/check.c
> >> +++ b/tools/objtool/check.c
> >> @@ -904,6 +904,12 @@ static int add_special_section_alts(struct
> >> objtool_file *file)
> >>     }
> >>   
> >>   		if (special_alt->group) {
> >> +			if (!special_alt->orig_len) {
> >> +				WARN_FUNC("empty alternative entry",
> >> +					  orig_insn->sec, orig_insn->offset);
> >> +				continue;
> >> +			}
> >> +
> >>      ret = handle_group_alt(file, special_alt, orig_insn,
> >>      		       &new_insn);
> >>      if (ret)
> > 
> > Probably the first time I am looking at alternatives handling in objtool,
> > so I must be missing something, but is this even possible now? I mean
> > get_alt_entry() in special.c sets alt->orig_len when alt->group is true
> > (which means .alternatives section) to something which cannot be zero.
> > 
> 
> What I see is:
> 
> 	if (alt->group) {
> 		alt->orig_len = *(unsigned char *)(sec->data->d_buf + offset +
> 						   entry->orig_len);
> 		alt->new_len = *(unsigned char *)(sec->data->d_buf + offset +
> 	                                                   entry->new_len);
> 	}

Now that you copy-pasted the code here, I see that I completely missed 
there is dereference (for obvious reasons) right before the type cast, so 
all is fine. My mistake, I need more tea.

> And as far as I can tell, "alt->orig_len" can be 0 if the entry in the
> .altinstructions section of the .o file has the length set to 0.

Yes

> I don't know how the alternative section generation works on x86, but on arm64
> it's just a computed assembly offset which can be 0.
> 
> > Is this a preparatory patch for arm64, where this could happen? If yes, it
> > would be better to mention it in the changelog.
> > 
> 
> It used to happen on arm64, but the fix [1] was picked.
> 
> I can add that link to the commit if necessary.

No, I think the check makes sense on its own.

Thanks
Miroslav

Re: [PATCH v2 07/10] objtool: check: Allow save/restore hint in non standard function symbols

[Miroslav Benes]

On Fri, 27 Mar 2020, Julien Thierry wrote:

> The kernel code base provides CODE_SYM_START/END to declare assembly
> code sequences that don't follow function standard calling conventions.
> 
> As non-C/non-standard code, these sequences can very much benefit from
> unwind hints. However, when a restore unwind_hint is used in a
> non-function code sequence, objtool will crash when looking for the
> corresponding save hint.
> 
> Record the code symbol an instruction belongs to and look for save hints
> belonging to the same code symbol as the restore hint.
> 
> Signed-off-by: Julien Thierry <jthierry@redhat.com>

Looks ok, but save/restore hints are about to go away. See 
https://lore.kernel.org/lkml/20200331222703.GH2452@worktop.programming.kicks-ass.net/

Miroslav

Re: [PATCH v2 07/10] objtool: check: Allow save/restore hint in non standard function symbols

[Julien Thierry]

On 4/1/20 2:54 PM, Miroslav Benes wrote:
> On Fri, 27 Mar 2020, Julien Thierry wrote:
> 
>> The kernel code base provides CODE_SYM_START/END to declare assembly
>> code sequences that don't follow function standard calling conventions.
>>
>> As non-C/non-standard code, these sequences can very much benefit from
>> unwind hints. However, when a restore unwind_hint is used in a
>> non-function code sequence, objtool will crash when looking for the
>> corresponding save hint.
>>
>> Record the code symbol an instruction belongs to and look for save hints
>> belonging to the same code symbol as the restore hint.
>>
>> Signed-off-by: Julien Thierry <jthierry@redhat.com>
> 
> Looks ok, but save/restore hints are about to go away. See
> https://lore.kernel.org/lkml/20200331222703.GH2452@worktop.programming.kicks-ass.net/
> 

Ah, just as I started bringing unwind hints to the arm64 side...

I'll have to scratch my head a bit more over this then. Thanks for 
bringing it to my attention.

Thanks,

-- 
Julien Thierry

Re: [PATCH v2 01/10] objtool: Move header sync-check ealier in build

[Josh Poimboeuf]

On Fri, Mar 27, 2020 at 03:28:38PM +0000, Julien Thierry wrote:
> Currently, the check of tools files against kernel equivalent is only
> done after every object file has been built. This means one might fix
> build issues against outdated headers without seeing a warning about
> this.

s/ealier/earlier/ in $SUBJECT

-- 
Josh

Re: [PATCH v2 01/10] objtool: Move header sync-check ealier in build

[Josh Poimboeuf]

On Thu, Apr 02, 2020 at 12:12:37PM -0500, Josh Poimboeuf wrote:
> On Fri, Mar 27, 2020 at 03:28:38PM +0000, Julien Thierry wrote:
> > Currently, the check of tools files against kernel equivalent is only
> > done after every object file has been built. This means one might fix
> > build issues against outdated headers without seeing a warning about
> > this.
> 
> s/ealier/earlier/ in $SUBJECT

Actually don't worry about fixing that up, after my review I'll take
this patch (and any other trivial ones) and do any trivial cleanups like
that and then forward them along to the tip tree.

-- 
Josh

Re: [PATCH v2 10/10] objtool: Support multiple stack_op per instruction

[Josh Poimboeuf]

On Fri, Mar 27, 2020 at 03:28:47PM +0000, Julien Thierry wrote:
> @@ -127,6 +129,10 @@ int arch_decode_instruction(struct elf *elf, struct section *sec,
>  	if (insn.sib.nbytes)
>  		sib = insn.sib.bytes[0];
>  
> +	op = calloc(1, sizeof(*op));
> +	if (!op)
> +		return -1;
> +

Why not malloc()?

> +static int handle_insn_ops(struct instruction *insn, struct insn_state *state)
> +{
> +	struct stack_op *op;
> +
> +	list_for_each_entry(op, &insn->stack_ops, list) {
> +		int res;
> +
> +		res = update_insn_state(insn, state, op);
> +		if (res)
> +			return res;

This should probably be like:

		if (update_insn_state(insn, state, op))
			return 1;

That way the error codes are converted to non-fatal warnings like before
(which I admit is confusing...)

> @@ -2205,29 +2244,8 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
>  			return 0;
>  
>  		case INSN_STACK:
> -			if (update_insn_state(insn, &state))
> +			if (handle_insn_ops(insn, &state))
>  				return 1;

How about "handle_stack_ops"?

-- 
Josh

Re: [PATCH v2 00/10] Objtool updates for easier portability

[Josh Poimboeuf]

On Fri, Mar 27, 2020 at 03:28:37PM +0000, Julien Thierry wrote:
> Hi,
> 
> This patchset includes some of the least controversial changes that
> were needed as part of the arm64 port [1].
> 
> I'm resending these rebase on top of linux-tip/core/objtool, following
> the addition of Peter's patches [2]
> 
> It consist mostly of small fixes or lifting some limitations to make it
> easier to support a new architecture in objtool. Of course, these will
> not be the only required changes, but these are the ones I hope make
> enough sense to be merged separately from the rest of arm64 port series.
> 
> Changes since v1[3]:
> - Really just rebased things
> 
> [1] https://lkml.org/lkml/2020/1/9/643
> [2] https://lkml.org/lkml/2020/3/25/807
> [3] https://www.spinics.net/lists/kernel/msg3453718.html
> 
> Thanks,
> 
> Julien

I'm taking everything except 5, 7, and 10.  I'll run them through
testing and then send them along to the tip maintainers.  Thanks!

-- 
Josh

Re: [PATCH v2 10/10] objtool: Support multiple stack_op per instruction

[Peter Zijlstra]

On Thu, Apr 02, 2020 at 12:54:26PM -0500, Josh Poimboeuf wrote:
> On Fri, Mar 27, 2020 at 03:28:47PM +0000, Julien Thierry wrote:
> > @@ -127,6 +129,10 @@ int arch_decode_instruction(struct elf *elf, struct section *sec,
> >  	if (insn.sib.nbytes)
> >  		sib = insn.sib.bytes[0];
> >  
> > +	op = calloc(1, sizeof(*op));
> > +	if (!op)
> > +		return -1;
> > +
> 
> Why not malloc()?

calloc() does a memset(), I wondered the same and read the manpage ;-)

Re: [PATCH v2 00/10] Objtool updates for easier portability

[Peter Zijlstra]

On Thu, Apr 02, 2020 at 12:58:27PM -0500, Josh Poimboeuf wrote:
> On Fri, Mar 27, 2020 at 03:28:37PM +0000, Julien Thierry wrote:
> > Hi,
> > 
> > This patchset includes some of the least controversial changes that
> > were needed as part of the arm64 port [1].
> > 
> > I'm resending these rebase on top of linux-tip/core/objtool, following
> > the addition of Peter's patches [2]
> > 
> > It consist mostly of small fixes or lifting some limitations to make it
> > easier to support a new architecture in objtool. Of course, these will
> > not be the only required changes, but these are the ones I hope make
> > enough sense to be merged separately from the rest of arm64 port series.
> > 
> > Changes since v1[3]:
> > - Really just rebased things
> > 
> > [1] https://lkml.org/lkml/2020/1/9/643
> > [2] https://lkml.org/lkml/2020/3/25/807
> > [3] https://www.spinics.net/lists/kernel/msg3453718.html
> > 
> > Thanks,
> > 
> > Julien
> 
> I'm taking everything except 5, 7, and 10.  I'll run them through
> testing and then send them along to the tip maintainers.  Thanks!

For that set:

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Re: [PATCH v2 00/10] Objtool updates for easier portability

[Miroslav Benes]

On Thu, 2 Apr 2020, Peter Zijlstra wrote:

> On Thu, Apr 02, 2020 at 12:58:27PM -0500, Josh Poimboeuf wrote:
> > On Fri, Mar 27, 2020 at 03:28:37PM +0000, Julien Thierry wrote:
> > > Hi,
> > > 
> > > This patchset includes some of the least controversial changes that
> > > were needed as part of the arm64 port [1].
> > > 
> > > I'm resending these rebase on top of linux-tip/core/objtool, following
> > > the addition of Peter's patches [2]
> > > 
> > > It consist mostly of small fixes or lifting some limitations to make it
> > > easier to support a new architecture in objtool. Of course, these will
> > > not be the only required changes, but these are the ones I hope make
> > > enough sense to be merged separately from the rest of arm64 port series.
> > > 
> > > Changes since v1[3]:
> > > - Really just rebased things
> > > 
> > > [1] https://lkml.org/lkml/2020/1/9/643
> > > [2] https://lkml.org/lkml/2020/3/25/807
> > > [3] https://www.spinics.net/lists/kernel/msg3453718.html
> > > 
> > > Thanks,
> > > 
> > > Julien
> > 
> > I'm taking everything except 5, 7, and 10.  I'll run them through
> > testing and then send them along to the tip maintainers.  Thanks!
> 
> For that set:
> 
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

And

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

for the same.

Re: [PATCH v2 10/10] objtool: Support multiple stack_op per instruction

[Julien Thierry]

On 4/2/20 6:54 PM, Josh Poimboeuf wrote:
> On Fri, Mar 27, 2020 at 03:28:47PM +0000, Julien Thierry wrote:
>> @@ -127,6 +129,10 @@ int arch_decode_instruction(struct elf *elf, struct section *sec,
>>   	if (insn.sib.nbytes)
>>   		sib = insn.sib.bytes[0];
>>   
>> +	op = calloc(1, sizeof(*op));
>> +	if (!op)
>> +		return -1;
>> +
> 
> Why not malloc()?
> 

It's just that previsously, stack_op was part of the instruction 
structure and was initialized to all 0 in decode_instructions(). Now 
that it's created here, I assumed it would be better to have the same 
thing here and initialized the new stack_op to all 0.

Do you prefer to have an explicit malloc() + memset()?

>> +static int handle_insn_ops(struct instruction *insn, struct insn_state *state)
>> +{
>> +	struct stack_op *op;
>> +
>> +	list_for_each_entry(op, &insn->stack_ops, list) {
>> +		int res;
>> +
>> +		res = update_insn_state(insn, state, op);
>> +		if (res)
>> +			return res;
> 
> This should probably be like:
> 
> 		if (update_insn_state(insn, state, op))
> 			return 1;
> 
> That way the error codes are converted to non-fatal warnings like before
> (which I admit is confusing...)
> 

Right, I'll change this.

>> @@ -2205,29 +2244,8 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
>>   			return 0;
>>   
>>   		case INSN_STACK:
>> -			if (update_insn_state(insn, &state))
>> +			if (handle_insn_ops(insn, &state))
>>   				return 1;
> 
> How about "handle_stack_ops"?
> 

Works for me!

Thanks,

-- 
Julien Thierry

Re: [PATCH v2 10/10] objtool: Support multiple stack_op per instruction

[Josh Poimboeuf]

On Fri, Apr 03, 2020 at 09:01:46AM +0100, Julien Thierry wrote:
> 
> 
> On 4/2/20 6:54 PM, Josh Poimboeuf wrote:
> > On Fri, Mar 27, 2020 at 03:28:47PM +0000, Julien Thierry wrote:
> > > @@ -127,6 +129,10 @@ int arch_decode_instruction(struct elf *elf, struct section *sec,
> > >   	if (insn.sib.nbytes)
> > >   		sib = insn.sib.bytes[0];
> > > +	op = calloc(1, sizeof(*op));
> > > +	if (!op)
> > > +		return -1;
> > > +
> > 
> > Why not malloc()?
> > 
> 
> It's just that previsously, stack_op was part of the instruction structure
> and was initialized to all 0 in decode_instructions(). Now that it's created
> here, I assumed it would be better to have the same thing here and
> initialized the new stack_op to all 0.
> 
> Do you prefer to have an explicit malloc() + memset()?

Maybe just add a comment that calloc() is equivalent to malloc() +
memset() zero, for us user-space neophytes :-)

-- 
Josh

[tip: objtool/core] objtool: Support multiple stack_op per instruction

[tip-bot2 for Julien Thierry]

The following commit has been merged into the objtool/core branch of tip:

Commit-ID:     dd4d6c5599065055dd89e5d7773c267cf86431e2
Gitweb:        https://git.kernel.org/tip/dd4d6c5599065055dd89e5d7773c267cf86431e2
Author:        Julien Thierry <jthierry@redhat.com>
AuthorDate:    Fri, 27 Mar 2020 15:28:47 
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Wed, 22 Apr 2020 23:10:05 +02:00

objtool: Support multiple stack_op per instruction

Instruction sets can include more or less complex operations which might
not fit the currently defined set of stack_ops.

Combining more than one stack_op provides more flexibility to describe
the behaviour of an instruction. This also reduces the need to define
new stack_ops specific to a single instruction set.

Allow instruction decoders to generate multiple stack_op per
instruction.

Signed-off-by: Julien Thierry <jthierry@redhat.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Miroslav Benes <mbenes@suse.cz>
Reviewed-by: Alexandre Chartre <alexandre.chartre@oracle.com>
Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
Link: https://lkml.kernel.org/r/20200327152847.15294-11-jthierry@redhat.com
---
 tools/objtool/arch.h            |  4 +-
 tools/objtool/arch/x86/decode.c | 13 +++++-
 tools/objtool/check.c           | 74 +++++++++++++++++++-------------
 tools/objtool/check.h           |  2 +-
 4 files changed, 62 insertions(+), 31 deletions(-)

diff --git a/tools/objtool/arch.h b/tools/objtool/arch.h
index a9a50a2..f9883c4 100644
--- a/tools/objtool/arch.h
+++ b/tools/objtool/arch.h
@@ -64,6 +64,7 @@ struct op_src {
 struct stack_op {
 	struct op_dest dest;
 	struct op_src src;
+	struct list_head list;
 };
 
 struct instruction;
@@ -73,7 +74,8 @@ void arch_initial_func_cfi_state(struct cfi_state *state);
 int arch_decode_instruction(struct elf *elf, struct section *sec,
 			    unsigned long offset, unsigned int maxlen,
 			    unsigned int *len, enum insn_type *type,
-			    unsigned long *immediate, struct stack_op *op);
+			    unsigned long *immediate,
+			    struct list_head *ops_list);
 
 bool arch_callee_saved_reg(unsigned char reg);
 
diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
index 7ce8650..199b408 100644
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -80,13 +80,15 @@ unsigned long arch_jump_destination(struct instruction *insn)
 int arch_decode_instruction(struct elf *elf, struct section *sec,
 			    unsigned long offset, unsigned int maxlen,
 			    unsigned int *len, enum insn_type *type,
-			    unsigned long *immediate, struct stack_op *op)
+			    unsigned long *immediate,
+			    struct list_head *ops_list)
 {
 	struct insn insn;
 	int x86_64, sign;
 	unsigned char op1, op2, rex = 0, rex_b = 0, rex_r = 0, rex_w = 0,
 		      rex_x = 0, modrm = 0, modrm_mod = 0, modrm_rm = 0,
 		      modrm_reg = 0, sib = 0;
+	struct stack_op *op;
 
 	x86_64 = is_x86_64(elf);
 	if (x86_64 == -1)
@@ -127,6 +129,10 @@ int arch_decode_instruction(struct elf *elf, struct section *sec,
 	if (insn.sib.nbytes)
 		sib = insn.sib.bytes[0];
 
+	op = calloc(1, sizeof(*op));
+	if (!op)
+		return -1;
+
 	switch (op1) {
 
 	case 0x1:
@@ -488,6 +494,11 @@ int arch_decode_instruction(struct elf *elf, struct section *sec,
 
 	*immediate = insn.immediate.nbytes ? insn.immediate.value : 0;
 
+	if (*type == INSN_STACK)
+		list_add_tail(&op->list, ops_list);
+	else
+		free(op);
+
 	return 0;
 }
 
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index f4254d4..819de0d 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -260,6 +260,7 @@ static int decode_instructions(struct objtool_file *file)
 			}
 			memset(insn, 0, sizeof(*insn));
 			INIT_LIST_HEAD(&insn->alts);
+			INIT_LIST_HEAD(&insn->stack_ops);
 			clear_insn_state(&insn->state);
 
 			insn->sec = sec;
@@ -269,7 +270,7 @@ static int decode_instructions(struct objtool_file *file)
 						      sec->len - offset,
 						      &insn->len, &insn->type,
 						      &insn->immediate,
-						      &insn->stack_op);
+						      &insn->stack_ops);
 			if (ret)
 				goto err;
 
@@ -770,6 +771,7 @@ static int handle_group_alt(struct objtool_file *file,
 		}
 		memset(fake_jump, 0, sizeof(*fake_jump));
 		INIT_LIST_HEAD(&fake_jump->alts);
+		INIT_LIST_HEAD(&fake_jump->stack_ops);
 		clear_insn_state(&fake_jump->state);
 
 		fake_jump->sec = special_alt->new_sec;
@@ -1468,10 +1470,11 @@ static bool has_valid_stack_frame(struct insn_state *state)
 	return false;
 }
 
-static int update_insn_state_regs(struct instruction *insn, struct insn_state *state)
+static int update_insn_state_regs(struct instruction *insn,
+				  struct insn_state *state,
+				  struct stack_op *op)
 {
 	struct cfi_reg *cfa = &state->cfa;
-	struct stack_op *op = &insn->stack_op;
 
 	if (cfa->base != CFI_SP)
 		return 0;
@@ -1561,9 +1564,9 @@ static void restore_reg(struct insn_state *state, unsigned char reg)
  *   41 5d			pop    %r13
  *   c3				retq
  */
-static int update_insn_state(struct instruction *insn, struct insn_state *state)
+static int update_insn_state(struct instruction *insn, struct insn_state *state,
+			     struct stack_op *op)
 {
-	struct stack_op *op = &insn->stack_op;
 	struct cfi_reg *cfa = &state->cfa;
 	struct cfi_reg *regs = state->regs;
 
@@ -1577,7 +1580,7 @@ static int update_insn_state(struct instruction *insn, struct insn_state *state)
 	}
 
 	if (state->type == ORC_TYPE_REGS || state->type == ORC_TYPE_REGS_IRET)
-		return update_insn_state_regs(insn, state);
+		return update_insn_state_regs(insn, state, op);
 
 	switch (op->dest.type) {
 
@@ -1914,6 +1917,42 @@ static int update_insn_state(struct instruction *insn, struct insn_state *state)
 	return 0;
 }
 
+static int handle_insn_ops(struct instruction *insn, struct insn_state *state)
+{
+	struct stack_op *op;
+
+	list_for_each_entry(op, &insn->stack_ops, list) {
+		int res;
+
+		res = update_insn_state(insn, state, op);
+		if (res)
+			return res;
+
+		if (op->dest.type == OP_DEST_PUSHF) {
+			if (!state->uaccess_stack) {
+				state->uaccess_stack = 1;
+			} else if (state->uaccess_stack >> 31) {
+				WARN_FUNC("PUSHF stack exhausted",
+					  insn->sec, insn->offset);
+				return 1;
+			}
+			state->uaccess_stack <<= 1;
+			state->uaccess_stack  |= state->uaccess;
+		}
+
+		if (op->src.type == OP_SRC_POPF) {
+			if (state->uaccess_stack) {
+				state->uaccess = state->uaccess_stack & 1;
+				state->uaccess_stack >>= 1;
+				if (state->uaccess_stack == 1)
+					state->uaccess_stack = 0;
+			}
+		}
+	}
+
+	return 0;
+}
+
 static bool insn_state_match(struct instruction *insn, struct insn_state *state)
 {
 	struct insn_state *state1 = &insn->state, *state2 = state;
@@ -2214,29 +2253,8 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
 			return 0;
 
 		case INSN_STACK:
-			if (update_insn_state(insn, &state))
+			if (handle_insn_ops(insn, &state))
 				return 1;
-
-			if (insn->stack_op.dest.type == OP_DEST_PUSHF) {
-				if (!state.uaccess_stack) {
-					state.uaccess_stack = 1;
-				} else if (state.uaccess_stack >> 31) {
-					WARN_FUNC("PUSHF stack exhausted", sec, insn->offset);
-					return 1;
-				}
-				state.uaccess_stack <<= 1;
-				state.uaccess_stack  |= state.uaccess;
-			}
-
-			if (insn->stack_op.src.type == OP_SRC_POPF) {
-				if (state.uaccess_stack) {
-					state.uaccess = state.uaccess_stack & 1;
-					state.uaccess_stack >>= 1;
-					if (state.uaccess_stack == 1)
-						state.uaccess_stack = 0;
-				}
-			}
-
 			break;
 
 		case INSN_STAC:
diff --git a/tools/objtool/check.h b/tools/objtool/check.h
index f0ce8ff..2c55f75 100644
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -42,7 +42,7 @@ struct instruction {
 	struct rela *jump_table;
 	struct list_head alts;
 	struct symbol *func;
-	struct stack_op stack_op;
+	struct list_head stack_ops;
 	struct insn_state state;
 	struct orc_entry orc;
 };

[tip: objtool/core] objtool: Support multiple stack_op per instruction

[tip-bot2 for Julien Thierry]

The following commit has been merged into the objtool/core branch of tip:

Commit-ID:     65ea47dcf4f936987a5fbf839c97acea00f4f196
Gitweb:        https://git.kernel.org/tip/65ea47dcf4f936987a5fbf839c97acea00f4f196
Author:        Julien Thierry <jthierry@redhat.com>
AuthorDate:    Fri, 27 Mar 2020 15:28:47 
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Wed, 22 Apr 2020 10:53:50 +02:00

objtool: Support multiple stack_op per instruction

Instruction sets can include more or less complex operations which might
not fit the currently defined set of stack_ops.

Combining more than one stack_op provides more flexibility to describe
the behaviour of an instruction. This also reduces the need to define
new stack_ops specific to a single instruction set.

Allow instruction decoders to generate multiple stack_op per
instruction.

Signed-off-by: Julien Thierry <jthierry@redhat.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Miroslav Benes <mbenes@suse.cz>
Reviewed-by: Alexandre Chartre <alexandre.chartre@oracle.com>
Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
Link: https://lkml.kernel.org/r/20200327152847.15294-11-jthierry@redhat.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 tools/objtool/arch.h            |  4 +-
 tools/objtool/arch/x86/decode.c | 13 +++++-
 tools/objtool/check.c           | 74 +++++++++++++++++++-------------
 tools/objtool/check.h           |  2 +-
 4 files changed, 62 insertions(+), 31 deletions(-)

diff --git a/tools/objtool/arch.h b/tools/objtool/arch.h
index a9a50a2..f9883c4 100644
--- a/tools/objtool/arch.h
+++ b/tools/objtool/arch.h
@@ -64,6 +64,7 @@ struct op_src {
 struct stack_op {
 	struct op_dest dest;
 	struct op_src src;
+	struct list_head list;
 };
 
 struct instruction;
@@ -73,7 +74,8 @@ void arch_initial_func_cfi_state(struct cfi_state *state);
 int arch_decode_instruction(struct elf *elf, struct section *sec,
 			    unsigned long offset, unsigned int maxlen,
 			    unsigned int *len, enum insn_type *type,
-			    unsigned long *immediate, struct stack_op *op);
+			    unsigned long *immediate,
+			    struct list_head *ops_list);
 
 bool arch_callee_saved_reg(unsigned char reg);
 
diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
index 7ce8650..199b408 100644
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -80,13 +80,15 @@ unsigned long arch_jump_destination(struct instruction *insn)
 int arch_decode_instruction(struct elf *elf, struct section *sec,
 			    unsigned long offset, unsigned int maxlen,
 			    unsigned int *len, enum insn_type *type,
-			    unsigned long *immediate, struct stack_op *op)
+			    unsigned long *immediate,
+			    struct list_head *ops_list)
 {
 	struct insn insn;
 	int x86_64, sign;
 	unsigned char op1, op2, rex = 0, rex_b = 0, rex_r = 0, rex_w = 0,
 		      rex_x = 0, modrm = 0, modrm_mod = 0, modrm_rm = 0,
 		      modrm_reg = 0, sib = 0;
+	struct stack_op *op;
 
 	x86_64 = is_x86_64(elf);
 	if (x86_64 == -1)
@@ -127,6 +129,10 @@ int arch_decode_instruction(struct elf *elf, struct section *sec,
 	if (insn.sib.nbytes)
 		sib = insn.sib.bytes[0];
 
+	op = calloc(1, sizeof(*op));
+	if (!op)
+		return -1;
+
 	switch (op1) {
 
 	case 0x1:
@@ -488,6 +494,11 @@ int arch_decode_instruction(struct elf *elf, struct section *sec,
 
 	*immediate = insn.immediate.nbytes ? insn.immediate.value : 0;
 
+	if (*type == INSN_STACK)
+		list_add_tail(&op->list, ops_list);
+	else
+		free(op);
+
 	return 0;
 }
 
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index e06a891..9e854fd 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -260,6 +260,7 @@ static int decode_instructions(struct objtool_file *file)
 			}
 			memset(insn, 0, sizeof(*insn));
 			INIT_LIST_HEAD(&insn->alts);
+			INIT_LIST_HEAD(&insn->stack_ops);
 			clear_insn_state(&insn->state);
 
 			insn->sec = sec;
@@ -269,7 +270,7 @@ static int decode_instructions(struct objtool_file *file)
 						      sec->len - offset,
 						      &insn->len, &insn->type,
 						      &insn->immediate,
-						      &insn->stack_op);
+						      &insn->stack_ops);
 			if (ret)
 				goto err;
 
@@ -754,6 +755,7 @@ static int handle_group_alt(struct objtool_file *file,
 		}
 		memset(fake_jump, 0, sizeof(*fake_jump));
 		INIT_LIST_HEAD(&fake_jump->alts);
+		INIT_LIST_HEAD(&fake_jump->stack_ops);
 		clear_insn_state(&fake_jump->state);
 
 		fake_jump->sec = special_alt->new_sec;
@@ -1452,10 +1454,11 @@ static bool has_valid_stack_frame(struct insn_state *state)
 	return false;
 }
 
-static int update_insn_state_regs(struct instruction *insn, struct insn_state *state)
+static int update_insn_state_regs(struct instruction *insn,
+				  struct insn_state *state,
+				  struct stack_op *op)
 {
 	struct cfi_reg *cfa = &state->cfa;
-	struct stack_op *op = &insn->stack_op;
 
 	if (cfa->base != CFI_SP)
 		return 0;
@@ -1545,9 +1548,9 @@ static void restore_reg(struct insn_state *state, unsigned char reg)
  *   41 5d			pop    %r13
  *   c3				retq
  */
-static int update_insn_state(struct instruction *insn, struct insn_state *state)
+static int update_insn_state(struct instruction *insn, struct insn_state *state,
+			     struct stack_op *op)
 {
-	struct stack_op *op = &insn->stack_op;
 	struct cfi_reg *cfa = &state->cfa;
 	struct cfi_reg *regs = state->regs;
 
@@ -1561,7 +1564,7 @@ static int update_insn_state(struct instruction *insn, struct insn_state *state)
 	}
 
 	if (state->type == ORC_TYPE_REGS || state->type == ORC_TYPE_REGS_IRET)
-		return update_insn_state_regs(insn, state);
+		return update_insn_state_regs(insn, state, op);
 
 	switch (op->dest.type) {
 
@@ -1898,6 +1901,42 @@ static int update_insn_state(struct instruction *insn, struct insn_state *state)
 	return 0;
 }
 
+static int handle_insn_ops(struct instruction *insn, struct insn_state *state)
+{
+	struct stack_op *op;
+
+	list_for_each_entry(op, &insn->stack_ops, list) {
+		int res;
+
+		res = update_insn_state(insn, state, op);
+		if (res)
+			return res;
+
+		if (op->dest.type == OP_DEST_PUSHF) {
+			if (!state->uaccess_stack) {
+				state->uaccess_stack = 1;
+			} else if (state->uaccess_stack >> 31) {
+				WARN_FUNC("PUSHF stack exhausted",
+					  insn->sec, insn->offset);
+				return 1;
+			}
+			state->uaccess_stack <<= 1;
+			state->uaccess_stack  |= state->uaccess;
+		}
+
+		if (op->src.type == OP_SRC_POPF) {
+			if (state->uaccess_stack) {
+				state->uaccess = state->uaccess_stack & 1;
+				state->uaccess_stack >>= 1;
+				if (state->uaccess_stack == 1)
+					state->uaccess_stack = 0;
+			}
+		}
+	}
+
+	return 0;
+}
+
 static bool insn_state_match(struct instruction *insn, struct insn_state *state)
 {
 	struct insn_state *state1 = &insn->state, *state2 = state;
@@ -2198,29 +2237,8 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
 			return 0;
 
 		case INSN_STACK:
-			if (update_insn_state(insn, &state))
+			if (handle_insn_ops(insn, &state))
 				return 1;
-
-			if (insn->stack_op.dest.type == OP_DEST_PUSHF) {
-				if (!state.uaccess_stack) {
-					state.uaccess_stack = 1;
-				} else if (state.uaccess_stack >> 31) {
-					WARN_FUNC("PUSHF stack exhausted", sec, insn->offset);
-					return 1;
-				}
-				state.uaccess_stack <<= 1;
-				state.uaccess_stack  |= state.uaccess;
-			}
-
-			if (insn->stack_op.src.type == OP_SRC_POPF) {
-				if (state.uaccess_stack) {
-					state.uaccess = state.uaccess_stack & 1;
-					state.uaccess_stack >>= 1;
-					if (state.uaccess_stack == 1)
-						state.uaccess_stack = 0;
-				}
-			}
-
 			break;
 
 		case INSN_STAC:
diff --git a/tools/objtool/check.h b/tools/objtool/check.h
index f0ce8ff..2c55f75 100644
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -42,7 +42,7 @@ struct instruction {
 	struct rela *jump_table;
 	struct list_head alts;
 	struct symbol *func;
-	struct stack_op stack_op;
+	struct list_head stack_ops;
 	struct insn_state state;
 	struct orc_entry orc;
 };

Re: [tip: objtool/core] objtool: Support multiple stack_op per instruction

[Peter Zijlstra]

On Thu, Apr 23, 2020 at 07:49:42AM -0000, tip-bot2 for Julien Thierry wrote:

> diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
> index 7ce8650..199b408 100644
> --- a/tools/objtool/arch/x86/decode.c
> +++ b/tools/objtool/arch/x86/decode.c
> @@ -80,13 +80,15 @@ unsigned long arch_jump_destination(struct instruction *insn)
>  int arch_decode_instruction(struct elf *elf, struct section *sec,
>  			    unsigned long offset, unsigned int maxlen,
>  			    unsigned int *len, enum insn_type *type,
> -			    unsigned long *immediate, struct stack_op *op)
> +			    unsigned long *immediate,
> +			    struct list_head *ops_list)
>  {
>  	struct insn insn;
>  	int x86_64, sign;
>  	unsigned char op1, op2, rex = 0, rex_b = 0, rex_r = 0, rex_w = 0,
>  		      rex_x = 0, modrm = 0, modrm_mod = 0, modrm_rm = 0,
>  		      modrm_reg = 0, sib = 0;
> +	struct stack_op *op;
>  
>  	x86_64 = is_x86_64(elf);
>  	if (x86_64 == -1)
> @@ -127,6 +129,10 @@ int arch_decode_instruction(struct elf *elf, struct section *sec,
>  	if (insn.sib.nbytes)
>  		sib = insn.sib.bytes[0];
>  
> +	op = calloc(1, sizeof(*op));
> +	if (!op)
> +		return -1;
> +
>  	switch (op1) {
>  
>  	case 0x1:
> @@ -488,6 +494,11 @@ int arch_decode_instruction(struct elf *elf, struct section *sec,
>  
>  	*immediate = insn.immediate.nbytes ? insn.immediate.value : 0;
>  
> +	if (*type == INSN_STACK)
> +		list_add_tail(&op->list, ops_list);
> +	else
> +		free(op);
> +
>  	return 0;
>  }

So I was playing around with the intra-function thing, trying to address
Josh's comments from last time, but that also got me staring at this
again because it adds yet another type with a stack-op.

How do people feel about something like so?

---
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -77,6 +77,17 @@ unsigned long arch_jump_destination(stru
 	return insn->offset + insn->len + insn->immediate;
 }
 
+#define PUSH_OP(op) \
+({ \
+	list_add_tail(&op->list, ops_list); \
+	NULL; \
+})
+
+#define ADD_OP(op) \
+	if (!(op = calloc(1, sizeof(*op)))) \
+		return -1; \
+	else for (; op; op = PUSH_OP(op))
+
 int arch_decode_instruction(struct elf *elf, struct section *sec,
 			    unsigned long offset, unsigned int maxlen,
 			    unsigned int *len, enum insn_type *type,
@@ -88,7 +99,7 @@ int arch_decode_instruction(struct elf *
 	unsigned char op1, op2, rex = 0, rex_b = 0, rex_r = 0, rex_w = 0,
 		      rex_x = 0, modrm = 0, modrm_mod = 0, modrm_rm = 0,
 		      modrm_reg = 0, sib = 0;
-	struct stack_op *op;
+	struct stack_op *op = NULL;
 
 	x86_64 = is_x86_64(elf);
 	if (x86_64 == -1)
@@ -129,10 +140,6 @@ int arch_decode_instruction(struct elf *
 	if (insn.sib.nbytes)
 		sib = insn.sib.bytes[0];
 
-	op = calloc(1, sizeof(*op));
-	if (!op)
-		return -1;
-
 	switch (op1) {
 
 	case 0x1:
@@ -141,10 +148,12 @@ int arch_decode_instruction(struct elf *
 
 			/* add/sub reg, %rsp */
 			*type = INSN_STACK;
-			op->src.type = OP_SRC_ADD;
-			op->src.reg = op_to_cfi_reg[modrm_reg][rex_r];
-			op->dest.type = OP_DEST_REG;
-			op->dest.reg = CFI_SP;
+			ADD_OP(op) {
+				op->src.type = OP_SRC_ADD;
+				op->src.reg = op_to_cfi_reg[modrm_reg][rex_r];
+				op->dest.type = OP_DEST_REG;
+				op->dest.reg = CFI_SP;
+			}
 		}
 		break;
 
@@ -152,9 +161,11 @@ int arch_decode_instruction(struct elf *
 
 		/* push reg */
 		*type = INSN_STACK;
-		op->src.type = OP_SRC_REG;
-		op->src.reg = op_to_cfi_reg[op1 & 0x7][rex_b];
-		op->dest.type = OP_DEST_PUSH;
+		ADD_OP(op) {
+			op->src.type = OP_SRC_REG;
+			op->src.reg = op_to_cfi_reg[op1 & 0x7][rex_b];
+			op->dest.type = OP_DEST_PUSH;
+		}
 
 		break;
 
@@ -162,9 +173,11 @@ int arch_decode_instruction(struct elf *
 
 		/* pop reg */
 		*type = INSN_STACK;
-		op->src.type = OP_SRC_POP;
-		op->dest.type = OP_DEST_REG;
-		op->dest.reg = op_to_cfi_reg[op1 & 0x7][rex_b];
+		ADD_OP(op) {
+			op->src.type = OP_SRC_POP;
+			op->dest.type = OP_DEST_REG;
+			op->dest.reg = op_to_cfi_reg[op1 & 0x7][rex_b];
+		}
 
 		break;
 
@@ -172,8 +185,10 @@ int arch_decode_instruction(struct elf *
 	case 0x6a:
 		/* push immediate */
 		*type = INSN_STACK;
-		op->src.type = OP_SRC_CONST;
-		op->dest.type = OP_DEST_PUSH;
+		ADD_OP(op) {
+			op->src.type = OP_SRC_CONST;
+			op->dest.type = OP_DEST_PUSH;
+		}
 		break;
 
 	case 0x70 ... 0x7f:
@@ -188,11 +203,13 @@ int arch_decode_instruction(struct elf *
 		if (modrm == 0xe4) {
 			/* and imm, %rsp */
 			*type = INSN_STACK;
-			op->src.type = OP_SRC_AND;
-			op->src.reg = CFI_SP;
-			op->src.offset = insn.immediate.value;
-			op->dest.type = OP_DEST_REG;
-			op->dest.reg = CFI_SP;
+			ADD_OP(op) {
+				op->src.type = OP_SRC_AND;
+				op->src.reg = CFI_SP;
+				op->src.offset = insn.immediate.value;
+				op->dest.type = OP_DEST_REG;
+				op->dest.reg = CFI_SP;
+			}
 			break;
 		}
 
@@ -205,11 +222,13 @@ int arch_decode_instruction(struct elf *
 
 		/* add/sub imm, %rsp */
 		*type = INSN_STACK;
-		op->src.type = OP_SRC_ADD;
-		op->src.reg = CFI_SP;
-		op->src.offset = insn.immediate.value * sign;
-		op->dest.type = OP_DEST_REG;
-		op->dest.reg = CFI_SP;
+		ADD_OP(op) {
+			op->src.type = OP_SRC_ADD;
+			op->src.reg = CFI_SP;
+			op->src.offset = insn.immediate.value * sign;
+			op->dest.type = OP_DEST_REG;
+			op->dest.reg = CFI_SP;
+		}
 		break;
 
 	case 0x89:
@@ -217,10 +236,12 @@ int arch_decode_instruction(struct elf *
 
 			/* mov %rsp, reg */
 			*type = INSN_STACK;
-			op->src.type = OP_SRC_REG;
-			op->src.reg = CFI_SP;
-			op->dest.type = OP_DEST_REG;
-			op->dest.reg = op_to_cfi_reg[modrm_rm][rex_b];
+			ADD_OP(op) {
+				op->src.type = OP_SRC_REG;
+				op->src.reg = CFI_SP;
+				op->dest.type = OP_DEST_REG;
+				op->dest.reg = op_to_cfi_reg[modrm_rm][rex_b];
+			}
 			break;
 		}
 
@@ -228,10 +249,12 @@ int arch_decode_instruction(struct elf *
 
 			/* mov reg, %rsp */
 			*type = INSN_STACK;
-			op->src.type = OP_SRC_REG;
-			op->src.reg = op_to_cfi_reg[modrm_reg][rex_r];
-			op->dest.type = OP_DEST_REG;
-			op->dest.reg = CFI_SP;
+			ADD_OP(op) {
+				op->src.type = OP_SRC_REG;
+				op->src.reg = op_to_cfi_reg[modrm_reg][rex_r];
+				op->dest.type = OP_DEST_REG;
+				op->dest.reg = CFI_SP;
+			}
 			break;
 		}
 
@@ -242,21 +265,25 @@ int arch_decode_instruction(struct elf *
 
 			/* mov reg, disp(%rbp) */
 			*type = INSN_STACK;
-			op->src.type = OP_SRC_REG;
-			op->src.reg = op_to_cfi_reg[modrm_reg][rex_r];
-			op->dest.type = OP_DEST_REG_INDIRECT;
-			op->dest.reg = CFI_BP;
-			op->dest.offset = insn.displacement.value;
+			ADD_OP(op) {
+				op->src.type = OP_SRC_REG;
+				op->src.reg = op_to_cfi_reg[modrm_reg][rex_r];
+				op->dest.type = OP_DEST_REG_INDIRECT;
+				op->dest.reg = CFI_BP;
+				op->dest.offset = insn.displacement.value;
+			}
 
 		} else if (rex_w && !rex_b && modrm_rm == 4 && sib == 0x24) {
 
 			/* mov reg, disp(%rsp) */
 			*type = INSN_STACK;
-			op->src.type = OP_SRC_REG;
-			op->src.reg = op_to_cfi_reg[modrm_reg][rex_r];
-			op->dest.type = OP_DEST_REG_INDIRECT;
-			op->dest.reg = CFI_SP;
-			op->dest.offset = insn.displacement.value;
+			ADD_OP(op) {
+				op->src.type = OP_SRC_REG;
+				op->src.reg = op_to_cfi_reg[modrm_reg][rex_r];
+				op->dest.type = OP_DEST_REG_INDIRECT;
+				op->dest.reg = CFI_SP;
+				op->dest.offset = insn.displacement.value;
+			}
 		}
 
 		break;
@@ -266,22 +293,26 @@ int arch_decode_instruction(struct elf *
 
 			/* mov disp(%rbp), reg */
 			*type = INSN_STACK;
-			op->src.type = OP_SRC_REG_INDIRECT;
-			op->src.reg = CFI_BP;
-			op->src.offset = insn.displacement.value;
-			op->dest.type = OP_DEST_REG;
-			op->dest.reg = op_to_cfi_reg[modrm_reg][rex_r];
+			ADD_OP(op) {
+				op->src.type = OP_SRC_REG_INDIRECT;
+				op->src.reg = CFI_BP;
+				op->src.offset = insn.displacement.value;
+				op->dest.type = OP_DEST_REG;
+				op->dest.reg = op_to_cfi_reg[modrm_reg][rex_r];
+			}
 
 		} else if (rex_w && !rex_b && sib == 0x24 &&
 			   modrm_mod != 3 && modrm_rm == 4) {
 
 			/* mov disp(%rsp), reg */
 			*type = INSN_STACK;
-			op->src.type = OP_SRC_REG_INDIRECT;
-			op->src.reg = CFI_SP;
-			op->src.offset = insn.displacement.value;
-			op->dest.type = OP_DEST_REG;
-			op->dest.reg = op_to_cfi_reg[modrm_reg][rex_r];
+			ADD_OP(op) {
+				op->src.type = OP_SRC_REG_INDIRECT;
+				op->src.reg = CFI_SP;
+				op->src.offset = insn.displacement.value;
+				op->dest.type = OP_DEST_REG;
+				op->dest.reg = op_to_cfi_reg[modrm_reg][rex_r];
+			}
 		}
 
 		break;
@@ -290,27 +321,31 @@ int arch_decode_instruction(struct elf *
 		if (sib == 0x24 && rex_w && !rex_b && !rex_x) {
 
 			*type = INSN_STACK;
-			if (!insn.displacement.value) {
-				/* lea (%rsp), reg */
-				op->src.type = OP_SRC_REG;
-			} else {
-				/* lea disp(%rsp), reg */
-				op->src.type = OP_SRC_ADD;
-				op->src.offset = insn.displacement.value;
+			ADD_OP(op) {
+				if (!insn.displacement.value) {
+					/* lea (%rsp), reg */
+					op->src.type = OP_SRC_REG;
+				} else {
+					/* lea disp(%rsp), reg */
+					op->src.type = OP_SRC_ADD;
+					op->src.offset = insn.displacement.value;
+				}
+				op->src.reg = CFI_SP;
+				op->dest.type = OP_DEST_REG;
+				op->dest.reg = op_to_cfi_reg[modrm_reg][rex_r];
 			}
-			op->src.reg = CFI_SP;
-			op->dest.type = OP_DEST_REG;
-			op->dest.reg = op_to_cfi_reg[modrm_reg][rex_r];
 
 		} else if (rex == 0x48 && modrm == 0x65) {
 
 			/* lea disp(%rbp), %rsp */
 			*type = INSN_STACK;
-			op->src.type = OP_SRC_ADD;
-			op->src.reg = CFI_BP;
-			op->src.offset = insn.displacement.value;
-			op->dest.type = OP_DEST_REG;
-			op->dest.reg = CFI_SP;
+			ADD_OP(op) {
+				op->src.type = OP_SRC_ADD;
+				op->src.reg = CFI_BP;
+				op->src.offset = insn.displacement.value;
+				op->dest.type = OP_DEST_REG;
+				op->dest.reg = CFI_SP;
+			}
 
 		} else if (rex == 0x49 && modrm == 0x62 &&
 			   insn.displacement.value == -8) {
@@ -322,11 +357,13 @@ int arch_decode_instruction(struct elf *
 			 * stack realignment.
 			 */
 			*type = INSN_STACK;
-			op->src.type = OP_SRC_ADD;
-			op->src.reg = CFI_R10;
-			op->src.offset = -8;
-			op->dest.type = OP_DEST_REG;
-			op->dest.reg = CFI_SP;
+			ADD_OP(op) {
+				op->src.type = OP_SRC_ADD;
+				op->src.reg = CFI_R10;
+				op->src.offset = -8;
+				op->dest.type = OP_DEST_REG;
+				op->dest.reg = CFI_SP;
+			}
 
 		} else if (rex == 0x49 && modrm == 0x65 &&
 			   insn.displacement.value == -16) {
@@ -338,11 +375,13 @@ int arch_decode_instruction(struct elf *
 			 * stack realignment.
 			 */
 			*type = INSN_STACK;
-			op->src.type = OP_SRC_ADD;
-			op->src.reg = CFI_R13;
-			op->src.offset = -16;
-			op->dest.type = OP_DEST_REG;
-			op->dest.reg = CFI_SP;
+			ADD_OP(op) {
+				op->src.type = OP_SRC_ADD;
+				op->src.reg = CFI_R13;
+				op->src.offset = -16;
+				op->dest.type = OP_DEST_REG;
+				op->dest.reg = CFI_SP;
+			}
 		}
 
 		break;
@@ -350,8 +389,10 @@ int arch_decode_instruction(struct elf *
 	case 0x8f:
 		/* pop to mem */
 		*type = INSN_STACK;
-		op->src.type = OP_SRC_POP;
-		op->dest.type = OP_DEST_MEM;
+		ADD_OP(op) {
+			op->src.type = OP_SRC_POP;
+			op->dest.type = OP_DEST_MEM;
+		}
 		break;
 
 	case 0x90:
@@ -361,15 +402,19 @@ int arch_decode_instruction(struct elf *
 	case 0x9c:
 		/* pushf */
 		*type = INSN_STACK;
-		op->src.type = OP_SRC_CONST;
-		op->dest.type = OP_DEST_PUSHF;
+		ADD_OP(op) {
+			op->src.type = OP_SRC_CONST;
+			op->dest.type = OP_DEST_PUSHF;
+		}
 		break;
 
 	case 0x9d:
 		/* popf */
 		*type = INSN_STACK;
-		op->src.type = OP_SRC_POPF;
-		op->dest.type = OP_DEST_MEM;
+		ADD_OP(op) {
+			op->src.type = OP_SRC_POPF;
+			op->dest.type = OP_DEST_MEM;
+		}
 		break;
 
 	case 0x0f:
@@ -405,15 +450,19 @@ int arch_decode_instruction(struct elf *
 
 			/* push fs/gs */
 			*type = INSN_STACK;
-			op->src.type = OP_SRC_CONST;
-			op->dest.type = OP_DEST_PUSH;
+			ADD_OP(op) {
+				op->src.type = OP_SRC_CONST;
+				op->dest.type = OP_DEST_PUSH;
+			}
 
 		} else if (op2 == 0xa1 || op2 == 0xa9) {
 
 			/* pop fs/gs */
 			*type = INSN_STACK;
-			op->src.type = OP_SRC_POP;
-			op->dest.type = OP_DEST_MEM;
+			ADD_OP(op) {
+				op->src.type = OP_SRC_POP;
+				op->dest.type = OP_DEST_MEM;
+			}
 		}
 
 		break;
@@ -427,7 +476,8 @@ int arch_decode_instruction(struct elf *
 		 * pop bp
 		 */
 		*type = INSN_STACK;
-		op->dest.type = OP_DEST_LEAVE;
+		ADD_OP(op)
+			op->dest.type = OP_DEST_LEAVE;
 
 		break;
 
@@ -449,12 +499,14 @@ int arch_decode_instruction(struct elf *
 	case 0xcf: /* iret */
 		*type = INSN_EXCEPTION_RETURN;
 
-		/* add $40, %rsp */
-		op->src.type = OP_SRC_ADD;
-		op->src.reg = CFI_SP;
-		op->src.offset = 5*8;
-		op->dest.type = OP_DEST_REG;
-		op->dest.reg = CFI_SP;
+		ADD_OP(op) {
+			/* add $40, %rsp */
+			op->src.type = OP_SRC_ADD;
+			op->src.reg = CFI_SP;
+			op->src.offset = 5*8;
+			op->dest.type = OP_DEST_REG;
+			op->dest.reg = CFI_SP;
+		}
 		break;
 
 	case 0xca: /* retf */
@@ -504,11 +556,6 @@ int arch_decode_instruction(struct elf *
 
 	*immediate = insn.immediate.nbytes ? insn.immediate.value : 0;
 
-	if (*type == INSN_STACK || *type == INSN_EXCEPTION_RETURN)
-		list_add_tail(&op->list, ops_list);
-	else
-		free(op);
-
 	return 0;
 }
 

[tip: objtool/core] objtool: Remove check preventing branches within alternative

[tip-bot2 for Julien Thierry]

The following commit has been merged into the objtool/core branch of tip:

Commit-ID:     9e98d62aa7ea1375052895650f3e6d362336c5c9
Gitweb:        https://git.kernel.org/tip/9e98d62aa7ea1375052895650f3e6d362336c5c9
Author:        Julien Thierry <jthierry@redhat.com>
AuthorDate:    Fri, 27 Mar 2020 15:28:42 
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Thu, 30 Apr 2020 20:14:31 +02:00

objtool: Remove check preventing branches within alternative

While jumping from outside an alternative region to the middle of an
alternative region is very likely wrong, jumping from an alternative
region into the same region is valid. It is a common pattern on arm64.

The first pattern is unlikely to happen in practice and checking only
for this adds a lot of complexity.

Just remove the current check.

Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Julien Thierry <jthierry@redhat.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Miroslav Benes <mbenes@suse.cz>
Link: https://lkml.kernel.org/r/20200327152847.15294-6-jthierry@redhat.com
---
 tools/objtool/check.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 12e2aea..cc52da6 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2162,12 +2162,6 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
 
 	sec = insn->sec;
 
-	if (insn->alt_group && list_empty(&insn->alts)) {
-		WARN_FUNC("don't know how to handle branch to middle of alternative instruction group",
-			  sec, insn->offset);
-		return 1;
-	}
-
 	while (1) {
 		next_insn = next_insn_same_sec(file, insn);