* [PATCH 1/3] objtool: Refactor ORC section generation
2020-12-23 5:18 [PATCH 0/3] Alternatives vs ORC, a slightly easier way Josh Poimboeuf
@ 2020-12-23 5:18 ` Josh Poimboeuf
2020-12-23 5:18 ` [PATCH 2/3] objtool: Add 'alt_group' struct Josh Poimboeuf
2020-12-23 5:18 ` [PATCH 3/3] objtool: Support stack layout changes in alternatives Josh Poimboeuf
2 siblings, 0 replies; 11+ messages in thread
From: Josh Poimboeuf @ 2020-12-23 5:18 UTC (permalink / raw)
To: Juergen Gross; +Cc: xen-devel, linux-kernel, Peter Zijlstra, Miroslav Benes
Decouple ORC entries from instructions. This simplifies the
control/data flow, and is going to make it easier to support alternative
instructions which change the stack layout.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
tools/objtool/Makefile | 4 -
tools/objtool/arch.h | 4 -
tools/objtool/builtin-orc.c | 6 +-
tools/objtool/check.h | 3 -
tools/objtool/objtool.h | 3 +-
tools/objtool/orc_gen.c | 272 ++++++++++++++++++------------------
tools/objtool/weak.c | 7 +-
7 files changed, 140 insertions(+), 159 deletions(-)
diff --git a/tools/objtool/Makefile b/tools/objtool/Makefile
index 5cdb19036d7f..a43096f713c7 100644
--- a/tools/objtool/Makefile
+++ b/tools/objtool/Makefile
@@ -46,10 +46,6 @@ ifeq ($(SRCARCH),x86)
SUBCMD_ORC := y
endif
-ifeq ($(SUBCMD_ORC),y)
- CFLAGS += -DINSN_USE_ORC
-endif
-
export SUBCMD_CHECK SUBCMD_ORC
export srctree OUTPUT CFLAGS SRCARCH AWK
include $(srctree)/tools/build/Makefile.include
diff --git a/tools/objtool/arch.h b/tools/objtool/arch.h
index 4a84c3081b8e..5e3f3ea8bb89 100644
--- a/tools/objtool/arch.h
+++ b/tools/objtool/arch.h
@@ -11,10 +11,6 @@
#include "objtool.h"
#include "cfi.h"
-#ifdef INSN_USE_ORC
-#include <asm/orc_types.h>
-#endif
-
enum insn_type {
INSN_JUMP_CONDITIONAL,
INSN_JUMP_UNCONDITIONAL,
diff --git a/tools/objtool/builtin-orc.c b/tools/objtool/builtin-orc.c
index 7b31121fa60b..508bdf6ae8dc 100644
--- a/tools/objtool/builtin-orc.c
+++ b/tools/objtool/builtin-orc.c
@@ -51,11 +51,7 @@ int cmd_orc(int argc, const char **argv)
if (list_empty(&file->insn_list))
return 0;
- ret = create_orc(file);
- if (ret)
- return ret;
-
- ret = create_orc_sections(file);
+ ret = orc_create(file);
if (ret)
return ret;
diff --git a/tools/objtool/check.h b/tools/objtool/check.h
index 5ec00a4b891b..4c10916ff1cf 100644
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -43,9 +43,6 @@ struct instruction {
struct symbol *func;
struct list_head stack_ops;
struct cfi_state cfi;
-#ifdef INSN_USE_ORC
- struct orc_entry orc;
-#endif
};
static inline bool is_static_jump(struct instruction *insn)
diff --git a/tools/objtool/objtool.h b/tools/objtool/objtool.h
index 4125d4578b23..5e58d3537e2f 100644
--- a/tools/objtool/objtool.h
+++ b/tools/objtool/objtool.h
@@ -26,7 +26,6 @@ struct objtool_file *objtool_open_read(const char *_objname);
int check(struct objtool_file *file);
int orc_dump(const char *objname);
-int create_orc(struct objtool_file *file);
-int create_orc_sections(struct objtool_file *file);
+int orc_create(struct objtool_file *file);
#endif /* _OBJTOOL_H */
diff --git a/tools/objtool/orc_gen.c b/tools/objtool/orc_gen.c
index 235663b96adc..73efba2bfa72 100644
--- a/tools/objtool/orc_gen.c
+++ b/tools/objtool/orc_gen.c
@@ -12,89 +12,84 @@
#include "check.h"
#include "warn.h"
-int create_orc(struct objtool_file *file)
+static int init_orc_entry(struct orc_entry *orc, struct cfi_state *cfi)
{
- struct instruction *insn;
+ struct instruction *insn = container_of(cfi, struct instruction, cfi);
+ struct cfi_reg *bp = &cfi->regs[CFI_BP];
- for_each_insn(file, insn) {
- struct orc_entry *orc = &insn->orc;
- struct cfi_reg *cfa = &insn->cfi.cfa;
- struct cfi_reg *bp = &insn->cfi.regs[CFI_BP];
+ memset(orc, 0, sizeof(*orc));
- if (!insn->sec->text)
- continue;
-
- orc->end = insn->cfi.end;
+ orc->end = cfi->end;
- if (cfa->base == CFI_UNDEFINED) {
- orc->sp_reg = ORC_REG_UNDEFINED;
- continue;
- }
-
- switch (cfa->base) {
- case CFI_SP:
- orc->sp_reg = ORC_REG_SP;
- break;
- case CFI_SP_INDIRECT:
- orc->sp_reg = ORC_REG_SP_INDIRECT;
- break;
- case CFI_BP:
- orc->sp_reg = ORC_REG_BP;
- break;
- case CFI_BP_INDIRECT:
- orc->sp_reg = ORC_REG_BP_INDIRECT;
- break;
- case CFI_R10:
- orc->sp_reg = ORC_REG_R10;
- break;
- case CFI_R13:
- orc->sp_reg = ORC_REG_R13;
- break;
- case CFI_DI:
- orc->sp_reg = ORC_REG_DI;
- break;
- case CFI_DX:
- orc->sp_reg = ORC_REG_DX;
- break;
- default:
- WARN_FUNC("unknown CFA base reg %d",
- insn->sec, insn->offset, cfa->base);
- return -1;
- }
+ if (cfi->cfa.base == CFI_UNDEFINED) {
+ orc->sp_reg = ORC_REG_UNDEFINED;
+ return 0;
+ }
- switch(bp->base) {
- case CFI_UNDEFINED:
- orc->bp_reg = ORC_REG_UNDEFINED;
- break;
- case CFI_CFA:
- orc->bp_reg = ORC_REG_PREV_SP;
- break;
- case CFI_BP:
- orc->bp_reg = ORC_REG_BP;
- break;
- default:
- WARN_FUNC("unknown BP base reg %d",
- insn->sec, insn->offset, bp->base);
- return -1;
- }
+ switch (cfi->cfa.base) {
+ case CFI_SP:
+ orc->sp_reg = ORC_REG_SP;
+ break;
+ case CFI_SP_INDIRECT:
+ orc->sp_reg = ORC_REG_SP_INDIRECT;
+ break;
+ case CFI_BP:
+ orc->sp_reg = ORC_REG_BP;
+ break;
+ case CFI_BP_INDIRECT:
+ orc->sp_reg = ORC_REG_BP_INDIRECT;
+ break;
+ case CFI_R10:
+ orc->sp_reg = ORC_REG_R10;
+ break;
+ case CFI_R13:
+ orc->sp_reg = ORC_REG_R13;
+ break;
+ case CFI_DI:
+ orc->sp_reg = ORC_REG_DI;
+ break;
+ case CFI_DX:
+ orc->sp_reg = ORC_REG_DX;
+ break;
+ default:
+ WARN_FUNC("unknown CFA base reg %d",
+ insn->sec, insn->offset, cfi->cfa.base);
+ return -1;
+ }
- orc->sp_offset = cfa->offset;
- orc->bp_offset = bp->offset;
- orc->type = insn->cfi.type;
+ switch (bp->base) {
+ case CFI_UNDEFINED:
+ orc->bp_reg = ORC_REG_UNDEFINED;
+ break;
+ case CFI_CFA:
+ orc->bp_reg = ORC_REG_PREV_SP;
+ break;
+ case CFI_BP:
+ orc->bp_reg = ORC_REG_BP;
+ break;
+ default:
+ WARN_FUNC("unknown BP base reg %d",
+ insn->sec, insn->offset, bp->base);
+ return -1;
}
+ orc->sp_offset = cfi->cfa.offset;
+ orc->bp_offset = bp->offset;
+ orc->type = cfi->type;
+
return 0;
}
-static int create_orc_entry(struct elf *elf, struct section *u_sec, struct section *ip_relocsec,
- unsigned int idx, struct section *insn_sec,
- unsigned long insn_off, struct orc_entry *o)
+static int write_orc_entry(struct elf *elf, struct section *orc_sec,
+ struct section *ip_rsec, unsigned int idx,
+ struct section *insn_sec, unsigned long insn_off,
+ struct orc_entry *o)
{
struct orc_entry *orc;
struct reloc *reloc;
/* populate ORC data */
- orc = (struct orc_entry *)u_sec->data->d_buf + idx;
+ orc = (struct orc_entry *)orc_sec->data->d_buf + idx;
memcpy(orc, o, sizeof(*orc));
/* populate reloc for ip */
@@ -133,102 +128,109 @@ static int create_orc_entry(struct elf *elf, struct section *u_sec, struct secti
reloc->type = R_X86_64_PC32;
reloc->offset = idx * sizeof(int);
- reloc->sec = ip_relocsec;
+ reloc->sec = ip_rsec;
elf_add_reloc(elf, reloc);
return 0;
}
-int create_orc_sections(struct objtool_file *file)
+struct orc_list_entry {
+ struct list_head list;
+ struct orc_entry orc;
+ struct section *insn_sec;
+ unsigned long insn_off;
+};
+
+static int orc_list_add(struct list_head *orc_list, struct orc_entry *orc,
+ struct section *sec, unsigned long offset)
+{
+ struct orc_list_entry *entry = malloc(sizeof(*entry));
+
+ if (!entry) {
+ WARN("malloc failed");
+ return -1;
+ }
+
+ entry->orc = *orc;
+ entry->insn_sec = sec;
+ entry->insn_off = offset;
+
+ list_add_tail(&entry->list, orc_list);
+ return 0;
+}
+
+int orc_create(struct objtool_file *file)
{
- struct instruction *insn, *prev_insn;
- struct section *sec, *u_sec, *ip_relocsec;
- unsigned int idx;
+ struct section *sec, *ip_rsec, *orc_sec;
+ unsigned int nr = 0, idx = 0;
+ struct orc_list_entry *entry;
+ struct list_head orc_list;
- struct orc_entry empty = {
- .sp_reg = ORC_REG_UNDEFINED,
+ struct orc_entry null = {
+ .sp_reg = ORC_REG_UNDEFINED,
.bp_reg = ORC_REG_UNDEFINED,
.type = UNWIND_HINT_TYPE_CALL,
};
- sec = find_section_by_name(file->elf, ".orc_unwind");
- if (sec) {
- WARN("file already has .orc_unwind section, skipping");
- return -1;
- }
-
- /* count the number of needed orcs */
- idx = 0;
+ /* Build a deduplicated list of ORC entries: */
+ INIT_LIST_HEAD(&orc_list);
for_each_sec(file, sec) {
+ struct orc_entry orc, prev_orc = {0};
+ struct instruction *insn;
+ bool empty = true;
+
if (!sec->text)
continue;
- prev_insn = NULL;
sec_for_each_insn(file, sec, insn) {
- if (!prev_insn ||
- memcmp(&insn->orc, &prev_insn->orc,
- sizeof(struct orc_entry))) {
- idx++;
- }
- prev_insn = insn;
+ if (init_orc_entry(&orc, &insn->cfi))
+ return -1;
+ if (!memcmp(&prev_orc, &orc, sizeof(orc)))
+ continue;
+ if (orc_list_add(&orc_list, &orc, sec, insn->offset))
+ return -1;
+ nr++;
+ prev_orc = orc;
+ empty = false;
}
- /* section terminator */
- if (prev_insn)
- idx++;
+ /* Add a section terminator */
+ if (!empty) {
+ orc_list_add(&orc_list, &null, sec, sec->len);
+ nr++;
+ }
}
- if (!idx)
- return -1;
+ if (!nr)
+ return 0;
+ /* Create .orc_unwind, .orc_unwind_ip and .rela.orc_unwind_ip sections: */
+ sec = find_section_by_name(file->elf, ".orc_unwind");
+ if (sec) {
+ WARN("file already has .orc_unwind section, skipping");
+ return -1;
+ }
+ orc_sec = elf_create_section(file->elf, ".orc_unwind", 0,
+ sizeof(struct orc_entry), nr);
+ if (!orc_sec)
+ return -1;
- /* create .orc_unwind_ip and .rela.orc_unwind_ip sections */
- sec = elf_create_section(file->elf, ".orc_unwind_ip", 0, sizeof(int), idx);
+ sec = elf_create_section(file->elf, ".orc_unwind_ip", 0, sizeof(int), nr);
if (!sec)
return -1;
-
- ip_relocsec = elf_create_reloc_section(file->elf, sec, SHT_RELA);
- if (!ip_relocsec)
+ ip_rsec = elf_create_reloc_section(file->elf, sec, SHT_RELA);
+ if (!ip_rsec)
return -1;
- /* create .orc_unwind section */
- u_sec = elf_create_section(file->elf, ".orc_unwind", 0,
- sizeof(struct orc_entry), idx);
-
- /* populate sections */
- idx = 0;
- for_each_sec(file, sec) {
- if (!sec->text)
- continue;
-
- prev_insn = NULL;
- sec_for_each_insn(file, sec, insn) {
- if (!prev_insn || memcmp(&insn->orc, &prev_insn->orc,
- sizeof(struct orc_entry))) {
-
- if (create_orc_entry(file->elf, u_sec, ip_relocsec, idx,
- insn->sec, insn->offset,
- &insn->orc))
- return -1;
-
- idx++;
- }
- prev_insn = insn;
- }
-
- /* section terminator */
- if (prev_insn) {
- if (create_orc_entry(file->elf, u_sec, ip_relocsec, idx,
- prev_insn->sec,
- prev_insn->offset + prev_insn->len,
- &empty))
- return -1;
-
- idx++;
- }
+ /* Write ORC entries to sections: */
+ list_for_each_entry(entry, &orc_list, list) {
+ if (write_orc_entry(file->elf, orc_sec, ip_rsec, idx++,
+ entry->insn_sec, entry->insn_off,
+ &entry->orc))
+ return -1;
}
- if (elf_rebuild_reloc_section(file->elf, ip_relocsec))
+ if (elf_rebuild_reloc_section(file->elf, ip_rsec))
return -1;
return 0;
diff --git a/tools/objtool/weak.c b/tools/objtool/weak.c
index 7843e9a7a72f..553ec9ce51ba 100644
--- a/tools/objtool/weak.c
+++ b/tools/objtool/weak.c
@@ -25,12 +25,7 @@ int __weak orc_dump(const char *_objname)
UNSUPPORTED("orc");
}
-int __weak create_orc(struct objtool_file *file)
-{
- UNSUPPORTED("orc");
-}
-
-int __weak create_orc_sections(struct objtool_file *file)
+int __weak orc_create(struct objtool_file *file)
{
UNSUPPORTED("orc");
}
--
2.29.2
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 2/3] objtool: Add 'alt_group' struct
2020-12-23 5:18 [PATCH 0/3] Alternatives vs ORC, a slightly easier way Josh Poimboeuf
2020-12-23 5:18 ` [PATCH 1/3] objtool: Refactor ORC section generation Josh Poimboeuf
@ 2020-12-23 5:18 ` Josh Poimboeuf
2020-12-23 5:18 ` [PATCH 3/3] objtool: Support stack layout changes in alternatives Josh Poimboeuf
2 siblings, 0 replies; 11+ messages in thread
From: Josh Poimboeuf @ 2020-12-23 5:18 UTC (permalink / raw)
To: Juergen Gross; +Cc: xen-devel, linux-kernel, Peter Zijlstra, Miroslav Benes
Create a new struct associated with each group of alternatives
instructions. This will help with the removal of fake jumps, and more
importantly with adding support for stack layout changes in
alternatives.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
tools/objtool/check.c | 29 +++++++++++++++++++++++------
tools/objtool/check.h | 13 ++++++++++++-
2 files changed, 35 insertions(+), 7 deletions(-)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index c6ab44543c92..67f39b57c6f7 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -984,20 +984,28 @@ static int handle_group_alt(struct objtool_file *file,
struct instruction *orig_insn,
struct instruction **new_insn)
{
- static unsigned int alt_group_next_index = 1;
struct instruction *last_orig_insn, *last_new_insn, *insn, *fake_jump = NULL;
- unsigned int alt_group = alt_group_next_index++;
+ struct alt_group *orig_alt_group, *new_alt_group;
unsigned long dest_off;
+
+ orig_alt_group = malloc(sizeof(*orig_alt_group));
+ if (!orig_alt_group) {
+ WARN("malloc failed");
+ return -1;
+ }
last_orig_insn = NULL;
insn = orig_insn;
sec_for_each_insn_from(file, insn) {
if (insn->offset >= special_alt->orig_off + special_alt->orig_len)
break;
- insn->alt_group = alt_group;
+ insn->alt_group = orig_alt_group;
last_orig_insn = insn;
}
+ orig_alt_group->orig_group = NULL;
+ orig_alt_group->first_insn = orig_insn;
+ orig_alt_group->last_insn = last_orig_insn;
if (next_insn_same_sec(file, last_orig_insn)) {
fake_jump = malloc(sizeof(*fake_jump));
@@ -1028,8 +1036,13 @@ static int handle_group_alt(struct objtool_file *file,
return 0;
}
+ new_alt_group = malloc(sizeof(*new_alt_group));
+ if (!new_alt_group) {
+ WARN("malloc failed");
+ return -1;
+ }
+
last_new_insn = NULL;
- alt_group = alt_group_next_index++;
insn = *new_insn;
sec_for_each_insn_from(file, insn) {
struct reloc *alt_reloc;
@@ -1041,7 +1054,7 @@ static int handle_group_alt(struct objtool_file *file,
insn->ignore = orig_insn->ignore_alts;
insn->func = orig_insn->func;
- insn->alt_group = alt_group;
+ insn->alt_group = new_alt_group;
/*
* Since alternative replacement code is copy/pasted by the
@@ -1090,6 +1103,10 @@ static int handle_group_alt(struct objtool_file *file,
return -1;
}
+ new_alt_group->orig_group = orig_alt_group;
+ new_alt_group->first_insn = *new_insn;
+ new_alt_group->last_insn = last_new_insn;
+
if (fake_jump)
list_add(&fake_jump->list, &last_new_insn->list);
@@ -2405,7 +2422,7 @@ static int validate_return(struct symbol *func, struct instruction *insn, struct
static void fill_alternative_cfi(struct objtool_file *file, struct instruction *insn)
{
struct instruction *first_insn = insn;
- int alt_group = insn->alt_group;
+ struct alt_group *alt_group = insn->alt_group;
sec_for_each_insn_continue(file, insn) {
if (insn->alt_group != alt_group)
diff --git a/tools/objtool/check.h b/tools/objtool/check.h
index 4c10916ff1cf..b74c383c2d83 100644
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -19,6 +19,17 @@ struct insn_state {
s8 instr;
};
+struct alt_group {
+ /*
+ * Pointer from a replacement group to the original group. NULL if it
+ * *is* the original group.
+ */
+ struct alt_group *orig_group;
+
+ /* First and last instructions in the group */
+ struct instruction *first_insn, *last_insn;
+};
+
struct instruction {
struct list_head list;
struct hlist_node hash;
@@ -34,7 +45,7 @@ struct instruction {
s8 instr;
u8 visited;
u8 ret_offset;
- int alt_group;
+ struct alt_group *alt_group;
struct symbol *call_dest;
struct instruction *jump_dest;
struct instruction *first_jump_src;
--
2.29.2
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 3/3] objtool: Support stack layout changes in alternatives
2020-12-23 5:18 [PATCH 0/3] Alternatives vs ORC, a slightly easier way Josh Poimboeuf
2020-12-23 5:18 ` [PATCH 1/3] objtool: Refactor ORC section generation Josh Poimboeuf
2020-12-23 5:18 ` [PATCH 2/3] objtool: Add 'alt_group' struct Josh Poimboeuf
@ 2020-12-23 5:18 ` Josh Poimboeuf
2021-01-04 14:09 ` Peter Zijlstra
2021-01-07 13:22 ` Miroslav Benes
2 siblings, 2 replies; 11+ messages in thread
From: Josh Poimboeuf @ 2020-12-23 5:18 UTC (permalink / raw)
To: Juergen Gross
Cc: xen-devel, linux-kernel, Peter Zijlstra, Miroslav Benes,
Shinichiro Kawasaki
The ORC unwinder showed a warning [1] which revealed the stack layout
didn't match what was expected. The problem was that paravirt patching
had replaced "CALL *pv_ops.irq.save_fl" with "PUSHF;POP". That changed
the stack layout between the PUSHF and the POP, so unwinding from an
interrupt which occurred between those two instructions would fail.
Part of the agreed upon solution was to rework the custom paravirt
patching code to use alternatives instead, since objtool already knows
how to read alternatives (and converging runtime patching infrastructure
is always a good thing anyway). But the main problem still remains,
which is that runtime patching can change the stack layout.
Making stack layout changes in alternatives was disallowed with commit
7117f16bf460 ("objtool: Fix ORC vs alternatives"), but now that paravirt
is going to be doing it, it needs to be supported.
One way to do so would be to modify the ORC table when the code gets
patched. But ORC is simple -- a good thing! -- and it's best to leave
it alone.
Instead, support stack layout changes by "flattening" all possible stack
states (CFI) from parallel alternative code streams into a single set of
linear states. The only necessary limitation is that CFI conflicts are
disallowed at all possible instruction boundaries.
For example, this scenario is allowed:
Alt1 Alt2 Alt3
0x00 CALL *pv_ops.save_fl CALL xen_save_fl PUSHF
0x01 POP %RAX
0x02 NOP
...
0x05 NOP
...
0x07 <insn>
The unwind information for offset-0x00 is identical for all 3
alternatives. Similarly offset-0x05 and higher also are identical (and
the same as 0x00). However offset-0x01 has deviating CFI, but that is
only relevant for Alt3, neither of the other alternative instruction
streams will ever hit that offset.
This scenario is NOT allowed:
Alt1 Alt2
0x00 CALL *pv_ops.save_fl PUSHF
0x01 NOP6
...
0x07 NOP POP %RAX
The problem here is that offset-0x7, which is an instruction boundary in
both possible instruction patch streams, has two conflicting stack
layouts.
[ The above examples were stolen from Peter Zijlstra. ]
The new flattened CFI array is used both for the detection of conflicts
(like the second example above) and the generation of linear ORC
entries.
BTW, another benefit of these changes is that, thanks to some related
cleanups (new fake nops and alt_group struct) objtool can finally be rid
of fake jumps, which were a constant source of headaches.
[1] https://lkml.kernel.org/r/20201111170536.arx2zbn4ngvjoov7@treble
Cc: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
.../Documentation/stack-validation.txt | 16 +-
tools/objtool/check.c | 175 ++++++++++--------
tools/objtool/check.h | 6 +
tools/objtool/orc_gen.c | 56 +++++-
4 files changed, 157 insertions(+), 96 deletions(-)
diff --git a/tools/objtool/Documentation/stack-validation.txt b/tools/objtool/Documentation/stack-validation.txt
index 0542e46c7552..30f38fdc0d56 100644
--- a/tools/objtool/Documentation/stack-validation.txt
+++ b/tools/objtool/Documentation/stack-validation.txt
@@ -315,13 +315,15 @@ they mean, and suggestions for how to fix them.
function tracing inserts additional calls, which is not obvious from the
sources).
-10. file.o: warning: func()+0x5c: alternative modifies stack
-
- This means that an alternative includes instructions that modify the
- stack. The problem is that there is only one ORC unwind table, this means
- that the ORC unwind entries must be valid for each of the alternatives.
- The easiest way to enforce this is to ensure alternatives do not contain
- any ORC entries, which in turn implies the above constraint.
+10. file.o: warning: func()+0x5c: stack layout conflict in alternatives
+
+ This means that in the use of the alternative() or ALTERNATIVE()
+ macro, the code paths have conflicting modifications to the stack.
+ The problem is that there is only one ORC unwind table, which means
+ that the ORC unwind entries must be consistent for all possible
+ instruction boundaries regardless of which code has been patched.
+ This limitation can be overcome by massaging the alternatives with
+ NOPs to shift the stack changes around so they no longer conflict.
11. file.o: warning: unannotated intra-function call
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 67f39b57c6f7..81d56fdef1c3 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -19,8 +19,6 @@
#include <linux/kernel.h>
#include <linux/static_call_types.h>
-#define FAKE_JUMP_OFFSET -1
-
struct alternative {
struct list_head list;
struct instruction *insn;
@@ -767,9 +765,6 @@ static int add_jump_destinations(struct objtool_file *file)
if (!is_static_jump(insn))
continue;
- if (insn->offset == FAKE_JUMP_OFFSET)
- continue;
-
reloc = find_reloc_by_dest_range(file->elf, insn->sec,
insn->offset, insn->len);
if (!reloc) {
@@ -984,7 +979,7 @@ static int handle_group_alt(struct objtool_file *file,
struct instruction *orig_insn,
struct instruction **new_insn)
{
- struct instruction *last_orig_insn, *last_new_insn, *insn, *fake_jump = NULL;
+ struct instruction *last_orig_insn, *last_new_insn = NULL, *insn, *nop = NULL;
struct alt_group *orig_alt_group, *new_alt_group;
unsigned long dest_off;
@@ -994,6 +989,13 @@ static int handle_group_alt(struct objtool_file *file,
WARN("malloc failed");
return -1;
}
+ orig_alt_group->cfi = calloc(special_alt->orig_len,
+ sizeof(struct cfi_state *));
+ if (!orig_alt_group->cfi) {
+ WARN("calloc failed");
+ return -1;
+ }
+
last_orig_insn = NULL;
insn = orig_insn;
sec_for_each_insn_from(file, insn) {
@@ -1007,42 +1009,45 @@ static int handle_group_alt(struct objtool_file *file,
orig_alt_group->first_insn = orig_insn;
orig_alt_group->last_insn = last_orig_insn;
- if (next_insn_same_sec(file, last_orig_insn)) {
- fake_jump = malloc(sizeof(*fake_jump));
- if (!fake_jump) {
- WARN("malloc failed");
- return -1;
- }
- memset(fake_jump, 0, sizeof(*fake_jump));
- INIT_LIST_HEAD(&fake_jump->alts);
- INIT_LIST_HEAD(&fake_jump->stack_ops);
- init_cfi_state(&fake_jump->cfi);
- fake_jump->sec = special_alt->new_sec;
- fake_jump->offset = FAKE_JUMP_OFFSET;
- fake_jump->type = INSN_JUMP_UNCONDITIONAL;
- fake_jump->jump_dest = list_next_entry(last_orig_insn, list);
- fake_jump->func = orig_insn->func;
+ new_alt_group = malloc(sizeof(*new_alt_group));
+ if (!new_alt_group) {
+ WARN("malloc failed");
+ return -1;
}
- if (!special_alt->new_len) {
- if (!fake_jump) {
- WARN("%s: empty alternative at end of section",
- special_alt->orig_sec->name);
+ if (special_alt->new_len < special_alt->orig_len) {
+ /*
+ * Insert a fake nop at the end to make the replacement
+ * alt_group the same size as the original. This is needed to
+ * allow propagate_alt_cfi() to do its magic. When the last
+ * instruction affects the stack, the instruction after it (the
+ * nop) will propagate the new state to the shared CFI array.
+ */
+ nop = malloc(sizeof(*nop));
+ if (!nop) {
+ WARN("malloc failed");
return -1;
}
+ memset(nop, 0, sizeof(*nop));
+ INIT_LIST_HEAD(&nop->alts);
+ INIT_LIST_HEAD(&nop->stack_ops);
+ init_cfi_state(&nop->cfi);
- *new_insn = fake_jump;
- return 0;
+ nop->sec = special_alt->new_sec;
+ nop->offset = special_alt->new_off + special_alt->new_len;
+ nop->len = special_alt->orig_len - special_alt->new_len;
+ nop->type = INSN_NOP;
+ nop->func = orig_insn->func;
+ nop->alt_group = new_alt_group;
+ nop->ignore = orig_insn->ignore_alts;
}
- new_alt_group = malloc(sizeof(*new_alt_group));
- if (!new_alt_group) {
- WARN("malloc failed");
- return -1;
+ if (!special_alt->new_len) {
+ *new_insn = nop;
+ goto end;
}
- last_new_insn = NULL;
insn = *new_insn;
sec_for_each_insn_from(file, insn) {
struct reloc *alt_reloc;
@@ -1081,14 +1086,8 @@ static int handle_group_alt(struct objtool_file *file,
continue;
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",
- special_alt->orig_sec->name);
- return -1;
- }
- insn->jump_dest = fake_jump;
- }
+ if (dest_off == special_alt->new_off + special_alt->new_len)
+ insn->jump_dest = next_insn_same_sec(file, last_orig_insn);
if (!insn->jump_dest) {
WARN_FUNC("can't find alternative jump destination",
@@ -1103,13 +1102,13 @@ static int handle_group_alt(struct objtool_file *file,
return -1;
}
+ if (nop)
+ list_add(&nop->list, &last_new_insn->list);
+end:
new_alt_group->orig_group = orig_alt_group;
new_alt_group->first_insn = *new_insn;
- new_alt_group->last_insn = last_new_insn;
-
- if (fake_jump)
- list_add(&fake_jump->list, &last_new_insn->list);
-
+ new_alt_group->last_insn = nop ? : last_new_insn;
+ new_alt_group->cfi = orig_alt_group->cfi;
return 0;
}
@@ -2202,22 +2201,47 @@ static int update_cfi_state(struct instruction *insn, struct cfi_state *cfi,
return 0;
}
-static int handle_insn_ops(struct instruction *insn, struct insn_state *state)
+/*
+ * The stack layouts of alternatives instructions can sometimes diverge when
+ * they have stack modifications. That's fine as long as the potential stack
+ * layouts don't conflict at any given potential instruction boundary.
+ *
+ * Flatten the CFIs of the different alternative code streams (both original
+ * and replacement) into a single shared CFI array which can be used to detect
+ * conflicts and nicely feed a linear array of ORC entries to the unwinder.
+ */
+static int propagate_alt_cfi(struct objtool_file *file, struct instruction *insn)
{
- struct stack_op *op;
+ struct cfi_state **alt_cfi;
+ int group_off;
- list_for_each_entry(op, &insn->stack_ops, list) {
- struct cfi_state old_cfi = state->cfi;
- int res;
+ if (!insn->alt_group)
+ return 0;
- res = update_cfi_state(insn, &state->cfi, op);
- if (res)
- return res;
+ alt_cfi = insn->alt_group->cfi;
+ group_off = insn->offset - insn->alt_group->first_insn->offset;
- if (insn->alt_group && memcmp(&state->cfi, &old_cfi, sizeof(struct cfi_state))) {
- WARN_FUNC("alternative modifies stack", insn->sec, insn->offset);
+ if (!alt_cfi[group_off]) {
+ alt_cfi[group_off] = &insn->cfi;
+ } else {
+ if (memcmp(alt_cfi[group_off], &insn->cfi, sizeof(struct cfi_state))) {
+ WARN_FUNC("stack layout conflict in alternatives",
+ insn->sec, insn->offset);
return -1;
}
+ }
+
+ 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) {
+
+ if (update_cfi_state(insn, &state->cfi, op))
+ return 1;
if (op->dest.type == OP_DEST_PUSHF) {
if (!state->uaccess_stack) {
@@ -2407,28 +2431,20 @@ static int validate_return(struct symbol *func, struct instruction *insn, struct
return 0;
}
-/*
- * Alternatives should not contain any ORC entries, this in turn means they
- * should not contain any CFI ops, which implies all instructions should have
- * the same same CFI state.
- *
- * It is possible to constuct alternatives that have unreachable holes that go
- * unreported (because they're NOPs), such holes would result in CFI_UNDEFINED
- * states which then results in ORC entries, which we just said we didn't want.
- *
- * Avoid them by copying the CFI entry of the first instruction into the whole
- * alternative.
- */
-static void fill_alternative_cfi(struct objtool_file *file, struct instruction *insn)
+static struct instruction *next_insn_to_validate(struct objtool_file *file,
+ struct instruction *insn)
{
- struct instruction *first_insn = insn;
struct alt_group *alt_group = insn->alt_group;
- sec_for_each_insn_continue(file, insn) {
- if (insn->alt_group != alt_group)
- break;
- insn->cfi = first_insn->cfi;
- }
+ /*
+ * Simulate the fact that alternatives are patched in-place. When the
+ * end of a replacement alt_group is reached, redirect objtool flow to
+ * the end of the original alt_group.
+ */
+ if (alt_group && insn == alt_group->last_insn && alt_group->orig_group)
+ return next_insn_same_sec(file, alt_group->orig_group->last_insn);
+
+ return next_insn_same_sec(file, insn);
}
/*
@@ -2449,7 +2465,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
sec = insn->sec;
while (1) {
- next_insn = next_insn_same_sec(file, insn);
+ next_insn = next_insn_to_validate(file, insn);
if (file->c_file && func && insn->func && func != insn->func->pfunc) {
WARN("%s() falls through to next function %s()",
@@ -2482,6 +2498,9 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
insn->visited |= visited;
+ if (propagate_alt_cfi(file, insn))
+ return 1;
+
if (!insn->ignore_alts && !list_empty(&insn->alts)) {
bool skip_orig = false;
@@ -2497,9 +2516,6 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
}
}
- if (insn->alt_group)
- fill_alternative_cfi(file, insn);
-
if (skip_orig)
return 0;
}
@@ -2733,9 +2749,6 @@ static bool ignore_unreachable_insn(struct objtool_file *file, struct instructio
!strcmp(insn->sec->name, ".altinstr_aux"))
return true;
- if (insn->type == INSN_JUMP_UNCONDITIONAL && insn->offset == FAKE_JUMP_OFFSET)
- return true;
-
if (!insn->func)
return false;
diff --git a/tools/objtool/check.h b/tools/objtool/check.h
index b74c383c2d83..45fe87ad662b 100644
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -28,6 +28,12 @@ struct alt_group {
/* First and last instructions in the group */
struct instruction *first_insn, *last_insn;
+
+ /*
+ * Byte-offset-addressed len-sized array of pointers to CFI structs.
+ * This is shared with the other alt_groups in the same alternative.
+ */
+ struct cfi_state **cfi;
};
struct instruction {
diff --git a/tools/objtool/orc_gen.c b/tools/objtool/orc_gen.c
index 73efba2bfa72..2c1e3b909be5 100644
--- a/tools/objtool/orc_gen.c
+++ b/tools/objtool/orc_gen.c
@@ -160,6 +160,13 @@ static int orc_list_add(struct list_head *orc_list, struct orc_entry *orc,
return 0;
}
+static unsigned long alt_group_len(struct alt_group *alt_group)
+{
+ return alt_group->last_insn->offset +
+ alt_group->last_insn->len -
+ alt_group->first_insn->offset;
+}
+
int orc_create(struct objtool_file *file)
{
struct section *sec, *ip_rsec, *orc_sec;
@@ -184,15 +191,48 @@ int orc_create(struct objtool_file *file)
continue;
sec_for_each_insn(file, sec, insn) {
- if (init_orc_entry(&orc, &insn->cfi))
- return -1;
- if (!memcmp(&prev_orc, &orc, sizeof(orc)))
+ struct alt_group *alt_group = insn->alt_group;
+ int i;
+
+ if (!alt_group) {
+ if (init_orc_entry(&orc, &insn->cfi))
+ return -1;
+ if (!memcmp(&prev_orc, &orc, sizeof(orc)))
+ continue;
+ if (orc_list_add(&orc_list, &orc, sec,
+ insn->offset))
+ return -1;
+ nr++;
+ prev_orc = orc;
+ empty = false;
continue;
- if (orc_list_add(&orc_list, &orc, sec, insn->offset))
- return -1;
- nr++;
- prev_orc = orc;
- empty = false;
+ }
+
+ /*
+ * Alternatives can have different stack layout
+ * possibilities (but they shouldn't conflict).
+ * Instead of traversing the instructions, use the
+ * alt_group's flattened byte-offset-addressed CFI
+ * array.
+ */
+ for (i = 0; i < alt_group_len(alt_group); i++) {
+ struct cfi_state *cfi = alt_group->cfi[i];
+ if (!cfi)
+ continue;
+ if (init_orc_entry(&orc, cfi))
+ return -1;
+ if (!memcmp(&prev_orc, &orc, sizeof(orc)))
+ continue;
+ if (orc_list_add(&orc_list, &orc, insn->sec,
+ insn->offset + i))
+ return -1;
+ nr++;
+ prev_orc = orc;
+ empty = false;
+ }
+
+ /* Skip to the end of the alt_group */
+ insn = alt_group->last_insn;
}
/* Add a section terminator */
--
2.29.2
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 3/3] objtool: Support stack layout changes in alternatives
2020-12-23 5:18 ` [PATCH 3/3] objtool: Support stack layout changes in alternatives Josh Poimboeuf
@ 2021-01-04 14:09 ` Peter Zijlstra
2021-01-04 15:16 ` Josh Poimboeuf
2021-01-07 13:22 ` Miroslav Benes
1 sibling, 1 reply; 11+ messages in thread
From: Peter Zijlstra @ 2021-01-04 14:09 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Juergen Gross, xen-devel, linux-kernel, Miroslav Benes,
Shinichiro Kawasaki
On Tue, Dec 22, 2020 at 11:18:10PM -0600, Josh Poimboeuf wrote:
> For example, this scenario is allowed:
>
> Alt1 Alt2 Alt3
>
> 0x00 CALL *pv_ops.save_fl CALL xen_save_fl PUSHF
> 0x01 POP %RAX
> 0x02 NOP
> ...
> 0x05 NOP
> ...
> 0x07 <insn>
>
> This scenario is NOT allowed:
>
> Alt1 Alt2
>
> 0x00 CALL *pv_ops.save_fl PUSHF
> 0x01 NOP6
> ...
> 0x07 NOP POP %RAX
>
> The problem here is that offset-0x7, which is an instruction boundary in
> both possible instruction patch streams, has two conflicting stack
> layouts.
There's another fun scenario:
0x00 CALL *pv_ops.save_fl PUSHF
0x01 NOP2
..
0x03 NOP5
..
0x07 NOP2
0x08 POP %RAX
0x09 <insn>
No conflicting boundary at 0x07, but still buggered.
Let me go read the actual patch to see if this is handled.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] objtool: Support stack layout changes in alternatives
2021-01-04 14:09 ` Peter Zijlstra
@ 2021-01-04 15:16 ` Josh Poimboeuf
2021-01-04 15:48 ` Peter Zijlstra
0 siblings, 1 reply; 11+ messages in thread
From: Josh Poimboeuf @ 2021-01-04 15:16 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Juergen Gross, xen-devel, linux-kernel, Miroslav Benes,
Shinichiro Kawasaki
On Mon, Jan 04, 2021 at 03:09:52PM +0100, Peter Zijlstra wrote:
> On Tue, Dec 22, 2020 at 11:18:10PM -0600, Josh Poimboeuf wrote:
>
> > For example, this scenario is allowed:
> >
> > Alt1 Alt2 Alt3
> >
> > 0x00 CALL *pv_ops.save_fl CALL xen_save_fl PUSHF
> > 0x01 POP %RAX
> > 0x02 NOP
> > ...
> > 0x05 NOP
> > ...
> > 0x07 <insn>
> >
>
> > This scenario is NOT allowed:
> >
> > Alt1 Alt2
> >
> > 0x00 CALL *pv_ops.save_fl PUSHF
> > 0x01 NOP6
> > ...
> > 0x07 NOP POP %RAX
> >
>
> > The problem here is that offset-0x7, which is an instruction boundary in
> > both possible instruction patch streams, has two conflicting stack
> > layouts.
>
> There's another fun scenario:
>
> 0x00 CALL *pv_ops.save_fl PUSHF
> 0x01 NOP2
> ..
> 0x03 NOP5
> ..
> 0x07 NOP2
> 0x08 POP %RAX
> 0x09 <insn>
>
> No conflicting boundary at 0x07, but still buggered.
>
> Let me go read the actual patch to see if this is handled.
That scenario looks good, see ORC below:
.diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index cad08703c4ad..4079a430ab3f 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -1483,3 +1483,8 @@ SYM_CODE_START(rewind_stack_do_exit)
call do_exit
SYM_CODE_END(rewind_stack_do_exit)
.popsection
+
+SYM_FUNC_START(peter)
+ ALTERNATIVE "call *pv_ops+288(%rip); .byte 0x66,0x90", "pushf; .byte 0x66,0x90; .byte 0x66,0x66,0x66,0x90; popq %rax", X86_FEATURE_ALWAYS
+ ret
+SYM_FUNC_END(peter)
00000000000014e0 <peter>:
14e0: ff 15 00 00 00 00 callq *0x0(%rip) # 14e6 <peter+0x6>
14e2: R_X86_64_PC32 pv_ops+0x11c
14e6: 66 90 xchg %ax,%ax
14e8: c3 retq
alt replacement:
cf: 9c pushfq
d0: 66 90 xchg %ax,%ax
d2: 66 66 66 90 data16 data16 xchg %ax,%ax
d6: 58 pop %rax
ORC:
.entry.text+14e0: sp:sp+8 bp:(und) type:call end:0
.entry.text+14e1: sp:sp+16 bp:(und) type:call end:0
.entry.text+14e6: sp:sp+8 bp:(und) type:call end:0
.entry.text+14e7: sp:sp+16 bp:(und) type:call end:0
.entry.text+14e8: sp:sp+8 bp:(und) type:call end:0
.entry.text+14e9: sp:(und) bp:(und) type:call end:0
--
Josh
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 3/3] objtool: Support stack layout changes in alternatives
2021-01-04 15:16 ` Josh Poimboeuf
@ 2021-01-04 15:48 ` Peter Zijlstra
0 siblings, 0 replies; 11+ messages in thread
From: Peter Zijlstra @ 2021-01-04 15:48 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Juergen Gross, xen-devel, linux-kernel, Miroslav Benes,
Shinichiro Kawasaki
On Mon, Jan 04, 2021 at 09:16:33AM -0600, Josh Poimboeuf wrote:
> > There's another fun scenario:
> >
> > 0x00 CALL *pv_ops.save_fl PUSHF
> > 0x01 NOP2
> > ..
> > 0x03 NOP5
> > ..
> > 0x07 NOP2
> > 0x08 POP %RAX
> > 0x09 <insn>
> >
> > No conflicting boundary at 0x07, but still buggered.
> >
> > Let me go read the actual patch to see if this is handled.
>
> That scenario looks good, see ORC below:
>
> .diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
> index cad08703c4ad..4079a430ab3f 100644
> --- a/arch/x86/entry/entry_64.S
> +++ b/arch/x86/entry/entry_64.S
> @@ -1483,3 +1483,8 @@ SYM_CODE_START(rewind_stack_do_exit)
> call do_exit
> SYM_CODE_END(rewind_stack_do_exit)
> .popsection
> +
> +SYM_FUNC_START(peter)
> + ALTERNATIVE "call *pv_ops+288(%rip); .byte 0x66,0x90", "pushf; .byte 0x66,0x90; .byte 0x66,0x66,0x66,0x90; popq %rax", X86_FEATURE_ALWAYS
> + ret
> +SYM_FUNC_END(peter)
>
>
> 00000000000014e0 <peter>:
> 14e0: ff 15 00 00 00 00 callq *0x0(%rip) # 14e6 <peter+0x6>
> 14e2: R_X86_64_PC32 pv_ops+0x11c
> 14e6: 66 90 xchg %ax,%ax
> 14e8: c3 retq
>
> alt replacement:
> cf: 9c pushfq
> d0: 66 90 xchg %ax,%ax
> d2: 66 66 66 90 data16 data16 xchg %ax,%ax
> d6: 58 pop %rax
>
>
>
> ORC:
>
> .entry.text+14e0: sp:sp+8 bp:(und) type:call end:0
> .entry.text+14e1: sp:sp+16 bp:(und) type:call end:0
> .entry.text+14e6: sp:sp+8 bp:(und) type:call end:0
> .entry.text+14e7: sp:sp+16 bp:(und) type:call end:0
> .entry.text+14e8: sp:sp+8 bp:(und) type:call end:0
> .entry.text+14e9: sp:(und) bp:(und) type:call end:0
Aaah, I was thinking the (LHS) NOP2 lookup would find the (RHS) PUSHF
and fail, but yes, it will emit it's own +8 and find that ofcourse!
So then yes, we only need to concern outselves with same offset
conflicts, and that does indeed simplify things considerably.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] objtool: Support stack layout changes in alternatives
2020-12-23 5:18 ` [PATCH 3/3] objtool: Support stack layout changes in alternatives Josh Poimboeuf
2021-01-04 14:09 ` Peter Zijlstra
@ 2021-01-07 13:22 ` Miroslav Benes
2021-01-07 18:18 ` Josh Poimboeuf
1 sibling, 1 reply; 11+ messages in thread
From: Miroslav Benes @ 2021-01-07 13:22 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Juergen Gross, xen-devel, linux-kernel, Peter Zijlstra,
Shinichiro Kawasaki
On Tue, 22 Dec 2020, Josh Poimboeuf wrote:
> BTW, another benefit of these changes is that, thanks to some related
> cleanups (new fake nops and alt_group struct) objtool can finally be rid
> of fake jumps, which were a constant source of headaches.
\o/
You may also want to remove/edit the comment right before
handle_group_alt() now that fake jumps are gone.
Anyway, I walked through the patch (set) and I think it should work fine
(but I am not confident enough to give it Reviewed-by. My head spins :)).
I even like the change.
Also, 1/3 is a benefit on its own, so if nothing else, it could go in.
Regards
Miroslav
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] objtool: Support stack layout changes in alternatives
2021-01-07 13:22 ` Miroslav Benes
@ 2021-01-07 18:18 ` Josh Poimboeuf
2021-01-08 8:15 ` Miroslav Benes
0 siblings, 1 reply; 11+ messages in thread
From: Josh Poimboeuf @ 2021-01-07 18:18 UTC (permalink / raw)
To: Miroslav Benes
Cc: Juergen Gross, xen-devel, linux-kernel, Peter Zijlstra,
Shinichiro Kawasaki
On Thu, Jan 07, 2021 at 02:22:27PM +0100, Miroslav Benes wrote:
> On Tue, 22 Dec 2020, Josh Poimboeuf wrote:
>
> > BTW, another benefit of these changes is that, thanks to some related
> > cleanups (new fake nops and alt_group struct) objtool can finally be rid
> > of fake jumps, which were a constant source of headaches.
>
> \o/
>
> You may also want to remove/edit the comment right before
> handle_group_alt() now that fake jumps are gone.
>
> Anyway, I walked through the patch (set) and I think it should work fine
> (but I am not confident enough to give it Reviewed-by. My head spins :)).
> I even like the change.
>
> Also, 1/3 is a benefit on its own, so if nothing else, it could go in.
Thanks for the review!
That comment is indeed now obsolete. I can squash something like so:
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 81d56fdef1c3..ce67437aaf3f 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -958,21 +958,8 @@ static int add_call_destinations(struct objtool_file *file)
}
/*
- * The .alternatives section requires some extra special care, over and above
- * what other special sections require:
- *
- * 1. Because alternatives are patched in-place, we need to insert a fake jump
- * instruction at the end so that validate_branch() skips all the original
- * replaced instructions when validating the new instruction path.
- *
- * 2. An added wrinkle is that the new instruction length might be zero. In
- * that case the old instructions are replaced with noops. We simulate that
- * by creating a fake jump as the only new instruction.
- *
- * 3. In some cases, the alternative section includes an instruction which
- * conditionally jumps to the _end_ of the entry. We have to modify these
- * jumps' destinations to point back to .text rather than the end of the
- * entry in .altinstr_replacement.
+ * The .alternatives section requires some extra special care over and above
+ * other special sections because alternatives are patched in place.
*/
static int handle_group_alt(struct objtool_file *file,
struct special_alt *special_alt,
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] objtool: Support stack layout changes in alternatives
2021-01-07 18:18 ` Josh Poimboeuf
@ 2021-01-08 8:15 ` Miroslav Benes
2021-01-14 0:21 ` Josh Poimboeuf
0 siblings, 1 reply; 11+ messages in thread
From: Miroslav Benes @ 2021-01-08 8:15 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Juergen Gross, xen-devel, linux-kernel, Peter Zijlstra,
Shinichiro Kawasaki
> That comment is indeed now obsolete. I can squash something like so:
>
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index 81d56fdef1c3..ce67437aaf3f 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -958,21 +958,8 @@ static int add_call_destinations(struct objtool_file *file)
> }
>
> /*
> - * The .alternatives section requires some extra special care, over and above
> - * what other special sections require:
> - *
> - * 1. Because alternatives are patched in-place, we need to insert a fake jump
> - * instruction at the end so that validate_branch() skips all the original
> - * replaced instructions when validating the new instruction path.
> - *
> - * 2. An added wrinkle is that the new instruction length might be zero. In
> - * that case the old instructions are replaced with noops. We simulate that
> - * by creating a fake jump as the only new instruction.
> - *
> - * 3. In some cases, the alternative section includes an instruction which
> - * conditionally jumps to the _end_ of the entry. We have to modify these
> - * jumps' destinations to point back to .text rather than the end of the
> - * entry in .altinstr_replacement.
> + * The .alternatives section requires some extra special care over and above
> + * other special sections because alternatives are patched in place.
> */
> static int handle_group_alt(struct objtool_file *file,
> struct special_alt *special_alt,
Looks good to me.
Miroslav
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] objtool: Support stack layout changes in alternatives
2021-01-08 8:15 ` Miroslav Benes
@ 2021-01-14 0:21 ` Josh Poimboeuf
0 siblings, 0 replies; 11+ messages in thread
From: Josh Poimboeuf @ 2021-01-14 0:21 UTC (permalink / raw)
To: Miroslav Benes
Cc: Juergen Gross, xen-devel, linux-kernel, Peter Zijlstra,
Shinichiro Kawasaki
On Fri, Jan 08, 2021 at 09:15:43AM +0100, Miroslav Benes wrote:
> > That comment is indeed now obsolete. I can squash something like so:
> >
> > diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> > index 81d56fdef1c3..ce67437aaf3f 100644
> > --- a/tools/objtool/check.c
> > +++ b/tools/objtool/check.c
> > @@ -958,21 +958,8 @@ static int add_call_destinations(struct objtool_file *file)
> > }
> >
> > /*
> > - * The .alternatives section requires some extra special care, over and above
> > - * what other special sections require:
> > - *
> > - * 1. Because alternatives are patched in-place, we need to insert a fake jump
> > - * instruction at the end so that validate_branch() skips all the original
> > - * replaced instructions when validating the new instruction path.
> > - *
> > - * 2. An added wrinkle is that the new instruction length might be zero. In
> > - * that case the old instructions are replaced with noops. We simulate that
> > - * by creating a fake jump as the only new instruction.
> > - *
> > - * 3. In some cases, the alternative section includes an instruction which
> > - * conditionally jumps to the _end_ of the entry. We have to modify these
> > - * jumps' destinations to point back to .text rather than the end of the
> > - * entry in .altinstr_replacement.
> > + * The .alternatives section requires some extra special care over and above
> > + * other special sections because alternatives are patched in place.
> > */
> > static int handle_group_alt(struct objtool_file *file,
> > struct special_alt *special_alt,
>
> Looks good to me.
Thanks, I squashed this in.
If there are no objections, I'll go ahead and merge this set in -tip.
It doesn't have any direct dependencies on the Juergen's changes, and it
would be nice to have some of these changes in-tree, especially the fake
jump removal.
--
Josh
^ permalink raw reply [flat|nested] 11+ messages in thread