Teach objtool a little more about IRET so that we can avoid using the
SAVE/RESTORE annotation. In particular, make the weird corner case in
insn->restore go away.
The purpose of that corner case is to deal with the fact that
UNWIND_HINT_RESTORE lands on the instruction after IRET, but that
instruction can end up being outside the basic block, consider:
if (cond)
sync_core()
foo();
Then the hint will land on foo(), and we'll encounter the restore
hint without ever having seen the save hint.
By teaching objtool about the arch specific exception frame size, and
assuming that any IRET in an STT_FUNC symbol is an exception frame
sized POP, we can remove the use of save/restore hints for this code.
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
---
arch/x86/include/asm/processor.h | 2 --
tools/objtool/arch.h | 1 +
tools/objtool/arch/x86/decode.c | 14 ++++++++++++--
tools/objtool/check.c | 29 ++++++++++++++++-------------
4 files changed, 29 insertions(+), 17 deletions(-)
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -727,7 +727,6 @@ static inline void sync_core(void)
unsigned int tmp;
asm volatile (
- UNWIND_HINT_SAVE
"mov %%ss, %0\n\t"
"pushq %q0\n\t"
"pushq %%rsp\n\t"
@@ -737,7 +736,6 @@ static inline void sync_core(void)
"pushq %q0\n\t"
"pushq $1f\n\t"
"iretq\n\t"
- UNWIND_HINT_RESTORE
"1:"
: "=&r" (tmp), ASM_CALL_CONSTRAINT : : "cc", "memory");
#endif
--- a/tools/objtool/arch.h
+++ b/tools/objtool/arch.h
@@ -19,6 +19,7 @@ enum insn_type {
INSN_CALL,
INSN_CALL_DYNAMIC,
INSN_RETURN,
+ INSN_EXCEPTION_RETURN,
INSN_CONTEXT_SWITCH,
INSN_STACK,
INSN_BUG,
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -435,9 +435,19 @@ int arch_decode_instruction(struct elf *
*type = INSN_RETURN;
break;
+ 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;
+ break;
+
case 0xca: /* retf */
case 0xcb: /* retf */
- case 0xcf: /* iret */
*type = INSN_CONTEXT_SWITCH;
break;
@@ -483,7 +493,7 @@ int arch_decode_instruction(struct elf *
*immediate = insn.immediate.nbytes ? insn.immediate.value : 0;
- if (*type == INSN_STACK)
+ if (*type == INSN_STACK || *type == INSN_EXCEPTION_RETURN)
list_add_tail(&op->list, ops_list);
else
free(op);
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2080,15 +2080,14 @@ static int validate_return(struct symbol
* tools/objtool/Documentation/stack-validation.txt.
*/
static int validate_branch(struct objtool_file *file, struct symbol *func,
- struct instruction *first, struct insn_state state)
+ struct instruction *insn, struct insn_state state)
{
struct alternative *alt;
- struct instruction *insn, *next_insn;
+ struct instruction *next_insn;
struct section *sec;
u8 visited;
int ret;
- insn = first;
sec = insn->sec;
if (insn->alt_group && list_empty(&insn->alts)) {
@@ -2141,16 +2140,6 @@ static int validate_branch(struct objtoo
}
if (!save_insn->visited) {
- /*
- * Oops, no state to copy yet.
- * Hopefully we can reach this
- * instruction from another branch
- * after the save insn has been
- * visited.
- */
- if (insn == first)
- return 0;
-
WARN_FUNC("objtool isn't smart enough to handle this CFI save/restore combo",
sec, insn->offset);
return 1;
@@ -2243,6 +2232,20 @@ static int validate_branch(struct objtoo
break;
+ case INSN_EXCEPTION_RETURN:
+ if (handle_insn_ops(insn, &state))
+ return 1;
+
+ /*
+ * This handles x86's sync_core() case, where we use an
+ * IRET to self. All 'normal' IRET instructions are in
+ * STT_NOTYPE entry symbols.
+ */
+ if (func)
+ break;
+
+ return 0;
+
case INSN_CONTEXT_SWITCH:
if (func && (!next_insn || !next_insn->hint)) {
WARN_FUNC("unsupported instruction in callable function",
On Thu, 16 Apr 2020, Peter Zijlstra wrote:
> Teach objtool a little more about IRET so that we can avoid using the
> SAVE/RESTORE annotation. In particular, make the weird corner case in
> insn->restore go away.
>
> The purpose of that corner case is to deal with the fact that
> UNWIND_HINT_RESTORE lands on the instruction after IRET, but that
> instruction can end up being outside the basic block, consider:
>
> if (cond)
> sync_core()
> foo();
>
> Then the hint will land on foo(), and we'll encounter the restore
> hint without ever having seen the save hint.
>
> By teaching objtool about the arch specific exception frame size, and
> assuming that any IRET in an STT_FUNC symbol is an exception frame
> sized POP, we can remove the use of save/restore hints for this code.
>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> ---
> arch/x86/include/asm/processor.h | 2 --
> tools/objtool/arch.h | 1 +
> tools/objtool/arch/x86/decode.c | 14 ++++++++++++--
> tools/objtool/check.c | 29 ++++++++++++++++-------------
> 4 files changed, 29 insertions(+), 17 deletions(-)
>
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -727,7 +727,6 @@ static inline void sync_core(void)
> unsigned int tmp;
>
> asm volatile (
> - UNWIND_HINT_SAVE
> "mov %%ss, %0\n\t"
> "pushq %q0\n\t"
> "pushq %%rsp\n\t"
> @@ -737,7 +736,6 @@ static inline void sync_core(void)
> "pushq %q0\n\t"
> "pushq $1f\n\t"
> "iretq\n\t"
> - UNWIND_HINT_RESTORE
> "1:"
> : "=&r" (tmp), ASM_CALL_CONSTRAINT : : "cc", "memory");
> #endif
> --- a/tools/objtool/arch.h
> +++ b/tools/objtool/arch.h
> @@ -19,6 +19,7 @@ enum insn_type {
> INSN_CALL,
> INSN_CALL_DYNAMIC,
> INSN_RETURN,
> + INSN_EXCEPTION_RETURN,
> INSN_CONTEXT_SWITCH,
> INSN_STACK,
> INSN_BUG,
> --- a/tools/objtool/arch/x86/decode.c
> +++ b/tools/objtool/arch/x86/decode.c
> @@ -435,9 +435,19 @@ int arch_decode_instruction(struct elf *
> *type = INSN_RETURN;
> break;
>
> + 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;
> + break;
> +
> case 0xca: /* retf */
> case 0xcb: /* retf */
> - case 0xcf: /* iret */
> *type = INSN_CONTEXT_SWITCH;
> break;
>
> @@ -483,7 +493,7 @@ int arch_decode_instruction(struct elf *
>
> *immediate = insn.immediate.nbytes ? insn.immediate.value : 0;
>
> - if (*type == INSN_STACK)
> + if (*type == INSN_STACK || *type == INSN_EXCEPTION_RETURN)
> list_add_tail(&op->list, ops_list);
> else
> free(op);
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -2080,15 +2080,14 @@ static int validate_return(struct symbol
> * tools/objtool/Documentation/stack-validation.txt.
> */
> static int validate_branch(struct objtool_file *file, struct symbol *func,
> - struct instruction *first, struct insn_state state)
> + struct instruction *insn, struct insn_state state)
> {
> struct alternative *alt;
> - struct instruction *insn, *next_insn;
> + struct instruction *next_insn;
> struct section *sec;
> u8 visited;
> int ret;
>
> - insn = first;
> sec = insn->sec;
>
> if (insn->alt_group && list_empty(&insn->alts)) {
> @@ -2141,16 +2140,6 @@ static int validate_branch(struct objtoo
> }
>
> if (!save_insn->visited) {
> - /*
> - * Oops, no state to copy yet.
> - * Hopefully we can reach this
> - * instruction from another branch
> - * after the save insn has been
> - * visited.
> - */
> - if (insn == first)
> - return 0;
> -
> WARN_FUNC("objtool isn't smart enough to handle this CFI save/restore combo",
> sec, insn->offset);
> return 1;
> @@ -2243,6 +2232,20 @@ static int validate_branch(struct objtoo
>
> break;
>
> + case INSN_EXCEPTION_RETURN:
> + if (handle_insn_ops(insn, &state))
> + return 1;
> +
> + /*
> + * This handles x86's sync_core() case, where we use an
> + * IRET to self. All 'normal' IRET instructions are in
> + * STT_NOTYPE entry symbols.
> + */
> + if (func)
> + break;
> +
> + return 0;
> +
> case INSN_CONTEXT_SWITCH:
> if (func && (!next_insn || !next_insn->hint)) {
> WARN_FUNC("unsupported instruction in callable function",
It looks really simple.
Have you tried Julien's proposal about removing INSN_STACK altogether,
move the x86 to arch/x86/ and call handle_insn_ops() unconditionally, or
have you just postponed it? As I said, I think it could be better in the
long term, but the above looks good for now as well.
Miroslav
On Fri, Apr 17, 2020 at 01:29:32PM +0200, Miroslav Benes wrote:
> On Thu, 16 Apr 2020, Peter Zijlstra wrote:
> > + case INSN_EXCEPTION_RETURN:
> > + if (handle_insn_ops(insn, &state))
> > + return 1;
> > +
> > + /*
> > + * This handles x86's sync_core() case, where we use an
> > + * IRET to self. All 'normal' IRET instructions are in
> > + * STT_NOTYPE entry symbols.
> > + */
> > + if (func)
> > + break;
> > +
> > + return 0;
> > +
> > case INSN_CONTEXT_SWITCH:
> > if (func && (!next_insn || !next_insn->hint)) {
> > WARN_FUNC("unsupported instruction in callable function",
>
> It looks really simple.
>
> Have you tried Julien's proposal about removing INSN_STACK altogether,
> move the x86 to arch/x86/ and call handle_insn_ops() unconditionally, or
> have you just postponed it? As I said, I think it could be better in the
> long term, but the above looks good for now as well.
If you look at this other set I send yesterday:
https://lkml.kernel.org/r/[email protected]
(also, sorry for not adding you to the Cc; also best look at the gitweb
version, the patches I send out are missing a hunk and lacking some
back-merges.. clearly I wasn't having a good day yesterday).
it has this intra_function_calls crud that needs explicit conditional
handle_insn_ops().
On Fri, 17 Apr 2020, Peter Zijlstra wrote:
> On Fri, Apr 17, 2020 at 01:29:32PM +0200, Miroslav Benes wrote:
> > On Thu, 16 Apr 2020, Peter Zijlstra wrote:
>
> > > + case INSN_EXCEPTION_RETURN:
> > > + if (handle_insn_ops(insn, &state))
> > > + return 1;
> > > +
> > > + /*
> > > + * This handles x86's sync_core() case, where we use an
> > > + * IRET to self. All 'normal' IRET instructions are in
> > > + * STT_NOTYPE entry symbols.
> > > + */
> > > + if (func)
> > > + break;
> > > +
> > > + return 0;
> > > +
> > > case INSN_CONTEXT_SWITCH:
> > > if (func && (!next_insn || !next_insn->hint)) {
> > > WARN_FUNC("unsupported instruction in callable function",
> >
> > It looks really simple.
> >
> > Have you tried Julien's proposal about removing INSN_STACK altogether,
> > move the x86 to arch/x86/ and call handle_insn_ops() unconditionally, or
> > have you just postponed it? As I said, I think it could be better in the
> > long term, but the above looks good for now as well.
>
> If you look at this other set I send yesterday:
>
> https://lkml.kernel.org/r/[email protected]
>
> (also, sorry for not adding you to the Cc; also best look at the gitweb
> version, the patches I send out are missing a hunk and lacking some
> back-merges.. clearly I wasn't having a good day yesterday).
>
> it has this intra_function_calls crud that needs explicit conditional
> handle_insn_ops().
Ah, ok. Thanks for letting me know. There are so many patches for objtool
flying around now that it is easy to miss something.
Miroslav
On 4/16/20 1:47 PM, Peter Zijlstra wrote:
> Teach objtool a little more about IRET so that we can avoid using the
> SAVE/RESTORE annotation. In particular, make the weird corner case in
> insn->restore go away.
>
> The purpose of that corner case is to deal with the fact that
> UNWIND_HINT_RESTORE lands on the instruction after IRET, but that
> instruction can end up being outside the basic block, consider:
>
> if (cond)
> sync_core()
> foo();
>
> Then the hint will land on foo(), and we'll encounter the restore
> hint without ever having seen the save hint.
>
> By teaching objtool about the arch specific exception frame size, and
> assuming that any IRET in an STT_FUNC symbol is an exception frame
> sized POP, we can remove the use of save/restore hints for this code.
>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> ---
> arch/x86/include/asm/processor.h | 2 --
> tools/objtool/arch.h | 1 +
> tools/objtool/arch/x86/decode.c | 14 ++++++++++++--
> tools/objtool/check.c | 29 ++++++++++++++++-------------
> 4 files changed, 29 insertions(+), 17 deletions(-)
>
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -727,7 +727,6 @@ static inline void sync_core(void)
> unsigned int tmp;
>
> asm volatile (
> - UNWIND_HINT_SAVE
> "mov %%ss, %0\n\t"
> "pushq %q0\n\t"
> "pushq %%rsp\n\t"
> @@ -737,7 +736,6 @@ static inline void sync_core(void)
> "pushq %q0\n\t"
> "pushq $1f\n\t"
> "iretq\n\t"
> - UNWIND_HINT_RESTORE
> "1:"
> : "=&r" (tmp), ASM_CALL_CONSTRAINT : : "cc", "memory");
> #endif
> --- a/tools/objtool/arch.h
> +++ b/tools/objtool/arch.h
> @@ -19,6 +19,7 @@ enum insn_type {
> INSN_CALL,
> INSN_CALL_DYNAMIC,
> INSN_RETURN,
> + INSN_EXCEPTION_RETURN,
> INSN_CONTEXT_SWITCH,
> INSN_STACK,
> INSN_BUG,
> --- a/tools/objtool/arch/x86/decode.c
> +++ b/tools/objtool/arch/x86/decode.c
> @@ -435,9 +435,19 @@ int arch_decode_instruction(struct elf *
> *type = INSN_RETURN;
> break;
>
> + 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;
> + break;
> +
> case 0xca: /* retf */
> case 0xcb: /* retf */
> - case 0xcf: /* iret */
> *type = INSN_CONTEXT_SWITCH;
> break;
>
> @@ -483,7 +493,7 @@ int arch_decode_instruction(struct elf *
>
> *immediate = insn.immediate.nbytes ? insn.immediate.value : 0;
>
> - if (*type == INSN_STACK)
> + if (*type == INSN_STACK || *type == INSN_EXCEPTION_RETURN)
> list_add_tail(&op->list, ops_list);
> else
> free(op);
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -2080,15 +2080,14 @@ static int validate_return(struct symbol
> * tools/objtool/Documentation/stack-validation.txt.
> */
> static int validate_branch(struct objtool_file *file, struct symbol *func,
> - struct instruction *first, struct insn_state state)
> + struct instruction *insn, struct insn_state state)
> {
> struct alternative *alt;
> - struct instruction *insn, *next_insn;
> + struct instruction *next_insn;
> struct section *sec;
> u8 visited;
> int ret;
>
> - insn = first;
> sec = insn->sec;
>
> if (insn->alt_group && list_empty(&insn->alts)) {
> @@ -2141,16 +2140,6 @@ static int validate_branch(struct objtoo
> }
>
> if (!save_insn->visited) {
> - /*
> - * Oops, no state to copy yet.
> - * Hopefully we can reach this
> - * instruction from another branch
> - * after the save insn has been
> - * visited.
> - */
> - if (insn == first)
> - return 0;
> -
> WARN_FUNC("objtool isn't smart enough to handle this CFI save/restore combo",
> sec, insn->offset);
> return 1;
> @@ -2243,6 +2232,20 @@ static int validate_branch(struct objtoo
>
> break;
>
> + case INSN_EXCEPTION_RETURN:
> + if (handle_insn_ops(insn, &state))
> + return 1;
Do we need to update the stack state for normal IRET? This wasn't done before.
So shouldn't this better be:
case INSN_EXCEPTION_RETURN:
if (!func)
return 0;
if (handle_insn_ops(insn, &state))
return 1;
break;
> +
> + /*
> + * This handles x86's sync_core() case, where we use an
> + * IRET to self. All 'normal' IRET instructions are in
> + * STT_NOTYPE entry symbols.
> + */
> + if (func)
> + break;
Is it worth checking that func->name is effectively "sync_core"?
alex.
> +
> + return 0;
> +
> case INSN_CONTEXT_SWITCH:
> if (func && (!next_insn || !next_insn->hint)) {
> WARN_FUNC("unsupported instruction in callable function",
>
>
On Fri, Apr 17, 2020 at 07:37:32PM +0200, Alexandre Chartre wrote:
> > @@ -2243,6 +2232,20 @@ static int validate_branch(struct objtoo
> > break;
> > + case INSN_EXCEPTION_RETURN:
> > + if (handle_insn_ops(insn, &state))
> > + return 1;
>
> Do we need to update the stack state for normal IRET? This wasn't done before.
> So shouldn't this better be:
>
> case INSN_EXCEPTION_RETURN:
> if (!func)
> return 0;
>
> if (handle_insn_ops(insn, &state))
> return 1;
>
> break;
Well, I was going to do the unconditional handle_insn_ops(), like
mentioned, but then that intra_function_call thing spoiled it.
It doesn't matter though; STT_NOTYPE doesn't care.
> > +
> > + /*
> > + * This handles x86's sync_core() case, where we use an
> > + * IRET to self. All 'normal' IRET instructions are in
> > + * STT_NOTYPE entry symbols.
> > + */
> > + if (func)
> > + break;
>
> Is it worth checking that func->name is effectively "sync_core"?
It's an inline..
On Fri, Apr 17, 2020 at 11:23 AM Peter Zijlstra <[email protected]> wrote:
>
> On Fri, Apr 17, 2020 at 07:37:32PM +0200, Alexandre Chartre wrote:
> > > @@ -2243,6 +2232,20 @@ static int validate_branch(struct objtoo
> > > break;
> > > + case INSN_EXCEPTION_RETURN:
> > > + if (handle_insn_ops(insn, &state))
> > > + return 1;
> >
> > Do we need to update the stack state for normal IRET? This wasn't done before.
> > So shouldn't this better be:
> >
> > case INSN_EXCEPTION_RETURN:
> > if (!func)
> > return 0;
> >
> > if (handle_insn_ops(insn, &state))
> > return 1;
> >
> > break;
>
> Well, I was going to do the unconditional handle_insn_ops(), like
> mentioned, but then that intra_function_call thing spoiled it.
>
> It doesn't matter though; STT_NOTYPE doesn't care.
>
> > > +
> > > + /*
> > > + * This handles x86's sync_core() case, where we use an
> > > + * IRET to self. All 'normal' IRET instructions are in
> > > + * STT_NOTYPE entry symbols.
> > > + */
> > > + if (func)
> > > + break;
> >
> > Is it worth checking that func->name is effectively "sync_core"?
>
> It's an inline..
I'm wondering if this would be easier if we just moved the guts of
sync_core() into asm.
In the near future, I think we want to rework it a tiny bit. In
particular, I think we're going to want to make sync_core() do:
if (static_cpu_has(X86_FEATURE_SERIALIZE))
asm volatile ("serialize");
else
iret_to_self();
where iret_to_self() is the meat of the IRET hack. And then we're
going to add a new thingy unmask_nmi() that does iret_to_self() on
everything except SEV-ES. The near-term motivation is that I think we
have some genuine bugs in a couple of corner cases:
1. On AMD chips, if NMI hits user code with invalid CS or SS, we will
enter on the NMI stack, switch to the normal stack, and return with
IRET, and the IRET will fail. And then we end up in a nasty state in
which NMIs are masked but the code path we run doesn't expect that.
So we should unmask_nmi() in fixup_bad_iret() or similar. Intel CPUs
are unaffected because Intel is differently quirky.
2. do_nmi() does this:
if (user_mode(regs))
mds_user_clear_cpu_buffers();
because it can't safely call prepare_exit_to_usermode(). This is a
gross wart and I'd like to fix it. Fixing it involves teaching the
relevant code paths to unmask_nmis() if they're going to so IRQs-on
exit work.
None of this is really relevant to the current patch, but it wouldn't
be unreasonable to turn the IRET thing from an inline asm into a real
asm function if it makes objtool's life easier.
On Fri, Apr 17, 2020 at 04:53:31PM -0700, Andy Lutomirski wrote:
> I'm wondering if this would be easier if we just moved the guts of
> sync_core() into asm.
>
> In the near future, I think we want to rework it a tiny bit. In
> particular, I think we're going to want to make sync_core() do:
>
> if (static_cpu_has(X86_FEATURE_SERIALIZE))
> asm volatile ("serialize");
> else
> iret_to_self();
>
> where iret_to_self() is the meat of the IRET hack. And then we're
> going to add a new thingy unmask_nmi() that does iret_to_self() on
> everything except SEV-ES. The near-term motivation is that I think we
> have some genuine bugs in a couple of corner cases:
>
> 1. On AMD chips, if NMI hits user code with invalid CS or SS, we will
> enter on the NMI stack, switch to the normal stack, and return with
> IRET, and the IRET will fail. And then we end up in a nasty state in
> which NMIs are masked but the code path we run doesn't expect that.
> So we should unmask_nmi() in fixup_bad_iret() or similar. Intel CPUs
> are unaffected because Intel is differently quirky.
>
> 2. do_nmi() does this:
>
> if (user_mode(regs))
> mds_user_clear_cpu_buffers();
>
> because it can't safely call prepare_exit_to_usermode(). This is a
> gross wart and I'd like to fix it. Fixing it involves teaching the
> relevant code paths to unmask_nmis() if they're going to so IRQs-on
> exit work.
>
> None of this is really relevant to the current patch, but it wouldn't
> be unreasonable to turn the IRET thing from an inline asm into a real
> asm function if it makes objtool's life easier.
I don't think that would make objtool's life any easier -- it still has
to understand the stack impact of the IRET-to-self thing regardless.
--
Josh
The following commit has been merged into the objtool/core branch of tip:
Commit-ID: 016db2d9c63e3ef0e7c3776efb38f352053cdd1e
Gitweb: https://git.kernel.org/tip/016db2d9c63e3ef0e7c3776efb38f352053cdd1e
Author: Peter Zijlstra <[email protected]>
AuthorDate: Thu, 02 Apr 2020 10:15:51 +02:00
Committer: Peter Zijlstra <[email protected]>
CommitterDate: Wed, 22 Apr 2020 23:10:05 +02:00
objtool: Better handle IRET
Teach objtool a little more about IRET so that we can avoid using the
SAVE/RESTORE annotation. In particular, make the weird corner case in
insn->restore go away.
The purpose of that corner case is to deal with the fact that
UNWIND_HINT_RESTORE lands on the instruction after IRET, but that
instruction can end up being outside the basic block, consider:
if (cond)
sync_core()
foo();
Then the hint will land on foo(), and we'll encounter the restore
hint without ever having seen the save hint.
By teaching objtool about the arch specific exception frame size, and
assuming that any IRET in an STT_FUNC symbol is an exception frame
sized POP, we can remove the use of save/restore hints for this code.
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Reviewed-by: Miroslav Benes <[email protected]>
Reviewed-by: Alexandre Chartre <[email protected]>
Acked-by: Josh Poimboeuf <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
arch/x86/include/asm/processor.h | 2 --
tools/objtool/arch.h | 1 +
tools/objtool/arch/x86/decode.c | 14 ++++++++++++--
tools/objtool/check.c | 29 ++++++++++++++++-------------
4 files changed, 29 insertions(+), 17 deletions(-)
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 3bcf27c..3eeaaeb 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -727,7 +727,6 @@ static inline void sync_core(void)
unsigned int tmp;
asm volatile (
- UNWIND_HINT_SAVE
"mov %%ss, %0\n\t"
"pushq %q0\n\t"
"pushq %%rsp\n\t"
@@ -737,7 +736,6 @@ static inline void sync_core(void)
"pushq %q0\n\t"
"pushq $1f\n\t"
"iretq\n\t"
- UNWIND_HINT_RESTORE
"1:"
: "=&r" (tmp), ASM_CALL_CONSTRAINT : : "cc", "memory");
#endif
diff --git a/tools/objtool/arch.h b/tools/objtool/arch.h
index f9883c4..55396df 100644
--- a/tools/objtool/arch.h
+++ b/tools/objtool/arch.h
@@ -19,6 +19,7 @@ enum insn_type {
INSN_CALL,
INSN_CALL_DYNAMIC,
INSN_RETURN,
+ INSN_EXCEPTION_RETURN,
INSN_CONTEXT_SWITCH,
INSN_STACK,
INSN_BUG,
diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
index 199b408..3273638 100644
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -446,9 +446,19 @@ int arch_decode_instruction(struct elf *elf, struct section *sec,
*type = INSN_RETURN;
break;
+ 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;
+ break;
+
case 0xca: /* retf */
case 0xcb: /* retf */
- case 0xcf: /* iret */
*type = INSN_CONTEXT_SWITCH;
break;
@@ -494,7 +504,7 @@ int arch_decode_instruction(struct elf *elf, struct section *sec,
*immediate = insn.immediate.nbytes ? insn.immediate.value : 0;
- if (*type == INSN_STACK)
+ if (*type == INSN_STACK || *type == INSN_EXCEPTION_RETURN)
list_add_tail(&op->list, ops_list);
else
free(op);
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 819de0d..72bf5cc 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2081,15 +2081,14 @@ static int validate_return(struct symbol *func, struct instruction *insn, struct
* tools/objtool/Documentation/stack-validation.txt.
*/
static int validate_branch(struct objtool_file *file, struct symbol *func,
- struct instruction *first, struct insn_state state)
+ struct instruction *insn, struct insn_state state)
{
struct alternative *alt;
- struct instruction *insn, *next_insn;
+ struct instruction *next_insn;
struct section *sec;
u8 visited;
int ret;
- insn = first;
sec = insn->sec;
if (insn->alt_group && list_empty(&insn->alts)) {
@@ -2142,16 +2141,6 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
}
if (!save_insn->visited) {
- /*
- * Oops, no state to copy yet.
- * Hopefully we can reach this
- * instruction from another branch
- * after the save insn has been
- * visited.
- */
- if (insn == first)
- return 0;
-
WARN_FUNC("objtool isn't smart enough to handle this CFI save/restore combo",
sec, insn->offset);
return 1;
@@ -2244,6 +2233,20 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
break;
+ case INSN_EXCEPTION_RETURN:
+ if (handle_insn_ops(insn, &state))
+ return 1;
+
+ /*
+ * This handles x86's sync_core() case, where we use an
+ * IRET to self. All 'normal' IRET instructions are in
+ * STT_NOTYPE entry symbols.
+ */
+ if (func)
+ break;
+
+ return 0;
+
case INSN_CONTEXT_SWITCH:
if (func && (!next_insn || !next_insn->hint)) {
WARN_FUNC("unsupported instruction in callable function",
The following commit has been merged into the objtool/core branch of tip:
Commit-ID: b746046238bb99b8f703c79f6d95357428fb6476
Gitweb: https://git.kernel.org/tip/b746046238bb99b8f703c79f6d95357428fb6476
Author: Peter Zijlstra <[email protected]>
AuthorDate: Thu, 02 Apr 2020 10:15:51 +02:00
Committer: Ingo Molnar <[email protected]>
CommitterDate: Wed, 22 Apr 2020 10:53:50 +02:00
objtool: Better handle IRET
Teach objtool a little more about IRET so that we can avoid using the
SAVE/RESTORE annotation. In particular, make the weird corner case in
insn->restore go away.
The purpose of that corner case is to deal with the fact that
UNWIND_HINT_RESTORE lands on the instruction after IRET, but that
instruction can end up being outside the basic block, consider:
if (cond)
sync_core()
foo();
Then the hint will land on foo(), and we'll encounter the restore
hint without ever having seen the save hint.
By teaching objtool about the arch specific exception frame size, and
assuming that any IRET in an STT_FUNC symbol is an exception frame
sized POP, we can remove the use of save/restore hints for this code.
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Reviewed-by: Miroslav Benes <[email protected]>
Reviewed-by: Alexandre Chartre <[email protected]>
Acked-by: Josh Poimboeuf <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
arch/x86/include/asm/processor.h | 2 --
tools/objtool/arch.h | 1 +
tools/objtool/arch/x86/decode.c | 14 ++++++++++++--
tools/objtool/check.c | 29 ++++++++++++++++-------------
4 files changed, 29 insertions(+), 17 deletions(-)
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 3bcf27c..3eeaaeb 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -727,7 +727,6 @@ static inline void sync_core(void)
unsigned int tmp;
asm volatile (
- UNWIND_HINT_SAVE
"mov %%ss, %0\n\t"
"pushq %q0\n\t"
"pushq %%rsp\n\t"
@@ -737,7 +736,6 @@ static inline void sync_core(void)
"pushq %q0\n\t"
"pushq $1f\n\t"
"iretq\n\t"
- UNWIND_HINT_RESTORE
"1:"
: "=&r" (tmp), ASM_CALL_CONSTRAINT : : "cc", "memory");
#endif
diff --git a/tools/objtool/arch.h b/tools/objtool/arch.h
index f9883c4..55396df 100644
--- a/tools/objtool/arch.h
+++ b/tools/objtool/arch.h
@@ -19,6 +19,7 @@ enum insn_type {
INSN_CALL,
INSN_CALL_DYNAMIC,
INSN_RETURN,
+ INSN_EXCEPTION_RETURN,
INSN_CONTEXT_SWITCH,
INSN_STACK,
INSN_BUG,
diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
index 199b408..3273638 100644
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -446,9 +446,19 @@ int arch_decode_instruction(struct elf *elf, struct section *sec,
*type = INSN_RETURN;
break;
+ 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;
+ break;
+
case 0xca: /* retf */
case 0xcb: /* retf */
- case 0xcf: /* iret */
*type = INSN_CONTEXT_SWITCH;
break;
@@ -494,7 +504,7 @@ int arch_decode_instruction(struct elf *elf, struct section *sec,
*immediate = insn.immediate.nbytes ? insn.immediate.value : 0;
- if (*type == INSN_STACK)
+ if (*type == INSN_STACK || *type == INSN_EXCEPTION_RETURN)
list_add_tail(&op->list, ops_list);
else
free(op);
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 9e854fd..781b3a3 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2065,15 +2065,14 @@ static int validate_return(struct symbol *func, struct instruction *insn, struct
* tools/objtool/Documentation/stack-validation.txt.
*/
static int validate_branch(struct objtool_file *file, struct symbol *func,
- struct instruction *first, struct insn_state state)
+ struct instruction *insn, struct insn_state state)
{
struct alternative *alt;
- struct instruction *insn, *next_insn;
+ struct instruction *next_insn;
struct section *sec;
u8 visited;
int ret;
- insn = first;
sec = insn->sec;
if (insn->alt_group && list_empty(&insn->alts)) {
@@ -2126,16 +2125,6 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
}
if (!save_insn->visited) {
- /*
- * Oops, no state to copy yet.
- * Hopefully we can reach this
- * instruction from another branch
- * after the save insn has been
- * visited.
- */
- if (insn == first)
- return 0;
-
WARN_FUNC("objtool isn't smart enough to handle this CFI save/restore combo",
sec, insn->offset);
return 1;
@@ -2228,6 +2217,20 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
break;
+ case INSN_EXCEPTION_RETURN:
+ if (handle_insn_ops(insn, &state))
+ return 1;
+
+ /*
+ * This handles x86's sync_core() case, where we use an
+ * IRET to self. All 'normal' IRET instructions are in
+ * STT_NOTYPE entry symbols.
+ */
+ if (func)
+ break;
+
+ return 0;
+
case INSN_CONTEXT_SWITCH:
if (func && (!next_insn || !next_insn->hint)) {
WARN_FUNC("unsupported instruction in callable function",