[PATCH 0/5] objtool: The stack swizzle again

[PATCH 0/5] objtool: The stack swizzle again

[Peter Zijlstra]

Hi

The stack swizzle is current again, these patches implement objtool support for
the native x86_64 stack swizzle pattern.

It allows Thomas to use his preferred minimal stack swizzle without (much)
limitations.

We used to use SP_INDIRECT with hints back before the entry code rewrite, the
current code relies on asm with a frame-pointer setup, but that all needs to
change again.

Avoid the hint abuse and detect the pattern directly.

[PATCH 1/5] objtool: Change REG_SP_INDIRECT

[Peter Zijlstra]

Currently REG_SP_INDIRECT is unused but means (%rsp + offset),
change it to mean (%rsp) + offset.

This is somewhat unfortunate, since REG_BP_INDIRECT is used (by DRAP)
and thus needs to retain the current (%rbp + offset).

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/kernel/unwind_orc.c |    5 ++++-
 tools/objtool/orc_dump.c     |    2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)

--- a/arch/x86/kernel/unwind_orc.c
+++ b/arch/x86/kernel/unwind_orc.c
@@ -471,7 +471,7 @@ bool unwind_next_frame(struct unwind_sta
 		break;
 
 	case ORC_REG_SP_INDIRECT:
-		sp = state->sp + orc->sp_offset;
+		sp = state->sp;
 		indirect = true;
 		break;
 
@@ -521,6 +521,9 @@ bool unwind_next_frame(struct unwind_sta
 	if (indirect) {
 		if (!deref_stack_reg(state, sp, &sp))
 			goto err;
+
+		if (orc->sp_reg == ORC_REG_SP_INDIRECT)
+			sp += orc->sp_offset;
 	}
 
 	/* Find IP, SP and possibly regs: */
--- a/tools/objtool/orc_dump.c
+++ b/tools/objtool/orc_dump.c
@@ -54,7 +54,7 @@ static void print_reg(unsigned int reg,
 	if (reg == ORC_REG_BP_INDIRECT)
 		printf("(bp%+d)", offset);
 	else if (reg == ORC_REG_SP_INDIRECT)
-		printf("(sp%+d)", offset);
+		printf("(sp)%+d", offset);
 	else if (reg == ORC_REG_UNDEFINED)
 		printf("(und)");
 	else

[PATCH 2/5] objtool: More consistent use of arch_callee_saved_reg()

[Peter Zijlstra]

Since save_regs() will only ever set a reg when it is
arch_callee_saved_reg() all the other regs will always be unused and
are effectively scratch space.

No point in comparing them.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 tools/objtool/check.c |    6 ++++++
 1 file changed, 6 insertions(+)

--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1725,6 +1725,9 @@ static bool has_modified_stack_frame(str
 		return false;
 
 	for (i = 0; i < CFI_NUM_REGS; i++) {
+		if (!arch_callee_saved_reg(i))
+			continue;
+
 		if (cfi->regs[i].base != initial_func_cfi.regs[i].base ||
 		    cfi->regs[i].offset != initial_func_cfi.regs[i].offset)
 			return true;
@@ -2248,6 +2251,9 @@ static bool insn_cfi_match(struct instru
 
 	} else if (memcmp(&cfi1->regs, &cfi2->regs, sizeof(cfi1->regs))) {
 		for (i = 0; i < CFI_NUM_REGS; i++) {
+			if (!arch_callee_saved_reg(i))
+				continue;
+
 			if (!memcmp(&cfi1->regs[i], &cfi2->regs[i],
 				    sizeof(struct cfi_reg)))
 				continue;

[PATCH 3/5] objtool: Prepare for scratch regs

[Peter Zijlstra]

Introduce __save_reg() which allows using the
!arch_callee_saved_reg()s and make sure they're wiped after every
stack op so they don't linger, allow for a single op exception so they
can be used on the very next stack-op.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 tools/objtool/check.c |   27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1775,15 +1775,20 @@ static int update_cfi_state_regs(struct
 	return 0;
 }
 
-static void save_reg(struct cfi_state *cfi, unsigned char reg, int base, int offset)
+static void __save_reg(struct cfi_state *cfi, unsigned char reg, int base, int offset)
 {
-	if (arch_callee_saved_reg(reg) &&
-	    cfi->regs[reg].base == CFI_UNDEFINED) {
+	if (cfi->regs[reg].base == CFI_UNDEFINED) {
 		cfi->regs[reg].base = base;
 		cfi->regs[reg].offset = offset;
 	}
 }
 
+static void save_reg(struct cfi_state *cfi, unsigned char reg, int base, int offset)
+{
+	if (arch_callee_saved_reg(reg))
+		__save_reg(cfi, reg, base, offset);
+}
+
 static void restore_reg(struct cfi_state *cfi, unsigned char reg)
 {
 	cfi->regs[reg].base = initial_func_cfi.regs[reg].base;
@@ -1848,6 +1853,7 @@ static int update_cfi_state(struct instr
 {
 	struct cfi_reg *cfa = &cfi->cfa;
 	struct cfi_reg *regs = cfi->regs;
+	bool skip_wipe = false;
 
 	/* stack operations don't make sense with an undefined CFA */
 	if (cfa->base == CFI_UNDEFINED) {
@@ -2192,6 +2198,21 @@ static int update_cfi_state(struct instr
 		return -1;
 	}
 
+	/*
+	 * Only callee saved registers are preserved; the rest is scratch space
+	 * preserved at most one instruction.
+	 */
+	if (!skip_wipe) {
+		int i;
+
+		for (i = 0; i < CFI_NUM_REGS; i++) {
+			if (arch_callee_saved_reg(i))
+				continue;
+
+			restore_reg(cfi, i);
+		}
+	}
+
 	return 0;
 }
 

[PATCH 4/5] objtool,x86: Additionally decode: mov %rsp, (%reg)

[Peter Zijlstra]

Where we already decode: mov %rsp, %reg, also decode mov %rsp, (%reg).

Nothing should match for this new stack-op.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 tools/objtool/arch/x86/decode.c |   23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -222,14 +222,24 @@ int arch_decode_instruction(const struct
 		break;
 
 	case 0x89:
-		if (rex_w && !rex_r && modrm_mod == 3 && modrm_reg == 4) {
+		if (rex_w && !rex_r && modrm_reg == 4) {
 
-			/* mov %rsp, reg */
+			/* mov %rsp, */
 			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];
+				if (modrm_mod == 3) {
+
+					/* mov %rsp, reg */
+					op->dest.type = OP_DEST_REG;
+					op->dest.reg = op_to_cfi_reg[modrm_rm][rex_b];
+
+				} else if (modrm_mod == 0) {
+
+					/* mov %rsp, (%reg) */
+					op->dest.type = OP_DEST_REG_INDIRECT;
+					op->dest.reg = op_to_cfi_reg[modrm_rm][rex_b];
+				}
 			}
 			break;
 		}
@@ -259,8 +269,10 @@ int arch_decode_instruction(const struct
 				op->dest.reg = CFI_BP;
 				op->dest.offset = insn.displacement.value;
 			}
+			break;
+		}
 
-		} else if (rex_w && !rex_b && modrm_rm == 4 && sib == 0x24) {
+		if (rex_w && !rex_b && modrm_rm == 4 && sib == 0x24) {
 
 			/* mov reg, disp(%rsp) */
 			ADD_OP(op) {
@@ -270,6 +282,7 @@ int arch_decode_instruction(const struct
 				op->dest.reg = CFI_SP;
 				op->dest.offset = insn.displacement.value;
 			}
+			break;
 		}
 
 		break;

[PATCH 5/5] objtool: Support stack-swizzle

[Peter Zijlstra]

Natively support the stack swizzle pattern:

	mov %rsp, (%[tos])
	mov %[tos], %rsp
	...
	pop %rsp

with the constraint that %[tos] must be !arch_callee_saved_reg().

It uses the (newly minted) scratch regs to link the first two
stack-ops, and detect the SP to SP_INDIRECT swizzle.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 tools/objtool/check.c |   43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1937,6 +1937,34 @@ static int update_cfi_state(struct instr
 					cfa->offset = -cfi->vals[op->src.reg].offset;
 					cfi->stack_size = cfa->offset;
 
+				} else if (cfa->base == CFI_SP &&
+					   cfi->regs[op->src.reg].base == CFI_SP_INDIRECT &&
+					   cfi->regs[op->src.reg].offset == cfa->offset) {
+
+					/*
+					 * Stack swizzle:
+					 *
+					 * 1: mov %rsp, (%[tos])
+					 * 2: mov %[tos], %rsp
+					 *    ...
+					 * 3: pop %rsp
+					 *
+					 * Where:
+					 *
+					 * 1 - places a pointer to the previous
+					 *     stack at the Top-of-Stack of the
+					 *     new stack.
+					 *
+					 * 2 - switches to the new stack.
+					 *
+					 * 3 - pops the Top-of-Stack to restore
+					 *     the original stack.
+					 *
+					 * Note:
+					 * %[tos] must not be a callee saved reg
+					 */
+					cfa->base = CFI_SP_INDIRECT;
+
 				} else {
 					cfa->base = CFI_UNDEFINED;
 					cfa->offset = 0;
@@ -2028,6 +2056,13 @@ static int update_cfi_state(struct instr
 
 		case OP_SRC_POP:
 		case OP_SRC_POPF:
+			if (op->dest.reg == CFI_SP && cfa->base == CFI_SP_INDIRECT) {
+
+				/* pop %rsp; # restore from a stack swizzle */
+				cfa->base = CFI_SP;
+				break;
+			}
+
 			if (!cfi->drap && op->dest.reg == cfa->base) {
 
 				/* pop %rbp */
@@ -2154,6 +2189,14 @@ static int update_cfi_state(struct instr
 			/* mov reg, disp(%rsp) */
 			save_reg(cfi, op->src.reg, CFI_CFA,
 				 op->dest.offset - cfi->cfa.offset);
+
+		} else if (op->src.reg == CFI_SP && op->dest.offset == 0) {
+
+			/* mov %rsp, (%reg); # setup a stack swizzle. */
+			if (!arch_callee_saved_reg(op->dest.reg)) {
+				__save_reg(cfi, op->dest.reg, CFI_SP_INDIRECT, cfi->cfa.offset);
+				skip_wipe = true;
+			}
 		}
 
 		break;

Re: [PATCH 1/5] objtool: Change REG_SP_INDIRECT

[Josh Poimboeuf]

On Wed, Feb 03, 2021 at 01:02:23PM +0100, Peter Zijlstra wrote:
> Currently REG_SP_INDIRECT is unused but means (%rsp + offset),
> change it to mean (%rsp) + offset.
> 
> This is somewhat unfortunate, since REG_BP_INDIRECT is used (by DRAP)
> and thus needs to retain the current (%rbp + offset).

Offset is going to be zero, should it not work either way?

-- 
Josh

Re: [PATCH 1/5] objtool: Change REG_SP_INDIRECT

[Peter Zijlstra]

On Wed, Feb 03, 2021 at 08:42:15AM -0600, Josh Poimboeuf wrote:
> On Wed, Feb 03, 2021 at 01:02:23PM +0100, Peter Zijlstra wrote:
> > Currently REG_SP_INDIRECT is unused but means (%rsp + offset),
> > change it to mean (%rsp) + offset.
> > 
> > This is somewhat unfortunate, since REG_BP_INDIRECT is used (by DRAP)
> > and thus needs to retain the current (%rbp + offset).
> 
> Offset is going to be zero, should it not work either way?

For DRAP? I couldn't tell in a hurry. I'm ever quite clear on how DRAP
works. Some day, when I figure it out, i'll write a comment.

Anyway, if it's always 0 for DRAP, then yes, I'll change both.

Re: [PATCH 1/5] objtool: Change REG_SP_INDIRECT

[Josh Poimboeuf]

On Wed, Feb 03, 2021 at 03:49:02PM +0100, Peter Zijlstra wrote:
> On Wed, Feb 03, 2021 at 08:42:15AM -0600, Josh Poimboeuf wrote:
> > On Wed, Feb 03, 2021 at 01:02:23PM +0100, Peter Zijlstra wrote:
> > > Currently REG_SP_INDIRECT is unused but means (%rsp + offset),
> > > change it to mean (%rsp) + offset.
> > > 
> > > This is somewhat unfortunate, since REG_BP_INDIRECT is used (by DRAP)
> > > and thus needs to retain the current (%rbp + offset).
> > 
> > Offset is going to be zero, should it not work either way?
> 
> For DRAP? I couldn't tell in a hurry. I'm ever quite clear on how DRAP
> works. Some day, when I figure it out, i'll write a comment.
> 
> Anyway, if it's always 0 for DRAP, then yes, I'll change both.

No, what I meant to say is that UNWIND_HINT_INDIRECT is used with no
arguments, which means that sp_offset is always zero.

-- 
Josh