[1/2,v2] tracing/arm64: Have max stack tracer handle the case of return address after data

Commit Message

Steven Rostedt Aug. 7, 2019, 5:28 p.m. UTC

From: "Steven Rostedt (VMware)" <[e-mail]>

Most archs (well at least x86) store the function call return address on the
stack before storing the local variables for the function. The max stack
tracer depends on this in its algorithm to display the stack size of each
function it finds in the back trace.

Some archs (arm64), may store the return address (from its link register)
just before calling a nested function. There's no reason to save the link
register on leaf functions, as it wont be updated. This breaks the algorithm
of the max stack tracer.

Add a new define ARCH_RET_ADDR_AFTER_LOCAL_VARS that an architecture may set
if it stores the return address (link register) after it stores the
function's local variables, and have the stack trace shift the values of the
mapped stack size to the appropriate functions.

Link: [e-mail]

Reported-by: Jiping Ma <[e-mail]>
Signed-off-by: Steven Rostedt (VMware) <[e-mail]>
---
 arch/arm64/include/asm/ftrace.h | 13 +++++++++++++
 kernel/trace/trace_stack.c      | 14 ++++++++++++++
 2 files changed, 27 insertions(+)

Comments

Steven Rostedt Aug. 7, 2019, 7:29 p.m. UTC | #1

[ I should have added Mark as Cc ]

Dear ARM64 folks,

Are you OK with this patch set?

If so, please ACK.

Should it be marked for stable?

Hmm, I'm starting to think not.

-- Steve


On Wed, 07 Aug 2019 13:28:27 -0400
Steven Rostedt <[e-mail]> wrote:

> From: "Steven Rostedt (VMware)" <[e-mail]>
> 
> Most archs (well at least x86) store the function call return address on the
> stack before storing the local variables for the function. The max stack
> tracer depends on this in its algorithm to display the stack size of each
> function it finds in the back trace.
> 
> Some archs (arm64), may store the return address (from its link register)
> just before calling a nested function. There's no reason to save the link
> register on leaf functions, as it wont be updated. This breaks the algorithm
> of the max stack tracer.
> 
> Add a new define ARCH_RET_ADDR_AFTER_LOCAL_VARS that an architecture may set
> if it stores the return address (link register) after it stores the
> function's local variables, and have the stack trace shift the values of the
> mapped stack size to the appropriate functions.
> 
> Link: [e-mail]
> 
> Reported-by: Jiping Ma <[e-mail]>
> Signed-off-by: Steven Rostedt (VMware) <[e-mail]>
> ---
>  arch/arm64/include/asm/ftrace.h | 13 +++++++++++++
>  kernel/trace/trace_stack.c      | 14 ++++++++++++++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
> index 5ab5200b2bdc..961e98618db4 100644
> --- a/arch/arm64/include/asm/ftrace.h
> +++ b/arch/arm64/include/asm/ftrace.h
> @@ -14,6 +14,19 @@
>  #define MCOUNT_ADDR		((unsigned long)_mcount)
>  #define MCOUNT_INSN_SIZE	AARCH64_INSN_SIZE
>  
> +/*
> + * Currently, gcc tends to save the link register after the local variables
> + * on the stack. This causes the max stack tracer to report the function
> + * frame sizes for the wrong functions. By defining
> + * ARCH_RET_ADDR_AFTER_LOCAL_VARS, it will tell the stack tracer to expect
> + * to find the return address on the stack after the local variables have
> + * been set up.
> + *
> + * Note, this may change in the future, and we will need to deal with that
> + * if it were to happen.
> + */
> +#define ARCH_RET_ADDR_AFTER_LOCAL_VARS 1
> +
>  #ifndef __ASSEMBLY__
>  #include <linux/compat.h>
>  
> diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
> index 5d16f73898db..40e4a88eea8f 100644
> --- a/kernel/trace/trace_stack.c
> +++ b/kernel/trace/trace_stack.c
> @@ -158,6 +158,20 @@ static void check_stack(unsigned long ip, unsigned long *stack)
>  			i++;
>  	}
>  
> +#ifdef ARCH_RET_ADDR_AFTER_LOCAL_VARS
> +	/*
> +	 * Some archs will store the link register before calling
> +	 * nested functions. This means the saved return address
> +	 * comes after the local storage, and we need to shift
> +	 * for that.
> +	 */
> +	if (x > 1) {
> +		memmove(&stack_trace_index[0], &stack_trace_index[1],
> +			sizeof(stack_trace_index[0]) * (x - 1));
> +		x--;
> +	}
> +#endif
> +
>  	stack_trace_nr_entries = x;
>  
>  	if (task_stack_end_corrupted(current)) {

Will Deacon Aug. 8, 2019, 4:28 p.m. UTC | #2

Hi Steve,

On Wed, Aug 07, 2019 at 01:28:27PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (VMware)" <[e-mail]>
> 
> Most archs (well at least x86) store the function call return address on the
> stack before storing the local variables for the function. The max stack
> tracer depends on this in its algorithm to display the stack size of each
> function it finds in the back trace.
> 
> Some archs (arm64), may store the return address (from its link register)
> just before calling a nested function. There's no reason to save the link
> register on leaf functions, as it wont be updated. This breaks the algorithm
> of the max stack tracer.
> 
> Add a new define ARCH_RET_ADDR_AFTER_LOCAL_VARS that an architecture may set
> if it stores the return address (link register) after it stores the
> function's local variables, and have the stack trace shift the values of the
> mapped stack size to the appropriate functions.
> 
> Link: [e-mail]
> 
> Reported-by: Jiping Ma <[e-mail]>
> Signed-off-by: Steven Rostedt (VMware) <[e-mail]>
> ---
>  arch/arm64/include/asm/ftrace.h | 13 +++++++++++++
>  kernel/trace/trace_stack.c      | 14 ++++++++++++++
>  2 files changed, 27 insertions(+)

I agree with your later comment that this should NOT go to stable.

> diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
> index 5ab5200b2bdc..961e98618db4 100644
> --- a/arch/arm64/include/asm/ftrace.h
> +++ b/arch/arm64/include/asm/ftrace.h
> @@ -14,6 +14,19 @@
>  #define MCOUNT_ADDR		((unsigned long)_mcount)
>  #define MCOUNT_INSN_SIZE	AARCH64_INSN_SIZE
>  
> +/*
> + * Currently, gcc tends to save the link register after the local variables
> + * on the stack. This causes the max stack tracer to report the function
> + * frame sizes for the wrong functions. By defining
> + * ARCH_RET_ADDR_AFTER_LOCAL_VARS, it will tell the stack tracer to expect
> + * to find the return address on the stack after the local variables have
> + * been set up.
> + *
> + * Note, this may change in the future, and we will need to deal with that
> + * if it were to happen.
> + */
> +#define ARCH_RET_ADDR_AFTER_LOCAL_VARS 1

I know it's long already, but prefixing this with FTRACE_ would be good so
that other code doesn't use it for anything. It's not the end of the world
if the ftrace stack usage statistics are wonky, but if people tried to use
this for crazy things like livepatching then we'd be in trouble.

Maybe FTRACE_ARCH_FRAME_AFTER_LOCALS, which is the same length as what
you currently have?

Will

Steven Rostedt Aug. 8, 2019, 4:36 p.m. UTC | #3

On Thu, 8 Aug 2019 17:28:26 +0100
Will Deacon <[e-mail]> wrote:

> > + * Note, this may change in the future, and we will need to deal with that
> > + * if it were to happen.
> > + */
> > +#define ARCH_RET_ADDR_AFTER_LOCAL_VARS 1  
> 
> I know it's long already, but prefixing this with FTRACE_ would be good so
> that other code doesn't use it for anything. It's not the end of the world
> if the ftrace stack usage statistics are wonky, but if people tried to use
> this for crazy things like livepatching then we'd be in trouble.
> 
> Maybe FTRACE_ARCH_FRAME_AFTER_LOCALS, which is the same length as what
> you currently have?

Note, it would still need to be prefixed with "ARCH_" as that's the way
of showing arch specific defines.

We could make it more descriptive of what it will do and not the reason
for why it is done...


  ARCH_FTRACE_SHIFT_STACK_TRACER

?

-- Steve

Will Deacon Aug. 8, 2019, 5:11 p.m. UTC | #4

On Thu, Aug 08, 2019 at 12:36:32PM -0400, Steven Rostedt wrote:
> On Thu, 8 Aug 2019 17:28:26 +0100
> Will Deacon <[e-mail]> wrote:
> 
> > > + * Note, this may change in the future, and we will need to deal with that
> > > + * if it were to happen.
> > > + */
> > > +#define ARCH_RET_ADDR_AFTER_LOCAL_VARS 1  
> > 
> > I know it's long already, but prefixing this with FTRACE_ would be good so
> > that other code doesn't use it for anything. It's not the end of the world
> > if the ftrace stack usage statistics are wonky, but if people tried to use
> > this for crazy things like livepatching then we'd be in trouble.
> > 
> > Maybe FTRACE_ARCH_FRAME_AFTER_LOCALS, which is the same length as what
> > you currently have?
> 
> Note, it would still need to be prefixed with "ARCH_" as that's the way
> of showing arch specific defines.
> 
> We could make it more descriptive of what it will do and not the reason
> for why it is done...
> 
> 
>   ARCH_FTRACE_SHIFT_STACK_TRACER

Acked-by: Will Deacon <[e-mail]>

Thanks, Steve.

Will

Steven Rostedt Aug. 8, 2019, 5:24 p.m. UTC | #5

On Thu, 8 Aug 2019 18:11:53 +0100
Will Deacon <[e-mail]> wrote:

> > We could make it more descriptive of what it will do and not the reason
> > for why it is done...
> > 
> > 
> >   ARCH_FTRACE_SHIFT_STACK_TRACER  
> 
> Acked-by: Will Deacon <[e-mail]>

Thanks Will!

Here's the official patch.

From: "Steven Rostedt (VMware)" <[e-mail]>

Most archs (well at least x86) store the function call return address on the
stack before storing the local variables for the function. The max stack
tracer depends on this in its algorithm to display the stack size of each
function it finds in the back trace.

Some archs (arm64), may store the return address (from its link register)
just before calling a nested function. There's no reason to save the link
register on leaf functions, as it wont be updated. This breaks the algorithm
of the max stack tracer.

Add a new define ARCH_RET_ADDR_AFTER_LOCAL_VARS that an architecture may set
if it stores the return address (link register) after it stores the
function's local variables, and have the stack trace shift the values of the
mapped stack size to the appropriate functions.

Link: [e-mail]

Reported-by: Jiping Ma <[e-mail]>
Acked-by: Will Deacon <[e-mail]>
Signed-off-by: Steven Rostedt (VMware) <[e-mail]>
---
 arch/arm64/include/asm/ftrace.h | 13 +++++++++++++
 kernel/trace/trace_stack.c      | 14 ++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
index 5ab5200b2bdc..d48667b04c41 100644
--- a/arch/arm64/include/asm/ftrace.h
+++ b/arch/arm64/include/asm/ftrace.h
@@ -14,6 +14,19 @@
 #define MCOUNT_ADDR		((unsigned long)_mcount)
 #define MCOUNT_INSN_SIZE	AARCH64_INSN_SIZE
 
+/*
+ * Currently, gcc tends to save the link register after the local variables
+ * on the stack. This causes the max stack tracer to report the function
+ * frame sizes for the wrong functions. By defining
+ * ARCH_FTRACE_SHIFT_STACK_TRACER, it will tell the stack tracer to expect
+ * to find the return address on the stack after the local variables have
+ * been set up.
+ *
+ * Note, this may change in the future, and we will need to deal with that
+ * if it were to happen.
+ */
+#define ARCH_FTRACE_SHIFT_STACK_TRACER 1
+
 #ifndef __ASSEMBLY__
 #include <linux/compat.h>
 
diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
index 5d16f73898db..642a850af81a 100644
--- a/kernel/trace/trace_stack.c
+++ b/kernel/trace/trace_stack.c
@@ -158,6 +158,20 @@ static void check_stack(unsigned long ip, unsigned long *stack)
 			i++;
 	}
 
+#ifdef ARCH_FTRACE_SHIFT_STACK_TRACER
+	/*
+	 * Some archs will store the link register before calling
+	 * nested functions. This means the saved return address
+	 * comes after the local storage, and we need to shift
+	 * for that.
+	 */
+	if (x > 1) {
+		memmove(&stack_trace_index[0], &stack_trace_index[1],
+			sizeof(stack_trace_index[0]) * (x - 1));
+		x--;
+	}
+#endif
+
 	stack_trace_nr_entries = x;
 
 	if (task_stack_end_corrupted(current)) {

Steven Rostedt Aug. 9, 2019, 2:24 a.m. UTC | #6

On Fri, 9 Aug 2019 10:17:19 +0800
Jiping Ma <[e-mail]> wrote:

> On 2019年08月09日 01:24, Steven Rostedt wrote:
> > On Thu, 8 Aug 2019 18:11:53 +0100
> > Will Deacon <[e-mail]> wrote:
> >  
> >>> We could make it more descriptive of what it will do and not the reason
> >>> for why it is done...
> >>>
> >>>
> >>>    ARCH_FTRACE_SHIFT_STACK_TRACER  
> >> Acked-by: Will Deacon <[e-mail]>  
> > Thanks Will!
> >
> > Here's the official patch.
> >
> > From: "Steven Rostedt (VMware)" <[e-mail]>
> >
> > Most archs (well at least x86) store the function call return address on the
> > stack before storing the local variables for the function. The max stack
> > tracer depends on this in its algorithm to display the stack size of each
> > function it finds in the back trace.
> >
> > Some archs (arm64), may store the return address (from its link register)
> > just before calling a nested function. There's no reason to save the link
> > register on leaf functions, as it wont be updated. This breaks the algorithm
> > of the max stack tracer.
> >
> > Add a new define ARCH_RET_ADDR_AFTER_LOCAL_VARS that an architecture may set  
> 
> ARCH_FTRACE_SHIFT_STACK_TRACER is used in the code.

Ah, I did a s/x/y/ to the diff of the patch, but not the change log.
Thanks for pointing that out. I also need to update the comment in 2/2.

-- Steve

> 
> Jiping
> 
>

Mark Rutland Aug. 9, 2019, 8:55 a.m. UTC | #7

On Wed, Aug 07, 2019 at 01:28:27PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (VMware)" <[e-mail]>
> 
> Most archs (well at least x86) store the function call return address on the
> stack before storing the local variables for the function. The max stack
> tracer depends on this in its algorithm to display the stack size of each
> function it finds in the back trace.
> 
> Some archs (arm64), may store the return address (from its link register)
> just before calling a nested function. There's no reason to save the link
> register on leaf functions, as it wont be updated. This breaks the algorithm
> of the max stack tracer.
> 
> Add a new define ARCH_RET_ADDR_AFTER_LOCAL_VARS that an architecture may set
> if it stores the return address (link register) after it stores the
> function's local variables, and have the stack trace shift the values of the
> mapped stack size to the appropriate functions.
> 
> Link: [e-mail]
> 
> Reported-by: Jiping Ma <[e-mail]>
> Signed-off-by: Steven Rostedt (VMware) <[e-mail]>
> ---
>  arch/arm64/include/asm/ftrace.h | 13 +++++++++++++
>  kernel/trace/trace_stack.c      | 14 ++++++++++++++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
> index 5ab5200b2bdc..961e98618db4 100644
> --- a/arch/arm64/include/asm/ftrace.h
> +++ b/arch/arm64/include/asm/ftrace.h
> @@ -14,6 +14,19 @@
>  #define MCOUNT_ADDR		((unsigned long)_mcount)
>  #define MCOUNT_INSN_SIZE	AARCH64_INSN_SIZE
>  
> +/*
> + * Currently, gcc tends to save the link register after the local variables
> + * on the stack. This causes the max stack tracer to report the function
> + * frame sizes for the wrong functions. By defining
> + * ARCH_RET_ADDR_AFTER_LOCAL_VARS, it will tell the stack tracer to expect
> + * to find the return address on the stack after the local variables have
> + * been set up.
> + *
> + * Note, this may change in the future, and we will need to deal with that
> + * if it were to happen.
> + */
> +#define ARCH_RET_ADDR_AFTER_LOCAL_VARS 1

FWIW (with whatever this got renamed to):

Acked-by: Mark Rutland <[e-mail]>

Thanks,
Mark.

Will Deacon Aug. 13, 2019, 5:31 p.m. UTC | #8

Hi Steve,

On Thu, Aug 08, 2019 at 10:24:40PM -0400, Steven Rostedt wrote:
> On Fri, 9 Aug 2019 10:17:19 +0800
> Jiping Ma <[e-mail]> wrote:
> > On 2019年08月09日 01:24, Steven Rostedt wrote:
> > > On Thu, 8 Aug 2019 18:11:53 +0100
> > > Will Deacon <[e-mail]> wrote:
> > >  
> > >>> We could make it more descriptive of what it will do and not the reason
> > >>> for why it is done...
> > >>>
> > >>>
> > >>>    ARCH_FTRACE_SHIFT_STACK_TRACER  
> > >> Acked-by: Will Deacon <[e-mail]>  
> > > Thanks Will!
> > >
> > > Here's the official patch.
> > >
> > > From: "Steven Rostedt (VMware)" <[e-mail]>
> > >
> > > Most archs (well at least x86) store the function call return address on the
> > > stack before storing the local variables for the function. The max stack
> > > tracer depends on this in its algorithm to display the stack size of each
> > > function it finds in the back trace.
> > >
> > > Some archs (arm64), may store the return address (from its link register)
> > > just before calling a nested function. There's no reason to save the link
> > > register on leaf functions, as it wont be updated. This breaks the algorithm
> > > of the max stack tracer.
> > >
> > > Add a new define ARCH_RET_ADDR_AFTER_LOCAL_VARS that an architecture may set  
> > 
> > ARCH_FTRACE_SHIFT_STACK_TRACER is used in the code.
> 
> Ah, I did a s/x/y/ to the diff of the patch, but not the change log.
> Thanks for pointing that out. I also need to update the comment in 2/2.

Are you going to post another version of this or have you queued it already?
Just want to make sure it doesn't slip through the cracks.

Cheers,

Will

Steven Rostedt Aug. 13, 2019, 5:47 p.m. UTC | #9

On Tue, 13 Aug 2019 18:31:14 +0100
Will Deacon <[e-mail]> wrote:

> Hi Steve,
> 
> On Thu, Aug 08, 2019 at 10:24:40PM -0400, Steven Rostedt wrote:
> > On Fri, 9 Aug 2019 10:17:19 +0800
> > Jiping Ma <[e-mail]> wrote:  
> > > On 2019年08月09日 01:24, Steven Rostedt wrote:  
> > > > On Thu, 8 Aug 2019 18:11:53 +0100
> > > > Will Deacon <[e-mail]> wrote:
> > > >    
> > > >>> We could make it more descriptive of what it will do and not the reason
> > > >>> for why it is done...
> > > >>>
> > > >>>
> > > >>>    ARCH_FTRACE_SHIFT_STACK_TRACER    
> > > >> Acked-by: Will Deacon <[e-mail]>    
> > > > Thanks Will!
> > > >
> > > > Here's the official patch.
> > > >
> > > > From: "Steven Rostedt (VMware)" <[e-mail]>
> > > >
> > > > Most archs (well at least x86) store the function call return address on the
> > > > stack before storing the local variables for the function. The max stack
> > > > tracer depends on this in its algorithm to display the stack size of each
> > > > function it finds in the back trace.
> > > >
> > > > Some archs (arm64), may store the return address (from its link register)
> > > > just before calling a nested function. There's no reason to save the link
> > > > register on leaf functions, as it wont be updated. This breaks the algorithm
> > > > of the max stack tracer.
> > > >
> > > > Add a new define ARCH_RET_ADDR_AFTER_LOCAL_VARS that an architecture may set    
> > > 
> > > ARCH_FTRACE_SHIFT_STACK_TRACER is used in the code.  
> > 
> > Ah, I did a s/x/y/ to the diff of the patch, but not the change log.
> > Thanks for pointing that out. I also need to update the comment in 2/2.  
> 
> Are you going to post another version of this or have you queued it already?
> Just want to make sure it doesn't slip through the cracks.
> 

Ah, it's in my queue. I should post a new version :-/

Thanks for the reminder.

-- Steve

Patch

diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
index 5ab5200b2bdc..961e98618db4 100644
--- a/arch/arm64/include/asm/ftrace.h
+++ b/arch/arm64/include/asm/ftrace.h
@@ -14,6 +14,19 @@ 
 #define MCOUNT_ADDR		((unsigned long)_mcount)
 #define MCOUNT_INSN_SIZE	AARCH64_INSN_SIZE
 
+/*
+ * Currently, gcc tends to save the link register after the local variables
+ * on the stack. This causes the max stack tracer to report the function
+ * frame sizes for the wrong functions. By defining
+ * ARCH_RET_ADDR_AFTER_LOCAL_VARS, it will tell the stack tracer to expect
+ * to find the return address on the stack after the local variables have
+ * been set up.
+ *
+ * Note, this may change in the future, and we will need to deal with that
+ * if it were to happen.
+ */
+#define ARCH_RET_ADDR_AFTER_LOCAL_VARS 1
+
 #ifndef __ASSEMBLY__
 #include <linux/compat.h>
 
diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
index 5d16f73898db..40e4a88eea8f 100644
--- a/kernel/trace/trace_stack.c
+++ b/kernel/trace/trace_stack.c
@@ -158,6 +158,20 @@  static void check_stack(unsigned long ip, unsigned long *stack)
 			i++;
 	}
 
+#ifdef ARCH_RET_ADDR_AFTER_LOCAL_VARS
+	/*
+	 * Some archs will store the link register before calling
+	 * nested functions. This means the saved return address
+	 * comes after the local storage, and we need to shift
+	 * for that.
+	 */
+	if (x > 1) {
+		memmove(&stack_trace_index[0], &stack_trace_index[1],
+			sizeof(stack_trace_index[0]) * (x - 1));
+		x--;
+	}
+#endif
+
 	stack_trace_nr_entries = x;
 
 	if (task_stack_end_corrupted(current)) {