There is a special case in the UNWIND_HINT_RESTORE code. When, upon
looking for the UNWIND_HINT_SAVE instruction to restore from, it finds
the instruction hasn't been visited yet, it normally issues a WARN,
except when this HINT_SAVE instruction is the first instruction of
this branch.
This special case is of dubious correctness and is certainly unused
(verified with an allmodconfig build), the two sites that employ
UNWIND_HINT_SAVE/RESTORE (sync_core() and ftrace_regs_caller()) have
the SAVE on unconditional instructions at the start of the function.
It is therefore impossible for the save_insn not to have been visited
when we do hit the RESTORE.
Remove this.
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Acked-by: Josh Poimboeuf <[email protected]>
---
tools/objtool/check.c | 15 ++-------------
1 file changed, 2 insertions(+), 13 deletions(-)
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2040,15 +2040,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 instruction *next_insn;
struct alternative *alt;
- struct instruction *insn, *next_insn;
struct section *sec;
u8 visited;
int ret;
- insn = first;
sec = insn->sec;
if (insn->alt_group && list_empty(&insn->alts)) {
@@ -2101,16 +2100,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;
On Wed, Mar 25, 2020 at 06:45:26PM +0100, Peter Zijlstra wrote:
> There is a special case in the UNWIND_HINT_RESTORE code. When, upon
> looking for the UNWIND_HINT_SAVE instruction to restore from, it finds
> the instruction hasn't been visited yet, it normally issues a WARN,
> except when this HINT_SAVE instruction is the first instruction of
> this branch.
>
> This special case is of dubious correctness and is certainly unused
> (verified with an allmodconfig build), the two sites that employ
> UNWIND_HINT_SAVE/RESTORE (sync_core() and ftrace_regs_caller()) have
> the SAVE on unconditional instructions at the start of the function.
> It is therefore impossible for the save_insn not to have been visited
> when we do hit the RESTORE.
Clearly I was too tired when I did that allmodconfig build, because it
very much does generate a warning :-/.
Thank you 0day:
kernel/sched/core.o: warning: objtool: finish_task_switch()+0x1c0: objtool isn't smart enough to handle this CFI save/restore combo
At least this gives clue as to what it was trying to do.
---
Subject: objtool: Remove CFI save/restore special case
From: Peter Zijlstra <[email protected]>
Date: Wed Mar 25 12:58:16 CET 2020
There is a special case in the UNWIND_HINT_RESTORE code. When, upon
looking for the UNWIND_HINT_SAVE instruction to restore from, it finds
the instruction hasn't been visited yet, it normally issues a WARN,
except when this HINT_SAVE instruction is the first instruction of
this branch.
The reason for this special case comes apparent when we remove it;
code like:
if (cond) {
UNWIND_HINT_SAVE
// do stuff
UNWIND_HINT_RESTORE
}
// more stuff
will now trigger the warning. This is because UNWIND_HINT_RESTORE is
just a label, and there is nothing keeping it inside the (extended)
basic block covered by @cond. It will attach itself to the first
instruction of 'more stuff' and we'll hit it outside of the @cond,
confusing things.
I don't much like this special case, it confuses things and will come
apart horribly if/when the annotation needs to support nesting.
Instead extend the affected code to at least form an extended basic
block.
In particular, of the 2 users of this annotation: ftrace_regs_caller()
and sync_core(), only the latter suffers this problem. Extend it's
code sequence with a NOP to make it an extended basic block.
This isn't ideal either; stuffing code with NOPs just to make
annotations work is certainly sub-optimal, but given that sync_core()
is stupid expensive in any case, one extra nop isn't going to be a
problem here.
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
---
arch/x86/include/asm/processor.h | 9 ++++++++-
tools/objtool/check.c | 15 ++-------------
2 files changed, 10 insertions(+), 14 deletions(-)
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -727,6 +727,13 @@ static inline void sync_core(void)
#else
unsigned int tmp;
+ /*
+ * The trailing NOP is required to make this an extended basic block,
+ * such that we can argue about it locally. Specifically this is
+ * important for the UNWIND_HINTs, without this the UNWIND_HINT_RESTORE
+ * can fall outside our extended basic block and objtool gets
+ * (rightfully) confused.
+ */
asm volatile (
UNWIND_HINT_SAVE
"mov %%ss, %0\n\t"
@@ -739,7 +746,7 @@ static inline void sync_core(void)
"pushq $1f\n\t"
"iretq\n\t"
UNWIND_HINT_RESTORE
- "1:"
+ "1: nop\n\t"
: "=&r" (tmp), ASM_CALL_CONSTRAINT : : "cc", "memory");
#endif
}
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2000,15 +2000,14 @@ static int validate_sibling_call(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 instruction *next_insn;
struct alternative *alt;
- struct instruction *insn, *next_insn;
struct section *sec;
u8 visited;
int ret;
- insn = first;
sec = insn->sec;
if (insn->alt_group && list_empty(&insn->alts)) {
@@ -2061,16 +2060,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;
On Thu, Mar 26, 2020 at 12:30:50PM +0100, Peter Zijlstra wrote:
> There is a special case in the UNWIND_HINT_RESTORE code. When, upon
> looking for the UNWIND_HINT_SAVE instruction to restore from, it finds
> the instruction hasn't been visited yet, it normally issues a WARN,
> except when this HINT_SAVE instruction is the first instruction of
> this branch.
>
> The reason for this special case comes apparent when we remove it;
> code like:
>
> if (cond) {
> UNWIND_HINT_SAVE
> // do stuff
> UNWIND_HINT_RESTORE
> }
> // more stuff
>
> will now trigger the warning. This is because UNWIND_HINT_RESTORE is
> just a label, and there is nothing keeping it inside the (extended)
> basic block covered by @cond. It will attach itself to the first
> instruction of 'more stuff' and we'll hit it outside of the @cond,
> confusing things.
>
> I don't much like this special case, it confuses things and will come
> apart horribly if/when the annotation needs to support nesting.
> Instead extend the affected code to at least form an extended basic
> block.
>
> In particular, of the 2 users of this annotation: ftrace_regs_caller()
> and sync_core(), only the latter suffers this problem. Extend it's
> code sequence with a NOP to make it an extended basic block.
>
> This isn't ideal either; stuffing code with NOPs just to make
> annotations work is certainly sub-optimal, but given that sync_core()
> is stupid expensive in any case, one extra nop isn't going to be a
> problem here.
So instr_begin() / instr_end() have this exact problem, but worse. Those
actually do nest and I've ran into the following situation:
if (cond1) {
instr_begin();
// code1
instr_end();
}
// code
if (cond2) {
instr_begin();
// code2
instr_end();
}
// tail
Where objtool then finds the path: !cond1, cond2, which ends up at code2
with 0, instead of 1.
I've also seen:
if (cond) {
instr_begin();
// code1
instr_end();
}
instr_begin();
// code2
instr_end();
Where instr_end() and instr_begin() merge onto the same instruction of
code2 as a 0, and again code2 will issue a false warning.
You can also not make objtool lift the end marker to the previous
instruction, because then:
if (cond1) {
instr_begin();
if (cond2) {
// code2
}
instr_end();
}
Suffers the reverse problem, instr_end() becomes part of the @cond2
block and cond1 grows a path that misses it entirely.
So far I've not had any actual solution except adding a NOP to anchor
the annotation on.
On Thu, Mar 26, 2020 at 12:30:50PM +0100, Peter Zijlstra wrote:
> Subject: objtool: Remove CFI save/restore special case
> From: Peter Zijlstra <[email protected]>
> Date: Wed Mar 25 12:58:16 CET 2020
>
> There is a special case in the UNWIND_HINT_RESTORE code. When, upon
> looking for the UNWIND_HINT_SAVE instruction to restore from, it finds
> the instruction hasn't been visited yet, it normally issues a WARN,
> except when this HINT_SAVE instruction is the first instruction of
> this branch.
>
> The reason for this special case comes apparent when we remove it;
> code like:
>
> if (cond) {
> UNWIND_HINT_SAVE
> // do stuff
> UNWIND_HINT_RESTORE
> }
> // more stuff
>
> will now trigger the warning. This is because UNWIND_HINT_RESTORE is
> just a label, and there is nothing keeping it inside the (extended)
> basic block covered by @cond. It will attach itself to the first
> instruction of 'more stuff' and we'll hit it outside of the @cond,
> confusing things.
>
> I don't much like this special case, it confuses things and will come
> apart horribly if/when the annotation needs to support nesting.
> Instead extend the affected code to at least form an extended basic
> block.
> @@ -727,6 +727,13 @@ static inline void sync_core(void)
> #else
> unsigned int tmp;
>
> + /*
> + * The trailing NOP is required to make this an extended basic block,
> + * such that we can argue about it locally. Specifically this is
> + * important for the UNWIND_HINTs, without this the UNWIND_HINT_RESTORE
> + * can fall outside our extended basic block and objtool gets
> + * (rightfully) confused.
> + */
> asm volatile (
> UNWIND_HINT_SAVE
> "mov %%ss, %0\n\t"
> @@ -739,7 +746,7 @@ static inline void sync_core(void)
> "pushq $1f\n\t"
> "iretq\n\t"
> UNWIND_HINT_RESTORE
> - "1:"
> + "1: nop\n\t"
> : "=&r" (tmp), ASM_CALL_CONSTRAINT : : "cc", "memory");
Note that the special case very much relies on the HINT_SAVE being the
first instruction of the (extended) basic block, which is only true in
this one usage anyway.
On Thu, Mar 26, 2020 at 01:58:44PM +0100, Peter Zijlstra wrote:
> So instr_begin() / instr_end() have this exact problem, but worse. Those
> actually do nest and I've ran into the following situation:
>
> if (cond1) {
> instr_begin();
> // code1
> instr_end();
> }
> // code
>
> if (cond2) {
> instr_begin();
> // code2
> instr_end();
> }
> // tail
>
> Where objtool then finds the path: !cond1, cond2, which ends up at code2
> with 0, instead of 1.
Hm, I don't see the nesting in this example, can you clarify?
> I've also seen:
>
> if (cond) {
> instr_begin();
> // code1
> instr_end();
> }
> instr_begin();
> // code2
> instr_end();
>
> Where instr_end() and instr_begin() merge onto the same instruction of
> code2 as a 0, and again code2 will issue a false warning.
>
> You can also not make objtool lift the end marker to the previous
> instruction, because then:
>
> if (cond1) {
> instr_begin();
> if (cond2) {
> // code2
> }
> instr_end();
> }
>
> Suffers the reverse problem, instr_end() becomes part of the @cond2
> block and cond1 grows a path that misses it entirely.
>
> So far I've not had any actual solution except adding a NOP to anchor
> the annotation on.
Are you adding the NOP to the instr_end() annotation itself? Seems like
that would be the cleanest/easiest.
Though it is sad that we have to change the code to make objtool happy
-- would be nice if we could come up with something less intrusive.
--
Josh
On Thu, Mar 26, 2020 at 12:30:49PM +0100, Peter Zijlstra wrote:
> This isn't ideal either; stuffing code with NOPs just to make
> annotations work is certainly sub-optimal, but given that sync_core()
> is stupid expensive in any case, one extra nop isn't going to be a
> problem here.
/me puts his hardened objtool maintainer's glasses on...
The problem is, when you do this kind of change to somebody else's code
-- like adding a NOP to driver code -- there's a 90% chance they'll NACK
it and tell you to fix your shit. Because they'll be happy to tell you
the code itself should never be changed just to "make objtool happy".
And honestly, they'd be right, and there's not much you can say in
reply. And then we end up having to fix it in objtool again anyway.
The 'insn == first' check isn't ideal, but at least it works (I think?).
Maybe there's some cleaner approach. I'll need to think about it later
after I make some progress on the massive oil well fire I call my INBOX.
--
Josh
On Thu, 26 Mar 2020, Peter Zijlstra wrote:
> On Thu, Mar 26, 2020 at 12:30:50PM +0100, Peter Zijlstra wrote:
>
> > There is a special case in the UNWIND_HINT_RESTORE code. When, upon
> > looking for the UNWIND_HINT_SAVE instruction to restore from, it finds
> > the instruction hasn't been visited yet, it normally issues a WARN,
> > except when this HINT_SAVE instruction is the first instruction of
> > this branch.
> >
> > The reason for this special case comes apparent when we remove it;
> > code like:
> >
> > if (cond) {
> > UNWIND_HINT_SAVE
> > // do stuff
> > UNWIND_HINT_RESTORE
> > }
> > // more stuff
> >
> > will now trigger the warning. This is because UNWIND_HINT_RESTORE is
> > just a label, and there is nothing keeping it inside the (extended)
> > basic block covered by @cond. It will attach itself to the first
> > instruction of 'more stuff' and we'll hit it outside of the @cond,
> > confusing things.
> >
> > I don't much like this special case, it confuses things and will come
> > apart horribly if/when the annotation needs to support nesting.
> > Instead extend the affected code to at least form an extended basic
> > block.
> >
> > In particular, of the 2 users of this annotation: ftrace_regs_caller()
> > and sync_core(), only the latter suffers this problem. Extend it's
> > code sequence with a NOP to make it an extended basic block.
> >
> > This isn't ideal either; stuffing code with NOPs just to make
> > annotations work is certainly sub-optimal, but given that sync_core()
> > is stupid expensive in any case, one extra nop isn't going to be a
> > problem here.
>
> So instr_begin() / instr_end() have this exact problem, but worse. Those
> actually do nest and I've ran into the following situation:
>
> if (cond1) {
> instr_begin();
> // code1
> instr_end();
> }
> // code
>
> if (cond2) {
> instr_begin();
> // code2
> instr_end();
> }
> // tail
>
> Where objtool then finds the path: !cond1, cond2, which ends up at code2
> with 0, instead of 1.
>
> I've also seen:
>
> if (cond) {
> instr_begin();
> // code1
> instr_end();
> }
> instr_begin();
> // code2
> instr_end();
>
> Where instr_end() and instr_begin() merge onto the same instruction of
> code2 as a 0, and again code2 will issue a false warning.
>
> You can also not make objtool lift the end marker to the previous
> instruction, because then:
>
> if (cond1) {
> instr_begin();
> if (cond2) {
> // code2
> }
> instr_end();
> }
>
> Suffers the reverse problem, instr_end() becomes part of the @cond2
> block and cond1 grows a path that misses it entirely.
One could argue that this is really nasty and the correct way should be
if (cond1) {
if (cond2) {
instr_begin();
// code2
instr_end();
}
}
Then it should work if instr_begin() marks the next instruction and
instr_end() marks the previous one, no? There is a corner case when code2
is exactly one instruction, so instr counting would have to be updated.
Hopefully there is a better way.
Miroslav
On Thu, 26 Mar 2020, Miroslav Benes wrote:
> On Thu, 26 Mar 2020, Peter Zijlstra wrote:
>
> > On Thu, Mar 26, 2020 at 12:30:50PM +0100, Peter Zijlstra wrote:
> >
> > > There is a special case in the UNWIND_HINT_RESTORE code. When, upon
> > > looking for the UNWIND_HINT_SAVE instruction to restore from, it finds
> > > the instruction hasn't been visited yet, it normally issues a WARN,
> > > except when this HINT_SAVE instruction is the first instruction of
> > > this branch.
> > >
> > > The reason for this special case comes apparent when we remove it;
> > > code like:
> > >
> > > if (cond) {
> > > UNWIND_HINT_SAVE
> > > // do stuff
> > > UNWIND_HINT_RESTORE
> > > }
> > > // more stuff
> > >
> > > will now trigger the warning. This is because UNWIND_HINT_RESTORE is
> > > just a label, and there is nothing keeping it inside the (extended)
> > > basic block covered by @cond. It will attach itself to the first
> > > instruction of 'more stuff' and we'll hit it outside of the @cond,
> > > confusing things.
> > >
> > > I don't much like this special case, it confuses things and will come
> > > apart horribly if/when the annotation needs to support nesting.
> > > Instead extend the affected code to at least form an extended basic
> > > block.
> > >
> > > In particular, of the 2 users of this annotation: ftrace_regs_caller()
> > > and sync_core(), only the latter suffers this problem. Extend it's
> > > code sequence with a NOP to make it an extended basic block.
> > >
> > > This isn't ideal either; stuffing code with NOPs just to make
> > > annotations work is certainly sub-optimal, but given that sync_core()
> > > is stupid expensive in any case, one extra nop isn't going to be a
> > > problem here.
> >
> > So instr_begin() / instr_end() have this exact problem, but worse. Those
> > actually do nest and I've ran into the following situation:
> >
> > if (cond1) {
> > instr_begin();
> > // code1
> > instr_end();
> > }
> > // code
> >
> > if (cond2) {
> > instr_begin();
> > // code2
> > instr_end();
> > }
> > // tail
> >
> > Where objtool then finds the path: !cond1, cond2, which ends up at code2
> > with 0, instead of 1.
> >
> > I've also seen:
> >
> > if (cond) {
> > instr_begin();
> > // code1
> > instr_end();
> > }
> > instr_begin();
> > // code2
> > instr_end();
> >
> > Where instr_end() and instr_begin() merge onto the same instruction of
> > code2 as a 0, and again code2 will issue a false warning.
> >
> > You can also not make objtool lift the end marker to the previous
> > instruction, because then:
> >
> > if (cond1) {
> > instr_begin();
> > if (cond2) {
> > // code2
> > }
> > instr_end();
> > }
> >
> > Suffers the reverse problem, instr_end() becomes part of the @cond2
> > block and cond1 grows a path that misses it entirely.
>
> One could argue that this is really nasty and the correct way should be
>
> if (cond1) {
> if (cond2) {
> instr_begin();
> // code2
> instr_end();
> }
> }
>
> Then it should work if instr_begin() marks the next instruction and
> instr_end() marks the previous one, no? There is a corner case when code2
> is exactly one instruction, so instr counting would have to be updated.
if (cond1) {
instr_begin()
if (cond2) {
// code2
}
// code1
instr_end();
}
is a counter example though, so I take it back.
M
On Thu, Mar 26, 2020 at 08:44:48AM -0500, Josh Poimboeuf wrote:
> On Thu, Mar 26, 2020 at 01:58:44PM +0100, Peter Zijlstra wrote:
> > So instr_begin() / instr_end() have this exact problem, but worse. Those
> > actually do nest and I've ran into the following situation:
> >
> > if (cond1) {
> > instr_begin();
> > // code1
> > instr_end();
> > }
> > // code
> >
> > if (cond2) {
> > instr_begin();
> > // code2
> > instr_end();
> > }
> > // tail
> >
> > Where objtool then finds the path: !cond1, cond2, which ends up at code2
> > with 0, instead of 1.
>
> Hm, I don't see the nesting in this example, can you clarify?
Indeed no nesting here, but because they can nest we have that begin as
+1, end as -1 and then we sum it over the code flow.
Then given that, the above, ends up as -1 + 1 in the !cond1,cond2 case,
because that -1 escapes the cond1 block.
> > I've also seen:
> >
> > if (cond) {
> > instr_begin();
> > // code1
> > instr_end();
> > }
> > instr_begin();
> > // code2
> > instr_end();
> >
> > Where instr_end() and instr_begin() merge onto the same instruction of
> > code2 as a 0, and again code2 will issue a false warning.
> >
> > You can also not make objtool lift the end marker to the previous
> > instruction, because then:
> >
> > if (cond1) {
> > instr_begin();
> > if (cond2) {
> > // code2
> > }
> > instr_end();
> > }
> >
> > Suffers the reverse problem, instr_end() becomes part of the @cond2
> > block and cond1 grows a path that misses it entirely.
> >
> > So far I've not had any actual solution except adding a NOP to anchor
> > the annotation on.
>
> Are you adding the NOP to the instr_end() annotation itself? Seems like
> that would be the cleanest/easiest.
That actually generates a whole bunch of 'stupid' unreachable warnings.
Also, in the hope of still coming up with something saner, we've been
carrying a minimal set of explicit nop()s.
> Though it is sad that we have to change the code to make objtool happy
> -- would be nice if we could come up with something less intrusive.
Very much yes, but so far that's been eluding me.
On Thu, Mar 26, 2020 at 08:56:20AM -0500, Josh Poimboeuf wrote:
> On Thu, Mar 26, 2020 at 12:30:49PM +0100, Peter Zijlstra wrote:
> > This isn't ideal either; stuffing code with NOPs just to make
> > annotations work is certainly sub-optimal, but given that sync_core()
> > is stupid expensive in any case, one extra nop isn't going to be a
> > problem here.
>
> /me puts his hardened objtool maintainer's glasses on...
>
> The problem is, when you do this kind of change to somebody else's code
> -- like adding a NOP to driver code -- there's a 90% chance they'll NACK
> it and tell you to fix your shit. Because they'll be happy to tell you
> the code itself should never be changed just to "make objtool happy".
So the only objtool annotation drivers tend to run into is that uaccess
crud. Drivers really had better not need the CFI annotations, otherwise
they doing massively dodgy things.
And the nice thing with the uaccess crud is that they're anchored to
actual instructions, as opposed to this.
> And honestly, they'd be right, and there's not much you can say in
> reply. And then we end up having to fix it in objtool again anyway.
Well, I agree. I just haven't managed to come up with anything sensible.
> The 'insn == first' check isn't ideal, but at least it works (I think?).
It works, yes, for exactly this one case.
On Thu, Mar 26, 2020 at 04:49:38PM +0100, Peter Zijlstra wrote:
> > The 'insn == first' check isn't ideal, but at least it works (I think?).
>
> It works, yes, for exactly this one case.
How's this? Ignore the ignore_cfi bits, that's a 'failed' experiment.
---
arch/x86/include/asm/orc_types.h | 2 +
arch/x86/include/asm/processor.h | 6 +-
arch/x86/include/asm/unwind_hints.h | 4 ++
tools/arch/x86/include/asm/orc_types.h | 2 +
tools/objtool/check.c | 109 +++++++++++++++++++--------------
tools/objtool/check.h | 3 +-
6 files changed, 75 insertions(+), 51 deletions(-)
diff --git a/arch/x86/include/asm/orc_types.h b/arch/x86/include/asm/orc_types.h
index 6e060907c163..82b5c685341a 100644
--- a/arch/x86/include/asm/orc_types.h
+++ b/arch/x86/include/asm/orc_types.h
@@ -60,6 +60,8 @@
#define ORC_TYPE_REGS_IRET 2
#define UNWIND_HINT_TYPE_SAVE 3
#define UNWIND_HINT_TYPE_RESTORE 4
+#define UNWIND_HINT_TYPE_IGNORE 5
+#define UNWIND_HINT_TYPE_IRET_CONT 6
#ifndef __ASSEMBLY__
/*
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 94789db550df..45c74cbc0a83 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -728,8 +728,8 @@ static inline void sync_core(void)
unsigned int tmp;
asm volatile (
- UNWIND_HINT_SAVE
"mov %%ss, %0\n\t"
+ UNWIND_HINT_SAVE
"pushq %q0\n\t"
"pushq %%rsp\n\t"
"addq $8, (%%rsp)\n\t"
@@ -737,9 +737,9 @@ static inline void sync_core(void)
"mov %%cs, %0\n\t"
"pushq %q0\n\t"
"pushq $1f\n\t"
+ UNWIND_HINT_IRET_CONT
"iretq\n\t"
- UNWIND_HINT_RESTORE
- "1:"
+ "1:\n\t"
: "=&r" (tmp), ASM_CALL_CONSTRAINT : : "cc", "memory");
#endif
}
diff --git a/arch/x86/include/asm/unwind_hints.h b/arch/x86/include/asm/unwind_hints.h
index f5e2eb12cb71..d8a07749c323 100644
--- a/arch/x86/include/asm/unwind_hints.h
+++ b/arch/x86/include/asm/unwind_hints.h
@@ -112,6 +112,10 @@
#define UNWIND_HINT_RESTORE UNWIND_HINT(0, 0, UNWIND_HINT_TYPE_RESTORE, 0)
+#define UNWIND_HINT_IGNORE UNWIND_HINT(0, 0, UNWIND_HINT_TYPE_IGNORE, 0)
+
+#define UNWIND_HINT_IRET_CONT UNWIND_HINT(0, 0, UNWIND_HINT_TYPE_IRET_CONT, 0)
+
#endif /* __ASSEMBLY__ */
#endif /* _ASM_X86_UNWIND_HINTS_H */
diff --git a/tools/arch/x86/include/asm/orc_types.h b/tools/arch/x86/include/asm/orc_types.h
index 6e060907c163..82b5c685341a 100644
--- a/tools/arch/x86/include/asm/orc_types.h
+++ b/tools/arch/x86/include/asm/orc_types.h
@@ -60,6 +60,8 @@
#define ORC_TYPE_REGS_IRET 2
#define UNWIND_HINT_TYPE_SAVE 3
#define UNWIND_HINT_TYPE_RESTORE 4
+#define UNWIND_HINT_TYPE_IGNORE 5
+#define UNWIND_HINT_TYPE_IRET_CONT 6
#ifndef __ASSEMBLY__
/*
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index e637a4a38d2a..03bac6cb313c 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1259,14 +1259,25 @@ static int read_unwind_hints(struct objtool_file *file)
cfa = &insn->state.cfa;
- if (hint->type == UNWIND_HINT_TYPE_SAVE) {
+ switch (hint->type) {
+ case UNWIND_HINT_TYPE_SAVE:
insn->save = true;
continue;
- } else if (hint->type == UNWIND_HINT_TYPE_RESTORE) {
+ case UNWIND_HINT_TYPE_RESTORE:
insn->restore = true;
- insn->hint = true;
continue;
+
+ case UNWIND_HINT_TYPE_IGNORE:
+ insn->ignore_cfi = true;
+ continue;
+
+ case UNWIND_HINT_TYPE_IRET_CONT:
+ insn->iret_cont = true;
+ continue;
+
+ default:
+ break;
}
insn->hint = true;
@@ -1558,6 +1569,9 @@ static int update_insn_state(struct instruction *insn, struct insn_state *state)
struct cfi_reg *cfa = &state->cfa;
struct cfi_reg *regs = state->regs;
+ if (insn->ignore_cfi)
+ return 0;
+
/* stack operations don't make sense with an undefined CFA */
if (cfa->base == CFI_UNDEFINED) {
if (insn->func) {
@@ -1993,6 +2007,37 @@ static int validate_sibling_call(struct instruction *insn, struct insn_state *st
return validate_call(insn, state);
}
+static int insn_hint_restore(struct objtool_file *file, struct section *sec,
+ struct symbol *func, struct instruction *insn,
+ struct insn_state *state)
+{
+ struct instruction *save_insn, *i;
+
+ i = insn;
+ save_insn = NULL;
+ func_for_each_insn_continue_reverse(file, func, i) {
+ if (i->save) {
+ save_insn = i;
+ break;
+ }
+ }
+
+ if (!save_insn) {
+ WARN_FUNC("no corresponding CFI save for CFI restore",
+ sec, insn->offset);
+ return 1;
+ }
+
+ if (!save_insn->visited) {
+ WARN_FUNC("objtool isn't smart enough to handle this CFI save/restore combo",
+ sec, insn->offset);
+ return 1;
+ }
+
+ *state = save_insn->state;
+ return 0;
+}
+
/*
* Follow the branch starting at the given instruction, and recursively follow
* any other branches (jumps). Meanwhile, track the frame pointer state at
@@ -2000,15 +2045,14 @@ static int validate_sibling_call(struct instruction *insn, struct insn_state *st
* 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 instruction *next_insn;
struct alternative *alt;
- struct instruction *insn, *next_insn;
struct section *sec;
u8 visited;
int ret;
- insn = first;
sec = insn->sec;
if (insn->alt_group && list_empty(&insn->alts)) {
@@ -2034,7 +2078,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
visited = 1 << state.uaccess;
if (insn->visited) {
- if (!insn->hint && !insn_state_match(insn, &state))
+ if ((!insn->hint && !insn->restore) && !insn_state_match(insn, &state))
return 1;
if (insn->visited & visited)
@@ -2042,47 +2086,12 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
}
if (insn->hint) {
- if (insn->restore) {
- struct instruction *save_insn, *i;
-
- i = insn;
- save_insn = NULL;
- func_for_each_insn_continue_reverse(file, func, i) {
- if (i->save) {
- save_insn = i;
- break;
- }
- }
-
- if (!save_insn) {
- WARN_FUNC("no corresponding CFI save for CFI restore",
- sec, insn->offset);
- return 1;
- }
-
- 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;
- }
-
- insn->state = save_insn->state;
- }
-
state = insn->state;
-
- } else
+ } else {
+ if (insn->restore)
+ insn_hint_restore(file, sec, func, insn, &state);
insn->state = state;
+ }
insn->visited |= visited;
@@ -2191,11 +2200,17 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
break;
case INSN_CONTEXT_SWITCH:
- if (func && (!next_insn || !next_insn->hint)) {
+ if (insn->iret_cont) {
+ insn_hint_restore(file, sec, func, insn, &state);
+ break;
+ }
+
+ if (func) {
WARN_FUNC("unsupported instruction in callable function",
sec, insn->offset);
return 1;
}
+
return 0;
case INSN_STACK:
diff --git a/tools/objtool/check.h b/tools/objtool/check.h
index 6d875ca6fce0..f2b6172e119b 100644
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -33,7 +33,8 @@ struct instruction {
unsigned int len;
enum insn_type type;
unsigned long immediate;
- bool alt_group, dead_end, ignore, hint, save, restore, ignore_alts;
+ bool alt_group, dead_end, ignore, ignore_alts;
+ bool hint, save, restore, ignore_cfi, iret_cont;
bool retpoline_safe;
u8 visited;
struct symbol *call_dest;
On Thu, Mar 26, 2020 at 08:57:18PM +0100, Peter Zijlstra wrote:
> On Thu, Mar 26, 2020 at 04:49:38PM +0100, Peter Zijlstra wrote:
> > > The 'insn == first' check isn't ideal, but at least it works (I think?).
> >
> > It works, yes, for exactly this one case.
>
> How's this? Ignore the ignore_cfi bits, that's a 'failed' experiment.
It still seems complex to me.
What do you think about this? If we store save_insn in the state when
we see insn->save, the restore logic becomes a lot easier. Then if we
get a restore without a save, we can just ignore the restore hint in
that path. Later, when we see the restore insn again from the save
path, we can then compare the insn state with the saved state to make
sure they match.
This assumes no crazy save/restore scenarios. It also means that the
restore path has to be reachable from the save path, for which I had to
make a change to make IRETQ *not* a dead end if there's a restore hint
immediately after it.
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index e637a4a38d2a..e9becd50f148 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1265,7 +1265,6 @@ static int read_unwind_hints(struct objtool_file *file)
} else if (hint->type == UNWIND_HINT_TYPE_RESTORE) {
insn->restore = true;
- insn->hint = true;
continue;
}
@@ -2003,7 +2002,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
struct instruction *first, struct insn_state state)
{
struct alternative *alt;
- struct instruction *insn, *next_insn;
+ struct instruction *insn, *next_insn, *save_insn = NULL;
struct section *sec;
u8 visited;
int ret;
@@ -2034,54 +2033,32 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
visited = 1 << state.uaccess;
if (insn->visited) {
- if (!insn->hint && !insn_state_match(insn, &state))
+ if (!insn->hint && !insn->restore &&
+ !insn_state_match(insn, &state)) {
return 1;
+ }
+
+ if (insn->restore && save_insn) {
+ if (!insn_state_match(insn, &save_insn->state))
+ return 1;
+ save_insn = NULL;
+ }
if (insn->visited & visited)
return 0;
}
- if (insn->hint) {
- if (insn->restore) {
- struct instruction *save_insn, *i;
-
- i = insn;
- save_insn = NULL;
- func_for_each_insn_continue_reverse(file, func, i) {
- if (i->save) {
- save_insn = i;
- break;
- }
- }
-
- if (!save_insn) {
- WARN_FUNC("no corresponding CFI save for CFI restore",
- sec, insn->offset);
- return 1;
- }
-
- 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;
- }
+ if (insn->save)
+ save_insn = insn;
- insn->state = save_insn->state;
- }
+ if (insn->restore && save_insn) {
+ insn->state = save_insn->state;
+ save_insn = NULL;
+ }
+ if (insn->hint)
state = insn->state;
-
- } else
+ else
insn->state = state;
insn->visited |= visited;
@@ -2191,12 +2168,17 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
break;
case INSN_CONTEXT_SWITCH:
- if (func && (!next_insn || !next_insn->hint)) {
- WARN_FUNC("unsupported instruction in callable function",
- sec, insn->offset);
- return 1;
+ if (!next_insn || !next_insn->restore) {
+ if (func) {
+ WARN_FUNC("unsupported instruction in callable function",
+ sec, insn->offset);
+ return 1;
+ }
+
+ return 0;
}
- return 0;
+
+ break;
case INSN_STACK:
if (update_insn_state(insn, &state))
@@ -2293,7 +2275,7 @@ static int validate_unwind_hints(struct objtool_file *file)
clear_insn_state(&state);
for_each_insn(file, insn) {
- if (insn->hint && !insn->visited) {
+ if ((insn->hint || insn->save || insn->restore) && !insn->visited) {
ret = validate_branch(file, insn->func, insn, state);
if (ret && backtrace)
BT_FUNC("<=== (hint)", insn);
On Thu, Mar 26, 2020 at 04:38:41PM +0100, Peter Zijlstra wrote:
> On Thu, Mar 26, 2020 at 08:44:48AM -0500, Josh Poimboeuf wrote:
> > On Thu, Mar 26, 2020 at 01:58:44PM +0100, Peter Zijlstra wrote:
> > > So instr_begin() / instr_end() have this exact problem, but worse. Those
> > > actually do nest and I've ran into the following situation:
> > >
> > > if (cond1) {
> > > instr_begin();
> > > // code1
> > > instr_end();
> > > }
> > > // code
> > >
> > > if (cond2) {
> > > instr_begin();
> > > // code2
> > > instr_end();
> > > }
> > > // tail
> > >
> > > Where objtool then finds the path: !cond1, cond2, which ends up at code2
> > > with 0, instead of 1.
> >
> > Hm, I don't see the nesting in this example, can you clarify?
>
> Indeed no nesting here, but because they can nest we have that begin as
> +1, end as -1 and then we sum it over the code flow.
>
> Then given that, the above, ends up as -1 + 1 in the !cond1,cond2 case,
> because that -1 escapes the cond1 block.
Ok, I see now. I wonder if you can take a similar approach to the
save/restore patch I sent out. Though I guess the nesting adds an
additional wrinkle I'm too tired to think about.
--
Josh
On Thu, Mar 26, 2020 at 08:00:01PM -0500, Josh Poimboeuf wrote:
> On Thu, Mar 26, 2020 at 08:57:18PM +0100, Peter Zijlstra wrote:
> > On Thu, Mar 26, 2020 at 04:49:38PM +0100, Peter Zijlstra wrote:
> > > > The 'insn == first' check isn't ideal, but at least it works (I think?).
> > >
> > > It works, yes, for exactly this one case.
> >
> > How's this? Ignore the ignore_cfi bits, that's a 'failed' experiment.
>
> It still seems complex to me.
>
> What do you think about this?
Perhaps, but I still don't really like it. After much thinking I have
yet another proposal, see below.
> If we store save_insn in the state when
> we see insn->save, the restore logic becomes a lot easier.
Yes, that does help.
> Then if we
> get a restore without a save, we can just ignore the restore hint in
> that path. Later, when we see the restore insn again from the save
> path, we can then compare the insn state with the saved state to make
> sure they match.
That basically removes the error.
> This assumes no crazy save/restore scenarios.
This.
> It also means that the
> restore path has to be reachable from the save path, for which I had to
> make a change to make IRETQ *not* a dead end if there's a restore hint
> immediately after it.
Right, I did that too. But that then makes me wonder why you needed that
change to validate_unwind_hints(), because if the restore hint makes it
continue, you'll not have unreachable code after it.
---
Subject: objtool: Implement RET_TAIL hint
This replaces the SAVE/RESTORE hints with a RET_TAIL hint that applies to:
- regular RETURN and sibling calls (which are also function exists)
it allows the stack-frame to be off by one word, ie. it allows a
return-tail-call.
- EXCEPTION_RETURN (a new INSN_type that splits IRET out of
CONTEXT_SWITCH) and here it denotes a return to self by having it
consume arch_exception_frame_size bytes off the stack and continuing.
Apply this hint to ftrace_64.S and sync_core(), the two existing users
of the SAVE/RESTORE hints.
For ftrace_64.S we split the return path and make sure the
ftrace_epilogue call is seen as a sibling/tail-call turning it into it's
own function.
By splitting the return path every instruction has a unique stack setup
and ORC can generate correct unwinds (XXX check if/how the ftrace
trampolines map into the ORC). Then employ the RET_TAIL hint to the
tail-call exit that has the direct-call (orig_eax) return-tail-call on.
For sync_core() annotate the IRET with RET_TAIL to mark it as a
control-flow NOP that consumes the exception frame.
XXX this adds a tail-call to ftrace_caller()
XXX compile tested only
Possibly-signed-off-by: Peter Zijlstra (Intel) <[email protected]>
---
arch/x86/include/asm/orc_types.h | 3 +-
arch/x86/include/asm/processor.h | 3 +-
arch/x86/include/asm/unwind_hints.h | 12 ++----
arch/x86/kernel/ftrace.c | 12 +++++-
arch/x86/kernel/ftrace_64.S | 27 +++++--------
tools/arch/x86/include/asm/orc_types.h | 3 +-
tools/objtool/Makefile | 2 +-
tools/objtool/arch.h | 3 ++
tools/objtool/arch/x86/decode.c | 5 ++-
tools/objtool/check.c | 74 ++++++++++------------------------
tools/objtool/check.h | 3 +-
11 files changed, 59 insertions(+), 88 deletions(-)
diff --git a/arch/x86/include/asm/orc_types.h b/arch/x86/include/asm/orc_types.h
index 6e060907c163..49536fdd29ce 100644
--- a/arch/x86/include/asm/orc_types.h
+++ b/arch/x86/include/asm/orc_types.h
@@ -58,8 +58,7 @@
#define ORC_TYPE_CALL 0
#define ORC_TYPE_REGS 1
#define ORC_TYPE_REGS_IRET 2
-#define UNWIND_HINT_TYPE_SAVE 3
-#define UNWIND_HINT_TYPE_RESTORE 4
+#define UNWIND_HINT_TYPE_RET_TAIL 3
#ifndef __ASSEMBLY__
/*
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 94789db550df..cb09ee18cbab 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -728,7 +728,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,8 +736,8 @@ static inline void sync_core(void)
"mov %%cs, %0\n\t"
"pushq %q0\n\t"
"pushq $1f\n\t"
+ UNWIND_HINT_RET_TAIL
"iretq\n\t"
- UNWIND_HINT_RESTORE
"1:"
: "=&r" (tmp), ASM_CALL_CONSTRAINT : : "cc", "memory");
#endif
diff --git a/arch/x86/include/asm/unwind_hints.h b/arch/x86/include/asm/unwind_hints.h
index f5e2eb12cb71..64ec9c9a6674 100644
--- a/arch/x86/include/asm/unwind_hints.h
+++ b/arch/x86/include/asm/unwind_hints.h
@@ -86,12 +86,8 @@
UNWIND_HINT sp_offset=\sp_offset
.endm
-.macro UNWIND_HINT_SAVE
- UNWIND_HINT type=UNWIND_HINT_TYPE_SAVE
-.endm
-
-.macro UNWIND_HINT_RESTORE
- UNWIND_HINT type=UNWIND_HINT_TYPE_RESTORE
+.macro UNWIND_HINT_RET_TAIL
+ UNWIND_HINT type=UNWIND_HINT_TYPE_RET_TAIL
.endm
#else /* !__ASSEMBLY__ */
@@ -108,9 +104,7 @@
".balign 4 \n\t" \
".popsection\n\t"
-#define UNWIND_HINT_SAVE UNWIND_HINT(0, 0, UNWIND_HINT_TYPE_SAVE, 0)
-
-#define UNWIND_HINT_RESTORE UNWIND_HINT(0, 0, UNWIND_HINT_TYPE_RESTORE, 0)
+#define UNWIND_HINT_RET_TAIL UNWIND_HINT(0, 0, UNWIND_HINT_TYPE_RET_TAIL, 0)
#endif /* __ASSEMBLY__ */
diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 37a0aeaf89e7..3fda2ee7bf71 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -282,7 +282,8 @@ static inline void tramp_free(void *tramp) { }
/* Defined as markers to the end of the ftrace default trampolines */
extern void ftrace_regs_caller_end(void);
-extern void ftrace_epilogue(void);
+extern void ftrace_regs_caller_ret(void);
+extern void ftrace_caller_end(void);
extern void ftrace_caller_op_ptr(void);
extern void ftrace_regs_caller_op_ptr(void);
@@ -334,7 +335,7 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
call_offset = (unsigned long)ftrace_regs_call;
} else {
start_offset = (unsigned long)ftrace_caller;
- end_offset = (unsigned long)ftrace_epilogue;
+ end_offset = (unsigned long)ftrace_caller_end;
op_offset = (unsigned long)ftrace_caller_op_ptr;
call_offset = (unsigned long)ftrace_call;
}
@@ -366,6 +367,13 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
if (WARN_ON(ret < 0))
goto fail;
+ if (ops->flags & FTRACE_OPS_FL_SAVE_REGS) {
+ ip = ftrace_regs_caller_ret;
+ ret = probe_kernel_read(ip, (void *)retq, RET_SIZE);
+ if (WARN_ON(ret < 0))
+ goto fail;
+ }
+
/*
* The address of the ftrace_ops that is used for this trampoline
* is stored at the end of the trampoline. This will be used to
diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S
index 369e61faacfe..28834d715817 100644
--- a/arch/x86/kernel/ftrace_64.S
+++ b/arch/x86/kernel/ftrace_64.S
@@ -157,8 +157,12 @@ SYM_INNER_LABEL(ftrace_call, SYM_L_GLOBAL)
* think twice before adding any new code or changing the
* layout here.
*/
-SYM_INNER_LABEL(ftrace_epilogue, SYM_L_GLOBAL)
+SYM_INNER_LABEL(ftrace_caller_end, SYM_L_GLOBAL)
+ jmp ftrace_epilogue
+SYM_FUNC_END(ftrace_caller);
+
+SYM_FUNC_START(ftrace_epilogue)
#ifdef CONFIG_FUNCTION_GRAPH_TRACER
SYM_INNER_LABEL(ftrace_graph_call, SYM_L_GLOBAL)
jmp ftrace_stub
@@ -170,14 +174,12 @@ SYM_INNER_LABEL(ftrace_graph_call, SYM_L_GLOBAL)
*/
SYM_INNER_LABEL_ALIGN(ftrace_stub, SYM_L_WEAK)
retq
-SYM_FUNC_END(ftrace_caller)
+SYM_FUNC_END(ftrace_epilogue)
SYM_FUNC_START(ftrace_regs_caller)
/* Save the current flags before any operations that can change them */
pushfq
- UNWIND_HINT_SAVE
-
/* added 8 bytes to save flags */
save_mcount_regs 8
/* save_mcount_regs fills in first two parameters */
@@ -244,20 +246,14 @@ SYM_INNER_LABEL(ftrace_regs_call, SYM_L_GLOBAL)
movq %rax, MCOUNT_REG_SIZE(%rsp)
restore_mcount_regs 8
+ /* Restore flags */
+ popfq
- jmp 2f
+SYM_INNER_LABEL(ftrace_regs_caller_ret, SYM_L_GLOBAL);
+ UNWIND_HINT_RET_TAIL
+ jmp ftrace_epilogue
1: restore_mcount_regs
-
-
-2:
- /*
- * The stack layout is nondetermistic here, depending on which path was
- * taken. This confuses objtool and ORC, rightfully so. For now,
- * pretend the stack always looks like the non-direct case.
- */
- UNWIND_HINT_RESTORE
-
/* Restore flags */
popfq
@@ -268,7 +264,6 @@ SYM_INNER_LABEL(ftrace_regs_call, SYM_L_GLOBAL)
* to the return.
*/
SYM_INNER_LABEL(ftrace_regs_caller_end, SYM_L_GLOBAL)
-
jmp ftrace_epilogue
SYM_FUNC_END(ftrace_regs_caller)
diff --git a/tools/arch/x86/include/asm/orc_types.h b/tools/arch/x86/include/asm/orc_types.h
index 6e060907c163..49536fdd29ce 100644
--- a/tools/arch/x86/include/asm/orc_types.h
+++ b/tools/arch/x86/include/asm/orc_types.h
@@ -58,8 +58,7 @@
#define ORC_TYPE_CALL 0
#define ORC_TYPE_REGS 1
#define ORC_TYPE_REGS_IRET 2
-#define UNWIND_HINT_TYPE_SAVE 3
-#define UNWIND_HINT_TYPE_RESTORE 4
+#define UNWIND_HINT_TYPE_RET_TAIL 3
#ifndef __ASSEMBLY__
/*
diff --git a/tools/objtool/Makefile b/tools/objtool/Makefile
index ee08aeff30a1..ebbf92a8836f 100644
--- a/tools/objtool/Makefile
+++ b/tools/objtool/Makefile
@@ -31,7 +31,7 @@ INCLUDES := -I$(srctree)/tools/include \
-I$(srctree)/tools/arch/$(HOSTARCH)/include/uapi \
-I$(srctree)/tools/arch/$(SRCARCH)/include
WARNINGS := $(EXTRA_WARNINGS) -Wno-switch-default -Wno-switch-enum -Wno-packed
-CFLAGS := -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -g $(INCLUDES) $(LIBELF_FLAGS)
+CFLAGS := -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -ggdb3 $(INCLUDES) $(LIBELF_FLAGS)
LDFLAGS += $(LIBELF_LIBS) $(LIBSUBCMD) $(KBUILD_HOSTLDFLAGS)
# Allow old libelf to be used:
diff --git a/tools/objtool/arch.h b/tools/objtool/arch.h
index ced3765c4f44..6d04a683f8cc 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,
@@ -73,6 +74,8 @@ int arch_decode_instruction(struct elf *elf, struct section *sec,
unsigned int *len, enum insn_type *type,
unsigned long *immediate, struct stack_op *op);
+static const int arch_exception_frame_size = 5*8;
+
bool arch_callee_saved_reg(unsigned char reg);
#endif /* _ARCH_H */
diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
index a62e032863a8..1f9dd97e4a50 100644
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -431,10 +431,13 @@ int arch_decode_instruction(struct elf *elf, struct section *sec,
case 0xca: /* retf */
case 0xcb: /* retf */
- case 0xcf: /* iret */
*type = INSN_CONTEXT_SWITCH;
break;
+ case 0xcf: /* iret */
+ *type = INSN_EXCEPTION_RETURN;
+ break;
+
case 0xe8:
*type = INSN_CALL;
break;
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index e637a4a38d2a..6eb9ae48c94f 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1259,13 +1259,8 @@ static int read_unwind_hints(struct objtool_file *file)
cfa = &insn->state.cfa;
- if (hint->type == UNWIND_HINT_TYPE_SAVE) {
- insn->save = true;
- continue;
-
- } else if (hint->type == UNWIND_HINT_TYPE_RESTORE) {
- insn->restore = true;
- insn->hint = true;
+ if (hint->type == UNWIND_HINT_TYPE_RET_TAIL) {
+ insn->ret_tail = true;
continue;
}
@@ -1429,16 +1424,22 @@ static bool is_fentry_call(struct instruction *insn)
return false;
}
-static bool has_modified_stack_frame(struct insn_state *state)
+static bool has_modified_stack_frame(struct instruction *insn, struct insn_state *state)
{
int i;
if (state->cfa.base != initial_func_cfi.cfa.base ||
- state->cfa.offset != initial_func_cfi.cfa.offset ||
- state->stack_size != initial_func_cfi.cfa.offset ||
state->drap)
return true;
+ if (state->cfa.offset != initial_func_cfi.cfa.offset ||
+ !(insn->ret_tail && state->cfa.offset == initial_func_cfi.cfa.offset + 8))
+
+
+ if (state->stack_size != initial_func_cfi.cfa.offset &&
+ !(insn->ret_tail && state->stack_size == initial_func_cfi.cfa.offset + 8))
+ return true;
+
for (i = 0; i < CFI_NUM_REGS; i++)
if (state->regs[i].base != initial_func_cfi.regs[i].base ||
state->regs[i].offset != initial_func_cfi.regs[i].offset)
@@ -1984,7 +1985,7 @@ static int validate_call(struct instruction *insn, struct insn_state *state)
static int validate_sibling_call(struct instruction *insn, struct insn_state *state)
{
- if (has_modified_stack_frame(state)) {
+ if (has_modified_stack_frame(insn, state)) {
WARN_FUNC("sibling call from callable instruction with modified stack frame",
insn->sec, insn->offset);
return 1;
@@ -2041,47 +2042,9 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
return 0;
}
- if (insn->hint) {
- if (insn->restore) {
- struct instruction *save_insn, *i;
-
- i = insn;
- save_insn = NULL;
- func_for_each_insn_continue_reverse(file, func, i) {
- if (i->save) {
- save_insn = i;
- break;
- }
- }
-
- if (!save_insn) {
- WARN_FUNC("no corresponding CFI save for CFI restore",
- sec, insn->offset);
- return 1;
- }
-
- 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;
- }
-
- insn->state = save_insn->state;
- }
-
+ if (insn->hint)
state = insn->state;
-
- } else
+ else
insn->state = state;
insn->visited |= visited;
@@ -2123,7 +2086,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
return 1;
}
- if (func && has_modified_stack_frame(&state)) {
+ if (func && has_modified_stack_frame(insn, &state)) {
WARN_FUNC("return with modified stack frame",
sec, insn->offset);
return 1;
@@ -2190,6 +2153,13 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
break;
+ case INSN_EXCEPTION_RETURN:
+ if (insn->ret_tail) {
+ state.stack_size -= arch_exception_frame_size;
+ break;
+ }
+
+ /* fallthrough */
case INSN_CONTEXT_SWITCH:
if (func && (!next_insn || !next_insn->hint)) {
WARN_FUNC("unsupported instruction in callable function",
diff --git a/tools/objtool/check.h b/tools/objtool/check.h
index 6d875ca6fce0..138533ac23e2 100644
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -33,7 +33,8 @@ struct instruction {
unsigned int len;
enum insn_type type;
unsigned long immediate;
- bool alt_group, dead_end, ignore, hint, save, restore, ignore_alts;
+ bool alt_group, dead_end, ignore, ignore_alts;
+ bool hint, ret_tail;
bool retpoline_safe;
u8 visited;
struct symbol *call_dest;
On Mon, Mar 30, 2020 at 07:02:00PM +0200, Peter Zijlstra wrote:
> Subject: objtool: Implement RET_TAIL hint
>
> This replaces the SAVE/RESTORE hints with a RET_TAIL hint that applies to:
>
> - regular RETURN and sibling calls (which are also function exists)
> it allows the stack-frame to be off by one word, ie. it allows a
> return-tail-call.
>
> - EXCEPTION_RETURN (a new INSN_type that splits IRET out of
> CONTEXT_SWITCH) and here it denotes a return to self by having it
> consume arch_exception_frame_size bytes off the stack and continuing.
>
> Apply this hint to ftrace_64.S and sync_core(), the two existing users
> of the SAVE/RESTORE hints.
>
> For ftrace_64.S we split the return path and make sure the
> ftrace_epilogue call is seen as a sibling/tail-call turning it into it's
> own function.
>
> By splitting the return path every instruction has a unique stack setup
> and ORC can generate correct unwinds (XXX check if/how the ftrace
> trampolines map into the ORC). Then employ the RET_TAIL hint to the
> tail-call exit that has the direct-call (orig_eax) return-tail-call on.
>
> For sync_core() annotate the IRET with RET_TAIL to mark it as a
> control-flow NOP that consumes the exception frame.
I do like the idea to get rid of SAVE/RESTORE altogether. And it's nice
to make that ftrace code unwinder-deterministic.
However sync_core() and ftrace_regs_caller() are very different from
each other and I find the RET_TAIL hint usage to be extremely confusing.
For example, IRETQ isn't even a tail cail.
And the need for the hint to come *before* the insn which changes the
state is different from the other hints.
And now objtool has to know the arch exception stack size because of a
single code site.
And for a proper tail call, the stack should be empty. I don't
understand the +8 thing in has_modified_stack_frame(). It seems
hard-coded for the weird ftrace case, rather than for tail calls in
general (which should already work as designed).
How about a more general hint like UNWIND_HINT_ADJUST?
For sync_core(), after the IRETQ:
UNWIND_HINT_ADJUST sp_add=40
And ftrace_regs_caller_ret could have:
UNWIND_HINT_ADJUST sp_add=8
--
Josh
On Mon, Mar 30, 2020 at 02:02:05PM -0500, Josh Poimboeuf wrote:
> On Mon, Mar 30, 2020 at 07:02:00PM +0200, Peter Zijlstra wrote:
> > Subject: objtool: Implement RET_TAIL hint
> >
> > This replaces the SAVE/RESTORE hints with a RET_TAIL hint that applies to:
> >
> > - regular RETURN and sibling calls (which are also function exists)
> > it allows the stack-frame to be off by one word, ie. it allows a
> > return-tail-call.
> >
> > - EXCEPTION_RETURN (a new INSN_type that splits IRET out of
> > CONTEXT_SWITCH) and here it denotes a return to self by having it
> > consume arch_exception_frame_size bytes off the stack and continuing.
> >
> > Apply this hint to ftrace_64.S and sync_core(), the two existing users
> > of the SAVE/RESTORE hints.
> >
> > For ftrace_64.S we split the return path and make sure the
> > ftrace_epilogue call is seen as a sibling/tail-call turning it into it's
> > own function.
> >
> > By splitting the return path every instruction has a unique stack setup
> > and ORC can generate correct unwinds (XXX check if/how the ftrace
> > trampolines map into the ORC). Then employ the RET_TAIL hint to the
> > tail-call exit that has the direct-call (orig_eax) return-tail-call on.
> >
> > For sync_core() annotate the IRET with RET_TAIL to mark it as a
> > control-flow NOP that consumes the exception frame.
>
> I do like the idea to get rid of SAVE/RESTORE altogether. And it's nice
> to make that ftrace code unwinder-deterministic.
>
> However sync_core() and ftrace_regs_caller() are very different from
> each other and I find the RET_TAIL hint usage to be extremely confusing.
I was going with the pattern:
push target
ret
which is an indirect tail-call that doesn't need a register. We use it
in various places. We use it here exactly because it preserves all
registers, but we use it in function-graph tracer and retprobes to
insert the return handler. But also in retpoline, because it uses the
return stack predictor, which by happy accident isn't the indirect
branch predictor.
> For example, IRETQ isn't even a tail cail.
It's the same indirect call, except with a bigger frame ;-)
push # ss
push # rsp
push # flags
push # cs
push # ip
iret
> And the need for the hint to come *before* the insn which changes the
> state is different from the other hints.
makes sense to me... but yah.
> And now objtool has to know the arch exception stack size because of a
> single code site.
Agreed.
> And for a proper tail call, the stack should be empty.
All depends what you call proper :-)
> I don't
> understand the +8 thing in has_modified_stack_frame().
push target
ret
means we hit ret with one extra word on the stack.
> It seems
> hard-coded for the weird ftrace case, rather than for tail calls in
> general (which should already work as designed).
Like I said, we have it all over the place, but I suspect they're all
mostly hidden from objtool.
> How about a more general hint like UNWIND_HINT_ADJUST?
>
> For sync_core(), after the IRETQ:
>
> UNWIND_HINT_ADJUST sp_add=40
>
> And ftrace_regs_caller_ret could have:
>
> UNWIND_HINT_ADJUST sp_add=8
I like, I'll make it happen in the morning.
On Mon, Mar 30, 2020 at 10:02:54PM +0200, Peter Zijlstra wrote:
> On Mon, Mar 30, 2020 at 02:02:05PM -0500, Josh Poimboeuf wrote:
> > However sync_core() and ftrace_regs_caller() are very different from
> > each other and I find the RET_TAIL hint usage to be extremely confusing.
>
> I was going with the pattern:
>
> push target
> ret
>
> which is an indirect tail-call that doesn't need a register. We use it
> in various places. We use it here exactly because it preserves all
> registers, but we use it in function-graph tracer and retprobes to
> insert the return handler. But also in retpoline, because it uses the
> return stack predictor, which by happy accident isn't the indirect
> branch predictor.
>
> > For example, IRETQ isn't even a tail cail.
>
> It's the same indirect call, except with a bigger frame ;-)
>
> push # ss
> push # rsp
> push # flags
> push # cs
> push # ip
> iret
>
> > And the need for the hint to come *before* the insn which changes the
> > state is different from the other hints.
>
> makes sense to me... but yah.
Also, naturally, there are no instructions after RET to stick the
annotation to.
On Mon, Mar 30, 2020 at 10:02:54PM +0200, Peter Zijlstra wrote:
> On Mon, Mar 30, 2020 at 02:02:05PM -0500, Josh Poimboeuf wrote:
> > How about a more general hint like UNWIND_HINT_ADJUST?
> >
> > For sync_core(), after the IRETQ:
> >
> > UNWIND_HINT_ADJUST sp_add=40
> >
> > And ftrace_regs_caller_ret could have:
> >
> > UNWIND_HINT_ADJUST sp_add=8
>
> I like, I'll make it happen in the morning.
Still compile tested only. But I did find orc_ftrace_find() which should
indeed allow ORC unwinding of the ftrace trampolines.
---
Subject: objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET
This replaces the SAVE/RESTORE hints with a RET_OFFSET hint that applies
to the following instructions:
- any instruction that terminates a function, like: RETURN and sibling
calls. It allows the stack-frame to be off by @sp_offset, ie. it
allows stuffing the return stack.
- EXCEPTION_RETURN (a new INSN_type that splits IRET out of
CONTEXT_SWITCH) and here it denotes a @sp_offset sized POP and makes
the instruction continue.
Apply this hint to ftrace_64.S and sync_core(), the two existing users
of the SAVE/RESTORE hints.
For ftrace_64.S we split the return path and make sure the
ftrace_epilogue call is seen as a sibling/tail-call turning it into it's
own function.
By splitting the return path every instruction has a unique stack setup
and ORC can generate correct unwinds. Then employ the RET_OFFSET hint to
the tail-call exit that has the direct-call (orig_eax) stuffed on the
return stack.
For sync_core() annotate the IRET with RET_OFFSET to mark it as a
control-flow NOP that consumes the exception frame.
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
---
arch/x86/include/asm/orc_types.h | 11 ++++-
arch/x86/include/asm/processor.h | 3 +-
arch/x86/include/asm/unwind_hints.h | 12 ++----
arch/x86/kernel/ftrace.c | 12 +++++-
arch/x86/kernel/ftrace_64.S | 31 ++++++--------
tools/arch/x86/include/asm/orc_types.h | 11 ++++-
tools/objtool/Makefile | 2 +-
tools/objtool/arch.h | 1 +
tools/objtool/arch/x86/decode.c | 5 ++-
tools/objtool/check.c | 75 +++++++++++-----------------------
tools/objtool/check.h | 4 +-
11 files changed, 77 insertions(+), 90 deletions(-)
diff --git a/arch/x86/include/asm/orc_types.h b/arch/x86/include/asm/orc_types.h
index 6e060907c163..0133ec8d19b6 100644
--- a/arch/x86/include/asm/orc_types.h
+++ b/arch/x86/include/asm/orc_types.h
@@ -58,8 +58,15 @@
#define ORC_TYPE_CALL 0
#define ORC_TYPE_REGS 1
#define ORC_TYPE_REGS_IRET 2
-#define UNWIND_HINT_TYPE_SAVE 3
-#define UNWIND_HINT_TYPE_RESTORE 4
+
+/*
+ * RET_OFFSET: Used on instructions that terminate a function; mostly RETURN
+ * and sibling calls. On these, sp_offset denotes the expected offset from
+ * initial_func_cfi. It can also be used on EXCEPTION_RETURN instructions
+ * where sp_offset will denote the exception frame size consumed and will
+ * make objtool assume the instruction is a fall-through.
+ */
+#define UNWIND_HINT_TYPE_RET_OFFSET 3
#ifndef __ASSEMBLY__
/*
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 94789db550df..c8e0496e25dd 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -728,7 +728,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,8 +736,8 @@ static inline void sync_core(void)
"mov %%cs, %0\n\t"
"pushq %q0\n\t"
"pushq $1f\n\t"
+ UNWIND_HINT_RET_OFFSET(5*8)
"iretq\n\t"
- UNWIND_HINT_RESTORE
"1:"
: "=&r" (tmp), ASM_CALL_CONSTRAINT : : "cc", "memory");
#endif
diff --git a/arch/x86/include/asm/unwind_hints.h b/arch/x86/include/asm/unwind_hints.h
index f5e2eb12cb71..c4acd99716c1 100644
--- a/arch/x86/include/asm/unwind_hints.h
+++ b/arch/x86/include/asm/unwind_hints.h
@@ -86,12 +86,8 @@
UNWIND_HINT sp_offset=\sp_offset
.endm
-.macro UNWIND_HINT_SAVE
- UNWIND_HINT type=UNWIND_HINT_TYPE_SAVE
-.endm
-
-.macro UNWIND_HINT_RESTORE
- UNWIND_HINT type=UNWIND_HINT_TYPE_RESTORE
+.macro UNWIND_HINT_RET_OFFSET sp_offset=8
+ UNWIND_HINT type=UNWIND_HINT_TYPE_RET_OFFSET sp_offset=\sp_offset
.endm
#else /* !__ASSEMBLY__ */
@@ -108,9 +104,7 @@
".balign 4 \n\t" \
".popsection\n\t"
-#define UNWIND_HINT_SAVE UNWIND_HINT(0, 0, UNWIND_HINT_TYPE_SAVE, 0)
-
-#define UNWIND_HINT_RESTORE UNWIND_HINT(0, 0, UNWIND_HINT_TYPE_RESTORE, 0)
+#define UNWIND_HINT_RET_OFFSET(offset) UNWIND_HINT(0, (offset), UNWIND_HINT_TYPE_RET_OFFSET, 0)
#endif /* __ASSEMBLY__ */
diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 37a0aeaf89e7..3fda2ee7bf71 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -282,7 +282,8 @@ static inline void tramp_free(void *tramp) { }
/* Defined as markers to the end of the ftrace default trampolines */
extern void ftrace_regs_caller_end(void);
-extern void ftrace_epilogue(void);
+extern void ftrace_regs_caller_ret(void);
+extern void ftrace_caller_end(void);
extern void ftrace_caller_op_ptr(void);
extern void ftrace_regs_caller_op_ptr(void);
@@ -334,7 +335,7 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
call_offset = (unsigned long)ftrace_regs_call;
} else {
start_offset = (unsigned long)ftrace_caller;
- end_offset = (unsigned long)ftrace_epilogue;
+ end_offset = (unsigned long)ftrace_caller_end;
op_offset = (unsigned long)ftrace_caller_op_ptr;
call_offset = (unsigned long)ftrace_call;
}
@@ -366,6 +367,13 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
if (WARN_ON(ret < 0))
goto fail;
+ if (ops->flags & FTRACE_OPS_FL_SAVE_REGS) {
+ ip = ftrace_regs_caller_ret;
+ ret = probe_kernel_read(ip, (void *)retq, RET_SIZE);
+ if (WARN_ON(ret < 0))
+ goto fail;
+ }
+
/*
* The address of the ftrace_ops that is used for this trampoline
* is stored at the end of the trampoline. This will be used to
diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S
index 369e61faacfe..1e4a82ff97ea 100644
--- a/arch/x86/kernel/ftrace_64.S
+++ b/arch/x86/kernel/ftrace_64.S
@@ -157,8 +157,12 @@ SYM_INNER_LABEL(ftrace_call, SYM_L_GLOBAL)
* think twice before adding any new code or changing the
* layout here.
*/
-SYM_INNER_LABEL(ftrace_epilogue, SYM_L_GLOBAL)
+SYM_INNER_LABEL(ftrace_caller_end, SYM_L_GLOBAL)
+ jmp ftrace_epilogue
+SYM_FUNC_END(ftrace_caller);
+
+SYM_FUNC_START(ftrace_epilogue)
#ifdef CONFIG_FUNCTION_GRAPH_TRACER
SYM_INNER_LABEL(ftrace_graph_call, SYM_L_GLOBAL)
jmp ftrace_stub
@@ -170,14 +174,12 @@ SYM_INNER_LABEL(ftrace_graph_call, SYM_L_GLOBAL)
*/
SYM_INNER_LABEL_ALIGN(ftrace_stub, SYM_L_WEAK)
retq
-SYM_FUNC_END(ftrace_caller)
+SYM_FUNC_END(ftrace_epilogue)
SYM_FUNC_START(ftrace_regs_caller)
/* Save the current flags before any operations that can change them */
pushfq
- UNWIND_HINT_SAVE
-
/* added 8 bytes to save flags */
save_mcount_regs 8
/* save_mcount_regs fills in first two parameters */
@@ -235,8 +237,8 @@ SYM_INNER_LABEL(ftrace_regs_call, SYM_L_GLOBAL)
/* If ORIG_RAX is anything but zero, make this a call to that */
movq ORIG_RAX(%rsp), %rax
- cmpq $0, %rax
- je 1f
+ testq %rax, %rax
+ jz 1f
/* Swap the flags with orig_rax */
movq MCOUNT_REG_SIZE(%rsp), %rdi
@@ -244,20 +246,14 @@ SYM_INNER_LABEL(ftrace_regs_call, SYM_L_GLOBAL)
movq %rax, MCOUNT_REG_SIZE(%rsp)
restore_mcount_regs 8
+ /* Restore flags */
+ popfq
- jmp 2f
+SYM_INNER_LABEL(ftrace_regs_caller_ret, SYM_L_GLOBAL);
+ UNWIND_HINT_RET_OFFSET
+ jmp ftrace_epilogue
1: restore_mcount_regs
-
-
-2:
- /*
- * The stack layout is nondetermistic here, depending on which path was
- * taken. This confuses objtool and ORC, rightfully so. For now,
- * pretend the stack always looks like the non-direct case.
- */
- UNWIND_HINT_RESTORE
-
/* Restore flags */
popfq
@@ -268,7 +264,6 @@ SYM_INNER_LABEL(ftrace_regs_call, SYM_L_GLOBAL)
* to the return.
*/
SYM_INNER_LABEL(ftrace_regs_caller_end, SYM_L_GLOBAL)
-
jmp ftrace_epilogue
SYM_FUNC_END(ftrace_regs_caller)
diff --git a/tools/arch/x86/include/asm/orc_types.h b/tools/arch/x86/include/asm/orc_types.h
index 6e060907c163..0133ec8d19b6 100644
--- a/tools/arch/x86/include/asm/orc_types.h
+++ b/tools/arch/x86/include/asm/orc_types.h
@@ -58,8 +58,15 @@
#define ORC_TYPE_CALL 0
#define ORC_TYPE_REGS 1
#define ORC_TYPE_REGS_IRET 2
-#define UNWIND_HINT_TYPE_SAVE 3
-#define UNWIND_HINT_TYPE_RESTORE 4
+
+/*
+ * RET_OFFSET: Used on instructions that terminate a function; mostly RETURN
+ * and sibling calls. On these, sp_offset denotes the expected offset from
+ * initial_func_cfi. It can also be used on EXCEPTION_RETURN instructions
+ * where sp_offset will denote the exception frame size consumed and will
+ * make objtool assume the instruction is a fall-through.
+ */
+#define UNWIND_HINT_TYPE_RET_OFFSET 3
#ifndef __ASSEMBLY__
/*
diff --git a/tools/objtool/Makefile b/tools/objtool/Makefile
index ee08aeff30a1..ebbf92a8836f 100644
--- a/tools/objtool/Makefile
+++ b/tools/objtool/Makefile
@@ -31,7 +31,7 @@ INCLUDES := -I$(srctree)/tools/include \
-I$(srctree)/tools/arch/$(HOSTARCH)/include/uapi \
-I$(srctree)/tools/arch/$(SRCARCH)/include
WARNINGS := $(EXTRA_WARNINGS) -Wno-switch-default -Wno-switch-enum -Wno-packed
-CFLAGS := -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -g $(INCLUDES) $(LIBELF_FLAGS)
+CFLAGS := -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -ggdb3 $(INCLUDES) $(LIBELF_FLAGS)
LDFLAGS += $(LIBELF_LIBS) $(LIBSUBCMD) $(KBUILD_HOSTLDFLAGS)
# Allow old libelf to be used:
diff --git a/tools/objtool/arch.h b/tools/objtool/arch.h
index ced3765c4f44..cba72e1c47ce 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 a62e032863a8..1f9dd97e4a50 100644
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -431,10 +431,13 @@ int arch_decode_instruction(struct elf *elf, struct section *sec,
case 0xca: /* retf */
case 0xcb: /* retf */
- case 0xcf: /* iret */
*type = INSN_CONTEXT_SWITCH;
break;
+ case 0xcf: /* iret */
+ *type = INSN_EXCEPTION_RETURN;
+ break;
+
case 0xe8:
*type = INSN_CALL;
break;
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index e637a4a38d2a..502062803cce 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1259,13 +1259,8 @@ static int read_unwind_hints(struct objtool_file *file)
cfa = &insn->state.cfa;
- if (hint->type == UNWIND_HINT_TYPE_SAVE) {
- insn->save = true;
- continue;
-
- } else if (hint->type == UNWIND_HINT_TYPE_RESTORE) {
- insn->restore = true;
- insn->hint = true;
+ if (hint->type == UNWIND_HINT_TYPE_RET_OFFSET) {
+ insn->ret_offset = hint->sp_offset;
continue;
}
@@ -1429,16 +1424,23 @@ static bool is_fentry_call(struct instruction *insn)
return false;
}
-static bool has_modified_stack_frame(struct insn_state *state)
+static bool has_modified_stack_frame(struct instruction *insn, struct insn_state *state)
{
+ u8 ret_offset = insn->ret_offset;
int i;
if (state->cfa.base != initial_func_cfi.cfa.base ||
- state->cfa.offset != initial_func_cfi.cfa.offset ||
- state->stack_size != initial_func_cfi.cfa.offset ||
state->drap)
return true;
+ if (state->cfa.offset != initial_func_cfi.cfa.offset &&
+ !(ret_offset && state->cfa.offset == initial_func_cfi.cfa.offset + ret_offset))
+ return true;
+
+ if (state->stack_size != initial_func_cfi.cfa.offset &&
+ !(ret_offset && state->stack_size == initial_func_cfi.cfa.offset + ret_offset))
+ return true;
+
for (i = 0; i < CFI_NUM_REGS; i++)
if (state->regs[i].base != initial_func_cfi.regs[i].base ||
state->regs[i].offset != initial_func_cfi.regs[i].offset)
@@ -1984,7 +1986,7 @@ static int validate_call(struct instruction *insn, struct insn_state *state)
static int validate_sibling_call(struct instruction *insn, struct insn_state *state)
{
- if (has_modified_stack_frame(state)) {
+ if (has_modified_stack_frame(insn, state)) {
WARN_FUNC("sibling call from callable instruction with modified stack frame",
insn->sec, insn->offset);
return 1;
@@ -2041,47 +2043,9 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
return 0;
}
- if (insn->hint) {
- if (insn->restore) {
- struct instruction *save_insn, *i;
-
- i = insn;
- save_insn = NULL;
- func_for_each_insn_continue_reverse(file, func, i) {
- if (i->save) {
- save_insn = i;
- break;
- }
- }
-
- if (!save_insn) {
- WARN_FUNC("no corresponding CFI save for CFI restore",
- sec, insn->offset);
- return 1;
- }
-
- 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;
- }
-
- insn->state = save_insn->state;
- }
-
+ if (insn->hint)
state = insn->state;
-
- } else
+ else
insn->state = state;
insn->visited |= visited;
@@ -2123,7 +2087,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
return 1;
}
- if (func && has_modified_stack_frame(&state)) {
+ if (func && has_modified_stack_frame(insn, &state)) {
WARN_FUNC("return with modified stack frame",
sec, insn->offset);
return 1;
@@ -2190,6 +2154,13 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
break;
+ case INSN_EXCEPTION_RETURN:
+ if (insn->ret_offset) {
+ state.stack_size -= insn->ret_offset;
+ break;
+ }
+
+ /* fallthrough */
case INSN_CONTEXT_SWITCH:
if (func && (!next_insn || !next_insn->hint)) {
WARN_FUNC("unsupported instruction in callable function",
diff --git a/tools/objtool/check.h b/tools/objtool/check.h
index 6d875ca6fce0..aaccaef8f074 100644
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -33,9 +33,11 @@ struct instruction {
unsigned int len;
enum insn_type type;
unsigned long immediate;
- bool alt_group, dead_end, ignore, hint, save, restore, ignore_alts;
+ bool alt_group, dead_end, ignore, ignore_alts;
+ bool hint;
bool retpoline_safe;
u8 visited;
+ u8 ret_offset;
struct symbol *call_dest;
struct instruction *jump_dest;
struct instruction *first_jump_src;
On Tue, 31 Mar 2020 13:16:52 +0200
Peter Zijlstra <[email protected]> wrote:
> @@ -235,8 +237,8 @@ SYM_INNER_LABEL(ftrace_regs_call, SYM_L_GLOBAL)
>
> /* If ORIG_RAX is anything but zero, make this a call to that */
> movq ORIG_RAX(%rsp), %rax
> - cmpq $0, %rax
> - je 1f
> + testq %rax, %rax
> + jz 1f
>
> /* Swap the flags with orig_rax */
> movq MCOUNT_REG_SIZE(%rsp), %rdi
Hi Peter,
Can you send this change as a separate patch as it has nothing to do with
this current change, and is a clean up patch that stands on its own.
Thanks,
-- Steve
On Tue, Mar 31, 2020 at 11:31:36AM -0400, Steven Rostedt wrote:
> On Tue, 31 Mar 2020 13:16:52 +0200
> Peter Zijlstra <[email protected]> wrote:
>
> > @@ -235,8 +237,8 @@ SYM_INNER_LABEL(ftrace_regs_call, SYM_L_GLOBAL)
> >
> > /* If ORIG_RAX is anything but zero, make this a call to that */
> > movq ORIG_RAX(%rsp), %rax
> > - cmpq $0, %rax
> > - je 1f
> > + testq %rax, %rax
> > + jz 1f
> >
> > /* Swap the flags with orig_rax */
> > movq MCOUNT_REG_SIZE(%rsp), %rdi
>
> Hi Peter,
>
> Can you send this change as a separate patch as it has nothing to do with
> this current change, and is a clean up patch that stands on its own.
Sure. But then I have to like write a Changelog for it... :/
---
Subject: x86,ftrace: Shrink ftrace_regs_caller() by one byte
'Optimize' ftrace_regs_caller. Instead of comparing against an
immediate, the more natural way to test for zero on x86 is: 'test
%r,%r'.
48 83 f8 00 cmp $0x0,%rax
74 49 je 226 <ftrace_regs_call+0xa3>
48 85 c0 test %rax,%rax
74 49 je 225 <ftrace_regs_call+0xa2>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
---
arch/x86/kernel/ftrace_64.S | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S
index 369e61faacfe..8e71c492d623 100644
--- a/arch/x86/kernel/ftrace_64.S
+++ b/arch/x86/kernel/ftrace_64.S
@@ -235,8 +235,8 @@ SYM_INNER_LABEL(ftrace_regs_call, SYM_L_GLOBAL)
/* If ORIG_RAX is anything but zero, make this a call to that */
movq ORIG_RAX(%rsp), %rax
- cmpq $0, %rax
- je 1f
+ testq %rax, %rax
+ jz 1f
/* Swap the flags with orig_rax */
movq MCOUNT_REG_SIZE(%rsp), %rdi
On Tue, Mar 31, 2020 at 11:31:36AM -0400, Steven Rostedt wrote:
> Can you send this change as a separate patch as it has nothing to do with
> this current change, and is a clean up patch that stands on its own.
I also found this.. should I write it up?
---
diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S
index 369e61faacfe..0f108096f278 100644
--- a/arch/x86/kernel/ftrace_64.S
+++ b/arch/x86/kernel/ftrace_64.S
@@ -23,7 +23,7 @@
#endif /* CONFIG_FRAME_POINTER */
/* Size of stack used to save mcount regs in save_mcount_regs */
-#define MCOUNT_REG_SIZE (SS+8 + MCOUNT_FRAME_SIZE)
+#define MCOUNT_REG_SIZE (SIZEOF_PTREGS + MCOUNT_FRAME_SIZE)
/*
* gcc -pg option adds a call to 'mcount' in most functions.
@@ -77,7 +77,7 @@
/*
* We add enough stack to save all regs.
*/
- subq $(MCOUNT_REG_SIZE - MCOUNT_FRAME_SIZE), %rsp
+ subq $(SIZEOF_PTREGS), %rsp
movq %rax, RAX(%rsp)
movq %rcx, RCX(%rsp)
movq %rdx, RDX(%rsp)
On Tue, Mar 31, 2020 at 01:16:52PM +0200, Peter Zijlstra wrote:
> Subject: objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET
>
> This replaces the SAVE/RESTORE hints with a RET_OFFSET hint that applies
> to the following instructions:
>
> - any instruction that terminates a function, like: RETURN and sibling
> calls. It allows the stack-frame to be off by @sp_offset, ie. it
> allows stuffing the return stack.
>
> - EXCEPTION_RETURN (a new INSN_type that splits IRET out of
> CONTEXT_SWITCH) and here it denotes a @sp_offset sized POP and makes
> the instruction continue.
Looking closer, I see how my UNWIND_HINT_ADJUST idea doesn't work for
the ftrace_regs_caller() case. The ORC data is actually correct there.
So basically we need a way to tell objtool to be quiet.
I now understand what you're trying to do with the RET_TAIL thing, and I
guess it's ok for the ftrace case. But I'd rather an UNWIND_HINT_IGNORE
before the tail cail, which would tell objtool to just silence the tail
call warning. It's simpler for the user to understand, it's simpler
logic in objtool, and I think an "ignore warnings for the next insn"
hint would be more generally applicable anyway.
But also... the RET_OFFSET usage for sync_core() *really* bugs me.
I know you said it's like an indirect tail call with a bigger frame, but
that's kind of stretching it because the function frame is still there.
And objtool doesn't treat it like a tail call at all. In fact, it
handles it *completely* differently from the normal ret-tail-call case.
Instead of silencing a tail call warning, it adjusts the stack offset
and continues the code path.
This basically adds *two* new hint types, while trying to call them the
same thing. There's no overlapping functionality between them in
objtool, other than the use of the same insn->ret_offset variable. But
it's two distinct functionalities, depending on the context (return/tail
vs IRETQ).
I'll try to work up some patches with a different approach in a bit.
--
Josh
On Tue, Mar 31, 2020 at 09:58:41PM +0200, Peter Zijlstra wrote:
> On Tue, Mar 31, 2020 at 11:31:36AM -0400, Steven Rostedt wrote:
>
> > Can you send this change as a separate patch as it has nothing to do with
> > this current change, and is a clean up patch that stands on its own.
>
> I also found this.. should I write it up?
>
> ---
> diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S
> index 369e61faacfe..0f108096f278 100644
> --- a/arch/x86/kernel/ftrace_64.S
> +++ b/arch/x86/kernel/ftrace_64.S
> @@ -23,7 +23,7 @@
> #endif /* CONFIG_FRAME_POINTER */
>
> /* Size of stack used to save mcount regs in save_mcount_regs */
> -#define MCOUNT_REG_SIZE (SS+8 + MCOUNT_FRAME_SIZE)
> +#define MCOUNT_REG_SIZE (SIZEOF_PTREGS + MCOUNT_FRAME_SIZE)
>
> /*
> * gcc -pg option adds a call to 'mcount' in most functions.
> @@ -77,7 +77,7 @@
> /*
> * We add enough stack to save all regs.
> */
> - subq $(MCOUNT_REG_SIZE - MCOUNT_FRAME_SIZE), %rsp
> + subq $(SIZEOF_PTREGS), %rsp
> movq %rax, RAX(%rsp)
> movq %rcx, RCX(%rsp)
> movq %rdx, RDX(%rsp)
I was going to suggest the same thing :-)
Acked-by: Josh Poimboeuf <[email protected]>
--
Josh
On Tue, Mar 31, 2020 at 03:23:15PM -0500, Josh Poimboeuf wrote:
> On Tue, Mar 31, 2020 at 01:16:52PM +0200, Peter Zijlstra wrote:
> > Subject: objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET
> >
> > This replaces the SAVE/RESTORE hints with a RET_OFFSET hint that applies
> > to the following instructions:
> >
> > - any instruction that terminates a function, like: RETURN and sibling
> > calls. It allows the stack-frame to be off by @sp_offset, ie. it
> > allows stuffing the return stack.
> >
> > - EXCEPTION_RETURN (a new INSN_type that splits IRET out of
> > CONTEXT_SWITCH) and here it denotes a @sp_offset sized POP and makes
> > the instruction continue.
>
> Looking closer, I see how my UNWIND_HINT_ADJUST idea doesn't work for
> the ftrace_regs_caller() case. The ORC data is actually correct there.
> So basically we need a way to tell objtool to be quiet.
Right.
> I now understand what you're trying to do with the RET_TAIL thing, and I
> guess it's ok for the ftrace case. But I'd rather an UNWIND_HINT_IGNORE
> before the tail cail, which would tell objtool to just silence the tail
> call warning. It's simpler for the user to understand, it's simpler
> logic in objtool, and I think an "ignore warnings for the next insn"
> hint would be more generally applicable anyway.
I like how this is specific on how far the stack can be off, as opposed
so say 'ignore any warning on this instruction'.
Because by saying this RET should be +8, we'll still get a warning when
this is not the case (and in fact I should strengthen the patch to
implement that).
Also, you don't want to suppress any other valid warning at that
instruction.
Furthermore, I really don't think we ought to worry about ease-of-use
here, there's really not that many people writing x86 assembly.
> But also... the RET_OFFSET usage for sync_core() *really* bugs me.
Fair enough.
> I know you said it's like an indirect tail call with a bigger frame, but
> that's kind of stretching it because the function frame is still there.
>
> And objtool doesn't treat it like a tail call at all. In fact, it
> handles it *completely* differently from the normal ret-tail-call case.
> Instead of silencing a tail call warning, it adjusts the stack offset
> and continues the code path.
>
> This basically adds *two* new hint types, while trying to call them the
> same thing. There's no overlapping functionality between them in
> objtool, other than the use of the same insn->ret_offset variable. But
> it's two distinct functionalities, depending on the context (return/tail
> vs IRETQ).
I'm not against adding a second/separate hint for this. In fact, I
almost considered teaching objtool how to interpret the whole IRET frame
so that we can do it without hints. It's just that that's too much code
for this one case.
HINT_IRET_SELF ?
On Tue, Mar 31, 2020 at 10:40:47PM +0200, Peter Zijlstra wrote:
> On Tue, Mar 31, 2020 at 03:23:15PM -0500, Josh Poimboeuf wrote:
> > I now understand what you're trying to do with the RET_TAIL thing, and I
> > guess it's ok for the ftrace case. But I'd rather an UNWIND_HINT_IGNORE
> > before the tail cail, which would tell objtool to just silence the tail
> > call warning. It's simpler for the user to understand, it's simpler
> > logic in objtool, and I think an "ignore warnings for the next insn"
> > hint would be more generally applicable anyway.
>
> I like how this is specific on how far the stack can be off, as opposed
> so say 'ignore any warning on this instruction'.
>
> Because by saying this RET should be +8, we'll still get a warning when
> this is not the case (and in fact I should strengthen the patch to
> implement that).
Like this; I'm confused on what cfa.offset is vs stack_size though.
But this way we're strict and always warn when the unexpected happens.
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1423,8 +1423,7 @@ static bool has_modified_stack_frame(str
!(ret_offset && state->cfa.offset == initial_func_cfi.cfa.offset + ret_offset))
return true;
- if (state->stack_size != initial_func_cfi.cfa.offset &&
- !(ret_offset && state->stack_size == initial_func_cfi.cfa.offset + ret_offset))
+ if (state->stack_size != initial_func_cfi.cfa.offset + ret_offset)
return true;
for (i = 0; i < CFI_NUM_REGS; i++) {
On Tue, Mar 31, 2020 at 10:40:47PM +0200, Peter Zijlstra wrote:
> > I now understand what you're trying to do with the RET_TAIL thing, and I
> > guess it's ok for the ftrace case. But I'd rather an UNWIND_HINT_IGNORE
> > before the tail cail, which would tell objtool to just silence the tail
> > call warning. It's simpler for the user to understand, it's simpler
> > logic in objtool, and I think an "ignore warnings for the next insn"
> > hint would be more generally applicable anyway.
>
> I like how this is specific on how far the stack can be off, as opposed
> so say 'ignore any warning on this instruction'.
>
> Because by saying this RET should be +8, we'll still get a warning when
> this is not the case (and in fact I should strengthen the patch to
> implement that).
>
> Also, you don't want to suppress any other valid warning at that
> instruction.
Ok, I guess I'm convinced :-) As we continue to add new warnings to
objtool, it is true that "ignore all warnings at this insn" is probably
too broad.
/me stops writing patch
BTW, if we're in agreement that this hint doesn't belong for
sync_core(), will sp_offset always be +8? Just wondering if we can
hard-code that assumption.
> > I know you said it's like an indirect tail call with a bigger frame, but
> > that's kind of stretching it because the function frame is still there.
> >
> > And objtool doesn't treat it like a tail call at all. In fact, it
> > handles it *completely* differently from the normal ret-tail-call case.
> > Instead of silencing a tail call warning, it adjusts the stack offset
> > and continues the code path.
> >
> > This basically adds *two* new hint types, while trying to call them the
> > same thing. There's no overlapping functionality between them in
> > objtool, other than the use of the same insn->ret_offset variable. But
> > it's two distinct functionalities, depending on the context (return/tail
> > vs IRETQ).
>
> I'm not against adding a second/separate hint for this. In fact, I
> almost considered teaching objtool how to interpret the whole IRET frame
> so that we can do it without hints. It's just that that's too much code
> for this one case.
>
> HINT_IRET_SELF ?
Despite my earlier complaint about stack size knowledge, we could just
forget the hint and make "iretq in C code" equivalent to "reduce stack
size by arch_exception_stack_size()" and keep going. There's
file->c_file which tells you it's a C file.
--
Josh
On Tue, Mar 31, 2020 at 04:17:58PM -0500, Josh Poimboeuf wrote:
> > I'm not against adding a second/separate hint for this. In fact, I
> > almost considered teaching objtool how to interpret the whole IRET frame
> > so that we can do it without hints. It's just that that's too much code
> > for this one case.
> >
> > HINT_IRET_SELF ?
>
> Despite my earlier complaint about stack size knowledge, we could just
> forget the hint and make "iretq in C code" equivalent to "reduce stack
> size by arch_exception_stack_size()" and keep going. There's
> file->c_file which tells you it's a C file.
Or maybe "iretq in an STT_FUNC" is better since this pattern could
presumably happen in a callable asm function.
--
Josh
On Tue, Mar 31, 2020 at 04:20:40PM -0500, Josh Poimboeuf wrote:
> On Tue, Mar 31, 2020 at 04:17:58PM -0500, Josh Poimboeuf wrote:
> > > I'm not against adding a second/separate hint for this. In fact, I
> > > almost considered teaching objtool how to interpret the whole IRET frame
> > > so that we can do it without hints. It's just that that's too much code
> > > for this one case.
> > >
> > > HINT_IRET_SELF ?
> >
> > Despite my earlier complaint about stack size knowledge, we could just
> > forget the hint and make "iretq in C code" equivalent to "reduce stack
> > size by arch_exception_stack_size()" and keep going. There's
> > file->c_file which tells you it's a C file.
>
> Or maybe "iretq in an STT_FUNC" is better since this pattern could
> presumably happen in a callable asm function.
Like so then?
---
Subject: objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET
From: Peter Zijlstra <[email protected]>
Date: Tue, 31 Mar 2020 13:16:52 +0200
This replaces the SAVE/RESTORE hints with a RET_OFFSET hint that
applies to any instruction that terminates a function, like: RETURN
and sibling calls. It allows the stack-frame to be off by @sp_offset,
ie. it allows stuffing the return stack.
For ftrace_64.S we split the return path and make sure the
ftrace_epilogue call is seen as a sibling/tail-call turning it into it's
own function.
By splitting the return path every instruction has a unique stack setup
and ORC can generate correct unwinds. Then employ the RET_OFFSET hint to
the tail-call exit that has the direct-call (orig_eax) stuffed on the
return stack.
For sync_core() we teach objtool that an IRET inside an STT_FUNC
simply consumes the exception stack and continues.
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
---
arch/x86/include/asm/orc_types.h | 9 ++-
arch/x86/include/asm/processor.h | 2
arch/x86/include/asm/unwind_hints.h | 12 +---
arch/x86/kernel/ftrace.c | 12 ++++
arch/x86/kernel/ftrace_64.S | 27 ++++-------
tools/arch/x86/include/asm/orc_types.h | 9 ++-
tools/objtool/Makefile | 2
tools/objtool/arch.h | 3 +
tools/objtool/arch/x86/decode.c | 5 +-
tools/objtool/check.c | 80 ++++++++++-----------------------
tools/objtool/check.h | 4 +
11 files changed, 74 insertions(+), 91 deletions(-)
--- a/arch/x86/include/asm/orc_types.h
+++ b/arch/x86/include/asm/orc_types.h
@@ -58,8 +58,13 @@
#define ORC_TYPE_CALL 0
#define ORC_TYPE_REGS 1
#define ORC_TYPE_REGS_IRET 2
-#define UNWIND_HINT_TYPE_SAVE 3
-#define UNWIND_HINT_TYPE_RESTORE 4
+
+/*
+ * RET_OFFSET: Used on instructions that terminate a function; mostly RETURN
+ * and sibling calls. On these, sp_offset denotes the expected offset from
+ * initial_func_cfi.
+ */
+#define UNWIND_HINT_TYPE_RET_OFFSET 3
#ifndef __ASSEMBLY__
/*
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -738,7 +738,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"
@@ -748,7 +747,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/arch/x86/include/asm/unwind_hints.h
+++ b/arch/x86/include/asm/unwind_hints.h
@@ -86,12 +86,8 @@
UNWIND_HINT sp_offset=\sp_offset
.endm
-.macro UNWIND_HINT_SAVE
- UNWIND_HINT type=UNWIND_HINT_TYPE_SAVE
-.endm
-
-.macro UNWIND_HINT_RESTORE
- UNWIND_HINT type=UNWIND_HINT_TYPE_RESTORE
+.macro UNWIND_HINT_RET_OFFSET sp_offset=8
+ UNWIND_HINT type=UNWIND_HINT_TYPE_RET_OFFSET sp_offset=\sp_offset
.endm
#else /* !__ASSEMBLY__ */
@@ -108,9 +104,7 @@
".balign 4 \n\t" \
".popsection\n\t"
-#define UNWIND_HINT_SAVE UNWIND_HINT(0, 0, UNWIND_HINT_TYPE_SAVE, 0)
-
-#define UNWIND_HINT_RESTORE UNWIND_HINT(0, 0, UNWIND_HINT_TYPE_RESTORE, 0)
+#define UNWIND_HINT_RET_OFFSET(offset) UNWIND_HINT(0, (offset), UNWIND_HINT_TYPE_RET_OFFSET, 0)
#endif /* __ASSEMBLY__ */
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -282,7 +282,8 @@ static inline void tramp_free(void *tram
/* Defined as markers to the end of the ftrace default trampolines */
extern void ftrace_regs_caller_end(void);
-extern void ftrace_epilogue(void);
+extern void ftrace_regs_caller_ret(void);
+extern void ftrace_caller_end(void);
extern void ftrace_caller_op_ptr(void);
extern void ftrace_regs_caller_op_ptr(void);
@@ -334,7 +335,7 @@ create_trampoline(struct ftrace_ops *ops
call_offset = (unsigned long)ftrace_regs_call;
} else {
start_offset = (unsigned long)ftrace_caller;
- end_offset = (unsigned long)ftrace_epilogue;
+ end_offset = (unsigned long)ftrace_caller_end;
op_offset = (unsigned long)ftrace_caller_op_ptr;
call_offset = (unsigned long)ftrace_call;
}
@@ -366,6 +367,13 @@ create_trampoline(struct ftrace_ops *ops
if (WARN_ON(ret < 0))
goto fail;
+ if (ops->flags & FTRACE_OPS_FL_SAVE_REGS) {
+ ip = ftrace_regs_caller_ret;
+ ret = probe_kernel_read(ip, (void *)retq, RET_SIZE);
+ if (WARN_ON(ret < 0))
+ goto fail;
+ }
+
/*
* The address of the ftrace_ops that is used for this trampoline
* is stored at the end of the trampoline. This will be used to
--- a/arch/x86/kernel/ftrace_64.S
+++ b/arch/x86/kernel/ftrace_64.S
@@ -157,8 +157,12 @@ SYM_INNER_LABEL(ftrace_call, SYM_L_GLOBA
* think twice before adding any new code or changing the
* layout here.
*/
-SYM_INNER_LABEL(ftrace_epilogue, SYM_L_GLOBAL)
+SYM_INNER_LABEL(ftrace_caller_end, SYM_L_GLOBAL)
+ jmp ftrace_epilogue
+SYM_FUNC_END(ftrace_caller);
+
+SYM_FUNC_START(ftrace_epilogue)
#ifdef CONFIG_FUNCTION_GRAPH_TRACER
SYM_INNER_LABEL(ftrace_graph_call, SYM_L_GLOBAL)
jmp ftrace_stub
@@ -170,14 +174,12 @@ SYM_INNER_LABEL(ftrace_graph_call, SYM_L
*/
SYM_INNER_LABEL_ALIGN(ftrace_stub, SYM_L_WEAK)
retq
-SYM_FUNC_END(ftrace_caller)
+SYM_FUNC_END(ftrace_epilogue)
SYM_FUNC_START(ftrace_regs_caller)
/* Save the current flags before any operations that can change them */
pushfq
- UNWIND_HINT_SAVE
-
/* added 8 bytes to save flags */
save_mcount_regs 8
/* save_mcount_regs fills in first two parameters */
@@ -244,20 +246,14 @@ SYM_INNER_LABEL(ftrace_regs_call, SYM_L_
movq %rax, MCOUNT_REG_SIZE(%rsp)
restore_mcount_regs 8
+ /* Restore flags */
+ popfq
- jmp 2f
+SYM_INNER_LABEL(ftrace_regs_caller_ret, SYM_L_GLOBAL);
+ UNWIND_HINT_RET_OFFSET
+ jmp ftrace_epilogue
1: restore_mcount_regs
-
-
-2:
- /*
- * The stack layout is nondetermistic here, depending on which path was
- * taken. This confuses objtool and ORC, rightfully so. For now,
- * pretend the stack always looks like the non-direct case.
- */
- UNWIND_HINT_RESTORE
-
/* Restore flags */
popfq
@@ -268,7 +264,6 @@ SYM_INNER_LABEL(ftrace_regs_call, SYM_L_
* to the return.
*/
SYM_INNER_LABEL(ftrace_regs_caller_end, SYM_L_GLOBAL)
-
jmp ftrace_epilogue
SYM_FUNC_END(ftrace_regs_caller)
--- a/tools/arch/x86/include/asm/orc_types.h
+++ b/tools/arch/x86/include/asm/orc_types.h
@@ -58,8 +58,13 @@
#define ORC_TYPE_CALL 0
#define ORC_TYPE_REGS 1
#define ORC_TYPE_REGS_IRET 2
-#define UNWIND_HINT_TYPE_SAVE 3
-#define UNWIND_HINT_TYPE_RESTORE 4
+
+/*
+ * RET_OFFSET: Used on instructions that terminate a function; mostly RETURN
+ * and sibling calls. On these, sp_offset denotes the expected offset from
+ * initial_func_cfi.
+ */
+#define UNWIND_HINT_TYPE_RET_OFFSET 3
#ifndef __ASSEMBLY__
/*
--- a/tools/objtool/Makefile
+++ b/tools/objtool/Makefile
@@ -31,7 +31,7 @@ INCLUDES := -I$(srctree)/tools/include \
-I$(srctree)/tools/arch/$(HOSTARCH)/include/uapi \
-I$(srctree)/tools/arch/$(SRCARCH)/include
WARNINGS := $(EXTRA_WARNINGS) -Wno-switch-default -Wno-switch-enum -Wno-packed
-CFLAGS := -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -g $(INCLUDES) $(LIBELF_FLAGS)
+CFLAGS := -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -ggdb3 $(INCLUDES) $(LIBELF_FLAGS)
LDFLAGS += $(LIBELF_LIBS) $(LIBSUBCMD) $(KBUILD_HOSTLDFLAGS)
# Allow old libelf to be used:
--- 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,
@@ -75,4 +76,6 @@ int arch_decode_instruction(struct elf *
bool arch_callee_saved_reg(unsigned char reg);
+static const int arch_exception_frame_size = 5*8;
+
#endif /* _ARCH_H */
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -431,10 +431,13 @@ int arch_decode_instruction(struct elf *
case 0xca: /* retf */
case 0xcb: /* retf */
- case 0xcf: /* iret */
*type = INSN_CONTEXT_SWITCH;
break;
+ case 0xcf: /* iret */
+ *type = INSN_EXCEPTION_RETURN;
+ break;
+
case 0xe8:
*type = INSN_CALL;
break;
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1246,13 +1246,8 @@ static int read_unwind_hints(struct objt
cfa = &insn->state.cfa;
- if (hint->type == UNWIND_HINT_TYPE_SAVE) {
- insn->save = true;
- continue;
-
- } else if (hint->type == UNWIND_HINT_TYPE_RESTORE) {
- insn->restore = true;
- insn->hint = true;
+ if (hint->type == UNWIND_HINT_TYPE_RET_OFFSET) {
+ insn->ret_offset = hint->sp_offset;
continue;
}
@@ -1416,20 +1411,26 @@ static bool is_fentry_call(struct instru
return false;
}
-static bool has_modified_stack_frame(struct insn_state *state)
+static bool has_modified_stack_frame(struct instruction *insn, struct insn_state *state)
{
+ u8 ret_offset = insn->ret_offset;
int i;
- if (state->cfa.base != initial_func_cfi.cfa.base ||
- state->cfa.offset != initial_func_cfi.cfa.offset ||
- state->stack_size != initial_func_cfi.cfa.offset ||
- state->drap)
+ if (state->cfa.base != initial_func_cfi.cfa.base || state->drap)
+ return true;
+
+ if (state->cfa.offset != initial_func_cfi.cfa.offset &&
+ !(ret_offset && state->cfa.offset == initial_func_cfi.cfa.offset + ret_offset))
+ return true;
+
+ if (state->stack_size != initial_func_cfi.cfa.offset + ret_offset)
return true;
- for (i = 0; i < CFI_NUM_REGS; i++)
+ for (i = 0; i < CFI_NUM_REGS; i++) {
if (state->regs[i].base != initial_func_cfi.regs[i].base ||
state->regs[i].offset != initial_func_cfi.regs[i].offset)
return true;
+ }
return false;
}
@@ -1971,7 +1972,7 @@ static int validate_call(struct instruct
static int validate_sibling_call(struct instruction *insn, struct insn_state *state)
{
- if (has_modified_stack_frame(state)) {
+ if (has_modified_stack_frame(insn, state)) {
WARN_FUNC("sibling call from callable instruction with modified stack frame",
insn->sec, insn->offset);
return 1;
@@ -2000,7 +2001,7 @@ static int validate_return(struct symbol
return 1;
}
- if (func && has_modified_stack_frame(state)) {
+ if (func && has_modified_stack_frame(insn, state)) {
WARN_FUNC("return with modified stack frame",
insn->sec, insn->offset);
return 1;
@@ -2063,47 +2064,9 @@ static int validate_branch(struct objtoo
return 0;
}
- if (insn->hint) {
- if (insn->restore) {
- struct instruction *save_insn, *i;
-
- i = insn;
- save_insn = NULL;
- sym_for_each_insn_continue_reverse(file, func, i) {
- if (i->save) {
- save_insn = i;
- break;
- }
- }
-
- if (!save_insn) {
- WARN_FUNC("no corresponding CFI save for CFI restore",
- sec, insn->offset);
- return 1;
- }
-
- 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;
- }
-
- insn->state = save_insn->state;
- }
-
+ if (insn->hint)
state = insn->state;
-
- } else
+ else
insn->state = state;
insn->visited |= visited;
@@ -2185,6 +2148,13 @@ static int validate_branch(struct objtoo
break;
+ case INSN_EXCEPTION_RETURN:
+ if (func) {
+ state.stack_size -= arch_exception_frame_size;
+ break;
+ }
+
+ /* fallthrough */
case INSN_CONTEXT_SWITCH:
if (func && (!next_insn || !next_insn->hint)) {
WARN_FUNC("unsupported instruction in callable function",
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -33,9 +33,11 @@ struct instruction {
unsigned int len;
enum insn_type type;
unsigned long immediate;
- bool alt_group, dead_end, ignore, hint, save, restore, ignore_alts;
+ bool alt_group, dead_end, ignore, ignore_alts;
+ bool hint;
bool retpoline_safe;
u8 visited;
+ u8 ret_offset;
struct symbol *call_dest;
struct instruction *jump_dest;
struct instruction *first_jump_src;
On Wed, Apr 01, 2020 at 12:27:03AM +0200, Peter Zijlstra wrote:
> On Tue, Mar 31, 2020 at 04:20:40PM -0500, Josh Poimboeuf wrote:
> > On Tue, Mar 31, 2020 at 04:17:58PM -0500, Josh Poimboeuf wrote:
> > > > I'm not against adding a second/separate hint for this. In fact, I
> > > > almost considered teaching objtool how to interpret the whole IRET frame
> > > > so that we can do it without hints. It's just that that's too much code
> > > > for this one case.
> > > >
> > > > HINT_IRET_SELF ?
> > >
> > > Despite my earlier complaint about stack size knowledge, we could just
> > > forget the hint and make "iretq in C code" equivalent to "reduce stack
> > > size by arch_exception_stack_size()" and keep going. There's
> > > file->c_file which tells you it's a C file.
> >
> > Or maybe "iretq in an STT_FUNC" is better since this pattern could
> > presumably happen in a callable asm function.
>
> Like so then?
I'd suggest a patch split like:
1) objtool: automagic IRET-in-func
2) objtool: add RET_OFFSET
3) ftrace: re-organize asm (and use RET_OFFSET hint)
4) objtool: remove now-unused SAVE/RESTORE
> --- a/arch/x86/include/asm/orc_types.h
> +++ b/arch/x86/include/asm/orc_types.h
> @@ -58,8 +58,13 @@
> #define ORC_TYPE_CALL 0
> #define ORC_TYPE_REGS 1
> #define ORC_TYPE_REGS_IRET 2
> -#define UNWIND_HINT_TYPE_SAVE 3
> -#define UNWIND_HINT_TYPE_RESTORE 4
> +
> +/*
> + * RET_OFFSET: Used on instructions that terminate a function; mostly RETURN
> + * and sibling calls. On these, sp_offset denotes the expected offset from
> + * initial_func_cfi.
> + */
> +#define UNWIND_HINT_TYPE_RET_OFFSET 3
I think this comment belongs at the UNWIND_HINT_RET_OFFSET macro
definition.
> --- a/arch/x86/kernel/ftrace.c
> +++ b/arch/x86/kernel/ftrace.c
> @@ -282,7 +282,8 @@ static inline void tramp_free(void *tram
>
> /* Defined as markers to the end of the ftrace default trampolines */
> extern void ftrace_regs_caller_end(void);
> -extern void ftrace_epilogue(void);
> +extern void ftrace_regs_caller_ret(void);
> +extern void ftrace_caller_end(void);
> extern void ftrace_caller_op_ptr(void);
> extern void ftrace_regs_caller_op_ptr(void);
>
> @@ -334,7 +335,7 @@ create_trampoline(struct ftrace_ops *ops
> call_offset = (unsigned long)ftrace_regs_call;
> } else {
> start_offset = (unsigned long)ftrace_caller;
> - end_offset = (unsigned long)ftrace_epilogue;
> + end_offset = (unsigned long)ftrace_caller_end;
> op_offset = (unsigned long)ftrace_caller_op_ptr;
> call_offset = (unsigned long)ftrace_call;
> }
> @@ -366,6 +367,13 @@ create_trampoline(struct ftrace_ops *ops
> if (WARN_ON(ret < 0))
> goto fail;
>
> + if (ops->flags & FTRACE_OPS_FL_SAVE_REGS) {
> + ip = ftrace_regs_caller_ret;
> + ret = probe_kernel_read(ip, (void *)retq, RET_SIZE);
> + if (WARN_ON(ret < 0))
> + goto fail;
> + }
> +
Hm? This function creates a trampoline but it looks like this change is
overwriting the original ftrace_64 code itself?
> --- a/tools/objtool/Makefile
> +++ b/tools/objtool/Makefile
> @@ -31,7 +31,7 @@ INCLUDES := -I$(srctree)/tools/include \
> -I$(srctree)/tools/arch/$(HOSTARCH)/include/uapi \
> -I$(srctree)/tools/arch/$(SRCARCH)/include
> WARNINGS := $(EXTRA_WARNINGS) -Wno-switch-default -Wno-switch-enum -Wno-packed
> -CFLAGS := -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -g $(INCLUDES) $(LIBELF_FLAGS)
> +CFLAGS := -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -ggdb3 $(INCLUDES) $(LIBELF_FLAGS)
> LDFLAGS += $(LIBELF_LIBS) $(LIBSUBCMD) $(KBUILD_HOSTLDFLAGS)
Why? Smells like a separate patch at least.
--
Josh
On Wed, Apr 01, 2020 at 09:14:02AM -0500, Josh Poimboeuf wrote:
> On Wed, Apr 01, 2020 at 12:27:03AM +0200, Peter Zijlstra wrote:
> > On Tue, Mar 31, 2020 at 04:20:40PM -0500, Josh Poimboeuf wrote:
> > > On Tue, Mar 31, 2020 at 04:17:58PM -0500, Josh Poimboeuf wrote:
> > > > > I'm not against adding a second/separate hint for this. In fact, I
> > > > > almost considered teaching objtool how to interpret the whole IRET frame
> > > > > so that we can do it without hints. It's just that that's too much code
> > > > > for this one case.
> > > > >
> > > > > HINT_IRET_SELF ?
> > > >
> > > > Despite my earlier complaint about stack size knowledge, we could just
> > > > forget the hint and make "iretq in C code" equivalent to "reduce stack
> > > > size by arch_exception_stack_size()" and keep going. There's
> > > > file->c_file which tells you it's a C file.
> > >
> > > Or maybe "iretq in an STT_FUNC" is better since this pattern could
> > > presumably happen in a callable asm function.
> >
> > Like so then?
>
> I'd suggest a patch split like:
>
> 1) objtool: automagic IRET-in-func
> 2) objtool: add RET_OFFSET
> 3) ftrace: re-organize asm (and use RET_OFFSET hint)
> 4) objtool: remove now-unused SAVE/RESTORE
Sure.
> > --- a/arch/x86/include/asm/orc_types.h
> > +++ b/arch/x86/include/asm/orc_types.h
> > @@ -58,8 +58,13 @@
> > #define ORC_TYPE_CALL 0
> > #define ORC_TYPE_REGS 1
> > #define ORC_TYPE_REGS_IRET 2
> > -#define UNWIND_HINT_TYPE_SAVE 3
> > -#define UNWIND_HINT_TYPE_RESTORE 4
> > +
> > +/*
> > + * RET_OFFSET: Used on instructions that terminate a function; mostly RETURN
> > + * and sibling calls. On these, sp_offset denotes the expected offset from
> > + * initial_func_cfi.
> > + */
> > +#define UNWIND_HINT_TYPE_RET_OFFSET 3
>
> I think this comment belongs at the UNWIND_HINT_RET_OFFSET macro
> definition.
Humph, ok, but there's two of those :/
> > --- a/arch/x86/kernel/ftrace.c
> > +++ b/arch/x86/kernel/ftrace.c
> > @@ -282,7 +282,8 @@ static inline void tramp_free(void *tram
> >
> > /* Defined as markers to the end of the ftrace default trampolines */
> > extern void ftrace_regs_caller_end(void);
> > -extern void ftrace_epilogue(void);
> > +extern void ftrace_regs_caller_ret(void);
> > +extern void ftrace_caller_end(void);
> > extern void ftrace_caller_op_ptr(void);
> > extern void ftrace_regs_caller_op_ptr(void);
> >
> > @@ -334,7 +335,7 @@ create_trampoline(struct ftrace_ops *ops
> > call_offset = (unsigned long)ftrace_regs_call;
> > } else {
> > start_offset = (unsigned long)ftrace_caller;
> > - end_offset = (unsigned long)ftrace_epilogue;
> > + end_offset = (unsigned long)ftrace_caller_end;
> > op_offset = (unsigned long)ftrace_caller_op_ptr;
> > call_offset = (unsigned long)ftrace_call;
> > }
> > @@ -366,6 +367,13 @@ create_trampoline(struct ftrace_ops *ops
> > if (WARN_ON(ret < 0))
> > goto fail;
> >
> > + if (ops->flags & FTRACE_OPS_FL_SAVE_REGS) {
> > + ip = ftrace_regs_caller_ret;
> > + ret = probe_kernel_read(ip, (void *)retq, RET_SIZE);
> > + if (WARN_ON(ret < 0))
> > + goto fail;
> > + }
> > +
>
> Hm? This function creates a trampoline but it looks like this change is
> overwriting the original ftrace_64 code itself?
Ahh. So if you look at what the trampoline copies, you'll note we'll
copy until -- but *NOT* including -- the jmp ftrace_epilogue. Instead
we'll write a RET at the end.
However, due to splitting the return path, such that each instruction
has a unique stack offset, we now have a second jmp ftrace_epilogue in
the middle of the function. That too needs to be overwritten by a RET.
> > --- a/tools/objtool/Makefile
> > +++ b/tools/objtool/Makefile
> > @@ -31,7 +31,7 @@ INCLUDES := -I$(srctree)/tools/include \
> > -I$(srctree)/tools/arch/$(HOSTARCH)/include/uapi \
> > -I$(srctree)/tools/arch/$(SRCARCH)/include
> > WARNINGS := $(EXTRA_WARNINGS) -Wno-switch-default -Wno-switch-enum -Wno-packed
> > -CFLAGS := -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -g $(INCLUDES) $(LIBELF_FLAGS)
> > +CFLAGS := -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -ggdb3 $(INCLUDES) $(LIBELF_FLAGS)
> > LDFLAGS += $(LIBELF_LIBS) $(LIBSUBCMD) $(KBUILD_HOSTLDFLAGS)
>
> Why? Smells like a separate patch at least.
Oh, whoops :-) I keep doing this every time I need to run gdb on it.
I'll make it go away.
On Wed, Apr 01, 2020 at 04:22:26PM +0200, Peter Zijlstra wrote:
> > > --- a/arch/x86/include/asm/orc_types.h
> > > +++ b/arch/x86/include/asm/orc_types.h
> > > @@ -58,8 +58,13 @@
> > > #define ORC_TYPE_CALL 0
> > > #define ORC_TYPE_REGS 1
> > > #define ORC_TYPE_REGS_IRET 2
> > > -#define UNWIND_HINT_TYPE_SAVE 3
> > > -#define UNWIND_HINT_TYPE_RESTORE 4
> > > +
> > > +/*
> > > + * RET_OFFSET: Used on instructions that terminate a function; mostly RETURN
> > > + * and sibling calls. On these, sp_offset denotes the expected offset from
> > > + * initial_func_cfi.
> > > + */
> > > +#define UNWIND_HINT_TYPE_RET_OFFSET 3
> >
> > I think this comment belongs at the UNWIND_HINT_RET_OFFSET macro
> > definition.
>
> Humph, ok, but there's two of those :/
Just commenting the asm one should be fine I think.
That way a reader of code using the macro is more likely to understand
its purpose.
> > > --- a/arch/x86/kernel/ftrace.c
> > > +++ b/arch/x86/kernel/ftrace.c
> > > @@ -282,7 +282,8 @@ static inline void tramp_free(void *tram
> > >
> > > /* Defined as markers to the end of the ftrace default trampolines */
> > > extern void ftrace_regs_caller_end(void);
> > > -extern void ftrace_epilogue(void);
> > > +extern void ftrace_regs_caller_ret(void);
> > > +extern void ftrace_caller_end(void);
> > > extern void ftrace_caller_op_ptr(void);
> > > extern void ftrace_regs_caller_op_ptr(void);
> > >
> > > @@ -334,7 +335,7 @@ create_trampoline(struct ftrace_ops *ops
> > > call_offset = (unsigned long)ftrace_regs_call;
> > > } else {
> > > start_offset = (unsigned long)ftrace_caller;
> > > - end_offset = (unsigned long)ftrace_epilogue;
> > > + end_offset = (unsigned long)ftrace_caller_end;
> > > op_offset = (unsigned long)ftrace_caller_op_ptr;
> > > call_offset = (unsigned long)ftrace_call;
> > > }
> > > @@ -366,6 +367,13 @@ create_trampoline(struct ftrace_ops *ops
> > > if (WARN_ON(ret < 0))
> > > goto fail;
> > >
> > > + if (ops->flags & FTRACE_OPS_FL_SAVE_REGS) {
> > > + ip = ftrace_regs_caller_ret;
> > > + ret = probe_kernel_read(ip, (void *)retq, RET_SIZE);
> > > + if (WARN_ON(ret < 0))
> > > + goto fail;
> > > + }
> > > +
> >
> > Hm? This function creates a trampoline but it looks like this change is
> > overwriting the original ftrace_64 code itself?
>
> Ahh. So if you look at what the trampoline copies, you'll note we'll
> copy until -- but *NOT* including -- the jmp ftrace_epilogue. Instead
> we'll write a RET at the end.
>
> However, due to splitting the return path, such that each instruction
> has a unique stack offset, we now have a second jmp ftrace_epilogue in
> the middle of the function. That too needs to be overwritten by a RET.
Right, but 'ip' needs to point to the trampoline's version of
'ftrace_regs_caller_ret', not the original ftrace_64 version.
> > > --- a/tools/objtool/Makefile
> > > +++ b/tools/objtool/Makefile
> > > @@ -31,7 +31,7 @@ INCLUDES := -I$(srctree)/tools/include \
> > > -I$(srctree)/tools/arch/$(HOSTARCH)/include/uapi \
> > > -I$(srctree)/tools/arch/$(SRCARCH)/include
> > > WARNINGS := $(EXTRA_WARNINGS) -Wno-switch-default -Wno-switch-enum -Wno-packed
> > > -CFLAGS := -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -g $(INCLUDES) $(LIBELF_FLAGS)
> > > +CFLAGS := -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -ggdb3 $(INCLUDES) $(LIBELF_FLAGS)
> > > LDFLAGS += $(LIBELF_LIBS) $(LIBSUBCMD) $(KBUILD_HOSTLDFLAGS)
> >
> > Why? Smells like a separate patch at least.
>
> Oh, whoops :-) I keep doing this every time I need to run gdb on it.
> I'll make it go away.
Ha, I do something similar, I just add -O0 to CFLAGS.
--
Josh
On Wed, Apr 01, 2020 at 09:39:16AM -0500, Josh Poimboeuf wrote:
> On Wed, Apr 01, 2020 at 04:22:26PM +0200, Peter Zijlstra wrote:
> > > > @@ -366,6 +367,13 @@ create_trampoline(struct ftrace_ops *ops
> > > > if (WARN_ON(ret < 0))
> > > > goto fail;
> > > >
> > > > + if (ops->flags & FTRACE_OPS_FL_SAVE_REGS) {
> > > > + ip = ftrace_regs_caller_ret;
> > > > + ret = probe_kernel_read(ip, (void *)retq, RET_SIZE);
> > > > + if (WARN_ON(ret < 0))
> > > > + goto fail;
> > > > + }
> > > > +
> Right, but 'ip' needs to point to the trampoline's version of
> 'ftrace_regs_caller_ret', not the original ftrace_64 version.
Duh,
ip = trampline + (ftrace_regs_caller_ret - ftrace_regs_caller);
it is.
Luckily this would've triggerd a splat the first time I'd have actually
booted this, because the regular text is RO, while the trampoline is
still RW here. Still, good spotting.
Hi Peter,
On 3/31/20 11:27 PM, Peter Zijlstra wrote:
> On Tue, Mar 31, 2020 at 04:20:40PM -0500, Josh Poimboeuf wrote:
>> On Tue, Mar 31, 2020 at 04:17:58PM -0500, Josh Poimboeuf wrote:
>>>> I'm not against adding a second/separate hint for this. In fact, I
>>>> almost considered teaching objtool how to interpret the whole IRET frame
>>>> so that we can do it without hints. It's just that that's too much code
>>>> for this one case.
>>>>
>>>> HINT_IRET_SELF ?
>>>
>>> Despite my earlier complaint about stack size knowledge, we could just
>>> forget the hint and make "iretq in C code" equivalent to "reduce stack
>>> size by arch_exception_stack_size()" and keep going. There's
>>> file->c_file which tells you it's a C file.
>>
>> Or maybe "iretq in an STT_FUNC" is better since this pattern could
>> presumably happen in a callable asm function.
>
> Like so then?
>
> ---
> Subject: objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET
> From: Peter Zijlstra <[email protected]>
> Date: Tue, 31 Mar 2020 13:16:52 +0200
>
> This replaces the SAVE/RESTORE hints with a RET_OFFSET hint that
> applies to any instruction that terminates a function, like: RETURN
> and sibling calls. It allows the stack-frame to be off by @sp_offset,
> ie. it allows stuffing the return stack.
>
> For ftrace_64.S we split the return path and make sure the
> ftrace_epilogue call is seen as a sibling/tail-call turning it into it's
> own function.
>
> By splitting the return path every instruction has a unique stack setup
> and ORC can generate correct unwinds. Then employ the RET_OFFSET hint to
> the tail-call exit that has the direct-call (orig_eax) stuffed on the
> return stack.
>
> For sync_core() we teach objtool that an IRET inside an STT_FUNC
> simply consumes the exception stack and continues.
>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> ---
> arch/x86/include/asm/orc_types.h | 9 ++-
> arch/x86/include/asm/processor.h | 2
> arch/x86/include/asm/unwind_hints.h | 12 +---
> arch/x86/kernel/ftrace.c | 12 ++++
> arch/x86/kernel/ftrace_64.S | 27 ++++-------
> tools/arch/x86/include/asm/orc_types.h | 9 ++-
> tools/objtool/Makefile | 2
> tools/objtool/arch.h | 3 +
> tools/objtool/arch/x86/decode.c | 5 +-
> tools/objtool/check.c | 80 ++++++++++-----------------------
> tools/objtool/check.h | 4 +
> 11 files changed, 74 insertions(+), 91 deletions(-)
>
[snip]
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -1246,13 +1246,8 @@ static int read_unwind_hints(struct objt
>
> cfa = &insn->state.cfa;
>
> - if (hint->type == UNWIND_HINT_TYPE_SAVE) {
> - insn->save = true;
> - continue;
> -
> - } else if (hint->type == UNWIND_HINT_TYPE_RESTORE) {
> - insn->restore = true;
> - insn->hint = true;
> + if (hint->type == UNWIND_HINT_TYPE_RET_OFFSET) {
> + insn->ret_offset = hint->sp_offset;
> continue;
> }
>
> @@ -1416,20 +1411,26 @@ static bool is_fentry_call(struct instru
> return false;
> }
>
> -static bool has_modified_stack_frame(struct insn_state *state)
> +static bool has_modified_stack_frame(struct instruction *insn, struct insn_state *state)
> {
> + u8 ret_offset = insn->ret_offset;
> int i;
>
> - if (state->cfa.base != initial_func_cfi.cfa.base ||
> - state->cfa.offset != initial_func_cfi.cfa.offset ||
> - state->stack_size != initial_func_cfi.cfa.offset ||
> - state->drap)
> + if (state->cfa.base != initial_func_cfi.cfa.base || state->drap)
> + return true;
> +
> + if (state->cfa.offset != initial_func_cfi.cfa.offset &&
> + !(ret_offset && state->cfa.offset == initial_func_cfi.cfa.offset + ret_offset))
Isn't that the same thing as "state->cfa.offset !=
initial_func_cfi.cfa.offset + ret_offset" ?
> + return true;
> +
> + if (state->stack_size != initial_func_cfi.cfa.offset + ret_offset)
> return true;
>
> - for (i = 0; i < CFI_NUM_REGS; i++)
> + for (i = 0; i < CFI_NUM_REGS; i++) {
> if (state->regs[i].base != initial_func_cfi.regs[i].base ||
> state->regs[i].offset != initial_func_cfi.regs[i].offset)
> return true;
> + }
>
> return false;
> }
> @@ -1971,7 +1972,7 @@ static int validate_call(struct instruct
>
> static int validate_sibling_call(struct instruction *insn, struct insn_state *state)
> {
> - if (has_modified_stack_frame(state)) {
> + if (has_modified_stack_frame(insn, state)) {
> WARN_FUNC("sibling call from callable instruction with modified stack frame",
> insn->sec, insn->offset);
> return 1;
> @@ -2000,7 +2001,7 @@ static int validate_return(struct symbol
> return 1;
> }
>
> - if (func && has_modified_stack_frame(state)) {
> + if (func && has_modified_stack_frame(insn, state)) {
> WARN_FUNC("return with modified stack frame",
> insn->sec, insn->offset);
> return 1;
> @@ -2063,47 +2064,9 @@ static int validate_branch(struct objtoo
> return 0;
> }
>
> - if (insn->hint) {
> - if (insn->restore) {
> - struct instruction *save_insn, *i;
> -
> - i = insn;
> - save_insn = NULL;
> - sym_for_each_insn_continue_reverse(file, func, i) {
> - if (i->save) {
> - save_insn = i;
> - break;
> - }
> - }
> -
> - if (!save_insn) {
> - WARN_FUNC("no corresponding CFI save for CFI restore",
> - sec, insn->offset);
> - return 1;
> - }
> -
> - 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;
> - }
> -
> - insn->state = save_insn->state;
> - }
> -
> + if (insn->hint)
> state = insn->state;
> -
> - } else
> + else
> insn->state = state;
>
> insn->visited |= visited;
> @@ -2185,6 +2148,13 @@ static int validate_branch(struct objtoo
>
> break;
>
> + case INSN_EXCEPTION_RETURN:
> + if (func) {
> + state.stack_size -= arch_exception_frame_size;
> + break;
Why break instead of returning? Shouldn't an exception return mark the
end of a branch (whether inside or outside a function) ?
Here it seems it will continue to the next instruction which might have
been unreachable.
> + }
> +
> + /* fallthrough */
What is the purpose of the fallthrough here? If the exception return was
in a function, it carried on to the next instruction, so it won't use
the WARN_FUNC(). So, if I'm looking at the right version of the code
only the "return 0;" will be used. And, unless my previous comment is
wrong, I'd argue that we should return both for func and !func.
> case INSN_CONTEXT_SWITCH:
> if (func && (!next_insn || !next_insn->hint)) {
> WARN_FUNC("unsupported instruction in callable function",
> --- a/tools/objtool/check.h
> +++ b/tools/objtool/check.h
> @@ -33,9 +33,11 @@ struct instruction {
> unsigned int len;
> enum insn_type type;
> unsigned long immediate;
> - bool alt_group, dead_end, ignore, hint, save, restore, ignore_alts;
> + bool alt_group, dead_end, ignore, ignore_alts;
> + bool hint;
> bool retpoline_safe;
> u8 visited;
> + u8 ret_offset;
> struct symbol *call_dest;
> struct instruction *jump_dest;
> struct instruction *first_jump_src;
>
>
Cheers,
--
Julien Thierry
On Wed, 1 Apr 2020 09:39:16 -0500
Josh Poimboeuf <[email protected]> wrote:
> > > > + if (ops->flags & FTRACE_OPS_FL_SAVE_REGS) {
> > > > + ip = ftrace_regs_caller_ret;
> > > > + ret = probe_kernel_read(ip, (void *)retq, RET_SIZE);
> > > > + if (WARN_ON(ret < 0))
> > > > + goto fail;
> > > > + }
> > > > +
> > >
> > > Hm? This function creates a trampoline but it looks like this change is
> > > overwriting the original ftrace_64 code itself?
> >
> > Ahh. So if you look at what the trampoline copies, you'll note we'll
> > copy until -- but *NOT* including -- the jmp ftrace_epilogue. Instead
> > we'll write a RET at the end.
> >
> > However, due to splitting the return path, such that each instruction
> > has a unique stack offset, we now have a second jmp ftrace_epilogue in
> > the middle of the function. That too needs to be overwritten by a RET.
>
> Right, but 'ip' needs to point to the trampoline's version of
> 'ftrace_regs_caller_ret', not the original ftrace_64 version.
I noticed the same thing. And after applying just this patch (with tweaking
the obtool mark ups to make it compile) it crashed.
-- Steve
On Wed, Apr 01, 2020 at 04:43:35PM +0100, Julien Thierry wrote:
> > +static bool has_modified_stack_frame(struct instruction *insn, struct insn_state *state)
> > {
> > + u8 ret_offset = insn->ret_offset;
> > int i;
> >
> > - if (state->cfa.base != initial_func_cfi.cfa.base ||
> > - state->cfa.offset != initial_func_cfi.cfa.offset ||
> > - state->stack_size != initial_func_cfi.cfa.offset ||
> > - state->drap)
> > + if (state->cfa.base != initial_func_cfi.cfa.base || state->drap)
> > + return true;
> > +
> > + if (state->cfa.offset != initial_func_cfi.cfa.offset &&
> > + !(ret_offset && state->cfa.offset == initial_func_cfi.cfa.offset + ret_offset))
>
> Isn't that the same thing as "state->cfa.offset !=
> initial_func_cfi.cfa.offset + ret_offset" ?
I'm confused on what cfa.offset is, sometimes it increase with
stack_size, sometimes it doesn't.
ISTR that for the ftrace case it was indeed cfa.offset + 8, but for the
IRET case below (where it is now not used anymore) it was cfa.offset
(not cfa.offset + 40, which I was expecting).
> > + return true;
> > +
> > + if (state->stack_size != initial_func_cfi.cfa.offset + ret_offset)
> > return true;
> >
> > - for (i = 0; i < CFI_NUM_REGS; i++)
> > + for (i = 0; i < CFI_NUM_REGS; i++) {
> > if (state->regs[i].base != initial_func_cfi.regs[i].base ||
> > state->regs[i].offset != initial_func_cfi.regs[i].offset)
> > return true;
> > + }
> >
> > return false;
> > }
> > @@ -2185,6 +2148,13 @@ static int validate_branch(struct objtoo
> >
> > break;
> >
> > + case INSN_EXCEPTION_RETURN:
> > + if (func) {
> > + state.stack_size -= arch_exception_frame_size;
> > + break;
>
> Why break instead of returning? Shouldn't an exception return mark the end
> of a branch (whether inside or outside a function) ?
>
> Here it seems it will continue to the next instruction which might have been
> unreachable.
The code in question (x86's sync_core()), is an exception return to
self. It pushes an exception frame that points to right after the
exception return instruction.
This is the only usage of IRET in STT_FUNC symbols.
So rather than teaching objtool how to interpret the whole
push;push;push;push;push;iret sequence, teach it how big the frame is
(arch_exception_frame_size) and let it continue.
All the other (real) IRETs are in STT_NOTYPE in the entry assembly.
> > + }
> > +
> > + /* fallthrough */
>
> What is the purpose of the fallthrough here? If the exception return was in
> a function, it carried on to the next instruction, so it won't use the
> WARN_FUNC(). So, if I'm looking at the right version of the code only the
> "return 0;" will be used. And, unless my previous comment is wrong, I'd
> argue that we should return both for func and !func.
That came from the fact that we split it out of INSN_CONTEXT_SWITCH.
You're right that it has now reduced to just return 0.
> > case INSN_CONTEXT_SWITCH:
> > if (func && (!next_insn || !next_insn->hint)) {
> > WARN_FUNC("unsupported instruction in callable function",
On Wed, Apr 01, 2020 at 07:09:10PM +0200, Peter Zijlstra wrote:
> On Wed, Apr 01, 2020 at 04:43:35PM +0100, Julien Thierry wrote:
>
> > > +static bool has_modified_stack_frame(struct instruction *insn, struct insn_state *state)
> > > {
> > > + u8 ret_offset = insn->ret_offset;
> > > int i;
> > >
> > > - if (state->cfa.base != initial_func_cfi.cfa.base ||
> > > - state->cfa.offset != initial_func_cfi.cfa.offset ||
> > > - state->stack_size != initial_func_cfi.cfa.offset ||
> > > - state->drap)
> > > + if (state->cfa.base != initial_func_cfi.cfa.base || state->drap)
> > > + return true;
> > > +
> > > + if (state->cfa.offset != initial_func_cfi.cfa.offset &&
> > > + !(ret_offset && state->cfa.offset == initial_func_cfi.cfa.offset + ret_offset))
> >
> > Isn't that the same thing as "state->cfa.offset !=
> > initial_func_cfi.cfa.offset + ret_offset" ?
>
> I'm confused on what cfa.offset is, sometimes it increase with
> stack_size, sometimes it doesn't.
>
> ISTR that for the ftrace case it was indeed cfa.offset + 8, but for the
> IRET case below (where it is now not used anymore) it was cfa.offset
> (not cfa.offset + 40, which I was expecting).
It depends on the value of cfa.base. If cfa.base is CFI_SP, then
cfa.offset changes with stack_size. If cfa.base is CFI_BP (i.e. if the
function uses a frame pointer), then cfa.offset is constant (the
distance between RBP on the stack and the previous frame).
--
Josh
On Wed, Apr 01, 2020 at 01:33:03PM -0400, Steven Rostedt wrote:
> On Wed, 1 Apr 2020 19:09:10 +0200
> Peter Zijlstra <[email protected]> wrote:
>
> > > > + if (state->cfa.offset != initial_func_cfi.cfa.offset &&
> > > > + !(ret_offset && state->cfa.offset == initial_func_cfi.cfa.offset + ret_offset))
> > >
> > > Isn't that the same thing as "state->cfa.offset !=
> > > initial_func_cfi.cfa.offset + ret_offset" ?
> >
> > I'm confused on what cfa.offset is, sometimes it increase with
> > stack_size, sometimes it doesn't.
>
> I believe what Julien is saying is the above logic is equivalent:
>
> if (x != y &&
> !(z && x == y + z))
>
> is the same as:
>
> if (x != y + z)
It is not, the former will accept either x==y || x==y+z, while the
latter will only accept x==y+z.
For stack_size, I'm confident in just x==y+z, for offset I saw
conflicting things.
Since the annotation is now used in only a single place, maybe x==y+z
will just work, I'll go try once I've managed to unconfuse myself on the
whole fp-unwind situation.
On Wed, 1 Apr 2020 19:45:44 +0200
Peter Zijlstra <[email protected]> wrote:
> > I believe what Julien is saying is the above logic is equivalent:
> >
> > if (x != y &&
> > !(z && x == y + z))
> >
> > is the same as:
> >
> > if (x != y + z)
>
> It is not, the former will accept either x==y || x==y+z, while the
> latter will only accept x==y+z.
No, the former accepts:
x==y || (z && x == y + z)
Which is the same as: x == y + z
As the second condition is only tested if z != 0, and x == y is the same
as x == y + 0
-- Steve
On Wed, 1 Apr 2020 19:09:10 +0200
Peter Zijlstra <[email protected]> wrote:
> > > + if (state->cfa.offset != initial_func_cfi.cfa.offset &&
> > > + !(ret_offset && state->cfa.offset == initial_func_cfi.cfa.offset + ret_offset))
> >
> > Isn't that the same thing as "state->cfa.offset !=
> > initial_func_cfi.cfa.offset + ret_offset" ?
>
> I'm confused on what cfa.offset is, sometimes it increase with
> stack_size, sometimes it doesn't.
I believe what Julien is saying is the above logic is equivalent:
if (x != y &&
!(z && x == y + z))
is the same as:
if (x != y + z)
-- Steve
On Wed, Apr 01, 2020 at 02:20:15PM -0400, Steven Rostedt wrote:
> On Wed, 1 Apr 2020 19:45:44 +0200
> Peter Zijlstra <[email protected]> wrote:
>
> > > I believe what Julien is saying is the above logic is equivalent:
> > >
> > > if (x != y &&
> > > !(z && x == y + z))
> > >
> > > is the same as:
> > >
> > > if (x != y + z)
> >
> > It is not, the former will accept either x==y || x==y+z, while the
> > latter will only accept x==y+z.
>
> No, the former accepts:
>
> x==y || (z && x == y + z)
>
> Which is the same as: x == y + z
>
> As the second condition is only tested if z != 0, and x == y is the same
> as x == y + 0
Right, so it accepts both +0 and +z, while the latter will only accept
+z.
( in the iret case I had offset at +0 and stack_size at +40, while with
the ftrace case I had both at +8; which is why I wrote the form that
accepts +0 and +z )
Anyway, I tested it, and for the ftrace case (the only current user of
the hint) +z is correct for both offset and stack_size. I build both FP
and ORC variants.
On 4/1/20 6:09 PM, Peter Zijlstra wrote:
> On Wed, Apr 01, 2020 at 04:43:35PM +0100, Julien Thierry wrote:
>
>>> +static bool has_modified_stack_frame(struct instruction *insn, struct insn_state *state)
>>> {
>>> + u8 ret_offset = insn->ret_offset;
>>> int i;
>>>
>>> - if (state->cfa.base != initial_func_cfi.cfa.base ||
>>> - state->cfa.offset != initial_func_cfi.cfa.offset ||
>>> - state->stack_size != initial_func_cfi.cfa.offset ||
>>> - state->drap)
>>> + if (state->cfa.base != initial_func_cfi.cfa.base || state->drap)
>>> + return true;
>>> +
>>> + if (state->cfa.offset != initial_func_cfi.cfa.offset &&
>>> + !(ret_offset && state->cfa.offset == initial_func_cfi.cfa.offset + ret_offset))
>>
>> Isn't that the same thing as "state->cfa.offset !=
>> initial_func_cfi.cfa.offset + ret_offset" ?
>
> I'm confused on what cfa.offset is, sometimes it increase with
> stack_size, sometimes it doesn't.
>
Steven already replied for me about that :) .
> ISTR that for the ftrace case it was indeed cfa.offset + 8, but for the
> IRET case below (where it is now not used anymore) it was cfa.offset
> (not cfa.offset + 40, which I was expecting).
>
>>> + return true;
>>> +
>>> + if (state->stack_size != initial_func_cfi.cfa.offset + ret_offset)
>>> return true;
>>>
>>> - for (i = 0; i < CFI_NUM_REGS; i++)
>>> + for (i = 0; i < CFI_NUM_REGS; i++) {
>>> if (state->regs[i].base != initial_func_cfi.regs[i].base ||
>>> state->regs[i].offset != initial_func_cfi.regs[i].offset)
>>> return true;
>>> + }
>>>
>>> return false;
>>> }
>
>>> @@ -2185,6 +2148,13 @@ static int validate_branch(struct objtoo
>>>
>>> break;
>>>
>>> + case INSN_EXCEPTION_RETURN:
>>> + if (func) {
>>> + state.stack_size -= arch_exception_frame_size;
>>> + break;
>>
>> Why break instead of returning? Shouldn't an exception return mark the end
>> of a branch (whether inside or outside a function) ?
>>
>> Here it seems it will continue to the next instruction which might have been
>> unreachable.
>
> The code in question (x86's sync_core()), is an exception return to
> self. It pushes an exception frame that points to right after the
> exception return instruction.
>
> This is the only usage of IRET in STT_FUNC symbols.
>
> So rather than teaching objtool how to interpret the whole
> push;push;push;push;push;iret sequence, teach it how big the frame is
> (arch_exception_frame_size) and let it continue.
>
> All the other (real) IRETs are in STT_NOTYPE in the entry assembly.
>
Right, I see.. However I'm not completely convinced by this. I must
admit I haven't followed the whole conversation, but what was the issue
with the HINT_IRET_SELF? It seemed more elegant, but I might be missing
some context.
Otherwise, it might be worth having a comment in the code to point that
this only handles the sync_core() case.
Also, instead of adding a special "arch_exception_frame_size", I could
suggest:
- Picking this patch [1] from a completely arbitrary source
- Getting rid of INSN_STACK type, any instruction could then include
stack ops on top of their existing semantics, they can just have an
empty list if they don't touch SP/BP
- x86 decoder adds a stack_op to the iret to modify the stack pointer by
the right amount
[1] https://www.spinics.net/lists/kernel/msg3453725.html
Thanks,
--
Julien Thierry
On 4/2/20 7:41 AM, Julien Thierry wrote:
>
>
> On 4/1/20 6:09 PM, Peter Zijlstra wrote:
>> On Wed, Apr 01, 2020 at 04:43:35PM +0100, Julien Thierry wrote:
>>>> + return true;
>>>> +
>>>> + if (state->stack_size != initial_func_cfi.cfa.offset + ret_offset)
>>>> return true;
>>>>
>>>> - for (i = 0; i < CFI_NUM_REGS; i++)
>>>> + for (i = 0; i < CFI_NUM_REGS; i++) {
>>>> if (state->regs[i].base != initial_func_cfi.regs[i].base ||
>>>> state->regs[i].offset !=
>>>> initial_func_cfi.regs[i].offset)
>>>> return true;
>>>> + }
>>>>
>>>> return false;
>>>> }
>>
>>>> @@ -2185,6 +2148,13 @@ static int validate_branch(struct objtoo
>>>>
>>>> break;
>>>>
>>>> + case INSN_EXCEPTION_RETURN:
>>>> + if (func) {
>>>> + state.stack_size -= arch_exception_frame_size;
>>>> + break;
>>>
>>> Why break instead of returning? Shouldn't an exception return mark
>>> the end
>>> of a branch (whether inside or outside a function) ?
>>>
>>> Here it seems it will continue to the next instruction which might
>>> have been
>>> unreachable.
>>
>> The code in question (x86's sync_core()), is an exception return to
>> self. It pushes an exception frame that points to right after the
>> exception return instruction.
>>
>> This is the only usage of IRET in STT_FUNC symbols.
>>
>> So rather than teaching objtool how to interpret the whole
>> push;push;push;push;push;iret sequence, teach it how big the frame is
>> (arch_exception_frame_size) and let it continue.
>>
>> All the other (real) IRETs are in STT_NOTYPE in the entry assembly.
>>
>
> Right, I see.. However I'm not completely convinced by this. I must
> admit I haven't followed the whole conversation, but what was the issue
> with the HINT_IRET_SELF? It seemed more elegant, but I might be missing
> some context.
>
> Otherwise, it might be worth having a comment in the code to point that
> this only handles the sync_core() case.
>
>
> Also, instead of adding a special "arch_exception_frame_size", I could
> suggest:
> - Picking this patch [1] from a completely arbitrary source
> - Getting rid of INSN_STACK type, any instruction could then include
> stack ops on top of their existing semantics, they can just have an
> empty list if they don't touch SP/BP
> - x86 decoder adds a stack_op to the iret to modify the stack pointer by
> the right amount
>
And the x86 decode could also lookup the symbol containing an IRET and
chose whether its type should be INSN_CONTEXT_SWITCH or INSN_OTHER
depending on whether the symbol is a function or not.
This would avoid having the arch specific pattern detected the generic
stack validation part of objtool.
--
Julien Thierry
On Thu, Apr 02, 2020 at 07:41:46AM +0100, Julien Thierry wrote:
> On 4/1/20 6:09 PM, Peter Zijlstra wrote:
> > The code in question (x86's sync_core()), is an exception return to
> > self. It pushes an exception frame that points to right after the
> > exception return instruction.
> >
> > This is the only usage of IRET in STT_FUNC symbols.
> >
> > So rather than teaching objtool how to interpret the whole
> > push;push;push;push;push;iret sequence, teach it how big the frame is
> > (arch_exception_frame_size) and let it continue.
> >
> > All the other (real) IRETs are in STT_NOTYPE in the entry assembly.
> >
>
> Right, I see.. However I'm not completely convinced by this. I must admit I
> haven't followed the whole conversation, but what was the issue with the
> HINT_IRET_SELF? It seemed more elegant, but I might be missing some context.
https://lkml.kernel.org/r/20200331211755.pb7f3wa6oxzjnswc@treble
Josh didn't think it was worth it, I think.
> Otherwise, it might be worth having a comment in the code to point that this
> only handles the sync_core() case.
I can certainly do that. Does ARM have any ERETs sprinkled around in
places it should not have? That is, is this going to be a problem for
you?
> Also, instead of adding a special "arch_exception_frame_size", I could
> suggest:
> - Picking this patch [1] from a completely arbitrary source
> - Getting rid of INSN_STACK type, any instruction could then include stack
> ops on top of their existing semantics, they can just have an empty list if
> they don't touch SP/BP
> - x86 decoder adds a stack_op to the iret to modify the stack pointer by the
> right amount
That's not the worst idea, lemme try that.
On Thu, Apr 02, 2020 at 09:50:36AM +0200, Peter Zijlstra wrote:
> On Thu, Apr 02, 2020 at 07:41:46AM +0100, Julien Thierry wrote:
> > Also, instead of adding a special "arch_exception_frame_size", I could
> > suggest:
> > - Picking this patch [1] from a completely arbitrary source
> > - Getting rid of INSN_STACK type, any instruction could then include stack
> > ops on top of their existing semantics, they can just have an empty list if
> > they don't touch SP/BP
> > - x86 decoder adds a stack_op to the iret to modify the stack pointer by the
> > right amount
>
> That's not the worst idea, lemme try that.
Something like so then?
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -738,7 +738,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"
@@ -748,7 +747,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
@@ -2224,6 +2224,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 4/2/20 8:50 AM, Peter Zijlstra wrote:
> On Thu, Apr 02, 2020 at 07:41:46AM +0100, Julien Thierry wrote:
>> On 4/1/20 6:09 PM, Peter Zijlstra wrote:
>
>>> The code in question (x86's sync_core()), is an exception return to
>>> self. It pushes an exception frame that points to right after the
>>> exception return instruction.
>>>
>>> This is the only usage of IRET in STT_FUNC symbols.
>>>
>>> So rather than teaching objtool how to interpret the whole
>>> push;push;push;push;push;iret sequence, teach it how big the frame is
>>> (arch_exception_frame_size) and let it continue.
>>>
>>> All the other (real) IRETs are in STT_NOTYPE in the entry assembly.
>>>
>>
>> Right, I see.. However I'm not completely convinced by this. I must admit I
>> haven't followed the whole conversation, but what was the issue with the
>> HINT_IRET_SELF? It seemed more elegant, but I might be missing some context.
>
> https://lkml.kernel.org/r/20200331211755.pb7f3wa6oxzjnswc@treble
>
> Josh didn't think it was worth it, I think.
>
>> Otherwise, it might be worth having a comment in the code to point that this
>> only handles the sync_core() case.
>
> I can certainly do that. Does ARM have any ERETs sprinkled around in
> places it should not have? That is, is this going to be a problem for
> you?
>
I had a quick look and I don't think there are ERETS in function
symbols. And, worst case scenario, I could also just keep the arm64
decoder making ERETS as INSN_CONTEXT_SWITCH as I didn't need more
semantics so far (and arm64 ERET don't affect the stack anyway).
After you pointed out this only affect this very specific pattern, I
admit that my concerns are more about "not having weird stuff in the
generic part".
If it's too much of a hassle I can understand if you prefer to just put
a comment. But if most of this can be kept to the arch specific decoder
I think it'd be nicer :) .
>> Also, instead of adding a special "arch_exception_frame_size", I could
>> suggest:
>> - Picking this patch [1] from a completely arbitrary source
>> - Getting rid of INSN_STACK type, any instruction could then include stack
>> ops on top of their existing semantics, they can just have an empty list if
>> they don't touch SP/BP
>> - x86 decoder adds a stack_op to the iret to modify the stack pointer by the
>> right amount
>
> That's not the worst idea, lemme try that.
>
Thanks, keep me in Cc if you post a new version using that!
Cheers,
--
Julien Thierry
On Thu, 2 Apr 2020, Julien Thierry wrote:
>
>
> On 4/2/20 9:17 AM, Peter Zijlstra wrote:
> > On Thu, Apr 02, 2020 at 09:50:36AM +0200, Peter Zijlstra wrote:
> >> On Thu, Apr 02, 2020 at 07:41:46AM +0100, Julien Thierry wrote:
> >
> >>> Also, instead of adding a special "arch_exception_frame_size", I could
> >>> suggest:
> >>> - Picking this patch [1] from a completely arbitrary source
> >>> - Getting rid of INSN_STACK type, any instruction could then include stack
> >>> ops on top of their existing semantics, they can just have an empty list
> >>> if
> >>> they don't touch SP/BP
> >>> - x86 decoder adds a stack_op to the iret to modify the stack pointer by
> >>> the
> >>> right amount
> >>
> >> That's not the worst idea, lemme try that.
> >
> > Something like so then?
> >
>
> Yes, you could even remove INSN_STACK from insn_type and just always call
> handle_insn_ops() before the switch statement on insn->type. If the list is
> empty it does nothing.
> This way you wouldn't need to call it for the INSN_EXCEPTION_RETURN case, and
> any type of instructions could use stack_ops.
>
>
> And the other suggestion is my other email was that you don't even need to add
> INSN_EXCEPTION_RETURN. You can keep IRET as INSN_CONTEXT_SWITCH by default and
> x86 decoder lookups the symbol conaining an iret. If it's a function symbol,
> it can just set the type to INSN_OTHER so that it caries on to the next
> instruction after having handled the stack_op.
>
> And everything fits under tools/objtool/arch/x86 :) .
>
> Or is it too far-fetch'd?
Imho no. Well, it depends. I can see benefits of both approach. PeterZ's
patch is quite minimal and it demonstrates itself as a one-off hack quite
well. On the other hand, it is in a generic code, which is not nice
especially when other archs do not have such thing. So your proposal would
indeed make sense to hide it in arch-specific code. Especially for the
future. And INSN_STACK is not used much in the code, so it can be removed
easily as you proposed.
And going more in direction of supporting more archs in the future, I'd
say it would make sense to allow more generic things such as "forget about
INSN_STACK. If an instruction has non-empty stack_ops list, just process
it". It is definitely more flexible.
So yes, I think it make sense unless I am missing something.
Miroslav
On 4/2/20 9:17 AM, Peter Zijlstra wrote:
> On Thu, Apr 02, 2020 at 09:50:36AM +0200, Peter Zijlstra wrote:
>> On Thu, Apr 02, 2020 at 07:41:46AM +0100, Julien Thierry wrote:
>
>>> Also, instead of adding a special "arch_exception_frame_size", I could
>>> suggest:
>>> - Picking this patch [1] from a completely arbitrary source
>>> - Getting rid of INSN_STACK type, any instruction could then include stack
>>> ops on top of their existing semantics, they can just have an empty list if
>>> they don't touch SP/BP
>>> - x86 decoder adds a stack_op to the iret to modify the stack pointer by the
>>> right amount
>>
>> That's not the worst idea, lemme try that.
>
> Something like so then?
>
Yes, you could even remove INSN_STACK from insn_type and just always
call handle_insn_ops() before the switch statement on insn->type. If the
list is empty it does nothing.
This way you wouldn't need to call it for the INSN_EXCEPTION_RETURN
case, and any type of instructions could use stack_ops.
And the other suggestion is my other email was that you don't even need
to add INSN_EXCEPTION_RETURN. You can keep IRET as INSN_CONTEXT_SWITCH
by default and x86 decoder lookups the symbol conaining an iret. If it's
a function symbol, it can just set the type to INSN_OTHER so that it
caries on to the next instruction after having handled the stack_op.
And everything fits under tools/objtool/arch/x86 :) .
Or is it too far-fetch'd?
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -738,7 +738,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"
> @@ -748,7 +747,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
> @@ -2224,6 +2224,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",
>
Cheers,
--
Julien Thierry