linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 00/10] Objtool updates for easier portability
@ 2020-03-27 15:28 Julien Thierry
  2020-03-27 15:28 ` [PATCH v2 01/10] objtool: Move header sync-check ealier in build Julien Thierry
                   ` (10 more replies)
  0 siblings, 11 replies; 32+ messages in thread
From: Julien Thierry @ 2020-03-27 15:28 UTC (permalink / raw)
  To: linux-kernel; +Cc: jpoimboe, peterz, raphael.gault, 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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v2 01/10] objtool: Move header sync-check ealier in build
  2020-03-27 15:28 [PATCH v2 00/10] Objtool updates for easier portability Julien Thierry
@ 2020-03-27 15:28 ` Julien Thierry
  2020-04-01 12:32   ` Miroslav Benes
  2020-04-02 17:12   ` Josh Poimboeuf
  2020-03-27 15:28 ` [PATCH v2 02/10] objtool: check: Remove redundant checks on operand type Julien Thierry
                   ` (9 subsequent siblings)
  10 siblings, 2 replies; 32+ messages in thread
From: Julien Thierry @ 2020-03-27 15:28 UTC (permalink / raw)
  To: linux-kernel; +Cc: jpoimboe, peterz, raphael.gault, 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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v2 02/10] objtool: check: Remove redundant checks on operand type
  2020-03-27 15:28 [PATCH v2 00/10] Objtool updates for easier portability Julien Thierry
  2020-03-27 15:28 ` [PATCH v2 01/10] objtool: Move header sync-check ealier in build Julien Thierry
@ 2020-03-27 15:28 ` Julien Thierry
  2020-03-27 15:28 ` [PATCH v2 03/10] objtool: check: Clean instruction state before each function validation Julien Thierry
                   ` (8 subsequent siblings)
  10 siblings, 0 replies; 32+ messages in thread
From: Julien Thierry @ 2020-03-27 15:28 UTC (permalink / raw)
  To: linux-kernel; +Cc: jpoimboe, peterz, raphael.gault, 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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v2 03/10] objtool: check: Clean instruction state before each function validation
  2020-03-27 15:28 [PATCH v2 00/10] Objtool updates for easier portability Julien Thierry
  2020-03-27 15:28 ` [PATCH v2 01/10] objtool: Move header sync-check ealier in build Julien Thierry
  2020-03-27 15:28 ` [PATCH v2 02/10] objtool: check: Remove redundant checks on operand type Julien Thierry
@ 2020-03-27 15:28 ` Julien Thierry
  2020-03-27 15:28 ` [PATCH v2 04/10] objtool: check: Ignore empty alternative groups Julien Thierry
                   ` (7 subsequent siblings)
  10 siblings, 0 replies; 32+ messages in thread
From: Julien Thierry @ 2020-03-27 15:28 UTC (permalink / raw)
  To: linux-kernel; +Cc: jpoimboe, peterz, raphael.gault, 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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v2 04/10] objtool: check: Ignore empty alternative groups
  2020-03-27 15:28 [PATCH v2 00/10] Objtool updates for easier portability Julien Thierry
                   ` (2 preceding siblings ...)
  2020-03-27 15:28 ` [PATCH v2 03/10] objtool: check: Clean instruction state before each function validation Julien Thierry
@ 2020-03-27 15:28 ` Julien Thierry
  2020-04-01 12:53   ` Miroslav Benes
  2020-03-27 15:28 ` [PATCH v2 05/10] objtool: check: Remove check preventing branches within alternative Julien Thierry
                   ` (6 subsequent siblings)
  10 siblings, 1 reply; 32+ messages in thread
From: Julien Thierry @ 2020-03-27 15:28 UTC (permalink / raw)
  To: linux-kernel; +Cc: jpoimboe, peterz, raphael.gault, 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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v2 05/10] objtool: check: Remove check preventing branches within alternative
  2020-03-27 15:28 [PATCH v2 00/10] Objtool updates for easier portability Julien Thierry
                   ` (3 preceding siblings ...)
  2020-03-27 15:28 ` [PATCH v2 04/10] objtool: check: Ignore empty alternative groups Julien Thierry
@ 2020-03-27 15:28 ` Julien Thierry
  2020-05-01 18:22   ` [tip: objtool/core] objtool: " tip-bot2 for Julien Thierry
  2020-03-27 15:28 ` [PATCH v2 06/10] objtool: check: Use arch specific values in restore_reg() Julien Thierry
                   ` (5 subsequent siblings)
  10 siblings, 1 reply; 32+ messages in thread
From: Julien Thierry @ 2020-03-27 15:28 UTC (permalink / raw)
  To: linux-kernel; +Cc: jpoimboe, peterz, raphael.gault, 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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v2 06/10] objtool: check: Use arch specific values in restore_reg()
  2020-03-27 15:28 [PATCH v2 00/10] Objtool updates for easier portability Julien Thierry
                   ` (4 preceding siblings ...)
  2020-03-27 15:28 ` [PATCH v2 05/10] objtool: check: Remove check preventing branches within alternative Julien Thierry
@ 2020-03-27 15:28 ` Julien Thierry
  2020-03-27 15:28 ` [PATCH v2 07/10] objtool: check: Allow save/restore hint in non standard function symbols Julien Thierry
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 32+ messages in thread
From: Julien Thierry @ 2020-03-27 15:28 UTC (permalink / raw)
  To: linux-kernel; +Cc: jpoimboe, peterz, raphael.gault, 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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v2 07/10] objtool: check: Allow save/restore hint in non standard function symbols
  2020-03-27 15:28 [PATCH v2 00/10] Objtool updates for easier portability Julien Thierry
                   ` (5 preceding siblings ...)
  2020-03-27 15:28 ` [PATCH v2 06/10] objtool: check: Use arch specific values in restore_reg() Julien Thierry
@ 2020-03-27 15:28 ` Julien Thierry
  2020-04-01 13:54   ` Miroslav Benes
  2020-03-27 15:28 ` [PATCH v2 08/10] objtool: Add abstraction for computation of symbols offsets Julien Thierry
                   ` (3 subsequent siblings)
  10 siblings, 1 reply; 32+ messages in thread
From: Julien Thierry @ 2020-03-27 15:28 UTC (permalink / raw)
  To: linux-kernel; +Cc: jpoimboe, peterz, raphael.gault, 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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v2 08/10] objtool: Add abstraction for computation of symbols offsets
  2020-03-27 15:28 [PATCH v2 00/10] Objtool updates for easier portability Julien Thierry
                   ` (6 preceding siblings ...)
  2020-03-27 15:28 ` [PATCH v2 07/10] objtool: check: Allow save/restore hint in non standard function symbols Julien Thierry
@ 2020-03-27 15:28 ` Julien Thierry
  2020-03-27 15:28 ` [PATCH v2 09/10] objtool: Split generic and arch specific CFI definitions Julien Thierry
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 32+ messages in thread
From: Julien Thierry @ 2020-03-27 15:28 UTC (permalink / raw)
  To: linux-kernel; +Cc: jpoimboe, peterz, raphael.gault, 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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v2 09/10] objtool: Split generic and arch specific CFI definitions
  2020-03-27 15:28 [PATCH v2 00/10] Objtool updates for easier portability Julien Thierry
                   ` (7 preceding siblings ...)
  2020-03-27 15:28 ` [PATCH v2 08/10] objtool: Add abstraction for computation of symbols offsets Julien Thierry
@ 2020-03-27 15:28 ` Julien Thierry
  2020-03-27 15:28 ` [PATCH v2 10/10] objtool: Support multiple stack_op per instruction Julien Thierry
  2020-04-02 17:58 ` [PATCH v2 00/10] Objtool updates for easier portability Josh Poimboeuf
  10 siblings, 0 replies; 32+ messages in thread
From: Julien Thierry @ 2020-03-27 15:28 UTC (permalink / raw)
  To: linux-kernel; +Cc: jpoimboe, peterz, raphael.gault, 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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v2 10/10] objtool: Support multiple stack_op per instruction
  2020-03-27 15:28 [PATCH v2 00/10] Objtool updates for easier portability Julien Thierry
                   ` (8 preceding siblings ...)
  2020-03-27 15:28 ` [PATCH v2 09/10] objtool: Split generic and arch specific CFI definitions Julien Thierry
@ 2020-03-27 15:28 ` Julien Thierry
  2020-04-02 17:54   ` Josh Poimboeuf
                     ` (2 more replies)
  2020-04-02 17:58 ` [PATCH v2 00/10] Objtool updates for easier portability Josh Poimboeuf
  10 siblings, 3 replies; 32+ messages in thread
From: Julien Thierry @ 2020-03-27 15:28 UTC (permalink / raw)
  To: linux-kernel; +Cc: jpoimboe, peterz, raphael.gault, 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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 01/10] objtool: Move header sync-check ealier in build
  2020-03-27 15:28 ` [PATCH v2 01/10] objtool: Move header sync-check ealier in build Julien Thierry
@ 2020-04-01 12:32   ` Miroslav Benes
  2020-04-01 12:44     ` Julien Thierry
  2020-04-02 17:12   ` Josh Poimboeuf
  1 sibling, 1 reply; 32+ messages in thread
From: Miroslav Benes @ 2020-04-01 12:32 UTC (permalink / raw)
  To: Julien Thierry; +Cc: linux-kernel, jpoimboe, peterz, raphael.gault

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
> 


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 01/10] objtool: Move header sync-check ealier in build
  2020-04-01 12:32   ` Miroslav Benes
@ 2020-04-01 12:44     ` Julien Thierry
  2020-04-01 12:55       ` Miroslav Benes
  0 siblings, 1 reply; 32+ messages in thread
From: Julien Thierry @ 2020-04-01 12:44 UTC (permalink / raw)
  To: Miroslav Benes; +Cc: linux-kernel, jpoimboe, peterz, raphael.gault



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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 04/10] objtool: check: Ignore empty alternative groups
  2020-03-27 15:28 ` [PATCH v2 04/10] objtool: check: Ignore empty alternative groups Julien Thierry
@ 2020-04-01 12:53   ` Miroslav Benes
  2020-04-01 13:43     ` Julien Thierry
  0 siblings, 1 reply; 32+ messages in thread
From: Miroslav Benes @ 2020-04-01 12:53 UTC (permalink / raw)
  To: Julien Thierry; +Cc: linux-kernel, jpoimboe, peterz, raphael.gault

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 01/10] objtool: Move header sync-check ealier in build
  2020-04-01 12:44     ` Julien Thierry
@ 2020-04-01 12:55       ` Miroslav Benes
  0 siblings, 0 replies; 32+ messages in thread
From: Miroslav Benes @ 2020-04-01 12:55 UTC (permalink / raw)
  To: Julien Thierry; +Cc: linux-kernel, jpoimboe, peterz, raphael.gault

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 04/10] objtool: check: Ignore empty alternative groups
  2020-04-01 12:53   ` Miroslav Benes
@ 2020-04-01 13:43     ` Julien Thierry
  2020-04-01 13:48       ` Miroslav Benes
  0 siblings, 1 reply; 32+ messages in thread
From: Julien Thierry @ 2020-04-01 13:43 UTC (permalink / raw)
  To: Miroslav Benes; +Cc: linux-kernel, jpoimboe, peterz, raphael.gault



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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 04/10] objtool: check: Ignore empty alternative groups
  2020-04-01 13:43     ` Julien Thierry
@ 2020-04-01 13:48       ` Miroslav Benes
  0 siblings, 0 replies; 32+ messages in thread
From: Miroslav Benes @ 2020-04-01 13:48 UTC (permalink / raw)
  To: Julien Thierry; +Cc: linux-kernel, jpoimboe, peterz, raphael.gault

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 07/10] objtool: check: Allow save/restore hint in non standard function symbols
  2020-03-27 15:28 ` [PATCH v2 07/10] objtool: check: Allow save/restore hint in non standard function symbols Julien Thierry
@ 2020-04-01 13:54   ` Miroslav Benes
  2020-04-01 14:06     ` Julien Thierry
  0 siblings, 1 reply; 32+ messages in thread
From: Miroslav Benes @ 2020-04-01 13:54 UTC (permalink / raw)
  To: Julien Thierry; +Cc: linux-kernel, jpoimboe, peterz, raphael.gault

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 07/10] objtool: check: Allow save/restore hint in non standard function symbols
  2020-04-01 13:54   ` Miroslav Benes
@ 2020-04-01 14:06     ` Julien Thierry
  0 siblings, 0 replies; 32+ messages in thread
From: Julien Thierry @ 2020-04-01 14:06 UTC (permalink / raw)
  To: Miroslav Benes; +Cc: linux-kernel, jpoimboe, peterz, raphael.gault



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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 01/10] objtool: Move header sync-check ealier in build
  2020-03-27 15:28 ` [PATCH v2 01/10] objtool: Move header sync-check ealier in build Julien Thierry
  2020-04-01 12:32   ` Miroslav Benes
@ 2020-04-02 17:12   ` Josh Poimboeuf
  2020-04-02 17:17     ` Josh Poimboeuf
  1 sibling, 1 reply; 32+ messages in thread
From: Josh Poimboeuf @ 2020-04-02 17:12 UTC (permalink / raw)
  To: Julien Thierry; +Cc: linux-kernel, peterz, raphael.gault

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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 01/10] objtool: Move header sync-check ealier in build
  2020-04-02 17:12   ` Josh Poimboeuf
@ 2020-04-02 17:17     ` Josh Poimboeuf
  0 siblings, 0 replies; 32+ messages in thread
From: Josh Poimboeuf @ 2020-04-02 17:17 UTC (permalink / raw)
  To: Julien Thierry; +Cc: linux-kernel, peterz, raphael.gault

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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 10/10] objtool: Support multiple stack_op per instruction
  2020-03-27 15:28 ` [PATCH v2 10/10] objtool: Support multiple stack_op per instruction Julien Thierry
@ 2020-04-02 17:54   ` Josh Poimboeuf
  2020-04-02 18:25     ` Peter Zijlstra
  2020-04-03  8:01     ` Julien Thierry
  2020-04-22 22:24   ` [tip: objtool/core] " tip-bot2 for Julien Thierry
  2020-04-23  7:49   ` tip-bot2 for Julien Thierry
  2 siblings, 2 replies; 32+ messages in thread
From: Josh Poimboeuf @ 2020-04-02 17:54 UTC (permalink / raw)
  To: Julien Thierry; +Cc: linux-kernel, peterz, raphael.gault

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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 00/10] Objtool updates for easier portability
  2020-03-27 15:28 [PATCH v2 00/10] Objtool updates for easier portability Julien Thierry
                   ` (9 preceding siblings ...)
  2020-03-27 15:28 ` [PATCH v2 10/10] objtool: Support multiple stack_op per instruction Julien Thierry
@ 2020-04-02 17:58 ` Josh Poimboeuf
  2020-04-02 18:30   ` Peter Zijlstra
  10 siblings, 1 reply; 32+ messages in thread
From: Josh Poimboeuf @ 2020-04-02 17:58 UTC (permalink / raw)
  To: Julien Thierry; +Cc: linux-kernel, peterz, raphael.gault

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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 10/10] objtool: Support multiple stack_op per instruction
  2020-04-02 17:54   ` Josh Poimboeuf
@ 2020-04-02 18:25     ` Peter Zijlstra
  2020-04-03  8:01     ` Julien Thierry
  1 sibling, 0 replies; 32+ messages in thread
From: Peter Zijlstra @ 2020-04-02 18:25 UTC (permalink / raw)
  To: Josh Poimboeuf; +Cc: Julien Thierry, linux-kernel, raphael.gault

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 ;-)


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 00/10] Objtool updates for easier portability
  2020-04-02 17:58 ` [PATCH v2 00/10] Objtool updates for easier portability Josh Poimboeuf
@ 2020-04-02 18:30   ` Peter Zijlstra
  2020-04-03  7:17     ` Miroslav Benes
  0 siblings, 1 reply; 32+ messages in thread
From: Peter Zijlstra @ 2020-04-02 18:30 UTC (permalink / raw)
  To: Josh Poimboeuf; +Cc: Julien Thierry, linux-kernel, raphael.gault

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>


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 00/10] Objtool updates for easier portability
  2020-04-02 18:30   ` Peter Zijlstra
@ 2020-04-03  7:17     ` Miroslav Benes
  0 siblings, 0 replies; 32+ messages in thread
From: Miroslav Benes @ 2020-04-03  7:17 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Josh Poimboeuf, Julien Thierry, linux-kernel, raphael.gault

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.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 10/10] objtool: Support multiple stack_op per instruction
  2020-04-02 17:54   ` Josh Poimboeuf
  2020-04-02 18:25     ` Peter Zijlstra
@ 2020-04-03  8:01     ` Julien Thierry
  2020-04-03 14:55       ` Josh Poimboeuf
  1 sibling, 1 reply; 32+ messages in thread
From: Julien Thierry @ 2020-04-03  8:01 UTC (permalink / raw)
  To: Josh Poimboeuf; +Cc: linux-kernel, peterz, raphael.gault



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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 10/10] objtool: Support multiple stack_op per instruction
  2020-04-03  8:01     ` Julien Thierry
@ 2020-04-03 14:55       ` Josh Poimboeuf
  0 siblings, 0 replies; 32+ messages in thread
From: Josh Poimboeuf @ 2020-04-03 14:55 UTC (permalink / raw)
  To: Julien Thierry; +Cc: linux-kernel, peterz, raphael.gault

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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [tip: objtool/core] objtool: Support multiple stack_op per instruction
  2020-03-27 15:28 ` [PATCH v2 10/10] objtool: Support multiple stack_op per instruction Julien Thierry
  2020-04-02 17:54   ` Josh Poimboeuf
@ 2020-04-22 22:24   ` tip-bot2 for Julien Thierry
  2020-04-23  7:49   ` tip-bot2 for Julien Thierry
  2 siblings, 0 replies; 32+ messages in thread
From: tip-bot2 for Julien Thierry @ 2020-04-22 22:24 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: Julien Thierry, Peter Zijlstra (Intel),
	Miroslav Benes, Alexandre Chartre, Josh Poimboeuf, x86, LKML

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;
 };

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [tip: objtool/core] objtool: Support multiple stack_op per instruction
  2020-03-27 15:28 ` [PATCH v2 10/10] objtool: Support multiple stack_op per instruction Julien Thierry
  2020-04-02 17:54   ` Josh Poimboeuf
  2020-04-22 22:24   ` [tip: objtool/core] " tip-bot2 for Julien Thierry
@ 2020-04-23  7:49   ` tip-bot2 for Julien Thierry
  2020-04-23 11:15     ` Peter Zijlstra
  2 siblings, 1 reply; 32+ messages in thread
From: tip-bot2 for Julien Thierry @ 2020-04-23  7:49 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: Julien Thierry, Peter Zijlstra (Intel),
	Miroslav Benes, Alexandre Chartre, Josh Poimboeuf, Ingo Molnar,
	x86, LKML

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;
 };

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [tip: objtool/core] objtool: Support multiple stack_op per instruction
  2020-04-23  7:49   ` tip-bot2 for Julien Thierry
@ 2020-04-23 11:15     ` Peter Zijlstra
  0 siblings, 0 replies; 32+ messages in thread
From: Peter Zijlstra @ 2020-04-23 11:15 UTC (permalink / raw)
  To: linux-kernel
  Cc: linux-tip-commits, Julien Thierry, Miroslav Benes,
	Alexandre Chartre, Josh Poimboeuf, Ingo Molnar, x86

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;
 }
 

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [tip: objtool/core] objtool: Remove check preventing branches within alternative
  2020-03-27 15:28 ` [PATCH v2 05/10] objtool: check: Remove check preventing branches within alternative Julien Thierry
@ 2020-05-01 18:22   ` tip-bot2 for Julien Thierry
  0 siblings, 0 replies; 32+ messages in thread
From: tip-bot2 for Julien Thierry @ 2020-05-01 18:22 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: Josh Poimboeuf, Julien Thierry, Peter Zijlstra (Intel),
	Miroslav Benes, x86, LKML

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);
 

^ permalink raw reply	[flat|nested] 32+ messages in thread

end of thread, other threads:[~2020-05-01 18:22 UTC | newest]

Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-27 15:28 [PATCH v2 00/10] Objtool updates for easier portability Julien Thierry
2020-03-27 15:28 ` [PATCH v2 01/10] objtool: Move header sync-check ealier in build Julien Thierry
2020-04-01 12:32   ` Miroslav Benes
2020-04-01 12:44     ` Julien Thierry
2020-04-01 12:55       ` Miroslav Benes
2020-04-02 17:12   ` Josh Poimboeuf
2020-04-02 17:17     ` Josh Poimboeuf
2020-03-27 15:28 ` [PATCH v2 02/10] objtool: check: Remove redundant checks on operand type Julien Thierry
2020-03-27 15:28 ` [PATCH v2 03/10] objtool: check: Clean instruction state before each function validation Julien Thierry
2020-03-27 15:28 ` [PATCH v2 04/10] objtool: check: Ignore empty alternative groups Julien Thierry
2020-04-01 12:53   ` Miroslav Benes
2020-04-01 13:43     ` Julien Thierry
2020-04-01 13:48       ` Miroslav Benes
2020-03-27 15:28 ` [PATCH v2 05/10] objtool: check: Remove check preventing branches within alternative Julien Thierry
2020-05-01 18:22   ` [tip: objtool/core] objtool: " tip-bot2 for Julien Thierry
2020-03-27 15:28 ` [PATCH v2 06/10] objtool: check: Use arch specific values in restore_reg() Julien Thierry
2020-03-27 15:28 ` [PATCH v2 07/10] objtool: check: Allow save/restore hint in non standard function symbols Julien Thierry
2020-04-01 13:54   ` Miroslav Benes
2020-04-01 14:06     ` Julien Thierry
2020-03-27 15:28 ` [PATCH v2 08/10] objtool: Add abstraction for computation of symbols offsets Julien Thierry
2020-03-27 15:28 ` [PATCH v2 09/10] objtool: Split generic and arch specific CFI definitions Julien Thierry
2020-03-27 15:28 ` [PATCH v2 10/10] objtool: Support multiple stack_op per instruction Julien Thierry
2020-04-02 17:54   ` Josh Poimboeuf
2020-04-02 18:25     ` Peter Zijlstra
2020-04-03  8:01     ` Julien Thierry
2020-04-03 14:55       ` Josh Poimboeuf
2020-04-22 22:24   ` [tip: objtool/core] " tip-bot2 for Julien Thierry
2020-04-23  7:49   ` tip-bot2 for Julien Thierry
2020-04-23 11:15     ` Peter Zijlstra
2020-04-02 17:58 ` [PATCH v2 00/10] Objtool updates for easier portability Josh Poimboeuf
2020-04-02 18:30   ` Peter Zijlstra
2020-04-03  7:17     ` Miroslav Benes

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).