diff mbox series

ftrace: Allow WITH_ARGS flavour of graph tracer with shadow call stack

Message ID 20221209143402.3332369-1-ardb@kernel.org
State New
Headers
Series ftrace: Allow WITH_ARGS flavour of graph tracer with shadow call stack |

Commit Message

Ard Biesheuvel Dec. 9, 2022, 2:34 p.m. UTC 
  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(-)

Comments

Mark Rutland Dec. 9, 2022, 2:40 p.m. UTC | #1 
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
>
Steven Rostedt Dec. 9, 2022, 9:51 p.m. UTC | #2 
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
Masami Hiramatsu (Google) Dec. 11, 2022, 3:27 a.m. UTC | #3 
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
>
Mark Rutland Dec. 12, 2022, 10:36 a.m. UTC | #4 
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.
Mark Rutland Dec. 12, 2022, 10:46 a.m. UTC | #5 
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>
Will Deacon Dec. 13, 2022, 11:36 a.m. UTC | #6 
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 mbox series

Patch

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