| Message ID | 20221209143402.3332369-1-ardb@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ftrace: Allow WITH_ARGS flavour of graph tracer with shadow call stack | expand |
On Fri, Dec 09, 2022 at 03:34:02PM +0100, Ard Biesheuvel wrote: > The recent switch on arm64 from DYNAMIC_FTRACE_WITH_REGS to > DYNAMIC_FTRACE_WITH_ARGS failed to take into account that we currently > require the former in order to allow the function graph tracer to be > enabled in combination with shadow call stacks. This means that this is > no longer permitted at all, in spite of the fact that either flavour of > ftrace works perfectly fine in this combination. > > Given that arm64 is the only arch that implements shadow call stacks in > the first place, let's update the condition to just reflect the arm64 > change. When other architectures adopt shadow call stack support, this > can be revisited if needed. > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> My bad; sorry about this, and thanks for the fix! Acked-by: Mark Rutland <mark.rutland@arm.com> We should probably also add: Fixes: 26299b3f6ba26bfc ("ftrace: arm64: move from REGS to ARGS") Mark. > --- > arch/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/Kconfig b/arch/Kconfig > index 072a1b39e3afd0d1..683f365b5e31c856 100644 > --- a/arch/Kconfig > +++ b/arch/Kconfig > @@ -635,7 +635,7 @@ config ARCH_SUPPORTS_SHADOW_CALL_STACK > config SHADOW_CALL_STACK > bool "Shadow Call Stack" > depends on ARCH_SUPPORTS_SHADOW_CALL_STACK > - depends on DYNAMIC_FTRACE_WITH_REGS || !FUNCTION_GRAPH_TRACER > + depends on DYNAMIC_FTRACE_WITH_ARGS || !FUNCTION_GRAPH_TRACER > help > This option enables the compiler's Shadow Call Stack, which > uses a shadow stack to protect function return addresses from > -- > 2.35.1 >
On Fri, 9 Dec 2022 14:40:25 +0000 Mark Rutland <mark.rutland@arm.com> wrote: > On Fri, Dec 09, 2022 at 03:34:02PM +0100, Ard Biesheuvel wrote: > > The recent switch on arm64 from DYNAMIC_FTRACE_WITH_REGS to > > DYNAMIC_FTRACE_WITH_ARGS failed to take into account that we currently > > require the former in order to allow the function graph tracer to be > > enabled in combination with shadow call stacks. This means that this is > > no longer permitted at all, in spite of the fact that either flavour of > > ftrace works perfectly fine in this combination. > > > > Given that arm64 is the only arch that implements shadow call stacks in > > the first place, let's update the condition to just reflect the arm64 > > change. When other architectures adopt shadow call stack support, this > > can be revisited if needed. > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > My bad; sorry about this, and thanks for the fix! > > Acked-by: Mark Rutland <mark.rutland@arm.com> > > We should probably also add: > > Fixes: 26299b3f6ba26bfc ("ftrace: arm64: move from REGS to ARGS") Actually, I believe it is: Fixes: ddc9863e9e90 ("scs: Disable when function graph tracing is enabled") As according to the comments, scs is broken with function graph unless function graph is using the ftrace mechanism. And that is only true if WITH_ARGS is set, not REGS. Acked-by: Steven Rostedt (Google) <rostedt@goodmis.org> -- Steve
On Fri, 9 Dec 2022 15:34:02 +0100 Ard Biesheuvel <ardb@kernel.org> wrote: > The recent switch on arm64 from DYNAMIC_FTRACE_WITH_REGS to > DYNAMIC_FTRACE_WITH_ARGS failed to take into account that we currently > require the former in order to allow the function graph tracer to be > enabled in combination with shadow call stacks. This means that this is > no longer permitted at all, in spite of the fact that either flavour of > ftrace works perfectly fine in this combination. > > Given that arm64 is the only arch that implements shadow call stacks in > the first place, let's update the condition to just reflect the arm64 > change. When other architectures adopt shadow call stack support, this > can be revisited if needed. This brings a question. Is the SCS safe if kretprobe(rethook) is enabled? it also changes the stack entry after a calling function. Thank you, > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > --- > arch/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/Kconfig b/arch/Kconfig > index 072a1b39e3afd0d1..683f365b5e31c856 100644 > --- a/arch/Kconfig > +++ b/arch/Kconfig > @@ -635,7 +635,7 @@ config ARCH_SUPPORTS_SHADOW_CALL_STACK > config SHADOW_CALL_STACK > bool "Shadow Call Stack" > depends on ARCH_SUPPORTS_SHADOW_CALL_STACK > - depends on DYNAMIC_FTRACE_WITH_REGS || !FUNCTION_GRAPH_TRACER > + depends on DYNAMIC_FTRACE_WITH_ARGS || !FUNCTION_GRAPH_TRACER > help > This option enables the compiler's Shadow Call Stack, which > uses a shadow stack to protect function return addresses from > -- > 2.35.1 >
On Fri, Dec 09, 2022 at 04:51:39PM -0500, Steven Rostedt wrote: > On Fri, 9 Dec 2022 14:40:25 +0000 > Mark Rutland <mark.rutland@arm.com> wrote: > > > On Fri, Dec 09, 2022 at 03:34:02PM +0100, Ard Biesheuvel wrote: > > > The recent switch on arm64 from DYNAMIC_FTRACE_WITH_REGS to > > > DYNAMIC_FTRACE_WITH_ARGS failed to take into account that we currently > > > require the former in order to allow the function graph tracer to be > > > enabled in combination with shadow call stacks. This means that this is > > > no longer permitted at all, in spite of the fact that either flavour of > > > ftrace works perfectly fine in this combination. > > > > > > Given that arm64 is the only arch that implements shadow call stacks in > > > the first place, let's update the condition to just reflect the arm64 > > > change. When other architectures adopt shadow call stack support, this > > > can be revisited if needed. > > > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > > > My bad; sorry about this, and thanks for the fix! > > > > Acked-by: Mark Rutland <mark.rutland@arm.com> > > > > We should probably also add: > > > > Fixes: 26299b3f6ba26bfc ("ftrace: arm64: move from REGS to ARGS") > > Actually, I believe it is: > > Fixes: ddc9863e9e90 ("scs: Disable when function graph tracing is enabled") Ah; it's slightly more subtle since these were on different branches that got merged. Either's correct in its own branch, and the merge is where things went wrong. I think the overall least confusing thing is to bite the bullet and list both REGS and ARGS, i.e. depends on DYNAMIC_FTRACE_WITH_ARGS || DYNAMIC_FTRACE_WITH_REGS || !FUNCTION_GRAPH_TRACER ... and for the fixes tag have: Fixes: ddc9863e9e90 ("scs: Disable when function graph tracing is enabled") That way the commit is correct regardless of the REGS -> ARGS conversion, and will work if backported independently. Thanks, Mark.
On Sun, Dec 11, 2022 at 12:27:31PM +0900, Masami Hiramatsu wrote: > On Fri, 9 Dec 2022 15:34:02 +0100 > Ard Biesheuvel <ardb@kernel.org> wrote: > > > The recent switch on arm64 from DYNAMIC_FTRACE_WITH_REGS to > > DYNAMIC_FTRACE_WITH_ARGS failed to take into account that we currently > > require the former in order to allow the function graph tracer to be > > enabled in combination with shadow call stacks. This means that this is > > no longer permitted at all, in spite of the fact that either flavour of > > ftrace works perfectly fine in this combination. > > > > Given that arm64 is the only arch that implements shadow call stacks in > > the first place, let's update the condition to just reflect the arm64 > > change. When other architectures adopt shadow call stack support, this > > can be revisited if needed. > > This brings a question. Is the SCS safe if kretprobe(rethook) is enabled? > it also changes the stack entry after a calling function. That should be safe. The SCS push is just an instruction somewhere in the function, and since kretprobe instruments the first instruction of a function, that intrumentation will run *before* the SCS push occurs, and so it can safely override the return address. The difficulty with ftrace is that the old mcount implementation calls into ftrace *after* the function prologue, after saving some GPRs to the stack, signing the return address with pointer authentication, and/or pushing the return address to the SCS. The DYNAMIC_FTRACE_WITH_{ARGS,REGS} forms use patchable-function-entry to hook functions *before* any of that happens, and are safe for the same reason as kretprobes. Thanks, Mark. > > Thank you, > > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > --- > > arch/Kconfig | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/Kconfig b/arch/Kconfig > > index 072a1b39e3afd0d1..683f365b5e31c856 100644 > > --- a/arch/Kconfig > > +++ b/arch/Kconfig > > @@ -635,7 +635,7 @@ config ARCH_SUPPORTS_SHADOW_CALL_STACK > > config SHADOW_CALL_STACK > > bool "Shadow Call Stack" > > depends on ARCH_SUPPORTS_SHADOW_CALL_STACK > > - depends on DYNAMIC_FTRACE_WITH_REGS || !FUNCTION_GRAPH_TRACER > > + depends on DYNAMIC_FTRACE_WITH_ARGS || !FUNCTION_GRAPH_TRACER > > help > > This option enables the compiler's Shadow Call Stack, which > > uses a shadow stack to protect function return addresses from > > -- > > 2.35.1 > > > > > -- > Masami Hiramatsu (Google) <mhiramat@kernel.org>
On Mon, Dec 12, 2022 at 10:36:23AM +0000, Mark Rutland wrote: > On Fri, Dec 09, 2022 at 04:51:39PM -0500, Steven Rostedt wrote: > > On Fri, 9 Dec 2022 14:40:25 +0000 > > Mark Rutland <mark.rutland@arm.com> wrote: > > > > > On Fri, Dec 09, 2022 at 03:34:02PM +0100, Ard Biesheuvel wrote: > > > > The recent switch on arm64 from DYNAMIC_FTRACE_WITH_REGS to > > > > DYNAMIC_FTRACE_WITH_ARGS failed to take into account that we currently > > > > require the former in order to allow the function graph tracer to be > > > > enabled in combination with shadow call stacks. This means that this is > > > > no longer permitted at all, in spite of the fact that either flavour of > > > > ftrace works perfectly fine in this combination. > > > > > > > > Given that arm64 is the only arch that implements shadow call stacks in > > > > the first place, let's update the condition to just reflect the arm64 > > > > change. When other architectures adopt shadow call stack support, this > > > > can be revisited if needed. > > > > > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > > > > > My bad; sorry about this, and thanks for the fix! > > > > > > Acked-by: Mark Rutland <mark.rutland@arm.com> > > > > > > We should probably also add: > > > > > > Fixes: 26299b3f6ba26bfc ("ftrace: arm64: move from REGS to ARGS") > > > > Actually, I believe it is: > > > > Fixes: ddc9863e9e90 ("scs: Disable when function graph tracing is enabled") > > Ah; it's slightly more subtle since these were on different branches that got > merged. Either's correct in its own branch, and the merge is where things went > wrong. > > I think the overall least confusing thing is to bite the bullet and list both > REGS and ARGS, i.e. > > depends on DYNAMIC_FTRACE_WITH_ARGS || DYNAMIC_FTRACE_WITH_REGS || !FUNCTION_GRAPH_TRACER > > ... and for the fixes tag have: > > Fixes: ddc9863e9e90 ("scs: Disable when function graph tracing is enabled") > > That way the commit is correct regardless of the REGS -> ARGS conversion, and > will work if backported independently. Ard -- please can you respin as per Mark's suggestion above? I can then send it as a fix later this week. Cheers, Will
diff --git a/arch/Kconfig b/arch/Kconfig index 072a1b39e3afd0d1..683f365b5e31c856 100644 --- a/arch/Kconfig +++ b/arch/Kconfig @@ -635,7 +635,7 @@ config ARCH_SUPPORTS_SHADOW_CALL_STACK config SHADOW_CALL_STACK bool "Shadow Call Stack" depends on ARCH_SUPPORTS_SHADOW_CALL_STACK - depends on DYNAMIC_FTRACE_WITH_REGS || !FUNCTION_GRAPH_TRACER + depends on DYNAMIC_FTRACE_WITH_ARGS || !FUNCTION_GRAPH_TRACER help This option enables the compiler's Shadow Call Stack, which uses a shadow stack to protect function return addresses from
The recent switch on arm64 from DYNAMIC_FTRACE_WITH_REGS to DYNAMIC_FTRACE_WITH_ARGS failed to take into account that we currently require the former in order to allow the function graph tracer to be enabled in combination with shadow call stacks. This means that this is no longer permitted at all, in spite of the fact that either flavour of ftrace works perfectly fine in this combination. Given that arm64 is the only arch that implements shadow call stacks in the first place, let's update the condition to just reflect the arm64 change. When other architectures adopt shadow call stack support, this can be revisited if needed. Signed-off-by: Ard Biesheuvel <ardb@kernel.org> --- arch/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)